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

@@ -35,6 +35,7 @@ bool AnimationFrameRetainedBuffer::InsertInternal(RefPtr<imgFrame>&& aFrame) {
   MOZ_ASSERT(!mSizeKnown);
   MOZ_ASSERT(mFrames.Length() < mThreshold);
 
+  ++mSize;
   mFrames.AppendElement(std::move(aFrame));
   MOZ_ASSERT(mSize == mFrames.Length());
   return mSize < mThreshold;
@@ -156,6 +157,16 @@ AnimationFrameDiscardingQueue::AnimationFrameDiscardingQueue(
 }
 
 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));
@@ -176,7 +187,6 @@ bool AnimationFrameDiscardingQueue::ResetInternal() {
 bool AnimationFrameDiscardingQueue::MarkComplete(
     const gfx::IntRect& aFirstFrameRefreshArea) {
   if (NS_WARN_IF(mInsertIndex != mSize)) {
-    MOZ_ASSERT(mSizeKnown);
     mRedecodeError = true;
     mPending = 0;
   }

image/AnimationFrameBuffer.h

@@ -189,10 +189,6 @@ class AnimationFrameBuffer {
     MOZ_ASSERT(aFrame);
 
     --mPending;
-    if (!mSizeKnown) {
-      ++mSize;
-    }
-
     bool retain = InsertInternal(std::move(aFrame));
 
     if (mAdvance > 0 && mSize > 1) {

image/AnimationSurfaceProvider.cpp

@@ -289,6 +289,16 @@ bool AnimationSurfaceProvider::CheckForNewFrameAtYield() {
     // 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;
@@ -353,6 +363,13 @@ bool AnimationSurfaceProvider::CheckForNewFrameAtTerminalState() {
     // 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:

image/test/gtest/TestAnimationFrameBuffer.cpp

@@ -660,6 +660,79 @@ TEST_F(ImageAnimationFrameBuffer, ResetBeforeDiscardingThreshold)
   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;