infrastructure necessary to allow discarding of animated images

RESOLVED FIXED in Firefox 55

Status

()

Core
ImageLib
RESOLVED FIXED
4 months ago
a month ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(11 attachments)

3.01 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
5.03 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
9.50 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
2.97 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
9.35 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
5.05 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
5.17 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
9.05 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
14.73 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
2.84 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
13.45 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 months ago
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.
(Assignee)

Comment 1

4 months ago
Created attachment 8842181 [details] [diff] [review]
Create a pref for discarding animated images
Attachment #8842181 - Flags: review?(aosmond)
(Assignee)

Comment 2

4 months ago
Created attachment 8842182 [details] [diff] [review]
Rename mDoneDecoding, the name doesn't make sense when we can discard
Attachment #8842182 - Flags: review?(aosmond)
(Assignee)

Comment 3

4 months ago
Created attachment 8842183 [details] [diff] [review]
Track new state on the AnimationState now that animated images can be discarded

Four bools seemed like a lot, but was necessary to track everything I wanted to track.
Attachment #8842183 - Flags: review?(aosmond)
(Assignee)

Comment 4

4 months ago
Created attachment 8842184 [details] [diff] [review]
Fix a bug with animations that have finished
Attachment #8842184 - Flags: review?(aosmond)
(Assignee)

Comment 5

4 months ago
Created attachment 8842185 [details] [diff] [review]
Allow GetTimeoutForFrame to return a maybe for non-decoded frames
Attachment #8842185 - Flags: review?(aosmond)
(Assignee)

Comment 6

4 months ago
Created attachment 8842187 [details] [diff] [review]
Remove the lifetime lock on animated images

This does the deed.
Attachment #8842187 - Flags: review?(aosmond)
(Assignee)

Comment 7

4 months ago
Created attachment 8842188 [details] [diff] [review]
Add a basic test
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 :).
(Assignee)

Comment 12

4 months ago
(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.

Comment 13

4 months ago
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

Comment 14

4 months ago
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef4042cfbd80
Include gfxPref.h to fix build bustage.
(Assignee)

Updated

4 months ago
Keywords: leave-open
(Assignee)

Comment 15

4 months ago
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
(Assignee)

Comment 16

4 months ago
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.

Comment 17

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5e3c5f0f5356
https://hg.mozilla.org/mozilla-central/rev/67ef66953267
https://hg.mozilla.org/mozilla-central/rev/ef4042cfbd80
(Assignee)

Comment 18

4 months ago
Created attachment 8846262 [details] [diff] [review]
Only respond to OnSurfaceDiscarded notifications for the animated frames (not static first-frame-only decode frames)

This is one of the bugs exposed by the intermittent failure that I got on landing/tryserver.
Attachment #8846262 - Flags: review?(aosmond)
(Assignee)

Updated

4 months ago
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+

Comment 20

4 months ago
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
(Assignee)

Comment 21

4 months ago
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

Comment 22

4 months ago
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
(Assignee)

Comment 23

4 months ago
(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.

Comment 24

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cd3a72a7a121
https://hg.mozilla.org/mozilla-central/rev/d4cafcc44bab

Comment 25

3 months ago
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

Comment 26

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/67c3d82f8cae
(Assignee)

Comment 27

3 months ago
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
(Assignee)

Comment 28

3 months ago
Created attachment 8849856 [details] [diff] [review]
In FrameAnimator look up our frames once and pass them around
Attachment #8849856 - Flags: review?(aosmond)
(Assignee)

Comment 29

3 months ago
Created attachment 8849857 [details] [diff] [review]
Only call AnimationState::SetDiscarded on the main thread
Attachment #8849857 - Flags: review?(aosmond)
(Assignee)

Comment 30

3 months ago
Created attachment 8849858 [details] [diff] [review]
Rewrite animation state updating to derive new state purely based on SurfaceCache
Attachment #8849858 - Flags: review?(aosmond)
Attachment #8849856 - Flags: review?(aosmond) → review+

Comment 31

3 months ago
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+

Comment 33

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/af363f677224

Comment 34

3 months ago
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

Comment 35

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1c5e91029034

Comment 36

3 months ago
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/432215553953
Only call AnimationState::SetDiscarded on the main thread. r=aosmond

Comment 37

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/432215553953

Comment 38

3 months ago
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
https://hg.mozilla.org/mozilla-central/rev/f2bc0c02b50c

Comment 40

3 months ago
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
(Assignee)

Updated

3 months ago
Keywords: leave-open

Comment 41

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/189dbcbf21e2
https://hg.mozilla.org/mozilla-central/rev/e91d83339d97
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

3 months ago
Depends on: 1351434
(Assignee)

Updated

3 months ago
Depends on: 1352008
(Assignee)

Updated

3 months ago
Depends on: 1353298
(Assignee)

Updated

3 months ago
Depends on: 1353299
status-firefox54: affected → ---
You need to log in before you can comment on or make changes to this bug.