Bug 1388733 - Ensure animations resume when the image surfaces are discarded while still decoding. r=tnikkel, a=gchang
authorAndrew Osmond <aosmond@mozilla.com>
Wed, 09 Aug 2017 22:26:55 -0400
changeset 421193 df818e102acdcce0430cd0d782b2fb46921f2f95
parent 421192 36c4a1d0b3bea5f7b55c54c7c2741e1dcc3a0c02
child 421194 dc46fc892938e6b8d449af75a129603ef55035c5
push id7617
push userryanvm@gmail.com
push dateTue, 15 Aug 2017 14:10:45 +0000
treeherdermozilla-beta@efabe9b3a88e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel, gchang
bugs1388733
milestone56.0
Bug 1388733 - Ensure animations resume when the image surfaces are discarded while still decoding. r=tnikkel, a=gchang When an animated image has been discarded, we avoided marking the composited frame invalid unless it had been previously decoded. Most of the time this was fine, but if the animated image was still decoding for the first time, then we still had a composited frame lingering that we did not mark as invalid. As a result, when we called RasterImage::LookupFrame (and indirectly FrameAnimator::GetCompositedFrame), it would always return the composited frame. This meant that RasterImage::Decode would never be called to trigger a redecode. At the same time, FrameAnimator::RequestRefresh would not cause us to advance the frame because the state was still discarded. With this patch we separate out the concepts of "has ever requested to be decoded" and "has ever completed decoding." The former is now used to control whether or not a composited frame is marked as invalid after we discover we currently have no surface for the animation -- this solves the animation remaining frozen as we now request the redecode as expected. The latter remains used to determine if we actually know the total number of frames.
image/FrameAnimator.cpp
image/FrameAnimator.h
--- a/image/FrameAnimator.cpp
+++ b/image/FrameAnimator.cpp
@@ -52,19 +52,21 @@ AnimationState::UpdateStateInternal(Look
   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;
+    mHasRequestedDecode = true;
   } else {
     MOZ_ASSERT(aResult.Type() == MatchType::EXACT);
     mDiscarded = false;
+    mHasRequestedDecode = true;
 
     // 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();
@@ -94,17 +96,17 @@ AnimationState::UpdateStateInternal(Look
       // and that will succeed because we have all the frames.
       if (mCompositedFrameInvalid) {
         // Invalidate if we are marking the composited frame valid.
         ret.SizeTo(aSize);
       }
       mCompositedFrameInvalid = false;
     } else if (aResult.Type() == MatchType::NOT_FOUND ||
                aResult.Type() == MatchType::PENDING) {
-      if (mHasBeenDecoded) {
+      if (mHasRequestedDecode) {
         MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable());
         mCompositedFrameInvalid = true;
       }
     }
     // Otherwise don't change the value of mCompositedFrameInvalid, it will be
     // updated by RequestRefresh.
   }
 
@@ -204,17 +206,17 @@ Maybe<TimeStamp>
 FrameAnimator::GetCurrentImgFrameEndTime(AnimationState& aState,
                                          DrawableSurface& aFrames) const
 {
   TimeStamp currentFrameTime = aState.mCurrentAnimationFrameTime;
   Maybe<FrameTimeout> timeout =
     GetTimeoutForFrame(aState, aFrames, aState.mCurrentAnimationFrameIndex);
 
   if (timeout.isNothing()) {
-    MOZ_ASSERT(aState.GetHasBeenDecoded() && !aState.GetIsCurrentlyDecoded());
+    MOZ_ASSERT(aState.GetHasRequestedDecode() && !aState.GetIsCurrentlyDecoded());
     return Nothing();
   }
 
   if (*timeout == FrameTimeout::Forever()) {
     // We need to return a sentinel value in this case, because our logic
     // doesn't work correctly if we have an infinitely long timeout. We use one
     // year in the future as the sentinel because it works with the loop in
     // RequestRefresh() below.
@@ -411,17 +413,17 @@ FrameAnimator::RequestRefresh(AnimationS
   }
 
   // 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()) {
     MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable());
-    MOZ_ASSERT(aState.GetHasBeenDecoded() && !aState.GetIsCurrentlyDecoded());
+    MOZ_ASSERT(aState.GetHasRequestedDecode() && !aState.GetIsCurrentlyDecoded());
     MOZ_ASSERT(aState.mCompositedFrameInvalid);
     // Nothing we can do but wait for our previous current frame to be decoded
     // again so we can determine what to do next.
     return ret;
   }
 
   while (*currentFrameEndTime <= aTime) {
     TimeStamp oldFrameEndTime = *currentFrameEndTime;
@@ -460,17 +462,17 @@ FrameAnimator::GetCompositedFrame(Animat
   LookupResult result =
     SurfaceCache::Lookup(ImageKey(mImage),
                          RasterSurfaceKey(mSize,
                                           DefaultSurfaceFlags(),
                                           PlaybackType::eAnimated));
 
   if (aState.mCompositedFrameInvalid) {
     MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable());
-    MOZ_ASSERT(aState.GetHasBeenDecoded());
+    MOZ_ASSERT(aState.GetHasRequestedDecode());
     MOZ_ASSERT(!aState.GetIsCurrentlyDecoded());
     if (result.Type() == MatchType::NOT_FOUND) {
       return result;
     }
     return LookupResult(MatchType::PENDING);
   }
 
   // If we have a composited version of this frame, return that.
@@ -507,17 +509,17 @@ FrameAnimator::GetTimeoutForFrame(Animat
                                   uint32_t aFrameNum) const
 {
   RawAccessFrameRef frame = GetRawFrame(aFrames, aFrameNum);
   if (frame) {
     AnimationData data = frame->GetAnimationData();
     return Some(data.mTimeout);
   }
 
-  MOZ_ASSERT(aState.mHasBeenDecoded && !aState.mIsCurrentlyDecoded);
+  MOZ_ASSERT(aState.mHasRequestedDecode && !aState.mIsCurrentlyDecoded);
   return Nothing();
 }
 
 static void
 DoCollectSizeOfCompositingSurfaces(const RawAccessFrameRef& aSurface,
                                    SurfaceMemoryCounterType aType,
                                    nsTArray<SurfaceMemoryCounter>& aCounters,
                                    MallocSizeOf aMallocSizeOf)
--- a/image/FrameAnimator.h
+++ b/image/FrameAnimator.h
@@ -29,16 +29,17 @@ public:
   explicit AnimationState(uint16_t aAnimationMode)
     : mFrameCount(0)
     , mCurrentAnimationFrameIndex(0)
     , mLoopRemainingCount(-1)
     , mLoopCount(-1)
     , mFirstFrameTimeout(FrameTimeout::FromRawMilliseconds(0))
     , mAnimationMode(aAnimationMode)
     , mHasBeenDecoded(false)
+    , mHasRequestedDecode(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,
@@ -62,16 +63,21 @@ public:
   void NotifyDecodeComplete();
 
   /**
    * Returns true if this image has been fully decoded before.
    */
   bool GetHasBeenDecoded() { return mHasBeenDecoded; }
 
   /**
+   * Returns true if this image has ever requested a decode before.
+   */
+  bool GetHasRequestedDecode() { return mHasRequestedDecode; }
+
+  /**
    * 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.
    */
@@ -216,16 +222,19 @@ private:
    * false when we are able to advance to the frame that should be showing
    * for the current time. mIsCurrentlyDecoded gets set to true when the
    * redecode finishes.
    */
 
   //! Whether this image has been decoded at least once.
   bool mHasBeenDecoded;
 
+  //! Whether this image has ever requested a decode.
+  bool mHasRequestedDecode;
+
   //! Whether this image is currently fully decoded.
   bool mIsCurrentlyDecoded;
 
   //! Whether the composited frame is valid to draw to the screen, note that
   //! the composited frame can exist and be filled with image data but not
   //! valid to draw to the screen.
   bool mCompositedFrameInvalid;