Bug 1343341. Rewrite animation state updating to derive new state purely based on SurfaceCache and RasterImage::mAnimationFinished. r=aosmond
authorTimothy Nikkel <tnikkel@gmail.com>
Sun, 26 Mar 2017 00:04:53 -0500
changeset 400160 f2bc0c02b50c5417f6a7f10345e22326df28fee4
parent 400159 27cda2c75c4616b79df0cebe977edb0992aa7305
child 400161 cc53710589fb500610495da5258b7b9221edf681
child 400220 2de4332125a345e2471a835ecc77f60acae6ff84
push id1490
push usermtabara@mozilla.com
push dateMon, 31 Jul 2017 14:08:16 +0000
treeherdermozilla-release@70e32e6bf15e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaosmond
bugs1343341
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
image/FrameAnimator.h
image/RasterImage.cpp
--- a/image/FrameAnimator.cpp
+++ b/image/FrameAnimator.cpp
@@ -21,46 +21,90 @@ using namespace gfx;
 
 namespace image {
 
 ///////////////////////////////////////////////////////////////////////////////
 // AnimationState implementation.
 ///////////////////////////////////////////////////////////////////////////////
 
 void
-AnimationState::NotifyDecodeComplete()
+AnimationState::UpdateState(bool aAnimationFinished,
+                            RasterImage *aImage,
+                            const gfx::IntSize& aSize)
+{
+  LookupResult result =
+    SurfaceCache::Lookup(ImageKey(aImage),
+                         RasterSurfaceKey(aSize,
+                                          DefaultSurfaceFlags(),
+                                          PlaybackType::eAnimated));
+
+  UpdateStateInternal(result, aAnimationFinished);
+}
+
+void
+AnimationState::UpdateStateInternal(LookupResult& aResult,
+                                    bool aAnimationFinished)
 {
-  // If we weren't discarded before the decode finished then mark ourselves as
-  // currently decoded.
-  if (!mDiscarded) {
-    mIsCurrentlyDecoded = true;
+  // 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
 AnimationState::ResetAnimation()
 {
   mCurrentAnimationFrameIndex = 0;
 }
 
@@ -302,17 +346,19 @@ FrameAnimator::AdvanceFrame(AnimationSta
 
   // If we're here, we successfully advanced the frame.
   ret.mFrameAdvanced = true;
 
   return ret;
 }
 
 RefreshResult
-FrameAnimator::RequestRefresh(AnimationState& aState, const TimeStamp& aTime)
+FrameAnimator::RequestRefresh(AnimationState& aState,
+                              const TimeStamp& aTime,
+                              bool aAnimationFinished)
 {
   // By default, an empty RefreshResult.
   RefreshResult ret;
 
   if (aState.IsDiscarded()) {
     return ret;
   }
 
@@ -321,22 +367,18 @@ FrameAnimator::RequestRefresh(AnimationS
   // must easier to reason about then trying to write code that is safe to
   // having the surface disappear at anytime.
   LookupResult result =
     SurfaceCache::Lookup(ImageKey(mImage),
                          RasterSurfaceKey(mSize,
                                           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;
   }
 
   // only advance the frame if the current time is greater than or
   // equal to the current frame's end time.
   Maybe<TimeStamp> currentFrameEndTime =
     GetCurrentImgFrameEndTime(aState, result.Surface());
   if (currentFrameEndTime.isNothing()) {
--- a/image/FrameAnimator.h
+++ b/image/FrameAnimator.h
@@ -35,32 +35,39 @@ public:
     , mAnimationMode(aAnimationMode)
     , mHasBeenDecoded(false)
     , mIsCurrentlyDecoded(false)
     , mCompositedFrameInvalid(false)
     , 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.
    */
   void NotifyDecodeComplete();
 
   /**
    * Returns true if this image has been fully decoded before.
    */
   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.
    */
   bool IsDiscarded() { return mDiscarded; }
 
   /**
    * Sets the composited frame as valid or invalid.
    */
@@ -271,17 +278,19 @@ public:
 
   /**
    * Re-evaluate what frame we're supposed to be on, and do whatever blending
    * is necessary to get us to that frame.
    *
    * 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
    * not have required compositing). It may not be available because it hasn't
    * been decoded yet, in which case we return an empty LookupResult.
    */
   LookupResult GetCompositedFrame(AnimationState& aState);
 
--- a/image/RasterImage.cpp
+++ b/image/RasterImage.cpp
@@ -168,17 +168,17 @@ RasterImage::RequestRefresh(const TimeSt
 
   if (!mAnimating) {
     return;
   }
 
   RefreshResult res;
   if (mAnimationState) {
     MOZ_ASSERT(mFrameAnimator);
-    res = mFrameAnimator->RequestRefresh(*mAnimationState, aTime);
+    res = mFrameAnimator->RequestRefresh(*mAnimationState, aTime, mAnimationFinished);
   }
 
   if (res.mFrameAdvanced) {
     // Notify listeners that our frame has actually changed, but do this only
     // once for all frames that we've now passed (if AdvanceFrame() was called
     // more than once).
     #ifdef DEBUG
       mFramesNotified++;
@@ -445,17 +445,17 @@ RasterImage::OnSurfaceDiscarded(const Su
 {
   MOZ_ASSERT(mProgressTracker);
 
   bool animatedFramesDiscarded =
     mAnimationState && aSurfaceKey.Playback() == PlaybackType::eAnimated;
 
   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;
   }
 
   RefPtr<RasterImage> image = this;
   NS_DispatchToMainThread(NS_NewRunnableFunction(
                             "RasterImage::OnSurfaceDiscarded",
@@ -466,17 +466,17 @@ RasterImage::OnSurfaceDiscarded(const Su
 
 void
 RasterImage::OnSurfaceDiscardedInternal(bool aAnimatedFramesDiscarded)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   if (aAnimatedFramesDiscarded && mAnimationState) {
     MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable());
-    mAnimationState->SetDiscarded(true);
+    mAnimationState->UpdateState(mAnimationFinished, this, mSize);
   }
 
   if (mProgressTracker) {
     mProgressTracker->OnDiscard();
   }
 }
 
 //******************************************************************************
@@ -1076,17 +1076,17 @@ RasterImage::Discard()
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(CanDiscard(), "Asked to discard but can't");
   MOZ_ASSERT(!mAnimationState, "Asked to discard for animated image");
 
   // Delete all the decoded frames.
   SurfaceCache::RemoveImage(ImageKey(this));
 
   if (mAnimationState) {
-    mAnimationState->SetDiscarded(true);
+    mAnimationState->UpdateState(mAnimationFinished, this, mSize);
   }
 
   // Notify that we discarded.
   if (mProgressTracker) {
     mProgressTracker->OnDiscard();
   }
 }
 
@@ -1248,20 +1248,20 @@ RasterImage::Decode(const IntSize& aSize
   }
 
   // Create a decoder.
   RefPtr<IDecodingTask> task;
   if (mAnimationState && aPlaybackType == PlaybackType::eAnimated) {
     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);
     }
   } else {
     task = DecoderFactory::CreateDecoder(mDecoderType, WrapNotNull(this),
                                          mSourceBuffer, mSize, aSize,
                                          decoderFlags, surfaceFlags);
   }
@@ -1712,16 +1712,17 @@ RasterImage::NotifyDecodeComplete(const 
   NotifyProgress(aProgress, aInvalidRect, aFrameCount,
                  aDecoderFlags, aSurfaceFlags);
 
   if (!(aDecoderFlags & DecoderFlags::FIRST_FRAME_ONLY) &&
       mHasBeenDecoded && mAnimationState) {
     // 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.
   if (!aStatus.mWasMetadataDecode) {
     if (aTelemetry.mChunkCount) {
       Telemetry::Accumulate(Telemetry::IMAGE_DECODE_CHUNKS, aTelemetry.mChunkCount);
     }