1146663 - Remove ImageLib's HQ scaling code, which has been replaced by downscale-during-decode

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

We should remove the "HQ scaling" code from ImageLib, since downscale-during-decode replaces it.

With it removed, we may also be able to remove imgFrame::Deoptimize, since I'm not sure that there are any other important users of it.

Comment 1

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

This seems to be a recent regression from 36, from bug 1144983. Is this a priority for 39? Should I be tracking this bug for 39 and 40 as well?

Comment 2

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

(In reply to Liz Henry (:lizzard) from comment #1)
> This seems to be a recent regression from 36, from bug 1144983. Is this a
> priority for 39? Should I be tracking this bug for 39 and 40 as well?

Any fix would not be upliftable. It's my goal to get this done during this quarter, but I can't guarantee this will land in Firefox 40. The problem is mainly that downscale-during-decode is not currently implemented for most image types - right now it only supports JPEGs. (That's why bug 1045926 is not yet resolved.) The image in bug 1144983 is a PNG, so we'd need PNG support to resolve that bug.

Comment 3

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

Attachment: (Part 1) - Remove HQ scaling, which is now dead code

At this point the HQ scaling code is dead code, so let's remove it.

Comment 4

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

Attachment: (Part 2) - Remove the concept of lifetimes from the SurfaceCache

After removing HQ scaling, there's nothing remaining that uses
Lifetime::Transient. (Except placeholders, but that's just because we had to
pick *some* lifetime for them. Regardless of their lifetime, placeholders don't
get locked.) That means the only possible lifetime is Lifetime::Persistent, so
there's no point in maintaining the lifetime concept at all. Let's remove that
idea from the SurfaceCache.

Comment 5

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

Attachment: (Part 3) - Make it impossible to deoptimize imgFrames

At this point there's no reason to ever deoptimize an imgFrame once it's been
optimized, so let's remove imgFrame::Deoptimize() totally.

Comment 6

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

Attachment: (Part 4) - Make all RasterImages support downscale-during-decode

Since downscale-during-decode is supported by all content types and is the
normal state now, we can go ahead and stop deciding whether RasterImages should
support it in ImageFactory. Instead, we can verify that the DDD pref is enabled,
and that the image isn't transient, in RasterImage::CanDownscaleDuringDecode().

Comment 7

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

Attachment: (Part 5) - Require that all image decoders support downscale-during-decode

Now that all image decoders suppor downscale-during-decode, we can make that a
requirement. That means that the downscaler can live on Decoder itself, and that
SetTargetSize() doesn't need to be virtual anymore.

Comment 8

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

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

Comment 9

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

With these patches combined, 562 fewer lines!

Comment 10

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

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

Comment 11

[Daniel Holbert [:dholbert]]

[Adding dependency on bug 1060869, which is where SurfaceCache lifetimes were introduced.]

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8662803 [details] [diff] [review]
(Part 2) - Remove the concept of lifetimes from the SurfaceCache

Nit: I found just one comment that still mentions "persistent" (in the lifetime sense) -- I think that word wants to be removed in this patch. It's in the documentation for OnSurfaceDiscarded, in Image.h:
220    * Called when the SurfaceCache discards a persistent surface belonging to
221    * this image.
222    */
223   virtual void OnSurfaceDiscarded() = 0;
http://mxr.mozilla.org/mozilla-central/source/image/Image.h?rev=8161b137af4a&force=1#220

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8662803 [details] [diff] [review]
(Part 2) - Remove the concept of lifetimes from the SurfaceCache

r=me on part 2, with comment 12 addressed. Hooray for removing/simplifying code!

Comment 14

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8662804 [details] [diff] [review]
(Part 3) - Make it impossible to deoptimize imgFrames

Hooray!

Comment 15

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

Thanks for the reviews! I'll upload an updated version of part 2 shortly with that nit addressed.

Comment 16

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

Attachment: (Part 2) - Remove the concept of lifetimes from the SurfaceCache

Nits addressed.

Comment 17

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b6a3ec30a14
https://hg.mozilla.org/integration/mozilla-inbound/rev/83a6dc9ee30c
https://hg.mozilla.org/integration/mozilla-inbound/rev/77357a5a1ad1
https://hg.mozilla.org/integration/mozilla-inbound/rev/07d6fcc70e7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0d3f6e2a3e3

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2b6a3ec30a14
https://hg.mozilla.org/mozilla-central/rev/83a6dc9ee30c
https://hg.mozilla.org/mozilla-central/rev/77357a5a1ad1
https://hg.mozilla.org/mozilla-central/rev/07d6fcc70e7c
https://hg.mozilla.org/mozilla-central/rev/c0d3f6e2a3e3

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backout: 
https://hg.mozilla.org/releases/mozilla-aurora/rev/968702bfd84b

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 21

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

Comment on attachment 8662801 [details] [diff] [review]
(Part 1) - Remove HQ scaling, which is now dead code

Approval Request Comment
[Feature/regressing bug #]: These patches transition 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 this into 43 if we can and ship those perf improvements.
[User impact if declined]: 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 there was never a regression related to it; it got backed out because it depended on bug 1201796, which we've since uplifted again.
[String/UUID change made/needed]: None.

Comment 22

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

Comment on attachment 8662801 [details] [diff] [review]
(Part 1) - Remove HQ scaling, which is now dead code

Approved for uplift to aurora, perf improvement, has tests. 
Please uplift parts 1-5.

Comment 23

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

Uplifted with trivial rebase:

https://hg.mozilla.org/releases/mozilla-aurora/rev/b1e93301c3cf
https://hg.mozilla.org/releases/mozilla-aurora/rev/dfb869dc6a74
https://hg.mozilla.org/releases/mozilla-aurora/rev/7ef75c70593d
https://hg.mozilla.org/releases/mozilla-aurora/rev/7e0e6b839983
https://hg.mozilla.org/releases/mozilla-aurora/rev/5fdb49f370a7