Bug 1510601 - Part 2. Improve recycling of frames when an animation is reset. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Thu, 29 Nov 2018 13:59:53 -0500
changeset 449438 d8a3f89d4bb67f3d9c17fa9751f97be3ef1a0a38
parent 449437 0861a85d13bb1664c019b76fc543d2679e39c99f
child 449439 7d32febb06fde6818e60f875f2e39f98e9107d7a
push id35163
push usershindli@mozilla.com
push dateWed, 05 Dec 2018 21:36:23 +0000
treeherdermozilla-central@643a4a6bbfe9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1510601
milestone65.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 1510601 - Part 2. Improve recycling of frames when an animation is reset. r=tnikkel When an animation is reset, we can actually avoid the reallocations by keeping the frames in the mRecycle and mDiscard queues. There is no real need to do this. We just need to make sure the recycle rect calculation is done appropriately, particularly when we haven't reached the end yet and thus don't know the first frame refresh area yet. Differential Revision: https://phabricator.services.mozilla.com/D13465
image/AnimationFrameBuffer.cpp
image/test/gtest/TestAnimationFrameBuffer.cpp
--- a/image/AnimationFrameBuffer.cpp
+++ b/image/AnimationFrameBuffer.cpp
@@ -303,16 +303,20 @@ void AnimationFrameDiscardingQueue::AddS
 AnimationFrameRecyclingQueue::AnimationFrameRecyclingQueue(
     AnimationFrameRetainedBuffer&& aQueue)
     : AnimationFrameDiscardingQueue(std::move(aQueue)),
       mForceUseFirstFrameRefreshArea(false) {
   // In an ideal world, we would always save the already displayed frames for
   // recycling but none of the frames were marked as recyclable. We will incur
   // the extra allocation cost for a few more frames.
   mRecycling = true;
+
+  // Until we reach the end of the animation, set the first frame refresh area
+  // to match that of the full area of the first frame.
+  mFirstFrameRefreshArea = mFirstFrame->GetRect();
 }
 
 void AnimationFrameRecyclingQueue::AddSizeOfExcludingThis(
     MallocSizeOf aMallocSizeOf, const AddSizeOfCb& aCallback) {
   AnimationFrameDiscardingQueue::AddSizeOfExcludingThis(aMallocSizeOf,
                                                         aCallback);
 
   for (const RecycleEntry& entry : mRecycle) {
@@ -377,30 +381,41 @@ void AnimationFrameRecyclingQueue::Advan
       // is still in use.
       newPending = 1;
     }
     mPending = newPending;
   }
 }
 
 bool AnimationFrameRecyclingQueue::ResetInternal() {
-  mRecycle.clear();
+  // We should save any display frames that we can to save on at least the
+  // allocation. The first frame refresh area is guaranteed to be the aggregate
+  // dirty rect or the entire frame, and so the bare minimum area we can
+  // recycle. We don't need to worry about updating the dirty rect for the
+  // existing mRecycle entries, because that will happen in RecycleFrame when
+  // we try to pull out a frame to redecode the first frame.
+  for (RefPtr<imgFrame>& frame : mDisplay) {
+    if (frame->ShouldRecycle()) {
+      RecycleEntry newEntry(mFirstFrameRefreshArea);
+      newEntry.mFrame = std::move(frame);
+      mRecycle.push_back(std::move(newEntry));
+    }
+  }
+
   return AnimationFrameDiscardingQueue::ResetInternal();
 }
 
 RawAccessFrameRef AnimationFrameRecyclingQueue::RecycleFrame(
     gfx::IntRect& aRecycleRect) {
   if (mInsertIndex == 0) {
     // If we are recreating the first frame, then we actually have already
     // precomputed aggregate of the dirty rects as the first frame refresh
     // area. We know that all of the frames still in the recycling queue
     // need to take into account the same dirty rect because they are also
     // frames which cross the boundary.
-    MOZ_ASSERT(mSizeKnown);
-    MOZ_ASSERT(!mFirstFrameRefreshArea.IsEmpty());
     for (RecycleEntry& entry : mRecycle) {
       MOZ_ASSERT(mFirstFrameRefreshArea.Contains(entry.mDirtyRect));
       entry.mDirtyRect = mFirstFrameRefreshArea;
     }
     // Until we advance to the first frame again, any subsequent recycled
     // frames should also use the first frame refresh area.
     mForceUseFirstFrameRefreshArea = true;
   }
@@ -446,18 +461,17 @@ RawAccessFrameRef AnimationFrameRecyclin
   return recycledFrame;
 }
 
 bool AnimationFrameRecyclingQueue::MarkComplete(
     const gfx::IntRect& aFirstFrameRefreshArea) {
   bool continueDecoding =
       AnimationFrameDiscardingQueue::MarkComplete(aFirstFrameRefreshArea);
 
-  MOZ_ASSERT_IF(!mRedecodeError, mFirstFrameRefreshArea.IsEmpty() ||
-                                     mFirstFrameRefreshArea.IsEqualEdges(
-                                         aFirstFrameRefreshArea));
-
-  mFirstFrameRefreshArea = aFirstFrameRefreshArea;
+  // If we encounter a redecode error, just make the first frame refresh area to
+  // be the full frame, because we don't really know what we can safely recycle.
+  mFirstFrameRefreshArea = mRedecodeError ? mFirstFrame->GetRect()
+                                          : aFirstFrameRefreshArea;
   return continueDecoding;
 }
 
 }  // namespace image
 }  // namespace mozilla
--- a/image/test/gtest/TestAnimationFrameBuffer.cpp
+++ b/image/test/gtest/TestAnimationFrameBuffer.cpp
@@ -141,17 +141,18 @@ VerifyInsertAndAdvance(AnimationFrameBuf
 static void
 VerifyMarkComplete(AnimationFrameBuffer& aQueue,
                    bool aExpectedContinue,
                    const IntRect& aRefreshArea = IntRect(0, 0, 1, 1))
 {
   if (aQueue.IsRecycling() && !aQueue.SizeKnown()) {
     const AnimationFrameRecyclingQueue& queue =
       *static_cast<AnimationFrameRecyclingQueue*>(&aQueue);
-    EXPECT_TRUE(queue.FirstFrameRefreshArea().IsEmpty());
+    EXPECT_EQ(queue.FirstFrame()->GetRect(),
+              queue.FirstFrameRefreshArea());
   }
 
   bool keepDecoding = aQueue.MarkComplete(aRefreshArea);
   EXPECT_EQ(aExpectedContinue, keepDecoding);
 
   if (aQueue.IsRecycling()) {
     const AnimationFrameRecyclingQueue& queue =
       *static_cast<AnimationFrameRecyclingQueue*>(&aQueue);
@@ -189,22 +190,16 @@ VerifyReset(AnimationFrameBuffer& aQueue
   } else {
     const AnimationFrameDiscardingQueue& queue =
       *static_cast<AnimationFrameDiscardingQueue*>(&aQueue);
     EXPECT_EQ(size_t(0), queue.PendingInsert());
     EXPECT_EQ(size_t(0), queue.Display().size());
     EXPECT_EQ(aFirstFrame, queue.FirstFrame());
     EXPECT_EQ(nullptr, aQueue.Get(0, false));
   }
-
-  if (aQueue.IsRecycling()) {
-    const AnimationFrameRecyclingQueue& queue =
-      *static_cast<AnimationFrameRecyclingQueue*>(&aQueue);
-    EXPECT_EQ(size_t(0), queue.Recycle().size());
-  }
 }
 
 class ImageAnimationFrameBuffer : public ::testing::Test
 {
 public:
   ImageAnimationFrameBuffer()
   { }
 
@@ -740,16 +735,70 @@ TEST_F(ImageAnimationFrameBuffer, Recycl
   const size_t kStartFrame = 0;
   AnimationFrameRetainedBuffer retained(kThreshold, kBatch, kStartFrame);
   PrepareForDiscardingQueue(retained);
   const imgFrame* firstFrame = retained.Frames()[0].get();
   AnimationFrameRecyclingQueue buffer(std::move(retained));
   TestDiscardingQueueReset(buffer, firstFrame, kThreshold, kBatch, kStartFrame);
 }
 
+TEST_F(ImageAnimationFrameBuffer, RecyclingResetBeforeComplete)
+{
+  const size_t kThreshold = 3;
+  const size_t kBatch = 1;
+  const size_t kStartFrame = 0;
+  const IntSize kImageSize(100, 100);
+  const IntRect kImageRect(IntPoint(0, 0), kImageSize);
+  AnimationFrameRetainedBuffer retained(kThreshold, kBatch, kStartFrame);
+
+  // Get the starting buffer to just before the point where we need to switch
+  // to a discarding buffer, reset the animation so advancing points at the
+  // first frame, and insert the last frame to cross the threshold.
+  RefPtr<imgFrame> frame;
+  frame = CreateEmptyFrame(kImageSize, kImageRect, false);
+  AnimationFrameBuffer::InsertStatus status = retained.Insert(std::move(frame));
+  EXPECT_EQ(AnimationFrameBuffer::InsertStatus::CONTINUE, status);
+
+  frame = CreateEmptyFrame(kImageSize, IntRect(IntPoint(10, 10), IntSize(1, 1)), false);
+  status = retained.Insert(std::move(frame));
+  EXPECT_EQ(AnimationFrameBuffer::InsertStatus::YIELD, status);
+
+  VerifyAdvance(retained, 1, true);
+
+  frame = CreateEmptyFrame(kImageSize, IntRect(IntPoint(20, 10), IntSize(1, 1)), false);
+  status = retained.Insert(std::move(frame));
+  EXPECT_EQ(AnimationFrameBuffer::InsertStatus::DISCARD_YIELD, status);
+
+  AnimationFrameRecyclingQueue buffer(std::move(retained));
+  bool restartDecoding = buffer.Reset();
+  EXPECT_TRUE(restartDecoding);
+
+  // None of the buffers were recyclable.
+  EXPECT_TRUE(buffer.Recycle().empty());
+
+  // Reinsert the first two frames as recyclable and reset again.
+  frame = CreateEmptyFrame(kImageSize, kImageRect, true);
+  status = buffer.Insert(std::move(frame));
+  EXPECT_EQ(AnimationFrameBuffer::InsertStatus::CONTINUE, status);
+
+  frame = CreateEmptyFrame(kImageSize, IntRect(IntPoint(10, 10), IntSize(1, 1)), true);
+  status = buffer.Insert(std::move(frame));
+  EXPECT_EQ(AnimationFrameBuffer::InsertStatus::YIELD, status);
+
+  restartDecoding = buffer.Reset();
+  EXPECT_TRUE(restartDecoding);
+
+  // Now both buffers should have been saved and the dirty rect replaced with
+  // the full image rect since we don't know the first frame refresh area yet.
+  EXPECT_EQ(size_t(2), buffer.Recycle().size());
+  for (const auto& entry : buffer.Recycle()) {
+    EXPECT_EQ(kImageRect, entry.mDirtyRect);
+  }
+}
+
 TEST_F(ImageAnimationFrameBuffer, RecyclingRect)
 {
   const size_t kThreshold = 5;
   const size_t kBatch = 2;
   const size_t kStartFrame = 0;
   const IntSize kImageSize(100, 100);
   AnimationFrameRetainedBuffer retained(kThreshold, kBatch, kStartFrame);