185195 - cleanup BMP Decoder

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

Now that there's a member variable mDecoded, mRow is no longer needed and can be
eliminated.

there's probably other stuff too.

Comment 1

[[e-mail]]

I have no problem with the idea of incrementally decoding but I don't see how to
handle e.g. DWORD padding, or split multi-byte colour data.
Other cleanup: rename mCurLine to mLastLine (or something).

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

well... I decided to not do what comment 0 suggested - doing that made the code
more unreadable, and only saved 300 bytes codesize, that was not worth it.

however, I found other cleanup I could do, which is contained in this patch

Comment 3

[[e-mail]]

Comment on attachment 134138 [details] [diff] [review]
patch

I can't speak for the Close() changes.

>-        byte = 0;
>+        mAlpha[cnt] = 0;
>         for (bit = 128; bit; bit >>= 1)
>-            byte |= *pos++ & bit;
>-        mAlpha[cnt] = byte;
>+            mAlpha[cnt] |= *pos++ & bit;
Do the 8 extra memory reads and writes optimize away?

>+            mNumColors = 1 << mBIH.bpp;
>             if (mBIH.colors)
>                 mNumColors = mBIH.colors;
Perhaps mNumColors = mBIH.colors ? mBIH.colors : 1 << mBIH.bpp;

>-    else if (aCount && mBIH.compression == BI_BITFIELDS && mPos < (mLOH + 12)) {
>-        PRUint32 toCopy = (mLOH + 12) - mPos;
>+    else if (aCount && mBIH.compression == BI_BITFIELDS && mPos < (mLOH + BITFIELD_LENGTH)) {
>+        PRUint32 toCopy = (mLOH + BITFIELD_LENGTH) - mPos;

>-    if (mBIH.compression == BI_BITFIELDS && mPos == mLOH+12) {
>+    if (mBIH.compression == BI_BITFIELDS && mPos == mLOH + BITFIELD_LENGTH) {
It occurs to me that only Windows bitmaps support compression, so mLOH is
always WIN_HEADER_LENGTH here...

>+#define BFH_LENGTH 18 // Note: For our purposes, we include bihsize in the BFH
Perhaps sizeof(BMPFILEHEADER) instead?

>+#define WIN_HEADER_LENGTH (BFH_LENGTH + 36)
Perhaps use sizeof(BMPINFOHEADER) instead of 36?

>+#define BITFIELD_LENGTH 12 // Length of the bitfields structure in the bmp file
Perhaps use sizeof(bitFields) instead?

>-// enums for mState
>+/// enums for mState
So, what's behind these comment changes?

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

>I can't speak for the Close() changes.

oh, I basically just decided that I don't need to set some variables to null;
and moved other stuff to the destructor.

>Do the 8 extra memory reads and writes optimize away?

No idea :) I can't read assembler... would you prefer it if I wouldn't change that?

>Perhaps sizeof(BMPFILEHEADER) instead?
>Perhaps use sizeof(BMPINFOHEADER) instead of 36?
>Perhaps use sizeof(bitFields) instead?

I believe the compiler is free to insert any padding into these structures that
he wants. so I can't trust sizeof() to give the sum of the member sizes.

>So, what's behind these comment changes?

Doxygen can automatically generate documentation from such comments (see
http://unstable.elemental.com/mozilla/)

I'll make a new patch addressing the other comments

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 134340 [details] [diff] [review]
patch v2

doesn't address your "8 read/write" question. tell me if you want that chagne
undone

Comment 7

[[e-mail]]

Comment on attachment 134340 [details] [diff] [review]
patch v2

>-    rv = mFrame->GetAlphaBytesPerRow(&alpha);
>+    nsresult rv = mFrame->GetAlphaBytesPerRow(&alpha);
>     NS_ENSURE_SUCCESS(rv, rv);
>     for (cnt = 0; cnt < alpha; cnt++) {
>-        byte = 0;
>+        mAlpha[cnt] = 0;
>         for (bit = 128; bit; bit >>= 1)
>-            byte |= *pos++ & bit;
>-        mAlpha[cnt] = byte;
>+            mAlpha[cnt] |= *pos++ & bit;
>     }
OK, so I wrote a test program, and my way mAlpha isn't stored in a register,
and your way mAlpha[cnt] isn't stored in a register. I then modified the test
and was able to get everything in registers. It was only then that I spotted
the "aliasing" - mAlpha[0] is in fact *pos, so you clobber the first bit your
way :-( FYI here is the register code:
PRUint8 *src = mAlpha, *dest = mAlpha;
while (alpha) {
  byte = 0;
  for (bit = 128; bit; bit >>= 1)
    byte |= *src++ & bit;
  *dest++ = byte;
  alpha--;
}
-O1 Registers: al: *src; bl: byte ecx: src dl: bit esi: alpha edi: dest
(-O2 is worse, puts byte in esi! Ideally alpha belongs in ecx.)

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v3

ok, I just undid that change

Comment 9

[[e-mail]]

Comment on attachment 136116 [details] [diff] [review]
patch v3

>+#define BFH_LENGTH 18 // Note: For our purposes, we include bihsize in the BFH
>+
>+#define OS2_BIH_LENGTH 12 // This is the real BIH size (as contained in the bihsize field of BMPFILEHEADER)
>+#define OS2_HEADER_LENGTH (BFH_LENGTH + 8)
>+#define WIN_HEADER_LENGTH (BFH_LENGTH + 36)
I don't like these meaningless additions - why not just say 26 and 54, and
comment that they are the sum of the file header and the appropriate info
header?

Comment 10

[[e-mail]]

Comment on attachment 136116 [details] [diff] [review]
patch v3

> nsBMPDecoder::nsBMPDecoder()
> {
>     mColors = nsnull;
>     mRow = nsnull;
>     mPos = mNumColors = mRowBytes = 0;
>     mCurLine = 1; // Otherwise decoder will never start
>     mState = eRLEStateInitial;
>     mStateData = 0;
>     mAlpha = mAlphaPtr = mDecoded = mDecoding = nsnull;
>+    mLOH = WIN_HEADER_LENGTH;
> }
For extra brownie points, use
: mLOH(WIN_HEADER_LENGTH) etc.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 136116 [details] [diff] [review]
patch v3

ok, I'll make those changes before checking in

Comment 12

[tor]

Comment on attachment 136116 [details] [diff] [review]
patch v3

Patch leaks mRow.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: don't leak mRow

Comment 14

[tor]

Comment on attachment 139270 [details] [diff] [review]
don't leak mRow

A bit odd you're using C-style allocation for two of the temporary
arrays and C++ for other two, but can be cleaned up later if you
like.  sr=tor

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in

I use C-style allocations to be able to use calloc.