Closed Bug 1146663 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox38 --- wontfix
firefox39 --- affected
firefox40 --- affected
firefox43 --- fixed
firefox44 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Depends on 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

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.
Blocks: 1144983
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?
Flags: needinfo?(seth)
(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.
Flags: needinfo?(seth)
At this point the HQ scaling code is dead code, so let's remove it.
Attachment #8662801 - Flags: review?(tnikkel)
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.
Attachment #8662803 - Flags: review?(dholbert)
At this point there's no reason to ever deoptimize an imgFrame once it's been
optimized, so let's remove imgFrame::Deoptimize() totally.
Attachment #8662804 - Flags: review?(tnikkel)
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().
Attachment #8662806 - Flags: review?(tnikkel)
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.
Attachment #8662808 - Flags: review?(tnikkel)
With these patches combined, 562 fewer lines!
[Adding dependency on bug 1060869, which is where SurfaceCache lifetimes were introduced.]
Depends on: 1060869
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 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!
Attachment #8662803 - Flags: review?(dholbert) → review+
Attachment #8662801 - Flags: review?(tnikkel) → review+
Comment on attachment 8662804 [details] [diff] [review]
(Part 3) - Make it impossible to deoptimize imgFrames

Hooray!
Attachment #8662804 - Flags: review?(tnikkel) → review+
Attachment #8662806 - Flags: review?(tnikkel) → review+
Attachment #8662808 - Flags: review?(tnikkel) → review+
Blocks: 1206206
Thanks for the reviews! I'll upload an updated version of part 2 shortly with that nit addressed.
Attachment #8662803 - Attachment is obsolete: true
I believe this is how the flags should be set at this point.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Flags: needinfo?(seth)
Resolution: --- → FIXED
Blocks: 1210145
Flags: needinfo?(seth)
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.
Attachment #8662801 - Flags: approval-mozilla-aurora?
Attachment #8663257 - Flags: approval-mozilla-aurora?
Attachment #8662804 - Flags: approval-mozilla-aurora?
Attachment #8662806 - Flags: approval-mozilla-aurora?
Attachment #8662808 - Flags: approval-mozilla-aurora?
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.
Attachment #8662801 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8662804 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8662806 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8662808 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8663257 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.