651482 - Yellow layered Bitmaps

Status: NEW

(Core :: Graphics: ImageLib, defect)

Description

[a471747]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0

Firefox shows bitmap images with a yellow transparent layer. In the provided link, actual image is gray. But Firefox shows it wrong.



Reproducible: Always

Comment 1

[Thomas Ahlblom]

Alleged bug reproduced with new clean profile in:
Mozilla/5.0 (X11; Linux x86_64; rv:6.0a1) Gecko/20110420 Firefox/6.0a1

Chromium doesn't show the image at all.
Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.16 (KHTML, like Gecko) Chrome/10.0.648.205 Safari/534.16

The server sends the image with "Content-Type: image/jpeg":

$ wget -S http://www.mediafire.com/imgbnc.php/4d286a3355459056a5765eeabd3f0916a5cf1d28784543afcc3ef4b08b4a5ca25g.jpg
--2011-04-20 17:01:40--  http://www.mediafire.com/imgbnc.php/4d286a3355459056a5765eeabd3f0916a5cf1d28784543afcc3ef4b08b4a5ca25g.jpg
Resolving www.mediafire.com... 205.196.120.8, 205.196.120.6
Connecting to www.mediafire.com|205.196.120.8|:80... connected.
HTTP request sent, awaiting response... 
  HTTP/1.1 302 Found
  Date: Wed, 20 Apr 2011 15:01:35 GMT
  Location: http://img5.mediafire.com/4d286a3355459056a5765eeabd3f0916a5cf1d28784543afcc3ef4b08b4a5ca25g.jpg
  Vary: Accept-Encoding
  Cache-Control: private
  Content-Length: 0
  Connection: close
  Content-Type: text/html; charset=UTF-8
  Server: MediaFire
Location: http://img5.mediafire.com/4d286a3355459056a5765eeabd3f0916a5cf1d28784543afcc3ef4b08b4a5ca25g.jpg [following]
--2011-04-20 17:01:41--  http://img5.mediafire.com/4d286a3355459056a5765eeabd3f0916a5cf1d28784543afcc3ef4b08b4a5ca25g.jpg
Resolving img5.mediafire.com... 205.196.123.205
Connecting to img5.mediafire.com|205.196.123.205|:80... connected.
HTTP request sent, awaiting response... 
  HTTP/1.1 200 OK
  Server: LRBD-88
  Date: Wed, 20 Apr 2011 15:01:35 GMT
  Connection: close
  Accept-Ranges: bytes
  Content-transfer-encoding: binary
  Content-Length: 706666
  "4d286a3355459056a5765eeabd3f0916a5cf1d28784543afcc3ef4b08b4a5ca2.jpg"
  Content-Type: image/jpeg
Length: 706666 (690K) [image/jpeg]
Saving to: “4d286a3355459056a5765eeabd3f0916a5cf1d28784543afcc3ef4b08b4a5ca25g.jpg”

100%[============================================================================================================================>] 706,666      315K/s   in 2.2s    

2011-04-20 17:01:43 (315 KB/s) - “4d286a3355459056a5765eeabd3f0916a5cf1d28784543afcc3ef4b08b4a5ca25g.jpg” saved [706666/706666]


The file, with a jpg extension,  isn't recognized as a jpeg image:

$ file 4d286a3355459056a5765eeabd3f0916a5cf1d28784543afcc3ef4b08b4a5ca25g.jpg
4d286a3355459056a5765eeabd3f0916a5cf1d28784543afcc3ef4b08b4a5ca25g.jpg: data


The image starts with 42 4d, thus it's actually a bmp image.
$ hexdump -C 4d286a3355459056a5765eeabd3f0916a5cf1d28784543afcc3ef4b08b4a5ca25g.jpg |head -n 1
00000000  42 4d 6a c8 0a 00 00 00  00 00 46 00 00 00 38 00  |BMj.......F...8.|

I renamed the file to foo.bmp. When I opened the bmp-file in GIMP it was shown a greyscale image, but when I opened the bmp-file in Firefox it still had a yellow tint.

Comment 2

[Thomas Ahlblom]

Wild guesses after playing around with a hex-editor and GIMP:

I think the problem is connected either with the 56 byte BITMAPV3INFOHEADER with its mandatory alpha channel or with the color depth of 32 bits per pixel.

Comment 3

[Thomas Ahlblom]

Attachment: A 2 px by 1 px, 78 bytes bmp file. Black pixel to the left, white to the right.

I've created a minimal 32 bit BITMAPV3INFOHEADER bmp image in GIMP, just 78 bytes big.

The left pixel is black and the right one is white. The white pixel gets yellow in Firefox. Use zoom or watch the tab icon :-)

Comment 4

[Joe Drew (not getting mail)]

This should be reasonably straightforward for someone who's reasonably adept at programming in C or C++. The code in question is in modules/libpr0n/decoders/nsBMPDecoder.cpp in nsBMPDecoder::WriteInternal(). This is the function that actually interprets the data that's coming in from the disk/network/etc; it gets called with the raw data, and it creates an RGB buffer that it passes to RasterImage::AppendFrame().

Comment 5

[Thomas Ahlblom]

OK yeah, it's time for one of my wild guesses again! :D

I think the problem is that nsBMPDecoder::WriteInternal() doesn't honor the channel bit masks for 32 bit BITMAPV3INFOHEADER bmp files with the BI_BITFIELDS compression method.

In line 329 in modules/libpr0n/decoders/nsBMPDecoder.cpp the data is in XBGR order, but in line 372 it's suddenly assumed that the data is stored as BGRX.

The black pixel is stored as 00 00 00 00. 
The white pixel is stored as 00 ff ff ff.

Excerpt from line 369 and on:
------------------------------
case 32:
case 24:
   while (lpos > 0) {
     SetPixel(d, p[2], p[1], p[0]);
     p += 2;
     --lpos;
     if (mBIH.bpp == 32)
       p++; // Padding byte
     ++p;
   }
------------------------------

Thus the black pixel calls SetPixel() with RBG 00 00 00. By a coincident - no problem! But, the white pixel calls SetPixel() with RGB ff ff 00, thus the blue channel gets zeroed by the byte from the non-used alpha channel - which causes the yellow tint.

As seen above the code jumps over a padding byte afterwards, just blindly assuming that the padding comes after the RGB data.

The channel bit masks in the appended bw_but_yellow.bmp-file look as follows:
R 36h: 0000 00ff
G 3ah: 0000 ff00
B 3eh: 00ff 0000
a 42h: 0000 0000

Here the padding comes *before* the RGB data - but of course the bit masks could be arbitrary!

Firefox doesn't care anyway... :-(

Comment 6

[Joe Drew (not getting mail)]

It seems to me that we should fix that incorrect BGRX assumption!

Comment 7

[Thomas Ahlblom]

I can imagine some possible levels of ambition here.

1. Check that data is in BGRX order, with every color channel 1 byte, otherwise write an error message. Thus, the images in this bug will not be shown at all, but the log will tell us why.

2. Check if data is in either BGRX or XBGR order, with every color channel 1 byte, otherwise write an error message. Make sure to put the padding byte correctly either after or before the color data.

3. Honor the channel bit masks, but assume that the color channels have bit masks with only adjacent ones. Thus a color channel is not necessarily 8 bits wide.

4. Honor all 2^32 possible channel bit masks. That means we can even handle interleaved color channels.


Level 1 - 3 are probably mostly an issue of how much code someone would like to write. Level 2 looks attractive in my eyes, requiring let's say just 10 lines of code, but it does not solve all future problems.

Level 4 may have some performance implications since all image data bits have to be compacted before further processing.

It's hard to know what kind of images are actually out there, but I assume level 2 above will cover most of the existing 32-bit BMP images. But I'm just guessing.

Another issue is what to do if the alpha channel actually is in use.

Comment 8

[Thomas Ahlblom]

Excerpt from the biBitCount 32 bit section of http://msdn.microsoft.com/en-us/library/dd183376%28v=VS.85%29.aspx:

"When the biCompression member is BI_BITFIELDS, bits set in each DWORD mask must be contiguous and should not overlap the bits of another mask. All the bits in the pixel do not need to be used."

That's straight from the horse's mouth what I wrote as point 3 in comment 7. That also means that we don't have to care about the theoretically possible interleave described under point 4.

Comment 9

[Jae Jang]

Attachment: make use of bitfields defined in header for 32bpp BI_BITFIELDS BMPs.

32bpp BI_BITFIELDS bmps should work like 16bpp BI_BITFIELDS bmps. rgb masks are defined in the header. Existing code read and stored them into mBitFields but never actually used them for 32bpp and assumed 8-8-8 in a fixed format. Fixes related bug in CalcBitShift where if the left most bit in the 32bit unsigned int is a 1 it does not calculate the length of the bitmask correctly. Previously it incorrectly calculates mBitFields.redLeftShift as 8 on sample BMP when it should be 0, because it does not check past (<< 31) for a zero.

Comment 10

[Joe Drew (not getting mail)]

Comment on attachment 540289 [details] [diff] [review]
make use of bitfields defined in header for 32bpp BI_BITFIELDS BMPs.

Review of attachment 540289 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great. Can we get a test for this too? A reftest should be good enough: http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/README.txt

Also, can you make sure we have a test that includes a 24-bit BMP?

::: modules/libpr0n/decoders/nsBMPDecoder.cpp
@@ +117,5 @@
>          }
>      }
> +    if (pos == 32) {
> +        aLength = 32 - aBegin;
> +    }

Can you add a comment here explaining what conditions lead to us getting to this state? i.e. "if we didn't find a 1, blah blah blah" :)

@@ -379,4 @@
>                            --lpos;
> -                          if (mBIH.bpp == 32)
> -                            p++; // Padding byte
> -                          ++p;

Man this code sucked! I'm glad you're fixing it.

So I guess 24 bit bitmaps are packed tightly?

Comment 11

[Jae Jang]

Attachment: revised patch after first review. make use of bitfields defined in header for 32bpp BI_BITFIELDS BMPs.

Thanks for the review, I moved the aLength computation out of the loop so it looks more natural and self-explanatory. It should be equivalent to the previous patch in function.

         else if (started && !(aMask & (1 << pos))) {
-            aLength = pos - aBegin;
             break;
         }
     }
+    // compute number of continuous 1s (aLength)
+    aLength = pos - aBegin;
 }

I created two reftests: one for 24bpp and another for 32bpp... 24bpp is always 888 (http://upload.wikimedia.org/wikipedia/commons/d/da/AllBMPformats.png). The 32bpp test image is clearly messed up when loaded with unpatched firefox. I'm not sure if the binaries work ok through the patch... I followed the directions at https://developer.mozilla.org/en/Mercurial_FAQ.

Comment 12

[Joe Drew (not getting mail)]

Comment on attachment 540961 [details] [diff] [review]
revised patch after first review. make use of bitfields defined in header for 32bpp BI_BITFIELDS BMPs.

Excellent!

Comment 13

[Jason Orendorff [:jorendorff]]

Is this checkin-needed? What's up?

Comment 14

[Brian R. Bondy [:bbondy]]

I would prefer to rebase this patch once Bug 600556 is in.

Comment 15

[Brian R. Bondy [:bbondy]]

Attachment: Rebased original patch

I rebased this task so that it can be merged in after Bug 600556 is in.  I preserved the original author name in the patch.

The previous patch which was rebased was already in r+ state.
I re-verified that all automated reftests still pass including the new ones in this patch.

Comment 16

[Joe Drew (not getting mail)]

Comment on attachment 547414 [details] [diff] [review]
Rebased original patch

Review of attachment 547414 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 17

[Josh Matthews [:jdm]]

Was this forgotten?

Comment 18

[Brian R. Bondy [:bbondy]]

Not forgotten, there might have been a problem with it.  I have to run through try and do extra testing.  It's in queue, I'll land within 2 weeks after the all hands.

Comment 19

[Brian R. Bondy [:bbondy]]

Attachment: Rebased original patch

Rebased current patch but one of the reftests that were introduced in this patch are currently failing.  Will investigate later.

Comment 20

[Josh Matthews [:jdm]]

Brian, is this something you could mentor, or is it no longer a good first bug?

Comment 21

[Brian R. Bondy [:bbondy]]

This is something that I could spend time on and investigate to fix myself, but not something that I can give good guidance on.  

I do think it's within the realm of possibility that someone can fix it within a week though. The change is done in the patch, it probably needs to be rebased, and then it needs to be investigated independently why the test is failing.

Comment 22

[u543359]

Attachment: patch

Comment 23

[Seth Fowler [:seth] [:s2h]]

Štěpán, I can't review this without more context. Why didn't you base your patch on the existing one in this bug? Did you test against the reftests added in the previous patch? Those reftests should be in your patch too, unless you have better ones, and they need to pass.

Comment 24

[u543359]

Sorry, I haven't noticed the previous patch. And I tried to run the added reftest with the previous patch and it ran successfully.

Comment 25

[Seth Fowler [:seth] [:s2h]]

(In reply to Štěpán Horáček from comment #24)
> Sorry, I haven't noticed the previous patch.

You're saying that you posted this patch without reading the rest of this bug? That makes me very uncomfortable, Štěpán.

Please upload a new version of your patch that includes the reftests from the previous patch. Then we can verify that they pass on all platforms.

Comment 26

[u543359]

I also ran reftest on Brian's patch and it was successfull, I'm not sure if it's just Windows. His patch seems definitely better to me than mine.

Comment 27

[Brian R. Bondy [:bbondy]]

I guess it's probably best to try pushing that original patch to the try server (rebased if necessary). Maybe something was fixed at another layer (Probably by Seth!) that fixed the previously failing test.

Comment 28

[Wesley Johnston (:wesj)]

Attachment: Bug 651482 - Allow decoding bitmaps with 52 and 56 byte info headers. r=tnikkel

Comment 29

[Wesley Johnston (:wesj)]

Attachment: Bug 651482 - Allow decoding bitmaps with 52 and 56 byte info headers. r=tnikkel

Comment 30

[Wesley Johnston (:wesj)]

Attachment: Bug 651482 - Update documentation. r=tnikkel

Depends on D75467

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8cc480c563c
Allow decoding bitmaps with 52 and 56 byte info headers. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/734c4f395eba
Update documentation. r=tnikkel

Comment 32

[Alexandru Michis [:malexandru]]

Backed out 2 changesets for causing bitmap reftest failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/94667ab68f350b5a407f44a6801966077ae58891

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedTaskRun=JW6c61QQRbC0qwQbKB18zQ-0&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Crunnable&searchStr=reftest&fromchange=37e68bff6c62be9e13f75685252445bece3f421c&tochange=94667ab68f350b5a407f44a6801966077ae58891

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302694955&repo=autoland&lineNumber=3175

Comment 33

[Timothy Nikkel (:tnikkel)]

Looks like you'll have to adjust this line

https://searchfox.org/mozilla-central/rev/09b8072a543c145de2dc9bb76eddddd4a6c09adc/image/test/reftest/bmp/bmpsuite/q/reftest.list#184

because we now accept that file.

Comment 34

[Wesley Johnston (:wesj)]

Attachment: * Bug 651482 - Allow decoding bitmaps with 52 and 56 byte info headers. r=tnikkel

Depends on D75467

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40784ea31188
* Bug 651482 - Allow decoding bitmaps with 52 and 56 byte info headers. r=tnikkel

Comment 36

[Dorel Luca [:dluca]]

Backed out changeset 40784ea31188 (bug 651482) for Reftest failures in bmp-corrupted/wrapper.html?invalid-compression-BITFIELDS.bmp. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=303340758&repo=autoland&lineNumber=2574

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=40784ea31188fe5d253e017ab6d8b8b1f58e7696

Backout:
https://hg.mozilla.org/integration/autoland/rev/b08a7f6f47bc51ad21324702aa9e9ffdb2020794

Comment 37

[Wesley Johnston (:wesj)]

That failure looks like a single black pixel in the upper left corner? I can't reproduce it locally. Is that common? Do I try to land again?

I do get lots of failures when I run these reftests locally, mostly in the pal ones. I assume that's some color correction setting somewhere (but not sure why it would affect BMP and PNG differently...). Its made it difficult to run the suite locally.

Comment 38

[Timothy Nikkel (:tnikkel)]

I'll push it to try and see if it reproduces.

Comment 39

[Timothy Nikkel (:tnikkel)]

Yeah, it reproduces there.

Looking at the reftest file

https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/image/test/reftest/bmp/bmp-corrupted/reftest.list#13

and the bug that added the test

https://bugzilla.mozilla.org/show_bug.cgi?id=1238551#c19

it sounds like the file is of the type that we are added support for in this bug. So we probably want to change the expectation of that test.

Comment 40

[Timothy Nikkel (:tnikkel)]

There are two active patches
https://phabricator.services.mozilla.com/D75803
https://phabricator.services.mozilla.com/D75462
Can you determine which is current so we can retire the old one?

Comment 41

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 42

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

For more information, please visit auto_nag documentation.

Comment 43

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:wesj, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.