Closed Bug 1343341 Opened 8 years ago Closed 8 years ago

infrastructure necessary to allow discarding of animated images

Categories

(Core :: Graphics: ImageLib, defect)

Product:
Core
Component:
Graphics: ImageLib
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

Attachments

(11 files)

Create a pref for discarding animated images
3.01 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
Rename mDoneDecoding, the name doesn't make sense when we can discard
5.03 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
Track new state on the AnimationState now that animated images can be discarded
9.50 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
Fix a bug with animations that have finished
2.97 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
Allow GetTimeoutForFrame to return a maybe for non-decoded frames
9.35 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
Remove the lifetime lock on animated images
5.05 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
Add a basic test
5.17 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
Only respond to OnSurfaceDiscarded notifications for the animated frames (not static first-frame-only decode frames)
9.05 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
In FrameAnimator look up our frames once and pass them around
14.73 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
Only call AnimationState::SetDiscarded on the main thread
2.84 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
Rewrite animation state updating to derive new state purely based on SurfaceCache
13.45 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
Add infrastructure necessary to allow discarding of animated images. We'll use the original bug (bug 686905) to flip the pref and turn this on.
Four bools seemed like a lot, but was necessary to track everything I wanted to track.
Attachment #8842183 - Flags: review?(aosmond)
This does the deed.
Attachment #8842187 - Flags: review?(aosmond)
Attached patch Add a basic test — — Splinter Review
Attachment #8842188 - Flags: review?(aosmond)
Attachment #8842181 - Flags: review?(aosmond) → review+
Attachment #8842182 - Flags: review?(aosmond) → review+
Comment on attachment 8842183 [details] [diff] [review] Track new state on the AnimationState now that animated images can be discarded Review of attachment 8842183 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/FrameAnimator.cpp @@ +324,5 @@ > { > + if (aState.mCompositedFrameInvalid) { > + MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable()); > + MOZ_ASSERT(aState.GetHasBeenDecoded()); > + MOZ_ASSERT(!aState.GetIsCurrentlyDecoded()); Is it possible for GetCompositedFrame to be called before RequestRefresh but after redecoding has finished due to a discard? (E.g. the caller wanted the current frame, lookup failed, requested redecode, but before a refresh tick happened to advance the animation to the desired point, the lookup was attempted again?) That would mean mHasBeenDecoded = true, mIsCurrentlyDecoded = true and mCompositedFrameInvalid = true.
(In reply to Andrew Osmond [:aosmond] from comment #8) > Comment on attachment 8842183 [details] [diff] [review] > Track new state on the AnimationState now that animated images can be > discarded > > Review of attachment 8842183 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: image/FrameAnimator.cpp > @@ +324,5 @@ > > { > > + if (aState.mCompositedFrameInvalid) { > > + MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable()); > > + MOZ_ASSERT(aState.GetHasBeenDecoded()); > > + MOZ_ASSERT(!aState.GetIsCurrentlyDecoded()); > > Is it possible for GetCompositedFrame to be called before RequestRefresh but > after redecoding has finished due to a discard? (E.g. the caller wanted the > current frame, lookup failed, requested redecode, but before a refresh tick > happened to advance the animation to the desired point, the lookup was > attempted again?) That would mean mHasBeenDecoded = true, > mIsCurrentlyDecoded = true and mCompositedFrameInvalid = true. Never mind, I think this gets addressed by the next patch in the series, attachment 8842184 [details] [diff] [review].
Attachment #8842183 - Flags: review?(aosmond) → review+
Attachment #8842184 - Flags: review?(aosmond) → review+
Attachment #8842185 - Flags: review?(aosmond) → review+
Attachment #8842187 - Flags: review?(aosmond) → review+
Comment on attachment 8842188 [details] [diff] [review] Add a basic test Review of attachment 8842188 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/test/mochitest/test_discardAnimatedImage.html @@ +1,4 @@ > +<!DOCTYPE HTML> > +<html> > +<!-- > +https://bugzilla.mozilla.org/show_bug.cgi?id=686905 nit: fix bug # @@ +9,5 @@ > + <script type="text/javascript" src="imgutils.js"></script> > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" /> > +</head> > +<body> > +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=686905">Mozilla Bug 686905</a> nit: fix bug # @@ +23,5 @@ > +</div> > +<pre id="test"> > +<script class="testbody" type="text/javascript"> > + > +/** Test for Bug 686905. **/ nit: fix bug #
Attachment #8842188 - Flags: review?(aosmond) → review+
(In reply to Andrew Osmond [:aosmond] from comment #10) > Comment on attachment 8842188 [details] [diff] [review] > Add a basic test > > Review of attachment 8842188 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: image/test/mochitest/test_discardAnimatedImage.html > @@ +1,4 @@ > > +<!DOCTYPE HTML> > > +<html> > > +<!-- > > +https://bugzilla.mozilla.org/show_bug.cgi?id=686905 > > nit: fix bug # > > @@ +9,5 @@ > > + <script type="text/javascript" src="imgutils.js"></script> > > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" /> > > +</head> > > +<body> > > +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=686905">Mozilla Bug 686905</a> > > nit: fix bug # > > @@ +23,5 @@ > > +</div> > > +<pre id="test"> > > +<script class="testbody" type="text/javascript"> > > + > > +/** Test for Bug 686905. **/ > > nit: fix bug # Grr, ignore that, I thought the bug was closed as a dupe, but instead other bugs were marked as a dupe against it :).
(In reply to Andrew Osmond [:aosmond] from comment #8) > Comment on attachment 8842183 [details] [diff] [review] > Track new state on the AnimationState now that animated images can be > discarded > > Review of attachment 8842183 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: image/FrameAnimator.cpp > @@ +324,5 @@ > > { > > + if (aState.mCompositedFrameInvalid) { > > + MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable()); > > + MOZ_ASSERT(aState.GetHasBeenDecoded()); > > + MOZ_ASSERT(!aState.GetIsCurrentlyDecoded()); > > Is it possible for GetCompositedFrame to be called before RequestRefresh but > after redecoding has finished due to a discard? (E.g. the caller wanted the > current frame, lookup failed, requested redecode, but before a refresh tick > happened to advance the animation to the desired point, the lookup was > attempted again?) That would mean mHasBeenDecoded = true, > mIsCurrentlyDecoded = true and mCompositedFrameInvalid = true. That's actually what we would want: if the composited frame doesn't have the correct frame for the current time then we don't want to display it (even if the image is fully decoded). (In reply to Andrew Osmond [:aosmond] from comment #9) > Never mind, I think this gets addressed by the next patch in the series, > attachment 8842184 [details] [diff] [review]. You're correct in that the next patch will mark the composited frame as valid whenever the image is fully decoded. This is okay because anything that works on the normal painting path will ask for the frame after RequestRefresh is called on the image in the refresh driver. Things that don't work off the normal painting path would include js calling drawImage on a canvas (either from a requestAnimateFrame callback or js called another way), or Mozilla code that uses images as part of the UI (menu's, notifications, etc). There's no way for js to know which frame of the image is current, so hopefully this isn't a problem. But if it is we can probably do something to fix that.
Pushed by tnikkel@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e3c5f0f5356 Create a pref to enable/disable discarding of animated images. r=aosmond https://hg.mozilla.org/integration/mozilla-inbound/rev/67ef66953267 Rename mDoneDecoding to mHasBeenDecoded. r=aosmond https://hg.mozilla.org/integration/mozilla-inbound/rev/7f13c7a84acb Create state on the AnimationState object to track whether the image is decoded or not. r=aosmond
Pushed by tnikkel@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ef4042cfbd80 Include gfxPref.h to fix build bustage.
Keywords: leave-open
Backout of the "adding state" patch because apparantly we can call RasterImage::OnSurfaceDiscarded on a RasterImage with a non-null animation state object. https://hg.mozilla.org/integration/mozilla-inbound/rev/b92b8655e634c865271a079b7709cb466fa44bdf
The log for that failure. https://treeherder.mozilla.org/logviewer.html#?job_id=81069835&repo=mozilla-inbound&lineNumber=1909 It only happened once, on a couple pushes.
This is one of the bugs exposed by the intermittent failure that I got on landing/tryserver.
Attachment #8846262 - Flags: review?(aosmond)
Depends on: 1346510
Comment on attachment 8846262 [details] [diff] [review] Only respond to OnSurfaceDiscarded notifications for the animated frames (not static first-frame-only decode frames) Review of attachment 8846262 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. I should have remembered this considering I dealt with this same cache split only a little while ago. Mea culpa.
Attachment #8846262 - Flags: review?(aosmond) → review+
Pushed by tnikkel@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a23edcf5b82a Create state on the AnimationState object to track whether the image is decoded or not. r=aosmond https://hg.mozilla.org/integration/mozilla-inbound/rev/29e2e6dd82d2 Only set the animation state as discarded if we discarded the animated frames (and not a static frame from a first-frame-only decode). r=aosmond
Pushed by tnikkel@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3a72a7a121 Create state on the AnimationState object to track whether the image is decoded or not. r=aosmond https://hg.mozilla.org/integration/mozilla-inbound/rev/d4cafcc44bab Only set the animation state as discarded if we discarded the animated frames (and not a static frame from a first-frame-only decode). r=aosmond
(In reply to Timothy Nikkel (:tnikkel) from comment #21) > And backout again for a build error > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > 0394f4a6e9e9436e70409bc9f0eff8378f945f27 > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > a22dff44c2abd5ddad3276cf7ebbb943dd343ccf This was caused by some build and static analysis failures because of passing a SurfaceKey directly to OnSurfaceDiscarded instead of passing it as a const reference.
Pushed by tnikkel@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/67c3d82f8cae Fix a bug with redecoding images whose animation is finished. r=aosmond
I realized that the surface cache can discard on any thread, at any time. So we need a few changes to handle that. 1) we have to dispatch the OnSurfaceDiscarded notification to the main thread so it can call AnimationState::SetDiscarded 2) this means we can't expect a SetDiscarded call before our frames are discarded 3) this also means we can't really depend on any ordering of notifications (I think, but haven't verified and don't want to depend on this anyway, that a decode in progress could have it's frames removed from the surface cache but also finish and notify of a complete decode, and the runnables could get to the main thread in any order) So I have three more patches that: 1) fix OnSurfaceDiscarded to do its work on the main thread 2) lookup our frames once in FrameAnimator and pass them around to every function that needs them. this means that we have one consistent set of frames during any call into FrameAnimator, the set of frames can grow, but not shrink (which is what the code has been able to deal with for a long time) 3) get rid of SetDiscarded and NotifyDecodeComplete notifications to track our state, and instead just have a UpdateState function that looks at the surface cache and derives our state from that. we now also update our state when we start RequestRefresh
Attachment #8849856 - Flags: review?(aosmond) → review+
Pushed by tnikkel@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/af363f677224 Change GetTimeoutForFrame to return a Maybe, and make all callers deal with a lack of a return value. r=aosmond
Attachment #8849857 - Flags: review?(aosmond) → review+
Comment on attachment 8849858 [details] [diff] [review] Rewrite animation state updating to derive new state purely based on SurfaceCache Review of attachment 8849858 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/FrameAnimator.cpp @@ +27,5 @@ > > void > +AnimationState::UpdateState(bool aAnimationFinished, > + RasterImage *aImage, > + gfx::IntSize aSize) nit: Use const gfx::IntSize& instead?
Attachment #8849858 - Flags: review?(aosmond) → review+
Pushed by tnikkel@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1c5e91029034 In FrameAnimator look up our frames once and pass them around. r=aosmond
Pushed by tnikkel@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/432215553953 Only call AnimationState::SetDiscarded on the main thread. r=aosmond
Pushed by tnikkel@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f2bc0c02b50c Rewrite animation state updating to derive new state purely based on SurfaceCache and RasterImage::mAnimationFinished. r=aosmond
Pushed by tnikkel@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/189dbcbf21e2 When the animated images discarding pref is enabled Remove the lifetime lock on animated images and adjust code in RasterImage to allow animated images to be discarded. r=aosmond https://hg.mozilla.org/integration/mozilla-inbound/rev/e91d83339d97 Add a test to check that discarding and redecoding of animated images works. r=aosmond
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/189dbcbf21e2 https://hg.mozilla.org/mozilla-central/rev/e91d83339d97
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1351434
Depends on: 1352008
Depends on: 1353298
Depends on: 1353299
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: