The cairo ifdefs in nsBMPDecoder, added in revision 1.30 for bug 331298, don't make any sense to me. As far as I can tell, what they do is (for RLE images with alpha, which I don't know where to find examples of) initialize the alpha component of one out of every 8 pixels to either 0x00 or 0xff, depending on whether all 8 adjacent pixels have some non-zero opacity in them -- and then leaving the other 7 adjacent pixels uninitialized (although presumably to 0xff, thanks to SetPixel).

Actually, sorry, it's not spreading the 8 pixels of alpha out; it's compressing it.

(See also bug 331671 for ICO issues.)

fixed by 332386

No, the wacky code's still there, all that bug did was change (cnt * 4) to (cnt << 2). I'm referring to: for (cnt = 0; cnt < abpr; cnt++) { PRUint8 byte = 0; for (bit = 128; bit; bit >>= 1) byte |= *pos++ & bit; mAlpha[cnt] = byte; #ifdef MOZ_CAIRO_GFX #ifdef IS_LITTLE_ENDIAN mDecoded[(cnt << 2) + 3] = byte ? 0 : 255; #else mDecoded[(cnt << 2)] = byte ? 0 : 255; #endif #endif } That said, I'm also not sure if the existing code to construct |byte| makes a whole lot of sense.

yeah the code is still a mess but it does the right thing now. the key is that abpr is different with that patch than it was before. i'd really like to see someone rewrite the whole bmp decoder now that we handle alpha very differently, but until that happens...

Let me take this up. I have a more or less working patch for the ICO/BMP decoders, removing all this alpha buffer compression/decompression stuff. By the way, do we need to keep the non-Cairo stuff around? (I have seen in bug 367273 that the ifdef MOZ_CAIRO_GFX are being removed, so that code only compiles with Cairo enabled)

Examples of RLE encoded BMP image not correctly decoded/displayed in current nightly builds (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070118 Minefield/3.0a2pre)

Changing subject. ifdefs are gone, but the actual issue: handling of transparancy does not make sense to Cairo...

Short overview of the patch: * Fix the alpha bits handling in BMP decoder * Directly write to Cairo image data, removing mAlpha and mDecodedBuffer * Removes SetData and WriteRLERows (no longer needed) * Use PRUint32* pointers for image data * As the BMP RLE decoder used mAlphaPtr to keep position info, replace that with mCurPos

Note, this also fixes the issues with RLE encoded images (see the test set in attachment #1 [details] [diff] [review])

Comment on attachment 269812 [details] [diff] [review] V4: The right version > /** Sets the pixel data in aDecoded to the given values. > * The variable passed in as aDecoded will be moved on 3 or 4 bytes! */ >-inline void SetPixel(PRUint8*& aDecoded, PRUint8 aRed, PRUint8 aGreen, PRUint8 aBlue, PRUint8 aAlpha = 0xFF) >+inline void SetPixel(PRUint32*& aDecoded, PRUint8 aRed, PRUint8 aGreen, PRUint8 aBlue, PRUint8 aAlpha = 0xFF) > { >- *(PRUint32*)aDecoded = (aAlpha << 24) | (aRed << 16) | (aGreen << 8) | aBlue; >- aDecoded += 4; >+ *aDecoded++ = (aAlpha << 24) | (aRed << 16) | (aGreen << 8) | aBlue; > } Shouldn't this use GFX_PACKED_PIXEL to do the premultiply if needed? Comment for this and Set4BitPixel should indicate that the pointer is moved 4 and 8 bytes respectively.

I knew there was a reason not to use GFX_PACKED_PIXEL. For 32bit ICO's the alpha byte is not always used. We can only detect that during the image parsing the alpha byte is used. So, for 32bit ICO's where alpha byte is not used we have to set it to 0xFF, but as soon as we find out that the alpha byte is used, we have to clear that assumption and clear the previous pixels. By using this 'working assumption' we prevent that we have to 'premultiply' afterwards the whole image, and we enable that the official 'GFX_PACKED_PIXEL' macro can be used.

Comment on attachment 270699 [details] [diff] [review] V6: Really handle alpha for 32bit ICO's in the right way >--- modules/libpr0n/decoders/bmp/Makefile.in 9 Dec 2004 19:28:21 -0000 1.12 >+++ modules/libpr0n/decoders/bmp/Makefile.in 3 Jul 2007 07:21:35 -0000 > REQUIRES = xpcom \ >+ string \ > gfx \ >+ thebes \ >+ cairo \ cairo isn't needed as a requirement. >--- modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp 27 Feb 2007 21:13:23 -0000 1.33 >+++ modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp 3 Jul 2007 07:21:36 -0000 >+#include "nsIImage.h" >+#include "nsIInterfaceRequestor.h" >+#include "nsIInterfaceRequestorUtils.h" >@@ -634,22 +568,36 @@ NS_METHOD nsBMPDecoder::ProcessData(cons >+ // Tell the image that it's data has been updated >+ nsCOMPtr<nsIImage> img(do_GetInterface(mFrame)); If you use "nsCOMPtr<nsIImage> img = do_QueryInterface(mFrame)" you don't need the includes of nsIInterfaceRequestor and nsIInterfaceRequestorUtils. nsICODecoder::ProcessData doesn't build with this patch because mDecodedBuffer no longer exists.

Removed the 'cairo' part of the Makefile.in Fixed the mDecodedBuffer part in nsICODecoder.cpp (simplifying the transparancy mask decoding) Note, the do_GetInterface is required, as gfxIImageFrame doesn't support the nsIImage interface directly (via QueryI), but only through GetInterface(). (see also bug 216682).

Comment on attachment 271116 [details] [diff] [review] V7: Fixed comments from Tor (2 out of 3) >--- modules/libpr0n/decoders/bmp/nsBMPDecoder.h 27 Feb 2007 21:13:24 -0000 1.26 >+++ modules/libpr0n/decoders/bmp/nsBMPDecoder.h 5 Jul 2007 20:16:47 -0000 >+ PRInt32 mOldLine; ///< Previoius index of the line nit: "Previous" > /** Sets the pixel data in aDecoded to the given values. > * The variable passed in as aDecoded will be moved on 3 or 4 bytes! */ >-inline void SetPixel(PRUint8*& aDecoded, PRUint8 aRed, PRUint8 aGreen, PRUint8 aBlue, PRUint8 aAlpha = 0xFF) >+static inline void SetPixel(PRUint32*& aDecoded, PRUint8 aRed, PRUint8 aGreen, PRUint8 aBlue, PRUint8 aAlpha = 0xFF) nit: Fix comment. This patch also fails to build: In file included from /home/tor/moz/trunk/mozilla/modules/libpr0n/build/nsImageModule.cpp:69: /home/tor/moz/trunk/mozilla/modules/libpr0n/build/../decoders/bmp/nsBMPDecoder.h:47:22: error: gfxColor.h: No such file or directory /home/tor/moz/trunk/mozilla/modules/libpr0n/build/../decoders/bmp/nsBMPDecoder.h: In function ‘void SetPixel(PRUint32*&, PRUint8, PRUint8, PRUint8, PRUint8)’: /home/tor/moz/trunk/mozilla/modules/libpr0n/build/../decoders/bmp/nsBMPDecoder.h:213: error: ‘GFX_PACKED_PIXEL’ was not declared in this scope

Re 'won't compile' comment: As the Makefile.in as changed, one has to remove the Makefile in the obj directory of modules/libpr0n/decoders/bmp (or something like that), so that it includes the changes from Makefile.in.

A test set of BMP images, including RLE encoded images (RLE's are very rare). Note, the zip is large (±41 MB) because of two very large versions of Sarah (RLE encoded and plain encoded). http://home.unet.nl/alfredkayser/bmp.zip

Comment on attachment 271836 [details] [diff] [review] V8: Fixed nits in comments This still won't build because you're missing the needed change to modules/libpr0n/build/Makefile.in. Patch also needs to be updated to the tip.

Comment on attachment 271885 [details] [diff] [review] V9: Updated to trunk, and included patch for build/Makefile.in >--- modules/libpr0n/build/Makefile.in 24 Apr 2006 16:30:30 -0000 1.14 >+++ modules/libpr0n/build/Makefile.in 11 Jul 2007 19:25:09 -0000 >@@ -49,16 +49,18 @@ IS_COMPONENT = 1 > MODULE_NAME = nsImageLib2Module > GRE_MODULE = 1 > LIBXUL_LIBRARY = 1 > > PACKAGE_FILE = imglib2.pkg > > REQUIRES = xpcom \ > string \ >+ thebes \ >+ cairo \ Cairo isn't a necessary requirement. sr=tor with that removed.

Who can do the checkin for me? Thanks in advance!

Please use the checkin-needed keyword to request check-ins now: http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree

Checked in: mozilla/modules/libpr0n/build/Makefile.in 1.15 mozilla/modules/libpr0n/decoders/bmp/Makefile.in 1.13 mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp 1.35 mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.h 1.27 mozilla/modules/libpr0n/decoders/bmp/nsICODecoder.cpp 1.45 mozilla/modules/libpr0n/decoders/bmp/nsICODecoder.h 1.16 Can this be auto-tested?

Verified with the example images in attachment #1 [details] [diff] [review] that this bug indeed now fixes the decoding issues.