Open Bug 651482 Opened 10 years ago Updated 1 month ago

Yellow layered Bitmaps


(Core :: ImageLib, defect)

Not set




(Reporter: a471747, Unassigned, NeedInfo)




(Keywords: good-first-bug, Whiteboard: [exterminationweek])


(4 files, 6 obsolete files)

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
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
--2011-04-20 17:01:40--
Connecting to||:80... connected.
HTTP request sent, awaiting response... 
  HTTP/1.1 302 Found
  Date: Wed, 20 Apr 2011 15:01:35 GMT
  Vary: Accept-Encoding
  Cache-Control: private
  Content-Length: 0
  Connection: close
  Content-Type: text/html; charset=UTF-8
  Server: MediaFire
Location: [following]
--2011-04-20 17:01:41--
Connecting to||: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
  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.
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.
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 :-)
OS: Windows 7 → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Component: General → ImageLib
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → imagelib
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().
Whiteboard: [good first bug][]
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;
     if (mBIH.bpp == 32)
       p++; // Padding byte

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... :-(
It seems to me that we should fix that incorrect BGRX assumption!
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.
Excerpt from the biBitCount 32 bit section of

"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.
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.
Attachment #540289 - Flags: review?(joe)
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:

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?
Attachment #540289 - Flags: review?(joe) → review+
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;
+    // compute number of continuous 1s (aLength)
+    aLength = pos - aBegin;

I created two reftests: one for 24bpp and another for 32bpp... 24bpp is always 888 ( 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
Attachment #540289 - Attachment is obsolete: true
Attachment #540961 - Flags: review?(joe)
Comment on attachment 540961 [details] [diff] [review]
revised patch after first review. make use of bitfields defined in header for 32bpp BI_BITFIELDS BMPs.

Attachment #540961 - Flags: review?(joe) → review+
Is this checkin-needed? What's up?
I would prefer to rebase this patch once Bug 600556 is in.
Attached patch Rebased original patch (obsolete) — Splinter Review
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.
Attachment #540961 - Attachment is obsolete: true
Attachment #547414 - Flags: review?(joe)
Depends on: 600556
Comment on attachment 547414 [details] [diff] [review]
Rebased original patch

Review of attachment 547414 [details] [diff] [review]:
Attachment #547414 - Flags: review?(joe) → review+
Was this forgotten?
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.
Rebased current patch but one of the reftests that were introduced in this patch are currently failing.  Will investigate later.
Attachment #547414 - Attachment is obsolete: true
Assignee: nobody → netzen
Assignee: netzen → nobody
Brian, is this something you could mentor, or is it no longer a good first bug?
Flags: needinfo?(netzen)
Whiteboard: [good first bug][] → [good first bug]
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.
Flags: needinfo?(netzen)
Whiteboard: [good first bug] → [good first bug][exterminationweek]
Attached patch patchSplinter Review
Attachment #8631518 - Flags: review?(netzen)
Attachment #8631518 - Flags: review?(netzen) → review?(seth)
Attachment #8631518 - Flags: review?(seth)
Š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.
Flags: needinfo?(stepan.horacek)
Sorry, I haven't noticed the previous patch. And I tried to run the added reftest with the previous patch and it ran successfully.
Flags: needinfo?(stepan.horacek)
(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.
Flags: needinfo?(stepan.horacek)
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.
Flags: needinfo?(stepan.horacek)
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.
Keywords: good-first-bug
Whiteboard: [good first bug][exterminationweek] → [exterminationweek]
Assignee: nobody → wjohnston2000

Depends on D75467

Pushed by
Allow decoding bitmaps with 52 and 56 byte info headers. r=tnikkel
Update documentation. r=tnikkel
Attachment #9149869 - Attachment description: * Bug 651482 - Update documentation. r=tnikkel → * Bug 651482 - Allow decoding bitmaps with 52 and 56 byte info headers. r=tnikkel
Attachment #9149237 - Attachment is obsolete: true
Attachment #9149245 - Attachment is obsolete: true
Attachment #9149663 - Attachment is obsolete: true
Pushed by
* Bug 651482 - Allow decoding bitmaps with 52 and 56 byte info headers. r=tnikkel

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.

Flags: needinfo?(wjohnston2000) → needinfo?(tnikkel)

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

Yeah, it reproduces there.

Looking at the reftest file

and the bug that added the test

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.

Flags: needinfo?(tnikkel) → needinfo?(wjohnston2000)

There are two active patches
Can you determine which is current so we can retire the old one?

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

Assignee: wjohnston2000 → nobody
You need to log in before you can comment on or make changes to this bug.