Bug 1510601 - Part 1. Move size increments into AnimationFrameBuffer::InsertInternal. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Thu, 29 Nov 2018 14:38:28 -0500
changeset 449414 0861a85d13bb1664c019b76fc543d2679e39c99f
parent 449413 3bc19b3fb522d366777d014e460b7e679bde5cc3
child 449415 d8a3f89d4bb67f3d9c17fa9751f97be3ef1a0a38
push id110365
push useraosmond@gmail.com
push dateWed, 05 Dec 2018 17:05:42 +0000
treeherdermozilla-inbound@d8a3f89d4bb6 [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 1. Move size increments into AnimationFrameBuffer::InsertInternal. r=tnikkel The size was originally incremented in AnimationFrameBuffer::Insert however if an animation was reset before we finished decoding, it would count some frames twice in the counter. Now we increment it inside InsertInternal, where AnimationFrameDiscardingQueue can make a more informed decision on whether the frame is a duplicate or not. Additionally we now fail explicitly when we insert more frames on subsequent decodes than the original decoders. This will help avoid getting out of sync with FrameAnimator. Differential Revision: https://phabricator.services.mozilla.com/D13464
image/AnimationFrameBuffer.cpp
image/AnimationFrameBuffer.h
image/AnimationSurfaceProvider.cpp
image/test/gtest/TestAnimationFrameBuffer.cpp
--- a/image/AnimationFrameBuffer.cpp
+++ b/image/AnimationFrameBuffer.cpp
@@ -30,16 +30,17 @@ AnimationFrameRetainedBuffer::AnimationF
   mPending = mBatch * 2;
 }
 
 bool AnimationFrameRetainedBuffer::InsertInternal(RefPtr<imgFrame>&& aFrame) {
   // We should only insert new frames if we actually asked for them.
   MOZ_ASSERT(!mSizeKnown);
   MOZ_ASSERT(mFrames.Length() < mThreshold);
 
+  ++mSize;
   mFrames.AppendElement(std::move(aFrame));
   MOZ_ASSERT(mSize == mFrames.Length());
   return mSize < mThreshold;
 }
 
 bool AnimationFrameRetainedBuffer::ResetInternal() {
   // If we haven't crossed the threshold, then we know by definition we have
   // not discarded any frames. If we previously requested more frames, but
@@ -151,16 +152,26 @@ AnimationFrameDiscardingQueue::Animation
   // the threshold without advancing further. That would mean mGetIndex is 0.
   for (size_t i = mGetIndex; i < mInsertIndex; ++i) {
     MOZ_ASSERT(aQueue.mFrames[i]);
     mDisplay.push_back(std::move(aQueue.mFrames[i]));
   }
 }
 
 bool AnimationFrameDiscardingQueue::InsertInternal(RefPtr<imgFrame>&& aFrame) {
+  if (mInsertIndex == mSize) {
+    if (mSizeKnown) {
+      // We produced more frames on a subsequent decode than on the first pass.
+      mRedecodeError = true;
+      mPending = 0;
+      return true;
+    }
+    ++mSize;
+  }
+
   // Even though we don't use redecoded first frames for display purposes, we
   // will still use them for recycling, so we still need to insert it.
   mDisplay.push_back(std::move(aFrame));
   ++mInsertIndex;
   MOZ_ASSERT(mInsertIndex <= mSize);
   return true;
 }
 
@@ -171,17 +182,16 @@ bool AnimationFrameDiscardingQueue::Rese
   bool restartDecoder = mPending == 0;
   mPending = 2 * mBatch;
   return restartDecoder;
 }
 
 bool AnimationFrameDiscardingQueue::MarkComplete(
     const gfx::IntRect& aFirstFrameRefreshArea) {
   if (NS_WARN_IF(mInsertIndex != mSize)) {
-    MOZ_ASSERT(mSizeKnown);
     mRedecodeError = true;
     mPending = 0;
   }
 
   // We reached the end of the animation, the next frame we get, if we get
   // another, will be the first frame again.
   mInsertIndex = 0;
   mSizeKnown = true;
--- a/image/AnimationFrameBuffer.h
+++ b/image/AnimationFrameBuffer.h
@@ -184,20 +184,16 @@ class AnimationFrameBuffer {
    *
    * @returns True if the decoder should decode another frame.
    */
   InsertStatus Insert(RefPtr<imgFrame>&& aFrame) {
     MOZ_ASSERT(mPending > 0);
     MOZ_ASSERT(aFrame);
 
     --mPending;
-    if (!mSizeKnown) {
-      ++mSize;
-    }
-
     bool retain = InsertInternal(std::move(aFrame));
 
     if (mAdvance > 0 && mSize > 1) {
       --mAdvance;
       ++mGetIndex;
       AdvanceInternal();
     }
 
--- a/image/AnimationSurfaceProvider.cpp
+++ b/image/AnimationSurfaceProvider.cpp
@@ -284,16 +284,26 @@ bool AnimationSurfaceProvider::CheckForN
     }
 
     // We should've gotten a different frame than last time.
     MOZ_ASSERT(!mFrames->IsLastInsertedFrame(frame));
 
     // Append the new frame to the list.
     AnimationFrameBuffer::InsertStatus status =
         mFrames->Insert(std::move(frame));
+
+    // If we hit a redecode error, then we actually want to stop. This happens
+    // when we tried to insert more frames than we originally had (e.g. the
+    // original decoder attempt hit an OOM error sooner than we did). Better to
+    // stop the animation than to get out of sync with FrameAnimator.
+    if (mFrames->HasRedecodeError()) {
+      mDecoder = nullptr;
+      return false;
+    }
+
     switch (status) {
       case AnimationFrameBuffer::InsertStatus::DISCARD_CONTINUE:
         continueDecoding = true;
         MOZ_FALLTHROUGH;
       case AnimationFrameBuffer::InsertStatus::DISCARD_YIELD:
         RequestFrameDiscarding();
         break;
       case AnimationFrameBuffer::InsertStatus::CONTINUE:
@@ -348,16 +358,23 @@ bool AnimationSurfaceProvider::CheckForN
 
     if (!frame || mFrames->IsLastInsertedFrame(frame)) {
       return mFrames->MarkComplete(mDecoder->GetFirstFrameRefreshArea());
     }
 
     // Append the new frame to the list.
     AnimationFrameBuffer::InsertStatus status =
         mFrames->Insert(std::move(frame));
+
+    // If we hit a redecode error, then we actually want to stop. This will be
+    // fully handled in FinishDecoding.
+    if (mFrames->HasRedecodeError()) {
+      return false;
+    }
+
     switch (status) {
       case AnimationFrameBuffer::InsertStatus::DISCARD_CONTINUE:
       case AnimationFrameBuffer::InsertStatus::DISCARD_YIELD:
         RequestFrameDiscarding();
         break;
       case AnimationFrameBuffer::InsertStatus::CONTINUE:
       case AnimationFrameBuffer::InsertStatus::YIELD:
         break;
--- a/image/test/gtest/TestAnimationFrameBuffer.cpp
+++ b/image/test/gtest/TestAnimationFrameBuffer.cpp
@@ -655,16 +655,89 @@ TEST_F(ImageAnimationFrameBuffer, ResetB
   EXPECT_TRUE(firstFrame != nullptr);
   AnimationFrameDiscardingQueue buffer(std::move(retained));
   const imgFrame* displayFirstFrame = buffer.Get(0, true);
   const imgFrame* advanceFirstFrame = buffer.Get(0, false);
   EXPECT_EQ(firstFrame, displayFirstFrame);
   EXPECT_EQ(firstFrame, advanceFirstFrame);
 }
 
+TEST_F(ImageAnimationFrameBuffer, DiscardingTooFewFrames)
+{
+  const size_t kThreshold = 3;
+  const size_t kBatch = 1;
+  const size_t kStartFrame = 0;
+
+  // First get us to a discarding buffer state.
+  AnimationFrameRetainedBuffer retained(kThreshold, kBatch, kStartFrame);
+  VerifyInsert(retained, AnimationFrameBuffer::InsertStatus::CONTINUE);
+  VerifyInsertAndAdvance(retained, 1, AnimationFrameBuffer::InsertStatus::YIELD);
+  VerifyInsert(retained, AnimationFrameBuffer::InsertStatus::DISCARD_YIELD);
+
+  // Insert one more frame.
+  AnimationFrameDiscardingQueue buffer(std::move(retained));
+  VerifyAdvance(buffer, 2, true);
+  VerifyInsert(buffer, AnimationFrameBuffer::InsertStatus::YIELD);
+
+  // Mark it as complete.
+  bool restartDecoder = buffer.MarkComplete(IntRect(0, 0, 1, 1));
+  EXPECT_FALSE(restartDecoder);
+  EXPECT_FALSE(buffer.HasRedecodeError());
+
+  // Insert one fewer frame than before.
+  VerifyAdvance(buffer, 3, true);
+  VerifyInsertAndAdvance(buffer, 0, AnimationFrameBuffer::InsertStatus::YIELD);
+  VerifyInsertAndAdvance(buffer, 1, AnimationFrameBuffer::InsertStatus::YIELD);
+  VerifyInsertAndAdvance(buffer, 2, AnimationFrameBuffer::InsertStatus::YIELD);
+
+  // When we mark it as complete, it should fail due to too few frames.
+  restartDecoder = buffer.MarkComplete(IntRect(0, 0, 1, 1));
+  EXPECT_TRUE(buffer.HasRedecodeError());
+  EXPECT_EQ(size_t(0), buffer.PendingDecode());
+  EXPECT_EQ(size_t(4), buffer.Size());
+}
+
+TEST_F(ImageAnimationFrameBuffer, DiscardingTooManyFrames)
+{
+  const size_t kThreshold = 3;
+  const size_t kBatch = 1;
+  const size_t kStartFrame = 0;
+
+  // First get us to a discarding buffer state.
+  AnimationFrameRetainedBuffer retained(kThreshold, kBatch, kStartFrame);
+  VerifyInsert(retained, AnimationFrameBuffer::InsertStatus::CONTINUE);
+  VerifyInsertAndAdvance(retained, 1, AnimationFrameBuffer::InsertStatus::YIELD);
+  VerifyInsert(retained, AnimationFrameBuffer::InsertStatus::DISCARD_YIELD);
+
+  // Insert one more frame.
+  AnimationFrameDiscardingQueue buffer(std::move(retained));
+  VerifyAdvance(buffer, 2, true);
+  VerifyInsert(buffer, AnimationFrameBuffer::InsertStatus::YIELD);
+
+  // Mark it as complete.
+  bool restartDecoder = buffer.MarkComplete(IntRect(0, 0, 1, 1));
+  EXPECT_FALSE(restartDecoder);
+  EXPECT_FALSE(buffer.HasRedecodeError());
+
+  // Advance and insert to get us back to the end on the redecode.
+  VerifyAdvance(buffer, 3, true);
+  VerifyInsertAndAdvance(buffer, 0, AnimationFrameBuffer::InsertStatus::YIELD);
+  VerifyInsertAndAdvance(buffer, 1, AnimationFrameBuffer::InsertStatus::YIELD);
+  VerifyInsertAndAdvance(buffer, 2, AnimationFrameBuffer::InsertStatus::YIELD);
+  VerifyInsertAndAdvance(buffer, 3, AnimationFrameBuffer::InsertStatus::YIELD);
+
+  // Attempt to insert a 5th frame, it should fail.
+  RefPtr<imgFrame> frame = CreateEmptyFrame();
+  AnimationFrameBuffer::InsertStatus status = buffer.Insert(std::move(frame));
+  EXPECT_EQ(AnimationFrameBuffer::InsertStatus::YIELD, status);
+  EXPECT_TRUE(buffer.HasRedecodeError());
+  EXPECT_EQ(size_t(0), buffer.PendingDecode());
+  EXPECT_EQ(size_t(4), buffer.Size());
+}
+
 TEST_F(ImageAnimationFrameBuffer, RecyclingReset)
 {
   const size_t kThreshold = 8;
   const size_t kBatch = 3;
   const size_t kStartFrame = 0;
   AnimationFrameRetainedBuffer retained(kThreshold, kBatch, kStartFrame);
   PrepareForDiscardingQueue(retained);
   const imgFrame* firstFrame = retained.Frames()[0].get();