Bug 1462355 - Part 6. Reuse RawAccessFrameRef in FrameAnimator where possible. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Tue, 29 May 2018 08:36:12 -0400
changeset 420279 a7331d229cc8add978906ccc5582795bceb58d81
parent 420278 c67a6f1315b49a4faeec778709ab0d3a956a57dd
child 420280 baeab7da1768fb4741c1ee8942a04e70340fc3b4
push id34069
push usernerli@mozilla.com
push dateTue, 29 May 2018 21:42:06 +0000
treeherdermozilla-central@89d79c2258be [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1462355
milestone62.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 1462355 - Part 6. Reuse RawAccessFrameRef in FrameAnimator where possible. r=tnikkel In FrameAnimator::RequestRefresh and AdvanceFrame, we currently create several RawAccessFrameRef objects to the same frames, either to get timeouts or perform the blending. With some tweaking, we can avoid requesting the same frame more than once. This will avoid mutex locks on the surface provider and the frame itself.
image/FrameAnimator.cpp
image/FrameAnimator.h
--- a/image/FrameAnimator.cpp
+++ b/image/FrameAnimator.cpp
@@ -207,50 +207,40 @@ AnimationState::LoopLength() const
   return *mLoopLength;
 }
 
 
 ///////////////////////////////////////////////////////////////////////////////
 // FrameAnimator implementation.
 ///////////////////////////////////////////////////////////////////////////////
 
-Maybe<TimeStamp>
+TimeStamp
 FrameAnimator::GetCurrentImgFrameEndTime(AnimationState& aState,
-                                         DrawableSurface& aFrames) const
+                                         FrameTimeout aCurrentTimeout) const
 {
-  TimeStamp currentFrameTime = aState.mCurrentAnimationFrameTime;
-  Maybe<FrameTimeout> timeout =
-    GetTimeoutForFrame(aState, aFrames, aState.mCurrentAnimationFrameIndex);
-
-  if (timeout.isNothing()) {
-    MOZ_ASSERT(aState.GetHasRequestedDecode() && !aState.GetIsCurrentlyDecoded());
-    return Nothing();
-  }
-
-  if (*timeout == FrameTimeout::Forever()) {
+  if (aCurrentTimeout == 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.
     // XXX(seth): It'd be preferable to make our logic work correctly with
     // infinitely long timeouts.
-    return Some(TimeStamp::NowLoRes() +
-                TimeDuration::FromMilliseconds(31536000.0));
+    return TimeStamp::NowLoRes() +
+           TimeDuration::FromMilliseconds(31536000.0);
   }
 
   TimeDuration durationOfTimeout =
-    TimeDuration::FromMilliseconds(double(timeout->AsMilliseconds()));
-  TimeStamp currentFrameEndTime = currentFrameTime + durationOfTimeout;
-
-  return Some(currentFrameEndTime);
+    TimeDuration::FromMilliseconds(double(aCurrentTimeout.AsMilliseconds()));
+  return aState.mCurrentAnimationFrameTime + durationOfTimeout;
 }
 
 RefreshResult
 FrameAnimator::AdvanceFrame(AnimationState& aState,
                             DrawableSurface& aFrames,
+                            RawAccessFrameRef& aCurrentFrame,
                             TimeStamp aTime)
 {
   NS_ASSERTION(aTime <= TimeStamp::Now(),
                "Given time appears to be in the future");
   AUTO_PROFILER_LABEL("FrameAnimator::AdvanceFrame", GRAPHICS);
 
   RefreshResult ret;
 
@@ -326,39 +316,37 @@ FrameAnimator::AdvanceFrame(AnimationSta
   if (nextFrame->GetTimeout() == FrameTimeout::Forever()) {
     ret.mAnimationFinished = true;
   }
 
   if (nextFrameIndex == 0) {
     ret.mDirtyRect = aState.FirstFrameRefreshArea();
   } else {
     MOZ_ASSERT(nextFrameIndex == currentFrameIndex + 1);
-    RawAccessFrameRef currentFrame = aFrames.RawAccessRef(currentFrameIndex);
 
     // Change frame
-    if (!DoBlend(currentFrame, nextFrame, nextFrameIndex, &ret.mDirtyRect)) {
+    if (!DoBlend(aCurrentFrame, nextFrame, nextFrameIndex, &ret.mDirtyRect)) {
       // something went wrong, move on to next
       NS_WARNING("FrameAnimator::AdvanceFrame(): Compositing of frame failed");
       nextFrame->SetCompositingFailed(true);
-      Maybe<TimeStamp> currentFrameEndTime = GetCurrentImgFrameEndTime(aState, aFrames);
-      MOZ_ASSERT(currentFrameEndTime.isSome());
-      aState.mCurrentAnimationFrameTime = *currentFrameEndTime;
+      aState.mCurrentAnimationFrameTime =
+        GetCurrentImgFrameEndTime(aState, aCurrentFrame->GetTimeout());
       aState.mCurrentAnimationFrameIndex = nextFrameIndex;
       aState.mCompositedFrameRequested = false;
+      aCurrentFrame = Move(nextFrame);
       aFrames.Advance(nextFrameIndex);
 
       return ret;
     }
 
     nextFrame->SetCompositingFailed(false);
   }
 
-  Maybe<TimeStamp> currentFrameEndTime = GetCurrentImgFrameEndTime(aState, aFrames);
-  MOZ_ASSERT(currentFrameEndTime.isSome());
-  aState.mCurrentAnimationFrameTime = *currentFrameEndTime;
+  aState.mCurrentAnimationFrameTime =
+    GetCurrentImgFrameEndTime(aState, aCurrentFrame->GetTimeout());
 
   // 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(). We also skip this for images with a finite loop
   // count if we have initialized mLoopRemainingCount (it only gets initialized
   // after one full loop).
   FrameTimeout loopTime = aState.LoopLength();
@@ -385,16 +373,17 @@ FrameAnimator::AdvanceFrame(AnimationSta
         aState.mLoopRemainingCount -= CheckedInt32(loops).value();
       }
     }
   }
 
   // Set currentAnimationFrameIndex at the last possible moment
   aState.mCurrentAnimationFrameIndex = nextFrameIndex;
   aState.mCompositedFrameRequested = false;
+  aCurrentFrame = Move(nextFrame);
   aFrames.Advance(nextFrameIndex);
 
   // If we're here, we successfully advanced the frame.
   ret.mFrameAdvanced = true;
 
   return ret;
 }
 
@@ -444,60 +433,65 @@ FrameAnimator::RequestRefresh(AnimationS
   if (aState.IsDiscarded() || !result) {
     aState.MaybeAdvanceAnimationFrameTime(aTime);
     if (!ret.mDirtyRect.IsEmpty()) {
       ret.mFrameAdvanced = true;
     }
     return ret;
   }
 
+  RawAccessFrameRef currentFrame =
+    result.Surface().RawAccessRef(aState.mCurrentAnimationFrameIndex);
+
   // 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()) {
+  if (!currentFrame) {
     MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable());
     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.
     aState.MaybeAdvanceAnimationFrameTime(aTime);
     return ret;
   }
 
+  TimeStamp currentFrameEndTime =
+    GetCurrentImgFrameEndTime(aState, currentFrame->GetTimeout());
+
   // If nothing has accessed the composited frame since the last time we
   // advanced, then there is no point in continuing to advance the animation.
   // This has the effect of freezing the animation while not in view.
   if (!aState.mCompositedFrameRequested &&
       aState.MaybeAdvanceAnimationFrameTime(aTime)) {
     return ret;
   }
 
-  while (*currentFrameEndTime <= aTime) {
-    TimeStamp oldFrameEndTime = *currentFrameEndTime;
+  while (currentFrameEndTime <= aTime) {
+    TimeStamp oldFrameEndTime = currentFrameEndTime;
 
-    RefreshResult frameRes = AdvanceFrame(aState, result.Surface(), aTime);
+    RefreshResult frameRes = AdvanceFrame(aState, result.Surface(),
+                                          currentFrame, aTime);
 
     // Accumulate our result for returning to callers.
     ret.Accumulate(frameRes);
 
-    currentFrameEndTime = GetCurrentImgFrameEndTime(aState, result.Surface());
-    // AdvanceFrame can't advance to a frame that doesn't exist yet.
-    MOZ_ASSERT(currentFrameEndTime.isSome());
+    // currentFrame was updated by AdvanceFrame so it is still current.
+    currentFrameEndTime =
+      GetCurrentImgFrameEndTime(aState, currentFrame->GetTimeout());
 
     // 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)) {
+    if (!frameRes.mFrameAdvanced && currentFrameEndTime == oldFrameEndTime) {
       break;
     }
   }
 
   // Advanced to the correct frame, the composited frame is now valid to be drawn.
-  if (*currentFrameEndTime > aTime) {
+  if (currentFrameEndTime > aTime) {
     aState.mCompositedFrameInvalid = false;
     ret.mDirtyRect = IntRect(IntPoint(0,0), mSize);
   }
 
   MOZ_ASSERT(!aState.mIsCurrentlyDecoded || !aState.mCompositedFrameInvalid);
 
   return ret;
 }
@@ -546,30 +540,16 @@ 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 = aFrames.RawAccessRef(aFrameNum);
-  if (frame) {
-    return Some(frame->GetTimeout());
-  }
-
-  MOZ_ASSERT(aState.mHasRequestedDecode && !aState.mIsCurrentlyDecoded);
-  return Nothing();
-}
-
 static void
 DoCollectSizeOfCompositingSurfaces(const RawAccessFrameRef& aSurface,
                                    SurfaceMemoryCounterType aType,
                                    nsTArray<SurfaceMemoryCounter>& aCounters,
                                    MallocSizeOf aMallocSizeOf)
 {
   // Concoct a SurfaceKey for this surface.
   SurfaceKey key = RasterSurfaceKey(aSurface->GetImageSize(),
--- a/image/FrameAnimator.h
+++ b/image/FrameAnimator.h
@@ -338,36 +338,36 @@ private: // methods
    * Advances the animation. Typically, this will advance a single frame, but it
    * may advance multiple frames. This may happen if we have infrequently
    * "ticking" refresh drivers (e.g. in background tabs), or extremely short-
    * lived animation frames.
    *
    * @param aTime the time that the animation should advance to. This will
    *              typically be <= TimeStamp::Now().
    *
+   * @param aCurrentFrame the currently displayed frame of the animation. If
+   *                      we advance, it will replace aCurrentFrame with the
+   *                      new current frame we advanced to.
+   *
    * @returns a RefreshResult that shows whether the frame was successfully
    *          advanced, and its resulting dirty rect.
    */
   RefreshResult AdvanceFrame(AnimationState& aState,
                              DrawableSurface& aFrames,
+                             RawAccessFrameRef& aCurrentFrame,
                              TimeStamp aTime);
 
-  /// @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,
-                                             DrawableSurface& aFrames) const;
+  TimeStamp GetCurrentImgFrameEndTime(AnimationState& aState,
+                                      FrameTimeout aCurrentTimeout) const;
 
   bool DoBlend(const RawAccessFrameRef& aPrevFrame,
                const RawAccessFrameRef& aNextFrame,
                uint32_t aNextFrameIndex,
                gfx::IntRect* aDirtyRect);
 
   /** Clears an area of <aFrame> with transparent black.
    *