1326989 - Favicon in tab is displayed with black areas in the corners (http://radiovesti.ru)

Status: RESOLVED WONTFIX

(Core :: Graphics: ImageLib, defect)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open url http://radiovesti.ru/special/archive
2. Look at the favicon displayed in tabs toolbar

STR_2:
1. Open url   data:text/html,<img src="http://radiovesti.ru/favicon.ico">
2. Look at the image

AR:  Corners of the image are black
ER:  Corners of the image should be transparent

Comment 1

[roxleitan]

Mozilla/5.0 (Windows NT 6.1; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID:20170103030204

Tested on Windows 7 x86 with FF Nightly 49 and FF Nightly 53.0a1 and managed to reproduce it.

Comment 2

[Timothy Nikkel (:tnikkel)]

So this favicon.ico is actually a bmp file. It has a WinBMPv5 header, with 32 bit color and RGB compression (ie no compression). And it actually has alpha data, ie it's ARGB as opposed to 0RGB in terms of the comment at

https://dxr.mozilla.org/mozilla-central/rev/a2741dd43eeae54f4dd7423bd832a761481c56ce/image/decoders/nsBMPDecoder.cpp#837

But in nsBMPDecoder::ReadInfoHeaderRest we set all 32 bmps that don't use bitfields compression as R8G8B8.

Chrome, Safari, and Preview on OS X all match each other.

So I guess we need to look for alpha data on all 32bpp bmps with RGB compression, and not just 32bpp WinBMPv3 bmps inside icos.

Nick, any of this still quick for you to recall? Does that sound reasonable?

Comment 3

[Timothy Nikkel (:tnikkel)]

Internet Explorer however treats this image as fully opaque (ignoring the alpha). If we look at the alpha in these images we render these types of bmps as intended, but maybe give alpha to some bmps that are intended to be opaque but have non-zero alpha data for some reason?

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: patch to treat these bmps as having alpha

Comment 5

[Timothy Nikkel (:tnikkel)]

Fails some tests as expected, those will need to be adjusted if we take this patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0efa1ef0bf352218c326b235111d64a9557ea8fc&selectedJob=66561095

The dom/canvas/test/test_toDataURL_alpha.html failure seems interesting though, as we draw to a canvas then convert it to data url (as a bmp) and then decode that data url. So I guess we need to change our bmp encoder too? Ugh.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8824257 [details] [diff] [review]
patch to treat these bmps as having alpha

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

The patch looks like it does the right thing to implement this change, modulo comments below.

There's no obviously right answer here. Having said that, I'm leaning against making this change for a few reasons.

- Different browsers do different things, but I favour copying Internet Explorer given that BMP is originally a Microsoft file format.

- There's a perfectly good way to do 32-bit alpha (using BITFIELDS compression combined with a non-zero alpha bitfield value), so overloading RGB in this way isn't necessary.

- It seems like this case is very rare in practice.

- If in doubt, not changing the code is easier/safer than changing it.

In BMPSuite (http://entropymine.com/jason/bmpsuite/bmpsuite/html/bmpsuite.html) I think this issue comes up as q/rgb32fakealpha.bmp. (tnikkel, you could try that test with and without your patch applied to confirm that.) Because of the lack of clarity, BMPSuite accepts both transparent and opaque renderings.

So I'm giving f- for now, but I'm happy to hear counter-arguments.

::: image/decoders/nsBMPDecoder.cpp
@@ -607,5 @@
>      // No bitfields specified; use the default 5-5-5 values.
>      mBitFields.SetR5G5B5();
> -  } else if (mH.mBpp == 32) {
> -    // No bitfields specified; use the default 8-8-8 values.
> -    mBitFields.SetR8G8B8();

Does this need to be removed? I'd be inclined to keep it. Even if mBitFields is never used, the consistency with the 16bpp case is good.

@@ +629,5 @@
>  
>    // Note that RLE-encoded BMPs might be transparent because the 'delta' mode
>    // can skip pixels and cause implicit transparency.
>    mMayHaveTransparency =
> +    mH.mBpp == 32 ||

Shouldn't this be `(mH.mCompression == RGB && mH.mBpp == 32) ||` ?

If so, that condition appears multiple times, so it would be good to encapsulate it in a function.

Comment 7

[Timothy Nikkel (:tnikkel)]

I don't have any good counter argument, so I'll just mark this wont fix for now. If we get more examples of bmp files in the wild that expect this kind of behaviour we would likely be open to re-opening this bug though.