Bug 1462355 - Part 5. Avoid converting from DrawableFrameRef to RawAccessFrameRef. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Tue, 29 May 2018 08:36:12 -0400
changeset 420278 c67a6f1315b49a4faeec778709ab0d3a956a57dd
parent 420277 93bdeed04a6b61139bc1c1b12088dafc260b3599
child 420279 a7331d229cc8add978906ccc5582795bceb58d81
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 5. Avoid converting from DrawableFrameRef to RawAccessFrameRef. r=tnikkel DrawableSurface only exposes DrawableFrameRef to its users. This is sufficient for the drawing related code in general, but FrameAnimator really needs RawAccessFrameRef to the underlying pixel data (which may be paletted). While one can get a RawAccessFrameRef from a DrawableFrameRef, it requires yet another lock of the imgFrame's mutex. We can avoid this extra lock if we just allow the callers to get the right data type in the first place.
image/AnimationFrameBuffer.cpp
image/AnimationFrameBuffer.h
image/AnimationSurfaceProvider.cpp
image/AnimationSurfaceProvider.h
image/FrameAnimator.cpp
image/FrameAnimator.h
image/ISurfaceProvider.h
image/test/gtest/TestAnimationFrameBuffer.cpp
--- a/image/AnimationFrameBuffer.cpp
+++ b/image/AnimationFrameBuffer.cpp
@@ -171,42 +171,42 @@ AnimationFrameBuffer::MarkComplete()
       // frames. If we also hit the last frame, we don't want to ask for more.
       mPending = 0;
     }
   }
 
   return mPending > 0;
 }
 
-DrawableFrameRef
+imgFrame*
 AnimationFrameBuffer::Get(size_t aFrame)
 {
   // We should not have asked for a frame if we never inserted.
   if (mFrames.IsEmpty()) {
     MOZ_ASSERT_UNREACHABLE("Calling Get() when we have no frames");
-    return DrawableFrameRef();
+    return nullptr;
   }
 
   // If we don't have that frame, return an empty frame ref.
   if (aFrame >= mFrames.Length()) {
-    return DrawableFrameRef();
+    return nullptr;
   }
 
   // We've got the requested frame because we are not discarding frames. While
   // we typically should have not run out of frames since we ask for more before
   // we want them, it is possible the decoder is behind.
   if (!mFrames[aFrame]) {
     MOZ_ASSERT(MayDiscard());
-    return DrawableFrameRef();
+    return nullptr;
   }
 
   // If we are advancing on behalf of the animation, we don't expect it to be
   // getting any frames (besides the first) until we get the desired frame.
   MOZ_ASSERT(aFrame == 0 || mAdvance == 0);
-  return mFrames[aFrame]->DrawableRef();
+  return mFrames[aFrame].get();
 }
 
 bool
 AnimationFrameBuffer::AdvanceTo(size_t aExpectedFrame)
 {
   // The owner should only be advancing once it has reached the requested frame
   // in the animation.
   MOZ_ASSERT(mAdvance == 0);
--- a/image/AnimationFrameBuffer.h
+++ b/image/AnimationFrameBuffer.h
@@ -58,17 +58,17 @@ public:
    * frames in sequential order, increasing in tandem with AdvanceTo calls. The
    * first frame may be accessed at any time. The access order should start with
    * the same value as that given in Initialize (aStartFrame).
    *
    * @param aFrame      The frame index to access.
    *
    * @returns The frame, if available.
    */
-  DrawableFrameRef Get(size_t aFrame);
+  imgFrame* Get(size_t aFrame);
 
   /**
    * Inserts a frame into the frame buffer. If it has yet to fully decode the
    * animated image yet, then it will append the frame to its internal buffer.
    * If it has been fully decoded, it will replace the next frame in its buffer
    * with the given frame.
    *
    * Once we have a sufficient number of frames buffered relative to the
--- a/image/AnimationSurfaceProvider.cpp
+++ b/image/AnimationSurfaceProvider.cpp
@@ -129,17 +129,40 @@ AnimationSurfaceProvider::DrawableRef(si
 {
   MutexAutoLock lock(mFramesMutex);
 
   if (Availability().IsPlaceholder()) {
     MOZ_ASSERT_UNREACHABLE("Calling DrawableRef() on a placeholder");
     return DrawableFrameRef();
   }
 
-  return mFrames.Get(aFrame);
+  imgFrame* frame = mFrames.Get(aFrame);
+  if (!frame) {
+    return DrawableFrameRef();
+  }
+
+  return frame->DrawableRef();
+}
+
+RawAccessFrameRef
+AnimationSurfaceProvider::RawAccessRef(size_t aFrame)
+{
+  MutexAutoLock lock(mFramesMutex);
+
+  if (Availability().IsPlaceholder()) {
+    MOZ_ASSERT_UNREACHABLE("Calling RawAccessRef() on a placeholder");
+    return RawAccessFrameRef();
+  }
+
+  imgFrame* frame = mFrames.Get(aFrame);
+  if (!frame) {
+    return RawAccessFrameRef();
+  }
+
+  return frame->RawAccessRef(/* aOnlyFinished */ true);
 }
 
 bool
 AnimationSurfaceProvider::IsFinished() const
 {
   MutexAutoLock lock(mFramesMutex);
 
   if (Availability().IsPlaceholder()) {
--- a/image/AnimationSurfaceProvider.h
+++ b/image/AnimationSurfaceProvider.h
@@ -52,16 +52,17 @@ public:
                               size_t& aHeapSizeOut,
                               size_t& aNonHeapSizeOut,
                               size_t& aExtHandlesOut) override;
   void Reset() override;
   void Advance(size_t aFrame) override;
 
 protected:
   DrawableFrameRef DrawableRef(size_t aFrame) override;
+  RawAccessFrameRef RawAccessRef(size_t aFrame) override;
 
   // Animation frames are always locked. This is because we only want to release
   // their memory atomically (due to the surface cache discarding them). If they
   // were unlocked, the OS could end up releasing the memory of random frames
   // from the middle of the animation, which is not worth the complexity of
   // dealing with.
   bool IsLocked() const override { return true; }
   void SetLocked(bool) override { }
--- a/image/FrameAnimator.cpp
+++ b/image/FrameAnimator.cpp
@@ -301,23 +301,24 @@ 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(aFrames, nextFrameIndex);
+  RawAccessFrameRef nextFrame = aFrames.RawAccessRef(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()) {
+  // the new ones. DrawableSurface::RawAccessRef promises to only return
+  // finished frames.
+  if (!nextFrame) {
     // Uh oh, the frame we want to show is currently being decoded (partial).
     // Similar to the above case, we could be blocked by network or decoding,
     // and so we should advance our current time rather than risk jumping
     // through the animation. We will wait until the next refresh driver tick
     // and try again.
     aState.mCurrentAnimationFrameTime = aTime;
     return ret;
   }
@@ -325,17 +326,17 @@ 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 = GetRawFrame(aFrames, currentFrameIndex);
+    RawAccessFrameRef currentFrame = aFrames.RawAccessRef(currentFrameIndex);
 
     // Change frame
     if (!DoBlend(currentFrame, 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());
@@ -550,17 +551,17 @@ FrameAnimator::GetCompositedFrame(Animat
   return result;
 }
 
 Maybe<FrameTimeout>
 FrameAnimator::GetTimeoutForFrame(AnimationState& aState,
                                   DrawableSurface& aFrames,
                                   uint32_t aFrameNum) const
 {
-  RawAccessFrameRef frame = GetRawFrame(aFrames, aFrameNum);
+  RawAccessFrameRef frame = aFrames.RawAccessRef(aFrameNum);
   if (frame) {
     return Some(frame->GetTimeout());
   }
 
   MOZ_ASSERT(aState.mHasRequestedDecode && !aState.mIsCurrentlyDecoded);
   return Nothing();
 }
 
@@ -606,29 +607,16 @@ FrameAnimator::CollectSizeOfCompositingS
   if (mCompositingPrevFrame) {
     DoCollectSizeOfCompositingSurfaces(mCompositingPrevFrame,
                                        SurfaceMemoryCounterType::COMPOSITING_PREV,
                                        aCounters,
                                        aMallocSizeOf);
   }
 }
 
-RawAccessFrameRef
-FrameAnimator::GetRawFrame(DrawableSurface& aFrames, uint32_t aFrameNum) const
-{
-  // 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(aFrames.Seek(aFrameNum))) {
-    return RawAccessFrameRef();  // Not available yet.
-  }
-
-  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(const RawAccessFrameRef& aPrevFrame,
                        const RawAccessFrameRef& aNextFrame,
                        uint32_t aNextFrameIndex,
                        IntRect* aDirtyRect)
--- a/image/FrameAnimator.h
+++ b/image/FrameAnimator.h
@@ -345,22 +345,16 @@ private: // methods
    *
    * @returns a RefreshResult that shows whether the frame was successfully
    *          advanced, and its resulting dirty rect.
    */
   RefreshResult AdvanceFrame(AnimationState& aState,
                              DrawableSurface& aFrames,
                              TimeStamp aTime);
 
-  /**
-   * Get the @aIndex-th frame in the frame index, ignoring results of blending.
-   */
-  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.
    *
--- a/image/ISurfaceProvider.h
+++ b/image/ISurfaceProvider.h
@@ -105,16 +105,25 @@ protected:
 
   virtual ~ISurfaceProvider() { }
 
   /// @return an eagerly computed drawable reference to a surface. For
   /// dynamically generated animation surfaces, @aFrame specifies the 0-based
   /// index of the desired frame.
   virtual DrawableFrameRef DrawableRef(size_t aFrame) = 0;
 
+  /// @return an eagerly computed raw access reference to a surface. For
+  /// dynamically generated animation surfaces, @aFrame specifies the 0-based
+  /// index of the desired frame.
+  virtual RawAccessFrameRef RawAccessRef(size_t aFrame)
+  {
+    MOZ_ASSERT_UNREACHABLE("Surface provider does not support raw access!");
+    return RawAccessFrameRef();
+  }
+
   /// @return true if this ISurfaceProvider is locked. (@see SetLocked())
   /// Should only be called from SurfaceCache code as it relies on SurfaceCache
   /// for synchronization.
   virtual bool IsLocked() const = 0;
 
   /// If @aLocked is true, hint that this ISurfaceProvider is in use and it
   /// should avoid releasing its resources. Should only be called from
   /// SurfaceCache code as it relies on SurfaceCache for synchronization.
@@ -194,16 +203,28 @@ public:
       return NS_ERROR_FAILURE;
     }
 
     mDrawableRef = mProvider->DrawableRef(aFrame);
 
     return mDrawableRef ? NS_OK : NS_ERROR_FAILURE;
   }
 
+  RawAccessFrameRef RawAccessRef(size_t aFrame)
+  {
+    MOZ_ASSERT(mHaveSurface, "Trying to get on an empty DrawableSurface?");
+
+    if (!mProvider) {
+      MOZ_ASSERT_UNREACHABLE("Trying to get on a static DrawableSurface?");
+      return RawAccessFrameRef();
+    }
+
+    return mProvider->RawAccessRef(aFrame);
+  }
+
   void Reset()
   {
     if (!mProvider) {
       MOZ_ASSERT_UNREACHABLE("Trying to reset a static DrawableSurface?");
       return;
     }
 
     mProvider->Reset();
--- a/image/test/gtest/TestAnimationFrameBuffer.cpp
+++ b/image/test/gtest/TestAnimationFrameBuffer.cpp
@@ -143,39 +143,38 @@ TEST_F(ImageAnimationFrameBuffer, Finish
       EXPECT_FALSE(keepDecoding);
       EXPECT_TRUE(buffer.SizeKnown());
       EXPECT_EQ(size_t(0), buffer.PendingDecode());
       EXPECT_FALSE(buffer.HasRedecodeError());
     }
 
     EXPECT_FALSE(buffer.MayDiscard());
 
-    DrawableFrameRef gotFrame = buffer.Get(i);
-    EXPECT_EQ(frame.get(), gotFrame.get());
+    imgFrame* gotFrame = buffer.Get(i);
+    EXPECT_EQ(frame.get(), gotFrame);
     ASSERT_EQ(i + 1, frames.Length());
     EXPECT_EQ(frame.get(), frames[i].get());
 
     if (i == 0) {
       firstFrame = Move(frame);
       EXPECT_EQ(size_t(0), buffer.Displayed());
     } else {
       EXPECT_EQ(i - 1, buffer.Displayed());
       bool restartDecoder = buffer.AdvanceTo(i);
       EXPECT_FALSE(restartDecoder);
       EXPECT_EQ(i, buffer.Displayed());
     }
 
     gotFrame = buffer.Get(0);
-    EXPECT_EQ(firstFrame.get(), gotFrame.get());
+    EXPECT_EQ(firstFrame.get(), gotFrame);
   }
 
   // Loop again over the animation and make sure it is still all there.
   for (size_t i = 0; i < frames.Length(); ++i) {
-    DrawableFrameRef gotFrame = buffer.Get(i);
-    EXPECT_TRUE(gotFrame);
+    EXPECT_TRUE(buffer.Get(i) != nullptr);
 
     bool restartDecoder = buffer.AdvanceTo(i);
     EXPECT_FALSE(restartDecoder);
   }
 }
 
 TEST_F(ImageAnimationFrameBuffer, FinishMultipleBatchesUnderThreshold)
 {
@@ -197,18 +196,17 @@ TEST_F(ImageAnimationFrameBuffer, Finish
 
   EXPECT_EQ(size_t(0), buffer.PendingDecode());
   EXPECT_EQ(size_t(4), frames.Length());
 
   // Progress through the animation until it lets us decode again.
   bool restartDecoder = false;
   size_t i = 0;
   do {
-    DrawableFrameRef gotFrame = buffer.Get(i);
-    EXPECT_TRUE(gotFrame);
+    EXPECT_TRUE(buffer.Get(i) != nullptr);
     if (i > 0) {
       restartDecoder = buffer.AdvanceTo(i);
     }
     ++i;
   } while (!restartDecoder);
 
   EXPECT_EQ(size_t(2), buffer.PendingDecode());
   EXPECT_EQ(size_t(2), buffer.Displayed());
@@ -220,34 +218,31 @@ TEST_F(ImageAnimationFrameBuffer, Finish
   EXPECT_FALSE(keepDecoding);
   EXPECT_TRUE(buffer.SizeKnown());
   EXPECT_EQ(size_t(0), buffer.PendingDecode());
   EXPECT_EQ(size_t(5), frames.Length());
   EXPECT_FALSE(buffer.HasRedecodeError());
 
   // Finish progressing through the animation.
   for ( ; i < frames.Length(); ++i) {
-    DrawableFrameRef gotFrame = buffer.Get(i);
-    EXPECT_TRUE(gotFrame);
+    EXPECT_TRUE(buffer.Get(i) != nullptr);
     restartDecoder = buffer.AdvanceTo(i);
     EXPECT_FALSE(restartDecoder);
   }
 
   // Loop again over the animation and make sure it is still all there.
   for (i = 0; i < frames.Length(); ++i) {
-    DrawableFrameRef gotFrame = buffer.Get(i);
-    EXPECT_TRUE(gotFrame);
+    EXPECT_TRUE(buffer.Get(i) != nullptr);
     restartDecoder = buffer.AdvanceTo(i);
     EXPECT_FALSE(restartDecoder);
   }
 
   // Loop to the third frame and then reset the animation.
   for (i = 0; i < 3; ++i) {
-    DrawableFrameRef gotFrame = buffer.Get(i);
-    EXPECT_TRUE(gotFrame);
+    EXPECT_TRUE(buffer.Get(i) != nullptr);
     restartDecoder = buffer.AdvanceTo(i);
     EXPECT_FALSE(restartDecoder);
   }
 
   // Since we are below the threshold, we can reset the get index only.
   // Nothing else should have changed.
   restartDecoder = buffer.Reset();
   EXPECT_FALSE(restartDecoder);
@@ -277,18 +272,17 @@ TEST_F(ImageAnimationFrameBuffer, MayDis
 
   EXPECT_EQ(size_t(0), buffer.PendingDecode());
   EXPECT_EQ(size_t(6), frames.Length());
 
   // Progress through the animation until it lets us decode again.
   bool restartDecoder = false;
   size_t i = 0;
   do {
-    DrawableFrameRef gotFrame = buffer.Get(i);
-    EXPECT_TRUE(gotFrame);
+    EXPECT_TRUE(buffer.Get(i) != nullptr);
     if (i > 0) {
       restartDecoder = buffer.AdvanceTo(i);
     }
     ++i;
   } while (!restartDecoder);
 
   EXPECT_EQ(size_t(3), buffer.PendingDecode());
   EXPECT_EQ(size_t(3), buffer.Displayed());
@@ -307,18 +301,17 @@ TEST_F(ImageAnimationFrameBuffer, MayDis
   // fourth frame.
   CheckRetained(buffer, 0, 1);
   CheckRemoved(buffer, 1, 3);
   CheckRetained(buffer, 3, 9);
 
   // Progress through the animation so more. Make sure it removes frames as we
   // go along.
   do {
-    DrawableFrameRef gotFrame = buffer.Get(i);
-    EXPECT_TRUE(gotFrame);
+    EXPECT_TRUE(buffer.Get(i) != nullptr);
     restartDecoder = buffer.AdvanceTo(i);
     EXPECT_FALSE(frames[i - 1]);
     EXPECT_TRUE(frames[i]);
     i++;
   } while (!restartDecoder);
 
   EXPECT_EQ(size_t(3), buffer.PendingDecode());
   EXPECT_EQ(size_t(6), buffer.Displayed());
@@ -344,18 +337,17 @@ TEST_F(ImageAnimationFrameBuffer, MayDis
 
   // Advance as far as we can. This should require us to loop the animation to
   // reach a missing frame.
   do {
     if (i == frames.Length()) {
       i = 0;
     }
 
-    DrawableFrameRef gotFrame = buffer.Get(i);
-    if (!gotFrame) {
+    if (!buffer.Get(i)) {
       break;
     }
 
     restartDecoder = buffer.AdvanceTo(i);
     ++i;
   } while (true);
 
   EXPECT_EQ(size_t(3), buffer.PendingDecode());
@@ -363,18 +355,17 @@ TEST_F(ImageAnimationFrameBuffer, MayDis
   EXPECT_EQ(size_t(1), buffer.Displayed());
 
   // Decode some more.
   keepDecoding = Fill(buffer, buffer.PendingDecode());
   EXPECT_FALSE(keepDecoding);
   EXPECT_EQ(size_t(0), buffer.PendingDecode());
 
   // Can we retry advancing again?
-  DrawableFrameRef gotFrame = buffer.Get(i);
-  EXPECT_TRUE(gotFrame);
+  EXPECT_TRUE(buffer.Get(i) != nullptr);
   restartDecoder = buffer.AdvanceTo(i);
   EXPECT_EQ(size_t(2), buffer.Displayed());
   EXPECT_FALSE(frames[i - 1]);
   EXPECT_TRUE(frames[i]);
 
   // Since we are above the threshold, we must reset everything.
   restartDecoder = buffer.Reset();
   EXPECT_FALSE(restartDecoder);