Closed Bug 1201796 Opened 4 years ago Closed 4 years ago

Add downscale-during-decode support for the ICO decoder

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 + fixed
firefox44 + fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, site-compat)

Attachments

(4 files, 2 obsolete files)

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.
Depends on: 1196066
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.
Attachment #8660623 - Flags: review?(tnikkel)
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).
Attachment #8660636 - Flags: review?(tnikkel)
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.
Flags: needinfo?(dolske)
Attachment #8660623 - Flags: review?(tnikkel) → review+
Attachment #8660636 - Attachment is obsolete: true
Attachment #8660636 - Flags: review?(tnikkel)
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.
Attachment #8661646 - Flags: review?(tnikkel)
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.
Attachment #8661657 - Flags: review?(tnikkel)
(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.
Attachment #8661660 - Flags: review?(tnikkel)
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.
Keywords: site-compat
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.
Attachment #8662202 - Flags: review?(tnikkel)
Attachment #8661657 - Attachment is obsolete: true
Attachment #8661657 - Flags: review?(tnikkel)
I think we're now in good shape once a frontend person takes one of these try builds for a spin.
See Also: → 1072703
Attachment #8661646 - Flags: review?(tnikkel) → review+
Attachment #8662202 - Flags: review?(tnikkel) → review+
Attachment #8661660 - Flags: review?(tnikkel) → review+
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
Flags: needinfo?(dolske)
Thanks for the reviews on this one!
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?
(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.
Flags: needinfo?(seth)
See Also: → 961893
Depends on: 1206358
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. =\
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?
Depends on: 1206651
Depends on: 1206753
Depends on: 1206673
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.
Flags: needinfo?(seth)
(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.
Flags: needinfo?(seth)
(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.)
Depends on: 1206836
I believe this is how the flags should be set at this point.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Flags: needinfo?(seth)
Resolution: --- → FIXED
Depends on: 1206973
Depends on: 1206657
Depends on: 1208715
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!
Flags: needinfo?(seth) → needinfo?(kohei.yoshino)
Does this still need to land in 43 to go along with bug 1207378? If not, let's wontfix it for 43
Flags: needinfo?(seth)
Depends on: 1210125
Depends on: 1208579
(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.
Flags: needinfo?(seth)
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.
Attachment #8660623 - Flags: approval-mozilla-aurora?
Attachment #8661646 - Flags: approval-mozilla-aurora?
Attachment #8661660 - Flags: approval-mozilla-aurora?
Attachment #8662202 - Flags: approval-mozilla-aurora?
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.
Attachment #8660623 - Flags: approval-mozilla-aurora? → approval-mozilla-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.
Attachment #8661646 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8661660 [details] [diff] [review]
(Part 4) - Add downscale-during-decode support for the ICO decoder

OK for aurora uplift.
Attachment #8661660 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8662202 [details] [diff] [review]
(Part 3) - Enable downscale-during-decode for imgITools::EncodeScaledImage()

OK for aurora uplift.
Attachment #8662202 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
setting flags for seth :)
Depends on: 1229720
Depends on: 1239392
No longer depends on: 1229720
Depends on: 1277741
Sigh. Looking...
Flags: needinfo?(edwin)
(In reply to Edwin Flores [:eflores] [:edwin] from comment #41)
> Sigh. Looking...

Totally wrong bug.
Flags: needinfo?(edwin)
Depends on: 1249578
Depends on: 1315554
Blocks: 888823
Depends on: 1327294
You need to log in before you can comment on or make changes to this bug.