We need downscale-during-decode support for the ICO decoder. The case of the ICO decoder is special in that we not only need to downscale the output of the decoder, but also choose the appropriate resolution layer based upon the target size. This will enable the removal of the -moz-resolution media fragment which is currently used to manually select a resolution layer.

A preliminary patch: it turns out that Blink does not render ICOs which have wrong widths and heights in their ICO directory entries. If the ICO directory entry doesn't match the actual resource's size, Blink considers the image corrupt. Since that's the case, we'll start doing the same thing, because to maintain support for this particular type of corrupt ICO would massively complicate part 2. This patch updates the code to enforce this requirement, and it converts the reftests for this case into crashtests.

This patch actually adds downscale-during-decode support for the ICO decoder. Since the actual downscaling is taken care of by the contained decoder, there's really not that much to do. The main important change is in how we decide which resource to decode. - We always use the largest resource (subject to the requirement that we try not to reduce the color depth to increase size) as the intrinsic size of the ICO. That's a necessity, since we only redecode to downscale, and not to upscale; if we used a smaller resource as the intrinsic size, we'd never be able to take advantage of any larger resources. - If we're doing a full decode and not downscaling, we again use the largest resource. That keeps the behavior of the ICO decoder in line with the other image decoders, which always produce a surface of the same size as their intrinsic size when doing a non-downscaling full decode. - If we're doing a full decode and we *are* downscaling, we instead try to match the target size we're downscaling to as closely as possible, using the same algorithm we used in the past to match the size chosen by #-moz-resolution. If we match the target size perfectly, we don't need to downscale at all and we go ahead and get rid of the downscaler. It was necessary to update several of the reftests because of this change. In all cases this was because the reftest compared an ICO file containing several resolutions with a PNG file with (obviously) only one resolution, and which resolution we chose from the ICO file had changed. The other main tweak is that we always have to fire PostHasTransparency(), because it's perfectly possible that some of the resources are transparent and others aren't, and without doing a metadata decode for every resource, we have no other option. It's also worth talking about where this leaves #-moz-resolution. After this patch lands, #-moz-resolution will no longer actually do anything, because it's not taken into account in the resource selection algorithm anymore. I considered retaining support for it and then removing it in bug 1118926, but the necessary changes ended up making the code significantly harder to follow. Since the plan was to remove #-moz-resolution immediately after this lands anyway, what I'm going to do instead is to remove it in this patch and make the frontend folks aware of that so I can get their feedback. Bug 1118926 can serve to remove all the remnants of #-moz-resolution in various places in the code (of which there are quite a few).

Justin, the patches in this bug disable #-moz-resolution as a side effect, so it'd be good to verify that there are no bad effects on the UI before we land this. Assuming nothing goes wrong, the try job in comment 3 should have builds for all our major platforms soon; I'd really appreciate it if you or someone else from the frontend team could take one of them for a quick spin and make sure nothing got busted.

So say we have a favicon with a 16x16 resource and a 64x64 resource. With the nsICODecoder code before this bug, the intrinsic size of this favicon would be 16x16, because that code always tried to select the resource closest to PREFICONSIZE x PREFICONSIZE in size, and PREFICONSIZE was 16. When the favicon service wants to re-encode an ICO as a PNG for storage in the favicon database, it uses imgITools::EncodeScaledImage(). imgITools::EncodeScaledImage() calls imgIContainer::GetFrame(), which will return a frame at the intrinsic size of the image - in the case of our favicon, 16x16. That means that we'll use the 16x16 resource of the favicon and we won't have to perform any scaling. On the other hand, after this bug, the same favicon would have an intrinsic size of 64x64, because we'll now always prefer the resource with the *largest* size. That means that imgITools::EncodeScaledImage(), calling GetFrame(), will end up using the 64x64 resource embedded in our favicon, and then scaling it down linearly to produce the 16x16 outupt that the favicon service requested. That's not great. We want to use the 16x16 resource when it's available. So, how do we do that? We need a downscale-during-decode-enabled version of GetFrame()! This patch adds GetFrameAtSize(), which we'll use in part 3 to make imgITools::EncodeScaledImage() support downscale-during-decode.

This patch actually makes EncodeScaledImage() support downscale-during-decode. To make this useful for the favicon service, imgITools::DecodeImage() also needs to return an image that was created with INIT_FLAG_DOWNSCALE_DURING_DECODE, so this patch also updates ImageFactory::CreateAnonymousImage() to do that.

(Part 2 has been renumbered to part 4.) This updated version of the patch fixes the test failures encountered on the last try job. There was one change necessary to the actual implementation: we need to report the hotspot during the metadata decode. I also updated a test in test_imgtools.js because we now detect certain errors during the full decode rather than the metadata decode, since we no longer even inspect the resources in the ICO during the metadata decode. The remaining failures in the try job above were fixed by parts 2 and 3.

Adding site-compat based on IRC discussion that this patch will affect the intrinsic size of ICO images with different sized frames. See comment 2.

The try job above contained one remaining failure, which resulted from the fact that I didn't update the reference for one of the tests in test_imgtools.js. I've updated it in this version of the patch. Otherwise this is identical to the last version.

I think we're now in good shape once a frontend person takes one of these try builds for a spin.

Looks like things appear to be ok: > <@dolske> seth: I gave it a spin on Windows a day or two ago, and didn't see any issues

Thanks for the reviews on this one!

https://hg.mozilla.org/integration/mozilla-inbound/rev/d58149411b7d https://hg.mozilla.org/integration/mozilla-inbound/rev/494e7553d641 https://hg.mozilla.org/integration/mozilla-inbound/rev/958988218976 https://hg.mozilla.org/integration/mozilla-inbound/rev/159d5d2946d3

I imagine this will mostly impact the display of favicons in tab labels and next to bookmarks, is that right? Do you expect this to affect tab rendering performance?

Backed out along with the other bug from that push in https://hg.mozilla.org/integration/mozilla-inbound/rev/9244da13f5e8 for mulet gij28 bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=14348078&repo=mozilla-inbound

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #16) > I imagine this will mostly impact the display of favicons in tab labels and > next to bookmarks, is that right? Do you expect this to affect tab rendering > performance? It will affect anywhere we display icons in the UI. It should improve tab rendering performance, since DDD is more efficient than our old approach of scaling at draw time. The benefits are lower than they otherwise would be because the favicon service stores all icons at 16x16, meaning we still end up upscaling (especially on retina devices) when we could have avoided it. We should change that policy, but that's separate from this bug.

Looks like this won't be able to land until bug 1206358, which temporarily disables a Gaia test that explicitly checks an ICO file's intrinsic size, has landed. =\

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ce4da000710 https://hg.mozilla.org/integration/mozilla-inbound/rev/7f20c039c994 https://hg.mozilla.org/integration/mozilla-inbound/rev/3685c9cadbed https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f5f19feef1

https://hg.mozilla.org/mozilla-central/rev/4ce4da000710 https://hg.mozilla.org/mozilla-central/rev/7f20c039c994 https://hg.mozilla.org/mozilla-central/rev/3685c9cadbed https://hg.mozilla.org/mozilla-central/rev/c5f5f19feef1

Could you check the site http://www.racjonalista.pl? I've just noticed that something's wrong with it's favicon. Could it be connected with this patch?

This seems to have caused pretty nasty crashes in different places as indicated by bug 1206753 and bug 1206673, I think we need to back this out on both nightly (44) and aurora (43).

Seth can you take a look? It sounds like these crashes should block release of 43 aurora. Backing out your patch may be the best option since we're at the last minute here.

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #24) > Seth can you take a look? It sounds like these crashes should block release > of 43 aurora. Backing out your patch may be the best option since we're at > the last minute here. I think our best bet is probably to back this bug and bug 1146663 out from Aurora. I'm investigating the problem now, and we may well be able to fix this easily on Nightly, but I don't want to delay the Aurora release over it.

(To be clear: we should back out immediately from Aurora, but let's hold off on a Nightly back out until later in the day.)

Backout: https://hg.mozilla.org/releases/mozilla-aurora/rev/577c248da8de

I believe this is how the flags should be set at this point.

Posted the site compatibility doc. Reviews are welcome. https://www.fxsitecompat.com/en-US/docs/2015/ico-images-now-use-the-largest-frame-as-the-intrinsic-dimention/

Kohei, looks good, but there are some small terminology nits: please edit the post to use the term "largest resource" instead of "largest frame", and "intrinsic size" rather than "intrinsic dimension". Thanks!

Thanks Seth! Updated the doc accordingly: https://www.fxsitecompat.com/en-US/docs/2015/ico-images-now-use-the-largest-resource-as-the-intrinsic-size/

Does this still need to land in 43 to go along with bug 1207378? If not, let's wontfix it for 43

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #32) > Does this still need to land in 43 to go along with bug 1207378? If not, > let's wontfix it for 43 It looks like this is OK to uplift at this point, as long as we uplift bug 1206836 too. Looks like all the crashes associated with this patch got resolved by bug 1206836. It'd be highly preferable to land this back in 43, because removing it forced us to back out bug 1146663 from 43 as well, and that patch has significant perf benefits. I'll go ahead and request uplift for this bug and bug 1206836 now.

Comment on attachment 8660623 [details] [diff] [review] (Part 1) - Treat ICOs with wrong widths and heights as corrupt Approval Request Comment [Feature/regressing bug #]: To support bug 1146663, which transitions us fully from the old "high-quality" downscaling code to downscale-during-decode. We measured a 96% reduction in time spent drawing images when painting the about:newtab page from making this change, so it'd be nice to get bug 1146663 into 43 if we can and ship those perf improvements. [User impact if declined]: Can't uplift bug 1146663, which means we lose out on those performance gains for drawing large images, and in particular we lose out on about:newtab. [Describe test coverage new/current, TreeHerder]: On m-c for weeks now, with tests. (And we're working to add more tests.) [Risks and why]: Not expected to be high risk as the code has been on m-c for so long, and the regression it caused appears to be long since fixed. [String/UUID change made/needed]: None.

Comment on attachment 8660623 [details] [diff] [review] (Part 1) - Treat ICOs with wrong widths and heights as corrupt Fix for recent regression, should have test coverage. We should uplift parts 1-4 to aurora.

Comment on attachment 8661646 [details] [diff] [review] (Part 2) - Add GetFrameAtSize() to support downscale-during-decode for GetFrame() use cases OK for aurora uplift.

Comment on attachment 8661660 [details] [diff] [review] (Part 4) - Add downscale-during-decode support for the ICO decoder OK for aurora uplift.

Comment on attachment 8662202 [details] [diff] [review] (Part 3) - Enable downscale-during-decode for imgITools::EncodeScaledImage() OK for aurora uplift.

Uplifted: https://hg.mozilla.org/releases/mozilla-aurora/rev/5d3c30fb455c https://hg.mozilla.org/releases/mozilla-aurora/rev/c09ed18c70a7 https://hg.mozilla.org/releases/mozilla-aurora/rev/77d98a497812 https://hg.mozilla.org/releases/mozilla-aurora/rev/99e1eba44836

setting flags for seth :)

Sigh. Looking...

(In reply to Edwin Flores [:eflores] [:edwin] from comment #41) > Sigh. Looking... Totally wrong bug.