Closed Bug 1343341 Opened 7 years ago Closed 7 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: 7 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: