Bug 1509408 - Fix animated image distortion regressions caused by frame recycling. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Mon, 26 Nov 2018 07:30:50 -0500
changeset 504942 c36ca97de3d05dd769986a1ce315c1a4ed9a7f12
parent 504941 d75d80adf8a159071845ff87a79867dedf0e8e0b
child 504943 94ff32de21d6fcb638a3759e12735edf31136033
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1509408, 1508393
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 1509408 - Fix animated image distortion regressions caused by frame recycling. r=tnikkel When bug 1508393 landed, it not only enabled producing of full frames on the decoder threads, it also enabled recycling of old animated image frame buffers it would have otherwise discarded. It reuses the contents of the buffer where possible given we know what pixels changed between the old frame and the frame we want to produce. However where this calculation was done was incorrect. We originally calculated it when we advanced the frame, but at that point there is no guarantee that we have all of the necessary information; we may have fallen behind on decoding. As such, we move the calculation to where we actually perform the recycling. At this point we are guaranteed to have all the necessary frames between the recycling and display queues. Differential Revision: https://phabricator.services.mozilla.com/D12903
image/AnimationFrameBuffer.cpp
image/AnimationFrameBuffer.h
image/test/gtest/TestAnimationFrameBuffer.cpp
--- a/image/AnimationFrameBuffer.cpp
+++ b/image/AnimationFrameBuffer.cpp
@@ -319,16 +319,17 @@ AnimationFrameDiscardingQueue::AddSizeOf
         aCallback(aMetadata);
       }
     );
   }
 }
 
 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;
 }
 
 void
@@ -356,41 +357,30 @@ AnimationFrameRecyclingQueue::AdvanceInt
   // 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();
+  // We have advanced past the first frame. That means the next frame we are
+  // putting in the queue to recycling is the first frame in the animation,
+  // and we no longer need to worry about having looped around.
+  if (mGetIndex == 1) {
+    mForceUseFirstFrameRefreshArea = false;
+  }
 
-  // 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.
-  MOZ_ASSERT_IF(mGetIndex == 1,
-                front->GetRect().IsEqualEdges(front->GetDirtyRect()));
-
-  RecycleEntry newEntry(mGetIndex == 1 ? mFirstFrameRefreshArea
-                                       : front->GetDirtyRect());
+  RefPtr<imgFrame>& front = mDisplay.front();
+  RecycleEntry newEntry(mForceUseFirstFrameRefreshArea ? mFirstFrameRefreshArea
+                                                       : front->GetDirtyRect());
 
   // If we are allowed to recycle the frame, then we should save it before the
   // base class's AdvanceInternal discards it.
   if (front->ShouldRecycle()) {
-    // Calculate the recycle rect for the recycled frame. This is the cumulative
-    // dirty rect of all of the frames ahead of us to be displayed, and to be
-    // used for recycling. Or in other words, the dirty rect between the
-    // recycled frame and the decoded frame which reuses the buffer.
-    for (const RefPtr<imgFrame>& frame : mDisplay) {
-      newEntry.mRecycleRect = newEntry.mRecycleRect.Union(frame->GetDirtyRect());
-    }
-    for (const RecycleEntry& entry : mRecycle) {
-      newEntry.mRecycleRect = newEntry.mRecycleRect.Union(entry.mDirtyRect);
-    }
-
     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));
   mDisplay.pop_front();
   MOZ_ASSERT(!mDisplay.empty());
@@ -423,30 +413,72 @@ AnimationFrameRecyclingQueue::ResetInter
 {
   mRecycle.clear();
   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;
+  }
+
   if (mRecycle.empty()) {
     return RawAccessFrameRef();
   }
 
-  RawAccessFrameRef frame;
+  RawAccessFrameRef recycledFrame;
   if (mRecycle.front().mFrame) {
-    frame = mRecycle.front().mFrame->RawAccessRef();
-    if (frame) {
-      aRecycleRect = mRecycle.front().mRecycleRect;
+    recycledFrame = mRecycle.front().mFrame->RawAccessRef();
+    MOZ_ASSERT(recycledFrame);
+    mRecycle.pop_front();
+
+    if (mForceUseFirstFrameRefreshArea) {
+      // We are still crossing the loop boundary and cannot rely upon the dirty
+      // rects of entries in mDisplay to be representative. E.g. The first frame
+      // is probably has a full frame dirty rect.
+      aRecycleRect = mFirstFrameRefreshArea;
+    } else {
+      // Calculate the recycle rect for the recycled frame. This is the
+      // cumulative dirty rect of all of the frames ahead of us to be displayed,
+      // and to be used for recycling. Or in other words, the dirty rect between
+      // the recycled frame and the decoded frame which reuses the buffer.
+      //
+      // We know at this point that mRecycle contains either frames from the end
+      // of the animation with the first frame refresh area as the dirty rect
+      // (plus the first frame likewise) and frames with their actual dirty rect
+      // from the start. mDisplay should also only contain frames from the start
+      // of the animation onwards.
+      aRecycleRect.SetRect(0, 0, 0, 0);
+      for (const RefPtr<imgFrame>& frame : mDisplay) {
+        aRecycleRect = aRecycleRect.Union(frame->GetDirtyRect());
+      }
+      for (const RecycleEntry& entry : mRecycle) {
+        aRecycleRect = aRecycleRect.Union(entry.mDirtyRect);
+      }
     }
+  } else {
+    mRecycle.pop_front();
   }
 
-  mRecycle.pop_front();
-  return frame;
+  return recycledFrame;
 }
 
 bool
 AnimationFrameRecyclingQueue::MarkComplete(const gfx::IntRect& aFirstFrameRefreshArea)
 {
   bool continueDecoding =
     AnimationFrameDiscardingQueue::MarkComplete(aFirstFrameRefreshArea);
 
--- a/image/AnimationFrameBuffer.h
+++ b/image/AnimationFrameBuffer.h
@@ -444,35 +444,30 @@ public:
   struct RecycleEntry {
     explicit RecycleEntry(const gfx::IntRect &aDirtyRect)
       : mDirtyRect(aDirtyRect)
     { }
 
     RecycleEntry(RecycleEntry&& aOther)
       : mFrame(std::move(aOther.mFrame))
       , mDirtyRect(aOther.mDirtyRect)
-      , mRecycleRect(aOther.mRecycleRect)
-    {
-    }
+    { }
 
     RecycleEntry& operator=(RecycleEntry&& aOther)
     {
       mFrame = std::move(aOther.mFrame);
       mDirtyRect = aOther.mDirtyRect;
-      mRecycleRect = aOther.mRecycleRect;
       return *this;
     }
 
     RecycleEntry(const RecycleEntry& aOther) = delete;
     RecycleEntry& operator=(const RecycleEntry& aOther) = delete;
 
     RefPtr<imgFrame> mFrame;   // The frame containing the buffer to recycle.
     gfx::IntRect mDirtyRect;   // The dirty rect of the frame itself.
-    gfx::IntRect mRecycleRect; // The dirty rect between the recycled frame and
-                               // the future frame that will be written to it.
   };
 
   const std::deque<RecycleEntry>& Recycle() const { return mRecycle; }
   const gfx::IntRect& FirstFrameRefreshArea() const
   {
     return mFirstFrameRefreshArea;
   }
 
@@ -483,14 +478,19 @@ protected:
   /// Queue storing frames to be recycled by the decoder to produce its future
   /// frames. May contain up to mBatch frames, where the last frame in the queue
   /// is adjacent to the first frame in the mDisplay queue.
   std::deque<RecycleEntry> mRecycle;
 
   /// The first frame refresh area. This is used instead of the dirty rect for
   /// the last frame when transitioning back to the first frame.
   gfx::IntRect mFirstFrameRefreshArea;
+
+  /// Force recycled frames to use the first frame refresh area as their dirty
+  /// rect. This is used when we are recycling frames from the end of an
+  /// animation to produce frames at the beginning of an animation.
+  bool mForceUseFirstFrameRefreshArea;
 };
 
 } // namespace image
 } // namespace mozilla
 
 #endif // mozilla_image_AnimationFrameBuffer_h
--- a/image/test/gtest/TestAnimationFrameBuffer.cpp
+++ b/image/test/gtest/TestAnimationFrameBuffer.cpp
@@ -22,16 +22,19 @@ CreateEmptyFrame(const IntSize& aSize = 
                                DisposalMethod::NOT_SPECIFIED };
   nsresult rv =
     frame->InitForDecoder(aSize, IntRect(IntPoint(0, 0), aSize),
                           SurfaceFormat::B8G8R8A8, 0, false,
                           Some(animParams), true, aCanRecycle);
   EXPECT_TRUE(NS_SUCCEEDED(rv));
   RawAccessFrameRef frameRef = frame->RawAccessRef();
   frame->SetRawAccessOnly();
+  // Normally the blend animation filter would set the dirty rect, but since
+  // we aren't producing an actual animation here, we need to fake it.
+  frame->SetDirtyRect(aFrameRect);
   frame->Finish();
   return frame.forget();
 }
 
 static void
 PrepareForDiscardingQueue(AnimationFrameRetainedBuffer& aQueue)
 {
   ASSERT_EQ(size_t(0), aQueue.Size());
@@ -100,26 +103,23 @@ VerifyAdvance(AnimationFrameBuffer& aQue
   }
 
   bool restartDecoder = aQueue.AdvanceTo(aExpectedFrame);
   EXPECT_EQ(aExpectedRestartDecoder, restartDecoder);
 
   if (aQueue.IsRecycling()) {
     const AnimationFrameRecyclingQueue& queue =
       *static_cast<AnimationFrameRecyclingQueue*>(&aQueue);
+    EXPECT_FALSE(queue.Recycle().back().mDirtyRect.IsEmpty());
+    EXPECT_TRUE(queue.Recycle().back().mDirtyRect.Contains(oldFrame->GetDirtyRect()));
+    EXPECT_EQ(totalRecycled + 1, queue.Recycle().size());
     if (oldFrame->ShouldRecycle()) {
       EXPECT_EQ(oldFrame.get(), queue.Recycle().back().mFrame.get());
-      EXPECT_FALSE(queue.Recycle().back().mDirtyRect.IsEmpty());
-      EXPECT_FALSE(queue.Recycle().back().mRecycleRect.IsEmpty());
-      EXPECT_EQ(totalRecycled + 1, queue.Recycle().size());
     } else {
-      EXPECT_EQ(totalRecycled, queue.Recycle().size());
-      if (!queue.Recycle().empty()) {
-        EXPECT_NE(oldFrame.get(), queue.Recycle().back().mFrame.get());
-      }
+      EXPECT_EQ(nullptr, queue.Recycle().back().mFrame.get());
     }
   }
 }
 
 static void
 VerifyInsertAndAdvance(AnimationFrameBuffer& aQueue,
                        size_t aExpectedFrame,
                        AnimationFrameBuffer::InsertStatus aExpectedStatus)
@@ -579,17 +579,17 @@ TEST_F(ImageAnimationFrameBuffer, Recycl
   // We should not start with any recycled frames.
   ASSERT_TRUE(buffer.Recycle().empty());
 
   TestDiscardingQueueLoop(buffer, firstFrame, kThreshold, kBatch, kStartFrame);
 
   // All the frames we inserted should have been recycleable.
   ASSERT_FALSE(buffer.Recycle().empty());
   while (!buffer.Recycle().empty()) {
-    IntRect expectedRect = buffer.Recycle().front().mRecycleRect;
+    IntRect expectedRect(0, 0, 1, 1);
     RefPtr<imgFrame> expectedFrame = buffer.Recycle().front().mFrame;
     EXPECT_FALSE(expectedRect.IsEmpty());
     EXPECT_TRUE(expectedFrame.get() != nullptr);
 
     IntRect gotRect;
     RawAccessFrameRef gotFrame = buffer.RecycleFrame(gotRect);
     EXPECT_EQ(expectedFrame.get(), gotFrame.get());
     EXPECT_EQ(expectedRect, gotRect);
@@ -641,8 +641,114 @@ TEST_F(ImageAnimationFrameBuffer, Recycl
   const size_t kBatch = 3;
   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, 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);
+
+  // Let's get to the recycling state while marking all of the frames as not
+  // recyclable, just like AnimationFrameBuffer / the decoders would do.
+  RefPtr<imgFrame> frame;
+  frame = CreateEmptyFrame(kImageSize, IntRect(IntPoint(0, 0), kImageSize), false);
+  AnimationFrameBuffer::InsertStatus status = retained.Insert(std::move(frame));
+  EXPECT_EQ(AnimationFrameBuffer::InsertStatus::CONTINUE, status);
+
+  frame = CreateEmptyFrame(kImageSize, IntRect(IntPoint(0, 0), kImageSize), false);
+  status = retained.Insert(std::move(frame));
+  EXPECT_EQ(AnimationFrameBuffer::InsertStatus::CONTINUE, status);
+
+  frame = CreateEmptyFrame(kImageSize, IntRect(IntPoint(0, 0), kImageSize), false);
+  status = retained.Insert(std::move(frame));
+  EXPECT_EQ(AnimationFrameBuffer::InsertStatus::CONTINUE, status);
+
+  frame = CreateEmptyFrame(kImageSize, IntRect(IntPoint(0, 0), kImageSize), false);
+  status = retained.Insert(std::move(frame));
+  EXPECT_EQ(AnimationFrameBuffer::InsertStatus::YIELD, status);
+
+  VerifyAdvance(retained, 1, false);
+  VerifyAdvance(retained, 2, true);
+  VerifyAdvance(retained, 3, false);
+
+  frame = CreateEmptyFrame(kImageSize, IntRect(IntPoint(0, 0), kImageSize), false);
+  status = retained.Insert(std::move(frame));
+  EXPECT_EQ(AnimationFrameBuffer::InsertStatus::DISCARD_CONTINUE, status);
+
+  AnimationFrameRecyclingQueue buffer(std::move(retained));
+
+  // The first frame is now the candidate for recycling. Since it was marked as
+  // not recyclable, we should get nothing.
+  VerifyAdvance(buffer, 4, false);
+
+  IntRect recycleRect;
+  EXPECT_FALSE(buffer.Recycle().empty());
+  RawAccessFrameRef frameRef = buffer.RecycleFrame(recycleRect);
+  EXPECT_FALSE(frameRef);
+  EXPECT_TRUE(recycleRect.IsEmpty());
+  EXPECT_TRUE(buffer.Recycle().empty());
+
+  // Insert a recyclable partial frame. Its dirty rect shouldn't matter since
+  // the previous frame was not recyclable.
+  frame = CreateEmptyFrame(kImageSize, IntRect(0, 0, 25, 25));
+  status = buffer.Insert(std::move(frame));
+  EXPECT_EQ(AnimationFrameBuffer::InsertStatus::YIELD, status);
+
+  VerifyAdvance(buffer, 5, true);
+  EXPECT_FALSE(buffer.Recycle().empty());
+  frameRef = buffer.RecycleFrame(recycleRect);
+  EXPECT_FALSE(frameRef);
+  EXPECT_TRUE(recycleRect.IsEmpty());
+  EXPECT_TRUE(buffer.Recycle().empty());
+
+  // Insert a recyclable partial frame. Its dirty rect should match the recycle
+  // rect since it is the only frame in the buffer.
+  frame = CreateEmptyFrame(kImageSize, IntRect(25, 0, 50, 50));
+  status = buffer.Insert(std::move(frame));
+  EXPECT_EQ(AnimationFrameBuffer::InsertStatus::YIELD, status);
+
+  VerifyAdvance(buffer, 6, true);
+  EXPECT_FALSE(buffer.Recycle().empty());
+  frameRef = buffer.RecycleFrame(recycleRect);
+  EXPECT_TRUE(frameRef);
+  EXPECT_EQ(IntRect(25, 0, 50, 50), recycleRect);
+  EXPECT_TRUE(buffer.Recycle().empty());
+
+  // Insert the last frame and mark us as complete. The next recycled frame is
+  // producing the first frame again, so we should use the first frame refresh
+  // area instead of its dirty rect.
+  frame = CreateEmptyFrame(kImageSize, IntRect(10, 10, 60, 10));
+  status = buffer.Insert(std::move(frame));
+  EXPECT_EQ(AnimationFrameBuffer::InsertStatus::YIELD, status);
+
+  bool continueDecoding = buffer.MarkComplete(IntRect(0, 0, 75, 50));
+  EXPECT_FALSE(continueDecoding);
+
+  VerifyAdvance(buffer, 7, true);
+  EXPECT_FALSE(buffer.Recycle().empty());
+  frameRef = buffer.RecycleFrame(recycleRect);
+  EXPECT_TRUE(frameRef);
+  EXPECT_EQ(IntRect(0, 0, 75, 50), recycleRect);
+  EXPECT_TRUE(buffer.Recycle().empty());
+
+  // Now let's reinsert the first frame. The recycle rect should still be the
+  // first frame refresh area instead of the dirty rect of the first frame (e.g.
+  // the full frame).
+  frame = CreateEmptyFrame(kImageSize, IntRect(IntPoint(0, 0), kImageSize), false);
+  status = buffer.Insert(std::move(frame));
+  EXPECT_EQ(AnimationFrameBuffer::InsertStatus::YIELD, status);
+
+  VerifyAdvance(buffer, 0, true);
+  EXPECT_FALSE(buffer.Recycle().empty());
+  frameRef = buffer.RecycleFrame(recycleRect);
+  EXPECT_TRUE(frameRef);
+  EXPECT_EQ(IntRect(0, 0, 75, 50), recycleRect);
+  EXPECT_TRUE(buffer.Recycle().empty());
+}