Thanks for the feedback Nathan. The code was based on a C implementation I did a while back, then converted the project to C++. I'm still planning to make it more "C++ like", although it's not a big priority at the moment. I tend to agree with most of the things you have mentioned.
1. I don't like "JPEG_Class" either. I came up with this quickly, without giving it much thought! :P I was focusing on encapsulating stuff in C++ classes at the time. Maybe I'll rename it to JPEG_Rec once I finalise the C++ conversion.
2. Theoretically the pointer checks should never fail in this code, as I do external checks before the function is called, but it's there for extra precaution. Having said that, I don't like it either, and I'm planning to turn them into refs later on anyway. Re. failing silently, I usually do this on small inner loop functions such as this one. Errors are picked up & handled by the parent function (i.e.. were it was called).
3. It's just a naming convention I use consistently though-out my image processing related code. I regard "Coord" as a 2D offet, while something named "Offset" is regarded as a linear offset (eg.: buffer offsets, etc). So in this case Coord is the [X, Y] grid coordinate of the image block I want to process.
5. Yep. :) (A habit where I compact simple statements into one line.)
6. Interesting point! I used to have long naming conventions similar to the "normalization_shift" example you gave me. I have abandoned this habit, purely for the reason that I want to visually compact things like indexing vars and simple arithmetic/logic operands. Personally this works quite well for me.
7. Indeed, negative shifts don't work unfortunately! (My ASM background helped to pick up little things like that.) Also, the reason for NL and NR is to avoid if-else statements in loops as much as possible.
8. Very good point re. modifying. I'll make the appropriate changes. (This function operates on the image block at the final stage of the decoding, so the image block is regarded as discardable by default, hence the lazyness.)
Commenting code - completely useless?
Quote:Original post by Tachikoma
6. Interesting point! I used to have long naming conventions similar to the "normalization_shift" example you gave me. I have abandoned this habit, purely for the reason that I want to visually compact things like indexing vars and simple arithmetic/logic operands. Personally this works quite well for me.
Personaly, I use a 6pt font spread over two monitors. With such an excess of screen real estate, I'd much rather save mental cycles figuring out what code does later.
Just want to mention that comments require maintenance, just like the code does. Bad comments are a problem; wrong comments are downright dangerous. And too much commenting can very quickly lead to situations where the comments slide out of sync with the code, which will lead to a real hellish block of code.
Code should absolutely be self documenting whenever possible, but it's still no replacement for documentation and comments. But it is a replacement for redundant comments.
Like this:
FireBullet(); // Fires a bullet
Like this:
FireBullet(); // Fires a bullet
Hi guys, I'm back. :)
/* C++ style double-slash comments show comments that I would actually write; * C style comments such as this are aside commentary on what I've done. * I pretty much never write C style comments in my actual code anyway ;) */template <typename T>inline void shiftLeft(T& value, int amount) { if (amount < 0) { value >>= -amount; } else { value <<= amount; }}// An image block represents sample data of JPEG_BLOCK_RES size.// TODO: Merge this with the SOFnRec somehow, so that the information about// bits per sample is bundled together with the samples.struct ImageBlock { /* Pseudo; but I assume the dimensions are compile-time constant; otherwise you were setting a global before calling the function so that it knows the line lengths? o_O */ int data[JPEG_BLOCK_RES.U * JPEG_BLOCK_RES.V];};// Save the image block onto the bitmap at the specified location.// The SOFnRec (start of frame) tells us the number of bits per sample in// the data so that it can be normalized appropriately: such that the MSB of// sample values moves to bit 7 of the corresponding byte in the bitmap// storage. Return whether successful./* I'd actually be tempted to put this functionality in the Bitmap. */bool JPEG::SaveImageBlock(const ImageBlock& IB, BitmapRec& Bitmap, const vector2i& topleft, const SOFnRec &SOFn) { // Ensure the ImageBlock overlaps the Bitmap before we begin. /* I added a case: the requested top-left could be to the right of the image * but still appear "within", unless the LinAddr() member function takes * care of that. It's possIBle that the FrameEnd check becomes redundant, if * we do need to check the width and height explicitly. * There are more cases that aren't considered: what if topleft is above or * to the left of the bitmap area? */ uint8* scanline_pos = Bitmap.LinAddr(topleft); if (scanline_pos >= Bitmap.FrameEnd) { return false; } int width = std::max(JPEG_BLOCK_RES.U, Bitmap->Res.U - topleft.U); int height = std::max(JPEG_BLOCK_RES.V, Bitmap->Res.V - topleft.V); if (width < 0 || height < 0) { return false; } int shiftAmount = 8 - static_cast<int>SOFn.BitsPerSamp; int* IB_end = IB.data + height * JPEG_BLOCK_RES.U; for (int* IB_pos = IB.data; IB_pos < IB_end; IB_pos += JPEG_BLOCK_RES.U, scanline_pos += Bitmap->BytesPerLine) { for (int j = 0; j < width; j += 1) { /* To keep the original code behaviour of normalizing the input * ImageBlock as a side effect, just change 'int' to 'int&' and make * the parameter non-const. :) * The clamping to [0, 255] was entirely unnecessary, but I show below * how to do it. (It's implicit in the cast to an unsigned type that * only holds values that size.) */ int pixel = IB_pos[j]; shiftLeft(pixel, shiftAmount); /* pixel = std::max(0x00, std::min(pixel, 0xff)); */ scanline_pos[j] = static_cast<uint8>(pixel); } } return true;}
Thanks for the feedback, Zahlman.
Did you mean std::min()?
Should be JPEG_BLOCK_RES.V. It doesn't matter anyway, as JPEG_BLOCK_RES.U == JPEG_BLOCK_RES.V (always), hence the reason why I opted for a single JPEG_BLOCK_RES const to represent both.
Clamping is required because of rounding in the iDCT stage. (Btw, I do have a clamp() template function, but I haven't added to this code yet.)
LinAddr() is just a straightforward address calculation and no checking is done. So yeah, if topleft is right to the image, LinAddr() would visually wrap the pointer around to the next scanline of the bitmap.
I'll add code to test if topleft contains negative values, and fail accordingly. Something like:
Quote:
int width = std::max(JPEG_BLOCK_RES.U, Bitmap->Res.U - topleft.U);
int height = std::max(JPEG_BLOCK_RES.V, Bitmap->Res.V - topleft.V);
if (width < 0 || height < 0) { return false; }
Did you mean std::min()?
Quote:int* IB_end = IB.data + height * JPEG_BLOCK_RES.U;
Should be JPEG_BLOCK_RES.V. It doesn't matter anyway, as JPEG_BLOCK_RES.U == JPEG_BLOCK_RES.V (always), hence the reason why I opted for a single JPEG_BLOCK_RES const to represent both.
Quote:* The clamping to [0, 255] was entirely unnecessary, but I show below
* how to do it.
Clamping is required because of rounding in the iDCT stage. (Btw, I do have a clamp() template function, but I haven't added to this code yet.)
Quote:/* I added a case: the requested top-left could be to the right of the image
* but still appear "within", unless the LinAddr() member function takes
* care of that. It's possIBle that the FrameEnd check becomes redundant, if
* we do need to check the width and height explicitly.
* There are more cases that aren't considered: what if topleft is above or
* to the left of the bitmap area? */
LinAddr() is just a straightforward address calculation and no checking is done. So yeah, if topleft is right to the image, LinAddr() would visually wrap the pointer around to the next scanline of the bitmap.
I'll add code to test if topleft contains negative values, and fail accordingly. Something like:
if ((topleft.U < 0) || (topleft.V < 0)) {return false;}int width = std::min(JPEG_BLOCK_RES.U, Bitmap->Res.U - topleft.U);int height = std::min(JPEG_BLOCK_RES.V, Bitmap->Res.V - topleft.V);if (width < 0 || height < 0) { return false; }uint8* scanline_pos = Bitmap.LinAddr(topleft);... etc.
I didn't agree with Washu at first, but while looking at my code I realized that there were a lot of places where variables were abbreviated only because there were comments to explain what they were, and there were functions that had vague names but extensive documentation.
So I rewrote a lot of code this weekend to see if what he said was true, and I've come to agree 100% with Washu that comments are rarely necessary if the code was well written to begin with. Take a look:
Does this code really need any comments??
So I rewrote a lot of code this weekend to see if what he said was true, and I've come to agree 100% with Washu that comments are rarely necessary if the code was well written to begin with. Take a look:
void FrameTimer::Initialize() { if (timeBeginPeriod(TIMER_GRANULARITY) == TIMERR_NOCANDO) throw EXCEPTION(TIMER_SET_GRANULARITY_ERROR, Exception::Inform); FramesPerSecond = DEFAULT_FRAMES_PER_SECOND; SecondsPerFrame = DEFAULT_SECONDS_PER_FRAME; CurrentTickCount = TickCountLastUpdate = timeGetTime(); } void FrameTimer::Update() { TickCountLastUpdate = CurrentTickCount; CurrentTickCount = timeGetTime(); unsigned long tickCountPerFrame = CurrentTickCount - TickCountLastUpdate; SecondsPerFrame = static_cast <float>(tickCountPerFrame) * TICK_COUNT_TO_SECONDS; SecondsPerFrame = MIN(SecondsPerFrame, MAX_SECONDS_PER_FRAME); if (SecondsPerFrame != 0) FramesPerSecond = 1 / SecondsPerFrame; else FramesPerSecond = 0.0f; }Does this code really need any comments??
skittleo:
I see that you are throwing an exception when you fail to initialize your timer, but I don't know why you do this, if it is expected to fail on some systems, what I should do to fix the issue if I get this exception, etc. It might make sense to document this in the FrameTimer::Initialize() declaration as this is info someone using the interface might want to know.
Similarly, when would SecondsPerFrame ever be zero? If this happens, is this an error or is it expected behavior? Why do you set FramesPerScond to 0 in this case?
I can definitely understand what your code is doing without comments. I had to read the update function a few times to see what it was updating - skimming it, I couldn't easily distinguish between the member variables and automatic variables. It would have been quicker to understand if your Update function had a comment to the effect of 'Updates current time and frames per second'.
It is more difficult to understand why/expected conditions vs error conditions, etc.
I see that you are throwing an exception when you fail to initialize your timer, but I don't know why you do this, if it is expected to fail on some systems, what I should do to fix the issue if I get this exception, etc. It might make sense to document this in the FrameTimer::Initialize() declaration as this is info someone using the interface might want to know.
Similarly, when would SecondsPerFrame ever be zero? If this happens, is this an error or is it expected behavior? Why do you set FramesPerScond to 0 in this case?
I can definitely understand what your code is doing without comments. I had to read the update function a few times to see what it was updating - skimming it, I couldn't easily distinguish between the member variables and automatic variables. It would have been quicker to understand if your Update function had a comment to the effect of 'Updates current time and frames per second'.
It is more difficult to understand why/expected conditions vs error conditions, etc.
BrianL:
Those are some great points!
And I noticed I misused exceptions here... pardon me this is my first project with them. Even though a standard exception is thrown and it gets caught somewhere, I should be reserving exceptions only for severe cases.
Those are some great points!
And I noticed I misused exceptions here... pardon me this is my first project with them. Even though a standard exception is thrown and it gets caught somewhere, I should be reserving exceptions only for severe cases.
Quote:Original post by BrianL
Similarly, when would SecondsPerFrame ever be zero? If this happens, is this an error or is it expected behavior?
It'd be zero if no "ticks" passed between this frame and the last one. Depending upon the granularity, that might reasonably occur if nothing is being rendered on that particular frame and vsync is disabled.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
