1220021 - 0RGB ICO files treated as if they're transparent

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Nicholas Nethercote [inactive]]

BMPs within ICO files have this fun feature whereby:

- if all the alpha values are zero (i.e. it's 0RGB) we treat is as opaque;

- otherwise, we treat it as having an alpha channel.

However, bug 1062066 broke the first case. The BMP decoders SetPixel() uses gfxPackedPixel(), which sets the entire color to 0x0 if the alpha channel is zero. As a result, such images are shown as fully transparent.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: 0RGB firefox icon; currently renders as transparent

Here's a test case that currently renders transparent. Prior to bug 1062066 it (correctly) rendered as opaque.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: ARGB firefox icon with only one non-transparent pixel

Here's a nice counter-example to the previous test. It differs only in that one pixel in the middle of the image has an alpha value of 0xff instead of 0. This should (and does) cause us to render it as ARGB instead of 0RGB, and thus the icon is entirely transparent except for that one blue pixel.

Comment 3

[Nicholas Nethercote [inactive]]

I tried using a variant of getPackedPixel that doesn't set the RGB channels to zero if A is zero, and this fixed the 0RGB test case.

However, it also caused *4* pixels in the ARGB test case to show up (in a horizontal line) as non-transparent, instead of the 1 pixel that's supposed to show up. Not sure what's wrong there.

Comment 4

[Nicholas Nethercote [inactive]]

Seth: this bug is on Beta now and less than a month away from hitting release. You said on IRC that you would take it on. Is that still your plan?

Comment 5

[Nicholas Nethercote [inactive]]

I took another look at this.

> I tried using a variant of getPackedPixel that doesn't set the RGB channels
> to zero if A is zero, and this fixed the 0RGB test case.

This is still true, but the ARGB case has a problem: in SetPixel() we need to use the *pre-multiplied* color values. But the change above doesn't do that for fully transparent pixels; instead it stores the "straight" alpha values. And we never go back and adjust them, so the fully transparent pixels end up contributing some color instead of no color.

I think we're going to have to do something like, when we come across the first non-zero-alpha pixel, go back and reset any pixels we've seen so far, which must be fully transparent.

Comment 6

[Nicholas Nethercote [inactive]]

> I think we're going to have to do something like, when we come across the
> first non-zero-alpha pixel, go back and reset any pixels we've seen so far,
> which must be fully transparent.

That was how it was done prior to bug 1062066 strategy and I've got it working locally. Now I just need to get a reftest working.

Comment 7

[Nicholas Nethercote [inactive]]

> That was how it was done prior to bug 1062066 strategy and I've got it
> working locally.

Oh, except it's not working when downscaling is applied. E.g. when I view the image directly, the copy in the main window looks good but the downscaled version used as the tab icon is messed up. I think I understand bug 1062066 comment 5 better now:

> If it ever encounters a non-zero alpha byte, it goes back over the image
> data and rewrites all the alpha bytes. That's not great for DDD, though, since
> we're not storing the input data once we process it. It's possible to rewrite
> the already-scaled output data, but the problem is that the wrong values will
> remain in the Downscaler's "window" of rows and will be used when subsequent
> rows are downscaled.

To make this work I need a way to tell the Downscaler to go back to the start. I'm not sure if that's possible.

Comment 8

[Nicholas Nethercote [inactive]]

> To make this work I need a way to tell the Downscaler to go back to the
> start. I'm not sure if that's possible.

ResetForNextProgressivePass() seems to fit the bill...

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Don't treat 0RGB ICO files as transparent

This approach is much the same as the one we had before bug 1062066 caused the
regression.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Add two reftests

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #10)
> Created attachment 8688828 [details] [diff] [review]
> (part 2) - Add two reftests

This doesn't test the downscale case. I'm not sure how to test that. Put the image in a webpage and set the dimensions manually?

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Add four reftests

I added downscaled tests and some fuzziness on Mac that is required to pass on
try server.

Comment 13

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ef0facad3f2

I cleared the NEEDINFO request. What's needed now is a review.

Comment 14

[Nicholas Nethercote [inactive]]

Hmm, the downscaled test isn't working as I expected. I tried deliberately breaking the downscaling handling, and yet the reftest still passes. If I use the Inspector to take a screenshot of the downscaled image the output is correct... so presumably in the reftest the non-downscaled image is being used somehow for the comparison?

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Add four reftests

This enables DDD for the reftests :)  The fuzziness was needed locally for me
on Linux. I'll do a try push to see if the numbers need to be tweaked at all.

Comment 16

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

Comment on attachment 8689740 [details] [diff] [review]
(part 2) - Add four reftests

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

Looks good, just need to move these tests to the appropriate directory.

::: image/test/reftest/ico/ico-bmp-32bpp/downscale-wrapper.html
@@ +7,5 @@
> +</style>
> +<script>
> +  // The image is loaded async after the page loads
> +  // wait for it to finish loading
> +  function onImageLoad() { 

Nit: whitespace.

::: image/test/reftest/ico/ico-bmp-32bpp/reftest.list
@@ +27,5 @@
> +
> +default-preferences pref(image.downscale-during-decode.enabled,true)
> +
> +fuzzy(1,3) == downscale-wrapper.html?ff-0RGB.ico downscale-wrapper.html?ff-0RGB.png
> +fuzzy(7,6) == downscale-wrapper.html?ff-ARGB.ico downscale-wrapper.html?ff-ARGB-downscaled.png

These tests belong in image/test/reftest/downscaling. Please follow the pattern there of running identical tests with and without the DDD pref enabled.

Comment 17

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

Comment on attachment 8688827 [details] [diff] [review]
(part 1) - Don't treat 0RGB ICO files as transparent

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

Looks good. Thanks for fixing this, Nick.

Comment 18

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a5b47980443ed37150929e0a477f74aa570185c
Bug 1220021 (part 1) - Don't treat 0RGB ICO files as transparent. r=seth.

https://hg.mozilla.org/integration/mozilla-inbound/rev/37ecff04dcd1a4bddd218248c2b46c90feab5407
Bug 1220021 (part 2) - Add four reftests. r=seth.

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Add four reftests

The patch that landed.

Comment 20

[Nicholas Nethercote [inactive]]

Comment on attachment 8691718 [details] [diff] [review]
(part 2) - Add four reftests

Carrying over r+.

Comment 21

[Nicholas Nethercote [inactive]]

Comment on attachment 8688827 [details] [diff] [review]
(part 1) - Don't treat 0RGB ICO files as transparent

Approval Request Comment
[Feature/regressing bug #]: bug 1062066

[User impact if declined]: Fully-opaque favicons are rendered as fully transparent, i.e. don't show up at all.

[Describe test coverage new/current, TreeHerder]: Four new reftests were added.

[Risks and why]: Low risk. Patch is fairly small. Behaviour of the new code is similar to the pre-1062066 behaviour.

[String/UUID change made/needed]: None.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0a5b47980443
https://hg.mozilla.org/mozilla-central/rev/37ecff04dcd1

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a recent regression.
This explains why my pinned tabs look so mysterious lately!

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8688827 [details] [diff] [review]
(part 1) - Don't treat 0RGB ICO files as transparent

Fix for icon regression in 43, includes new reftests, please uplift to aurora and beta.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8691718 [details] [diff] [review]
(part 2) - Add four reftests

Adds downscaling reftests. OK to uplift to aurora and beta.

Comment 26

[Nicholas Nethercote [inactive]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #23)
> Tracking since this is a recent regression.
> This explains why my pinned tabs look so mysterious lately!

Do you have a URL for an affected site(s)? I found this regression by code inspection and then hand-constructed an ICO file that demonstrated it(!)  So I haven't actually seen the problem in the wild myself and I would be interested to do so. Thank you.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

I just restarted and updated to beta 7, and my favicons are back. The one I noticed disappearing was the google calendar icon.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9bb2dd1ff7f6
https://hg.mozilla.org/releases/mozilla-aurora/rev/77de40d65ca2

Comment 29

[Carsten Book [:Tomcat]]

this has problems during uplift to beta:

grafting 317597:9bb2dd1ff7f6 "Bug 1220021 (part 1) - Don't treat 0RGB ICO files as transparent. r=seth, a=lizzard"
merging image/decoders/nsBMPDecoder.cpp
warning: conflicts while merging image/decoders/nsBMPDecoder.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg revert -a

could you take a look, thanks!

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

I don't think this will make beta 8, since I'm about to go to build this morning. It could still make beta 9 though.

Comment 31

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Don't treat 0RGB ICO files as transparent

Seth, I had to rework this quite a bit to backport to Beta because Beta
doesn't have the big rewrite of nsBMPDecoder. It's now a mixture of (a) the
current trunk code, and (b) the old pre-bug 1062066 code.

It passes tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf0b8dbbf5d2

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/9bb2dd1ff7f6
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/77de40d65ca2

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Good that it passes tests. This could potentially still make it into the beta 9 build tomorrow morning if it gets an r+, but then we only have the weekend (while the entire company is traveling to the work week) to catch anything going wrong.   Do you think it would be annoying enough of a regression to users to be worth the risk of having to back out or do an rc2 next week?

Comment 34

[Nicholas Nethercote [inactive]]

> Do you think it would be annoying enough of a regression to users
> to be worth the risk of having to back out or do an rc2 next week?

Given that nobody has noticed it so far (as mentioned, I found the problem via code inspection) I'd be happy to not take the patch on beta and tolerate the regression for one release. Seth might have something to add.

Comment 35

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8688827 [details] [diff] [review]
(part 1) - Don't treat 0RGB ICO files as transparent

Too late for the RC build. This can ride with 44

Comment 36

[Loic]

A French user reported his website (http://www.annees-marabout.com/) has a broken favicon (black square) in FF43, but it's fixed by this bug.

Comment 37

[u279076]

(In reply to Loic from comment #36)
> A French user reported his website (http://www.annees-marabout.com/) has a
> broken favicon (black square) in FF43, but it's fixed by this bug.

Yeah, I can see that. Firefox 42 shows the favicon, Firefox 43 shows a black square, Firefox 46 shows the favicon again. Based on comment 35 this should be fixed when Firefox 44 is released, tentatively scheduled for January 26, 2016.