Bug 1343341. In FrameAnimator look up our frames once and pass them around. r=aosmond
authorTimothy Nikkel <tnikkel@gmail.com>
Fri, 24 Mar 2017 00:57:30 -0500
changeset 397582 1c5e910290349546d1b3b32f86e842b96a982e80
parent 397581 c9be65ad95f69fbd12638347bba33cf9e97bccdb
child 397583 41fac0336580b03487cfdf53d3f746f3b41b020e
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [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. In FrameAnimator look up our frames once and pass them around. r=aosmond The SurfaceCache can discard on any thread at any time. So we could be in the middle of advancing frames of a fully decoded animated image and then the frames could disappear out from under us. Making the code deal with that kind of a situation would make the logic very complicated. So instead just look up the frames once and pass them around, that way they never change during while we are advancing the frame.
image/FrameAnimator.cpp
image/FrameAnimator.h
image/RasterImage.cpp
--- a/image/FrameAnimator.cpp
+++ b/image/FrameAnimator.cpp
@@ -137,20 +137,22 @@ AnimationState::LoopLength() const
 }
 
 
 ///////////////////////////////////////////////////////////////////////////////
 // FrameAnimator implementation.
 ///////////////////////////////////////////////////////////////////////////////
 
 Maybe<TimeStamp>
-FrameAnimator::GetCurrentImgFrameEndTime(AnimationState& aState) const
+FrameAnimator::GetCurrentImgFrameEndTime(AnimationState& aState,
+                                         DrawableSurface& aFrames) const
 {
   TimeStamp currentFrameTime = aState.mCurrentAnimationFrameTime;
-  Maybe<FrameTimeout> timeout = GetTimeoutForFrame(aState, aState.mCurrentAnimationFrameIndex);
+  Maybe<FrameTimeout> timeout =
+    GetTimeoutForFrame(aState, aFrames, aState.mCurrentAnimationFrameIndex);
 
   if (timeout.isNothing()) {
     MOZ_ASSERT(aState.GetHasBeenDecoded() && !aState.GetIsCurrentlyDecoded());
     return Nothing();
   }
 
   if (*timeout == FrameTimeout::Forever()) {
     // We need to return a sentinel value in this case, because our logic
@@ -166,17 +168,19 @@ FrameAnimator::GetCurrentImgFrameEndTime
   TimeDuration durationOfTimeout =
     TimeDuration::FromMilliseconds(double(timeout->AsMilliseconds()));
   TimeStamp currentFrameEndTime = currentFrameTime + durationOfTimeout;
 
   return Some(currentFrameEndTime);
 }
 
 RefreshResult
-FrameAnimator::AdvanceFrame(AnimationState& aState, TimeStamp aTime)
+FrameAnimator::AdvanceFrame(AnimationState& aState,
+                            DrawableSurface& aFrames,
+                            TimeStamp aTime)
 {
   NS_ASSERTION(aTime <= TimeStamp::Now(),
                "Given time appears to be in the future");
   PROFILER_LABEL_FUNC(js::ProfileEntry::Category::GRAPHICS);
 
   RefreshResult ret;
 
   // Determine what the next frame is, taking into account looping.
@@ -226,58 +230,58 @@ FrameAnimator::AdvanceFrame(AnimationSta
   }
 
   // There can be frames in the surface cache with index >= KnownFrameCount()
   // which GetRawFrame() can access because an async decoder has decoded them,
   // but which AnimationState doesn't know about yet because we haven't received
   // the appropriate notification on the main thread. Make sure we stay in sync
   // with AnimationState.
   MOZ_ASSERT(nextFrameIndex < aState.KnownFrameCount());
-  RawAccessFrameRef nextFrame = GetRawFrame(nextFrameIndex);
+  RawAccessFrameRef nextFrame = GetRawFrame(aFrames, nextFrameIndex);
 
   // We should always check to see if we have the next frame even if we have
   // previously finished decoding. If we needed to redecode (e.g. due to a draw
   // failure) we would have discarded all the old frames and may not yet have
   // the new ones.
   if (!nextFrame || !nextFrame->IsFinished()) {
     // Uh oh, the frame we want to show is currently being decoded (partial)
     // Wait until the next refresh driver tick and try again
     return ret;
   }
 
-  Maybe<FrameTimeout> nextFrameTimeout = GetTimeoutForFrame(aState, nextFrameIndex);
+  Maybe<FrameTimeout> nextFrameTimeout = GetTimeoutForFrame(aState, aFrames, nextFrameIndex);
   // GetTimeoutForFrame can only return none if frame doesn't exist,
   // but we just got it above.
   MOZ_ASSERT(nextFrameTimeout.isSome());
   if (*nextFrameTimeout == FrameTimeout::Forever()) {
     ret.mAnimationFinished = true;
   }
 
   if (nextFrameIndex == 0) {
     ret.mDirtyRect = aState.FirstFrameRefreshArea();
   } else {
     MOZ_ASSERT(nextFrameIndex == currentFrameIndex + 1);
 
     // Change frame
-    if (!DoBlend(&ret.mDirtyRect, currentFrameIndex, nextFrameIndex)) {
+    if (!DoBlend(aFrames, &ret.mDirtyRect, currentFrameIndex, nextFrameIndex)) {
       // something went wrong, move on to next
       NS_WARNING("FrameAnimator::AdvanceFrame(): Compositing of frame failed");
       nextFrame->SetCompositingFailed(true);
-      Maybe<TimeStamp> currentFrameEndTime = GetCurrentImgFrameEndTime(aState);
+      Maybe<TimeStamp> currentFrameEndTime = GetCurrentImgFrameEndTime(aState, aFrames);
       MOZ_ASSERT(currentFrameEndTime.isSome());
       aState.mCurrentAnimationFrameTime = *currentFrameEndTime;
       aState.mCurrentAnimationFrameIndex = nextFrameIndex;
 
       return ret;
     }
 
     nextFrame->SetCompositingFailed(false);
   }
 
-  Maybe<TimeStamp> currentFrameEndTime = GetCurrentImgFrameEndTime(aState);
+  Maybe<TimeStamp> currentFrameEndTime = GetCurrentImgFrameEndTime(aState, aFrames);
   MOZ_ASSERT(currentFrameEndTime.isSome());
   aState.mCurrentAnimationFrameTime = *currentFrameEndTime;
 
   // If we can get closer to the current time by a multiple of the image's loop
   // time, we should. We can only do this if we're done decoding; otherwise, we
   // don't know the full loop length, and LoopLength() will have to return
   // FrameTimeout::Forever().
   FrameTimeout loopTime = aState.LoopLength();
@@ -307,37 +311,57 @@ FrameAnimator::RequestRefresh(AnimationS
 {
   // By default, an empty RefreshResult.
   RefreshResult ret;
 
   if (aState.IsDiscarded()) {
     return ret;
   }
 
+  // Get the animation frames once now, and pass them down to callees because
+  // the surface could be discarded at anytime on a different thread. This is
+  // 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);
+    }
+    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);
+  Maybe<TimeStamp> currentFrameEndTime =
+    GetCurrentImgFrameEndTime(aState, result.Surface());
   if (currentFrameEndTime.isNothing()) {
     MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable());
     MOZ_ASSERT(aState.GetHasBeenDecoded() && !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;
 
-    RefreshResult frameRes = AdvanceFrame(aState, aTime);
+    RefreshResult frameRes = AdvanceFrame(aState, result.Surface(), aTime);
 
     // Accumulate our result for returning to callers.
     ret.Accumulate(frameRes);
 
-    currentFrameEndTime = GetCurrentImgFrameEndTime(aState);
+    currentFrameEndTime = GetCurrentImgFrameEndTime(aState, result.Surface());
     // AdvanceFrame can't advance to a frame that doesn't exist yet.
     MOZ_ASSERT(currentFrameEndTime.isSome());
 
     // If we didn't advance a frame, and our frame end time didn't change,
     // then we need to break out of this loop & wait for the frame(s)
     // to finish downloading.
     if (!frameRes.mFrameAdvanced && (*currentFrameEndTime == oldFrameEndTime)) {
       break;
@@ -391,19 +415,20 @@ FrameAnimator::GetCompositedFrame(Animat
   MOZ_ASSERT(!result.Surface()->GetIsPaletted(),
              "About to return a paletted frame");
 
   return result;
 }
 
 Maybe<FrameTimeout>
 FrameAnimator::GetTimeoutForFrame(AnimationState& aState,
+                                  DrawableSurface& aFrames,
                                   uint32_t aFrameNum) const
 {
-  RawAccessFrameRef frame = GetRawFrame(aFrameNum);
+  RawAccessFrameRef frame = GetRawFrame(aFrames, aFrameNum);
   if (frame) {
     AnimationData data = frame->GetAnimationData();
     return Some(data.mTimeout);
   }
 
   MOZ_ASSERT(aState.mHasBeenDecoded && !aState.mIsCurrentlyDecoded);
   return Nothing();
 }
@@ -449,47 +474,39 @@ FrameAnimator::CollectSizeOfCompositingS
     DoCollectSizeOfCompositingSurfaces(mCompositingPrevFrame,
                                        SurfaceMemoryCounterType::COMPOSITING_PREV,
                                        aCounters,
                                        aMallocSizeOf);
   }
 }
 
 RawAccessFrameRef
-FrameAnimator::GetRawFrame(uint32_t aFrameNum) const
+FrameAnimator::GetRawFrame(DrawableSurface& aFrames, uint32_t aFrameNum) const
 {
-  LookupResult result =
-    SurfaceCache::Lookup(ImageKey(mImage),
-                         RasterSurfaceKey(mSize,
-                                          DefaultSurfaceFlags(),
-                                          PlaybackType::eAnimated));
-  if (!result) {
-    return RawAccessFrameRef();
-  }
-
   // Seek to the frame we want. If seeking fails, it means we couldn't get the
   // frame we're looking for, so we bail here to avoid returning the wrong frame
   // to the caller.
-  if (NS_FAILED(result.Surface().Seek(aFrameNum))) {
+  if (NS_FAILED(aFrames.Seek(aFrameNum))) {
     return RawAccessFrameRef();  // Not available yet.
   }
 
-  return result.Surface()->RawAccessRef();
+  return aFrames->RawAccessRef();
 }
 
 //******************************************************************************
 // DoBlend gets called when the timer for animation get fired and we have to
 // update the composited frame of the animation.
 bool
-FrameAnimator::DoBlend(IntRect* aDirtyRect,
+FrameAnimator::DoBlend(DrawableSurface& aFrames,
+                       IntRect* aDirtyRect,
                        uint32_t aPrevFrameIndex,
                        uint32_t aNextFrameIndex)
 {
-  RawAccessFrameRef prevFrame = GetRawFrame(aPrevFrameIndex);
-  RawAccessFrameRef nextFrame = GetRawFrame(aNextFrameIndex);
+  RawAccessFrameRef prevFrame = GetRawFrame(aFrames, aPrevFrameIndex);
+  RawAccessFrameRef nextFrame = GetRawFrame(aFrames, aNextFrameIndex);
 
   MOZ_ASSERT(prevFrame && nextFrame, "Should have frames here");
 
   AnimationData prevFrameData = prevFrame->GetAnimationData();
   if (prevFrameData.mDisposalMethod == DisposalMethod::RESTORE_PREVIOUS &&
       !mCompositingPrevFrame) {
     prevFrameData.mDisposalMethod = DisposalMethod::CLEAR;
   }
--- a/image/FrameAnimator.h
+++ b/image/FrameAnimator.h
@@ -16,16 +16,17 @@
 #include "nsRect.h"
 #include "SurfaceCache.h"
 #include "gfxPrefs.h"
 
 namespace mozilla {
 namespace image {
 
 class RasterImage;
+class DrawableSurface;
 
 class AnimationState
 {
 public:
   explicit AnimationState(uint16_t aAnimationMode)
     : mFrameCount(0)
     , mCurrentAnimationFrameIndex(0)
     , mLoopRemainingCount(-1)
@@ -300,36 +301,42 @@ private: // methods
    * lived animation frames.
    *
    * @param aTime the time that the animation should advance to. This will
    *              typically be <= TimeStamp::Now().
    *
    * @returns a RefreshResult that shows whether the frame was successfully
    *          advanced, and its resulting dirty rect.
    */
-  RefreshResult AdvanceFrame(AnimationState& aState, TimeStamp aTime);
+  RefreshResult AdvanceFrame(AnimationState& aState,
+                             DrawableSurface& aFrames,
+                             TimeStamp aTime);
 
   /**
    * Get the @aIndex-th frame in the frame index, ignoring results of blending.
    */
-  RawAccessFrameRef GetRawFrame(uint32_t aFrameNum) const;
+  RawAccessFrameRef GetRawFrame(DrawableSurface& aFrames,
+                                uint32_t aFrameNum) const;
 
   /// @return the given frame's timeout if it is available
   Maybe<FrameTimeout> GetTimeoutForFrame(AnimationState& aState,
+                                         DrawableSurface& aFrames,
                                          uint32_t aFrameNum) const;
 
   /**
    * Get the time the frame we're currently displaying is supposed to end.
    *
    * In the error case (like if the requested frame is not currently
    * decoded), returns None().
    */
-  Maybe<TimeStamp> GetCurrentImgFrameEndTime(AnimationState& aState) const;
+  Maybe<TimeStamp> GetCurrentImgFrameEndTime(AnimationState& aState,
+                                             DrawableSurface& aFrames) const;
 
-  bool DoBlend(gfx::IntRect* aDirtyRect,
+  bool DoBlend(DrawableSurface& aFrames,
+               gfx::IntRect* aDirtyRect,
                uint32_t aPrevFrameIndex,
                uint32_t aNextFrameIndex);
 
   /** Clears an area of <aFrame> with transparent black.
    *
    * @param aFrameData Target Frame data
    * @param aFrameRect The rectangle of the data pointed ot by aFrameData
    *
--- a/image/RasterImage.cpp
+++ b/image/RasterImage.cpp
@@ -1220,26 +1220,26 @@ RasterImage::Decode(const IntSize& aSize
     // If there's no transparency, it doesn't matter whether we premultiply
     // alpha or not.
     surfaceFlags &= ~SurfaceFlags::NO_PREMULTIPLY_ALPHA;
   }
 
   // Create a decoder.
   RefPtr<IDecodingTask> task;
   if (mAnimationState && aPlaybackType == PlaybackType::eAnimated) {
+    task = DecoderFactory::CreateAnimationDecoder(mDecoderType, WrapNotNull(this),
+                                                  mSourceBuffer, mSize,
+                                                  decoderFlags, surfaceFlags);
     mAnimationState->SetDiscarded(false);
     // 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.
     if (mAnimationFinished) {
       mAnimationState->SetCompositedFrameInvalid(false);
     }
-    task = DecoderFactory::CreateAnimationDecoder(mDecoderType, WrapNotNull(this),
-                                                  mSourceBuffer, mSize,
-                                                  decoderFlags, surfaceFlags);
   } else {
     task = DecoderFactory::CreateDecoder(mDecoderType, WrapNotNull(this),
                                          mSourceBuffer, mSize, aSize,
                                          decoderFlags, surfaceFlags);
   }
 
   // Make sure DecoderFactory was able to create a decoder successfully.
   if (!task) {