334144 - Handling of alpha bits in nsBMPDecoder don't make sense in Cairo terms

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(See also bug 331671 for ICO issues.)

Comment 3

[Stuart Parmenter]

fixed by 332386

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 5

[Stuart Parmenter]

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...

Comment 6

[Alfred Kayser]

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)

Comment 7

[Alfred Kayser]

Attachment: RLE encoded bitmaps going wrong

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)

Comment 8

[Alfred Kayser]

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

Comment 9

[Alfred Kayser]

Attachment: V3: Do the right thing for BMP/ICO decoding in 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

Comment 10

[Alfred Kayser]

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

Comment 11

[Alfred Kayser]

Attachment: V4: Also update Makefile.in for Cairo/Thebes

Comment 12

[Alfred Kayser]

Attachment: V4: The right version

Comment 13

[tor]

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.

Comment 14

[Alfred Kayser]

Attachment: V5: addressing comments from T Rowley.

Comment 15

[Alfred Kayser]

Attachment: V6: Really handle alpha for 32bit ICO's in the right way

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 16

[tor]

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.

Comment 17

[Alfred Kayser]

Attachment: V7: Fixed comments from Tor (2 out of 3)

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 18

[tor]

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

Comment 19

[Alfred Kayser]

Attachment: V8: Fixed nits in comments

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.

Comment 20

[Alfred Kayser]

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 21

[tor]

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 22

[Alfred Kayser]

Attachment: V9: Updated to trunk, and included patch for build/Makefile.in

Comment 23

[tor]

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.

Comment 24

[Alfred Kayser]

Attachment: V10: 'cairo' removed from Makefile.in, ready for checkin

Comment 25

[Alfred Kayser]

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

Comment 26

[Nickolay_Ponomarev]

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

Comment 27

[Nickolay_Ponomarev]

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?

Comment 28

[Alfred Kayser]

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