Seth Fowler [:seth] [:s2h] Assignee
	Description • 9 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 1 • 9 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 2 • 9 years ago

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).

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 3 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a5275ccde8b

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 4 • 9 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 5 • 9 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 6 • 9 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 7 • 9 years ago

(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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 8 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e806bca03b34

	Matthew N. [:MattN]
	Comment 9 • 9 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 10 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba34fa6bd968

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 11 • 9 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 12 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 13 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 14 • 9 years ago

Thanks for the reviews on this one!

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 15 • 9 years ago

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

	Emanuel Hoogeveen [:ehoogeveen]
	Comment 16 • 9 years ago

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?

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 18 • 9 years ago

(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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 19 • 9 years ago

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. =\

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 20 • 9 years ago

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

	Phil Ringnalda (:philor)
	Comment 21 • 9 years ago

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

	Krzysztof
	Comment 22 • 9 years ago

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?

	Robert Kaiser
	Comment 23 • 9 years ago

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).

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 24 • 9 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 25 • 9 years ago

(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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 26 • 9 years ago

(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.)

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 27 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 28 • 9 years ago

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

	Kohei Yoshino [:kohei]
	Comment 29 • 9 years ago

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/

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 30 • 9 years ago

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!

	Kohei Yoshino [:kohei]
	Comment 31 • 9 years ago

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/

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 32 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 33 • 9 years ago

(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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 34 • 9 years ago

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.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 35 • 9 years ago

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.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 36 • 9 years ago

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

OK for aurora uplift.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 37 • 9 years ago

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

OK for aurora uplift.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 38 • 9 years ago

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

OK for aurora uplift.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 39 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 40 • 9 years ago

setting flags for seth :)

	Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]
	Comment 41 • 8 years ago

Sigh. Looking...

	Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]
	Comment 42 • 8 years ago

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

Totally wrong bug.