Bug 1343341. Rewrite animation state updating to derive new state purely based on SurfaceCache and RasterImage::mAnimationFinished. r=aosmond

If the SurfaceCache discards our frames on another thread, the runnable that notifies us of that discard could race with a decode complete notification. So we can't rely on any ordering of SetDiscarded and NotifyDecodeComplete. Thus we must derive our state purely from the SurfaceCache (and mAnimationFinished from RasterImage).

We also update the image state in RequestRefresh (the main place where we use the state that is updated).

The other main place we use the state is GetCompositedFrame, but we don't update the state there. It should be fine because the only time this might lag behind reality is if the frames are discarded, and it should be fine to continue drawing the composited frame until the discard notification arrives.

The way that we tell that an animated image has all of its frames complete in the surface cache is less than ideal.

image/FrameAnimator.cpp

@@ -26,36 +26,80 @@ namespace image {
 ///////////////////////////////////////////////////////////////////////////////
 
 void
-AnimationState::NotifyDecodeComplete()
+AnimationState::UpdateState(bool aAnimationFinished,
+                            RasterImage *aImage,
+                            const gfx::IntSize& aSize)
 {
-  // If we weren't discarded before the decode finished then mark ourselves as
-  // currently decoded.
-  if (!mDiscarded) {
-    mIsCurrentlyDecoded = true;
+  LookupResult result =
+    SurfaceCache::Lookup(ImageKey(aImage),
+                         RasterSurfaceKey(aSize,
+                                          DefaultSurfaceFlags(),
+                                          PlaybackType::eAnimated));
 
+  UpdateStateInternal(result, aAnimationFinished);
+}
+
+void
+AnimationState::UpdateStateInternal(LookupResult& aResult,
+                                    bool aAnimationFinished)
+{
+  // Update mDiscarded and mIsCurrentlyDecoded.
+  if (aResult.Type() == MatchType::NOT_FOUND) {
+    // no frames, we've either been discarded, or never been decoded before.
+    mDiscarded = mHasBeenDecoded;
+    mIsCurrentlyDecoded = false;
+  } else if (aResult.Type() == MatchType::PENDING) {
+    // no frames yet, but a decoder is or will be working on it.
+    mDiscarded = false;
+    mIsCurrentlyDecoded = false;
+  } else {
+    MOZ_ASSERT(aResult.Type() == MatchType::EXACT);
+    mDiscarded = false;
+
+    // If mHasBeenDecoded is true then we know the true total frame count and
+    // we can use it to determine if we have all the frames now so we know if
+    // we are currently fully decoded.
+    // If mHasBeenDecoded is false then we'll get another UpdateState call
+    // when the decode finishes.
+    if (mHasBeenDecoded) {
+      Maybe<uint32_t> frameCount = FrameCount();
+      MOZ_ASSERT(frameCount.isSome());
+      aResult.Surface().Seek(*frameCount - 1);
+      if (aResult.Surface() && aResult.Surface()->IsFinished()) {
+        mIsCurrentlyDecoded = true;
+      } else {
+        mIsCurrentlyDecoded = false;
+      }
+    }
+  }
+
+  // Update the value of mCompositedFrameInvalid.
+  if (mIsCurrentlyDecoded || aAnimationFinished) {
     // Animated images that have finished their animation (ie because it is a
     // finite length animation) don't have RequestRefresh called on them, and so
     // mCompositedFrameInvalid would never get cleared. We clear it here (and
     // also in RasterImage::Decode when we create a decoder for an image that
     // has finished animated so it can display sooner than waiting until the
-    // decode completes). This is safe to do for images that aren't finished
-    // animating because before we paint the refresh driver will call into us
-    // to advance to the correct frame, and that will succeed because we have
-    // all the frames.
+    // decode completes). We also do it if we are fully decoded. This is safe
+    // to do for images that aren't finished animating because before we paint
+    // the refresh driver will call into us to advance to the correct frame,
+    // and that will succeed because we have all the frames.
     mCompositedFrameInvalid = false;
+  } else if (aResult.Type() == MatchType::NOT_FOUND ||
+             aResult.Type() == MatchType::PENDING) {
+    if (mHasBeenDecoded) {
+      MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable());
+      mCompositedFrameInvalid = true;
+    }
   }
-  mHasBeenDecoded = true;
+  // Otherwise don't change the value of mCompositedFrameInvalid, it will be
+  // updated by RequestRefresh.
 }
 
 void
-AnimationState::SetDiscarded(bool aDiscarded)
+AnimationState::NotifyDecodeComplete()
 {
-  if (aDiscarded) {
-    MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable());
-    mIsCurrentlyDecoded = false;
-    mCompositedFrameInvalid = true;
-  }
-  mDiscarded = aDiscarded;
+  mHasBeenDecoded = true;
 }
 
 void
@@ -307,7 +351,9 @@ FrameAnimator::AdvanceFrame(AnimationState& aState,
 }
 
 RefreshResult
-FrameAnimator::RequestRefresh(AnimationState& aState, const TimeStamp& aTime)
+FrameAnimator::RequestRefresh(AnimationState& aState,
+                              const TimeStamp& aTime,
+                              bool aAnimationFinished)
 {
   // By default, an empty RefreshResult.
   RefreshResult ret;
@@ -326,12 +372,8 @@ FrameAnimator::RequestRefresh(AnimationState& aState, const TimeStamp& aTime)
                                           DefaultSurfaceFlags(),
                                           PlaybackType::eAnimated));
 
-  if (!result) {
-    if (result.Type() == MatchType::NOT_FOUND) {
-      // No surface, and nothing pending, must have been discarded but
-      // we haven't been notified yet.
-      aState.SetDiscarded(true);
-    }
+  aState.UpdateStateInternal(result, aAnimationFinished);
+  if (aState.IsDiscarded() || !result) {
     return ret;
   }
 

image/FrameAnimator.h

@@ -39,6 +39,19 @@ public:
     , mDiscarded(false)
   { }
 
+  /**
+   * Call this whenever a decode completes, a decode starts, or the image is
+   * discarded. It will update the internal state. Specifically mDiscarded,
+   * mCompositedFrameInvalid, and mIsCurrentlyDecoded.
+   */
+  void UpdateState(bool aAnimationFinished,
+                   RasterImage *aImage,
+                   const gfx::IntSize& aSize);
+private:
+  void UpdateStateInternal(LookupResult& aResult,
+                           bool aAnimationFinished);
+
+public:
   /**
    * Call when a decode of this image has been completed.
    */
@@ -49,12 +62,6 @@ public:
    */
   bool GetHasBeenDecoded() { return mHasBeenDecoded; }
 
-  /**
-   * Call this with true when this image is discarded. Call this with false
-   * when a decoder is created to decode the image.
-   */
-  void SetDiscarded(bool aDiscarded);
-
   /**
    * Returns true if this image has been discarded and a decoded has not yet
    * been created to redecode it.
@@ -276,7 +283,9 @@ public:
    * Returns the result of that blending, including whether the current frame
    * changed and what the resulting dirty rectangle is.
    */
-  RefreshResult RequestRefresh(AnimationState& aState, const TimeStamp& aTime);
+  RefreshResult RequestRefresh(AnimationState& aState,
+                               const TimeStamp& aTime,
+                               bool aAnimationFinished);
 
   /**
    * Get the full frame for the current frame of the animation (it may or may

image/RasterImage.cpp

@@ -173,7 +173,7 @@ RasterImage::RequestRefresh(const TimeStamp& aTime)
   RefreshResult res;
   if (mAnimationState) {
     MOZ_ASSERT(mFrameAnimator);
-    res = mFrameAnimator->RequestRefresh(*mAnimationState, aTime);
+    res = mFrameAnimator->RequestRefresh(*mAnimationState, aTime, mAnimationFinished);
   }
 
   if (res.mFrameAdvanced) {
@@ -450,7 +450,7 @@ RasterImage::OnSurfaceDiscarded(const SurfaceKey& aSurfaceKey)
 
   if (animatedFramesDiscarded && NS_IsMainThread()) {
     MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable());
-    mAnimationState->SetDiscarded(true);
+    mAnimationState->UpdateState(mAnimationFinished, this, mSize);
     // We don't need OnSurfaceDiscardedInternal to handle the animated frames
     // being discarded because we just did.
     animatedFramesDiscarded = false;
@@ -471,7 +471,7 @@ RasterImage::OnSurfaceDiscardedInternal(bool aAnimatedFramesDiscarded)
 
   if (aAnimatedFramesDiscarded && mAnimationState) {
     MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable());
-    mAnimationState->SetDiscarded(true);
+    mAnimationState->UpdateState(mAnimationFinished, this, mSize);
   }
 
   if (mProgressTracker) {
@@ -1081,7 +1081,7 @@ RasterImage::Discard()
   SurfaceCache::RemoveImage(ImageKey(this));
 
   if (mAnimationState) {
-    mAnimationState->SetDiscarded(true);
+    mAnimationState->UpdateState(mAnimationFinished, this, mSize);
   }
 
   // Notify that we discarded.
@@ -1253,10 +1253,10 @@ RasterImage::Decode(const IntSize& aSize,
     task = DecoderFactory::CreateAnimationDecoder(mDecoderType, WrapNotNull(this),
                                                   mSourceBuffer, mSize,
                                                   decoderFlags, surfaceFlags);
-    mAnimationState->SetDiscarded(false);
+    mAnimationState->UpdateState(mAnimationFinished, this, mSize);
     // If the animation is finished we can draw right away because we just draw
     // the final frame all the time from now on. See comment in
-    // AnimationState::NotifyDecodeComplete.
+    // AnimationState::UpdateState.
     if (mAnimationFinished) {
       mAnimationState->SetCompositedFrameInvalid(false);
     }
@@ -1717,6 +1717,7 @@ RasterImage::NotifyDecodeComplete(const DecoderFinalStatus& aStatus,
     // We've finished a full decode of all animation frames and our AnimationState
     // has been notified about them all, so let it know not to expect anymore.
     mAnimationState->NotifyDecodeComplete();
+    mAnimationState->UpdateState(mAnimationFinished, this, mSize);
   }
 
   // Do some telemetry if this isn't a metadata decode.