Bug 1504699 - Part 7. Update animated image recycling queue to work well with WebRender. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Tue, 16 Oct 2018 08:45:26 -0400
changeset 447457 ccfa531dea0301244c99db4ff26d5610382487e2
parent 447456 ecddcae1b26630a30263749186c5250030f72dfb
child 447458 3545e39d77b5e1cb998ce99bc1e0254a34261a62
push id110044
push useraosmond@gmail.com
push dateWed, 21 Nov 2018 11:48:50 +0000
treeherdermozilla-inbound@ccfa531dea03 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1504699
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 1504699 - Part 7. Update animated image recycling queue to work well with WebRender. r=tnikkel WebRender takes longer than OMTP to release its hold on the current frame. This is because it is in a separate process and holds onto the surface in between rendering frames, rather than getting a reference for each repaint. This patch makes us less aggressive about taking the most recent surface placed in the recycling queue out to avoid blocking on waiting for the surface to be released. Differential Revision: https://phabricator.services.mozilla.com/D10903
image/AnimationFrameBuffer.cpp
image/AnimationSurfaceProvider.cpp
image/test/gtest/TestAnimationFrameBuffer.cpp
--- a/image/AnimationFrameBuffer.cpp
+++ b/image/AnimationFrameBuffer.cpp
@@ -216,21 +216,20 @@ AnimationFrameDiscardingQueue::MarkCompl
 void
 AnimationFrameDiscardingQueue::AdvanceInternal()
 {
   // We only want to change the current frame index if we have advanced. This
   // means either a higher frame index, or going back to the beginning.
   // We should never have advanced beyond the frame buffer.
   MOZ_ASSERT(mGetIndex < mSize);
 
-  // Unless we are recycling, we should have the current frame still in the
-  // display queue. Either way, we should at least have an entry in the queue
-  // which we need to consume.
-  MOZ_ASSERT(mRecycling || bool(mDisplay.front()));
+  // We should have the current frame still in the display queue. Either way,
+  // we should at least have an entry in the queue which we need to consume.
   MOZ_ASSERT(!mDisplay.empty());
+  MOZ_ASSERT(mDisplay.front());
   mDisplay.pop_front();
   MOZ_ASSERT(!mDisplay.empty());
   MOZ_ASSERT(mDisplay.front());
 
   if (mDisplay.size() + mPending - 1 < mBatch) {
     // If we have fewer frames than the batch size, then ask for more. If we
     // do not have any pending, then we know that there is no active decoding.
     mPending += mBatch;
@@ -349,16 +348,21 @@ AnimationFrameRecyclingQueue::AddSizeOfE
       );
     }
   }
 }
 
 void
 AnimationFrameRecyclingQueue::AdvanceInternal()
 {
+  // We only want to change the current frame index if we have advanced. This
+  // means either a higher frame index, or going back to the beginning.
+  // We should never have advanced beyond the frame buffer.
+  MOZ_ASSERT(mGetIndex < mSize);
+
   MOZ_ASSERT(!mDisplay.empty());
   MOZ_ASSERT(mDisplay.front());
 
   RefPtr<imgFrame>& front = mDisplay.front();
 
   // The first frame should always have a dirty rect that matches the frame
   // rect. As such, we should use mFirstFrameRefreshArea instead for recycle
   // rect calculations.
@@ -383,17 +387,40 @@ AnimationFrameRecyclingQueue::AdvanceInt
     }
 
     newEntry.mFrame = std::move(front);
   }
 
   // Even if the frame itself isn't saved, we want the dirty rect to calculate
   // the recycle rect for future recycled frames.
   mRecycle.push_back(std::move(newEntry));
-  AnimationFrameDiscardingQueue::AdvanceInternal();
+  mDisplay.pop_front();
+  MOZ_ASSERT(!mDisplay.empty());
+  MOZ_ASSERT(mDisplay.front());
+
+  if (mDisplay.size() + mPending - 1 < mBatch) {
+    // If we have fewer frames than the batch size, then ask for more. If we
+    // do not have any pending, then we know that there is no active decoding.
+    //
+    // We limit the batch to avoid using the frame we just added to the queue.
+    // This gives other parts of the system time to switch to the new current
+    // frame, and maximize buffer reuse. In particular this is useful for
+    // WebRender which holds onto the previous frame for much longer.
+    size_t newPending = std::min(mPending + mBatch, mRecycle.size() - 1);
+    if (newPending == 0 && (mDisplay.size() <= 1 || mPending > 0)) {
+      // If we already have pending frames, then the decoder is active and we
+      // cannot go below one. If we are displaying the only frame we have, and
+      // there are none pending, then we must request at least one more frame to
+      // continue to animation, because we won't advance again without a new
+      // frame. This may cause us to skip recycling because the previous frame
+      // is still in use.
+      newPending = 1;
+    }
+    mPending = newPending;
+  }
 }
 
 bool
 AnimationFrameRecyclingQueue::ResetInternal()
 {
   mRecycle.clear();
   return AnimationFrameDiscardingQueue::ResetInternal();
 }
--- a/image/AnimationSurfaceProvider.cpp
+++ b/image/AnimationSurfaceProvider.cpp
@@ -421,18 +421,17 @@ AnimationSurfaceProvider::RequestFrameDi
   auto oldFrameQueue = static_cast<AnimationFrameRetainedBuffer*>(mFrames.get());
 
   // We only recycle if it is a full frame. Partial frames may be sized
   // differently from each other. We do not support recycling with WebRender
   // and shared surfaces at this time as there is additional synchronization
   // required to know when it is safe to recycle.
   MOZ_ASSERT(!mDecoder->GetFrameRecycler());
   if (gfxPrefs::ImageAnimatedDecodeOnDemandRecycle() &&
-      mDecoder->ShouldBlendAnimation() &&
-      (!gfxVars::GetUseWebRenderOrDefault() || !gfxPrefs::ImageMemShared())) {
+      mDecoder->ShouldBlendAnimation()) {
     mFrames.reset(new AnimationFrameRecyclingQueue(std::move(*oldFrameQueue)));
     mDecoder->SetFrameRecycler(this);
   } else {
     mFrames.reset(new AnimationFrameDiscardingQueue(std::move(*oldFrameQueue)));
   }
 }
 
 void
--- a/image/test/gtest/TestAnimationFrameBuffer.cpp
+++ b/image/test/gtest/TestAnimationFrameBuffer.cpp
@@ -483,17 +483,25 @@ static void TestDiscardingQueueLoop(Anim
   EXPECT_EQ(size_t(3), aQueue.PendingDecode());
   VerifyDiscardingQueueContents(aQueue);
 
   // Make sure frames get removed as we advance.
   VerifyInsertAndAdvance(aQueue, 5, AnimationFrameBuffer::InsertStatus::CONTINUE);
   EXPECT_EQ(size_t(1), aQueue.Display().size());
   VerifyInsertAndAdvance(aQueue, 6, AnimationFrameBuffer::InsertStatus::CONTINUE);
   EXPECT_EQ(size_t(1), aQueue.Display().size());
-  VerifyInsertAndAdvance(aQueue, 7, AnimationFrameBuffer::InsertStatus::CONTINUE);
+
+  // We actually will yield if we are recycling instead of continuing because
+  // the pending calculation is slightly different. We will actually request one
+  // less frame than we have to recycle.
+  if (aQueue.IsRecycling()) {
+    VerifyInsertAndAdvance(aQueue, 7, AnimationFrameBuffer::InsertStatus::YIELD);
+  } else {
+    VerifyInsertAndAdvance(aQueue, 7, AnimationFrameBuffer::InsertStatus::CONTINUE);
+  }
   EXPECT_EQ(size_t(1), aQueue.Display().size());
 
   // We should get throttled if we insert too much.
   VerifyInsert(aQueue, AnimationFrameBuffer::InsertStatus::CONTINUE);
   EXPECT_EQ(size_t(2), aQueue.Display().size());
   EXPECT_EQ(size_t(1), aQueue.PendingDecode());
   VerifyInsert(aQueue, AnimationFrameBuffer::InsertStatus::YIELD);
   EXPECT_EQ(size_t(3), aQueue.Display().size());