Backed out 2 changesets (bug 1444537) for causing crashes on test_discardFramesAnimatedImage.html. a=backout

Backed out changeset 0d23d74448c8 (bug 1444537)
Backed out changeset 07f5d48b90cb (bug 1444537)

image/AnimationFrameBuffer.cpp

@@ -17,7 +17,6 @@ AnimationFrameBuffer::AnimationFrameBuffer()
   , mInsertIndex(0)
   , mGetIndex(0)
   , mSizeKnown(false)
-  , mRedecodeError(false)
 { }
 
 void
@@ -72,16 +71,7 @@ AnimationFrameBuffer::Insert(RawAccessFrameRef&& aFrame)
     // and we did not keep all of the frames. Replace whatever is there
     // (probably an empty frame) with the new frame.
     MOZ_ASSERT(MayDiscard());
-
+    MOZ_ASSERT(mInsertIndex < mFrames.Length());
-    // The first decode produced fewer frames than the redecodes, presumably
-    // because it hit an out-of-memory error which later attempts avoided. Just
-    // stop the animation because we can't tell the image that we have more
-    // frames now.
-    if (mInsertIndex >= mFrames.Length()) {
-      mRedecodeError = true;
-      mPending = 0;
-      return false;
-    }
 
     if (mInsertIndex > 0) {
       MOZ_ASSERT(!mFrames[mInsertIndex]);
@@ -137,23 +127,9 @@ AnimationFrameBuffer::Insert(RawAccessFrameRef&& aFrame)
 bool
 AnimationFrameBuffer::MarkComplete()
 {
-  // We may have stopped decoding at a different point in the animation than we
-  // did previously. That means the decoder likely hit a new error, e.g. OOM.
-  // This will prevent us from advancing as well, because we are missing the
-  // required frames to blend.
-  //
-  // XXX(aosmond): In an ideal world, we would be generating full frames, and
-  // the consumer of our data doesn't care about our internal state. It simply
-  // knows about the first frame, the current frame, and how long to display the
-  // current frame.
-  if (NS_WARN_IF(mInsertIndex != mFrames.Length())) {
-    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.
+  MOZ_ASSERT(mInsertIndex == mFrames.Length());
   mInsertIndex = 0;
 
   // Since we only request advancing when we want to resume at a certain point
@@ -255,7 +231,7 @@ AnimationFrameBuffer::AdvanceInternal()
     }
   }
 
-  if (!mRedecodeError && (!mSizeKnown || MayDiscard())) {
+  if (!mSizeKnown || MayDiscard()) {
     // Calculate how many frames we have requested ahead of the current frame.
     size_t buffered = mPending;
     if (mGetIndex > mInsertIndex) {
@@ -300,6 +276,13 @@ AnimationFrameBuffer::Reset()
     return false;
   }
 
+  // If we are over the threshold, then we know we will have missing frames in
+  // our buffer. The easiest thing to do is to drop everything but the first
+  // frame and go back to the initial state.
+  bool restartDecoder = mPending == 0;
+  mInsertIndex = 0;
+  mPending = 2 * mBatch;
+
   // Discard all frames besides the first, because the decoder always expects
   // that when it re-inserts a frame, it is not present. (It doesn't re-insert
   // the first frame.)
@@ -307,16 +290,6 @@ AnimationFrameBuffer::Reset()
     RawAccessFrameRef discard = Move(mFrames[i]);
   }
 
-  mInsertIndex = 0;
-
-  // If we hit an error after redecoding, we never want to restart decoding.
-  if (mRedecodeError) {
-    MOZ_ASSERT(mPending == 0);
-    return false;
-  }
-
-  bool restartDecoder = mPending == 0;
-  mPending = 2 * mBatch;
   return restartDecoder;
 }
 

image/AnimationFrameBuffer.h

@@ -129,12 +129,6 @@ public:
    */
   bool SizeKnown() const { return mSizeKnown; }
 
-  /**
-   * @returns True if encountered an error during redecode which should cause
-   *          the caller to stop inserting frames.
-   */
-  bool HasRedecodeError() const { return mRedecodeError; }
-
   /**
    * @returns The current frame index we have advanced to.
    */
@@ -193,9 +187,6 @@ private:
 
   // True if the total number of frames is known.
   bool mSizeKnown;
-
-  // True if we encountered an error while redecoding.
-  bool mRedecodeError;
 };
 
 } // namespace image

image/AnimationSurfaceProvider.cpp

@@ -224,14 +224,13 @@ AnimationSurfaceProvider::Run()
       FinishDecoding();
 
       // Even if it is the last frame, we may not have enough frames buffered
-      // ahead of the current. If we are shutting down, we want to ensure we
+      // ahead of the current.
-      // release the thread as soon as possible. The animation may advance even
+      if (continueDecoding) {
-      // during shutdown, which keeps us decoding, and thus blocking the decode
+        MOZ_ASSERT(mDecoder);
-      // pool during teardown.
+        continue;
-      if (!mDecoder || !continueDecoding ||
-          DecodePool::Singleton()->IsShuttingDown()) {
-        return;
       }
+
+      return;
     }
 
     // Notify for the progress we've made so far.
@@ -246,13 +245,9 @@ AnimationSurfaceProvider::Run()
     }
 
     // There's new output available - a new frame! Grab it. If we don't need any
-    // more for the moment we can break out of the loop. If we are shutting
+    // more for the moment we can break out of the loop.
-    // down, we want to ensure we release the thread as soon as possible. The
-    // animation may advance even during shutdown, which keeps us decoding, and
-    // thus blocking the decode pool during teardown.
     MOZ_ASSERT(result == LexerResult(Yield::OUTPUT_AVAILABLE));
-    if (!CheckForNewFrameAtYield() ||
+    if (!CheckForNewFrameAtYield()) {
-        DecodePool::Singleton()->IsShuttingDown()) {
       return;
     }
   }
@@ -299,7 +294,10 @@ AnimationSurfaceProvider::CheckForNewFrameAtYield()
     AnnounceSurfaceAvailable();
   }
 
-  return continueDecoding;
+  // If we are shutting down, we want to ensure we release the thread as soon
+  // as possible. The animation may advance even during shutdown, which keeps
+  // us decoding, and thus blocking the decode pool during teardown.
+  return continueDecoding && !DecodePool::Singleton()->IsShuttingDown();
 }
 
 bool
@@ -349,7 +347,10 @@ AnimationSurfaceProvider::CheckForNewFrameAtTerminalState()
     AnnounceSurfaceAvailable();
   }
 
-  return continueDecoding;
+  // If we are shutting down, we want to ensure we release the thread as soon
+  // as possible. The animation may advance even during shutdown, which keeps
+  // us decoding, and thus blocking the decode pool during teardown.
+  return continueDecoding && !DecodePool::Singleton()->IsShuttingDown();
 }
 
 void
@@ -377,15 +378,15 @@ AnimationSurfaceProvider::FinishDecoding()
     NotifyDecodeComplete(WrapNotNull(mImage), WrapNotNull(mDecoder));
   }
 
-  // Determine if we need to recreate the decoder, in case we are discarding
+  // Destroy our decoder; we don't need it anymore.
-  // frames and need to loop back to the beginning.
+  bool mayDiscard;
-  bool recreateDecoder;
   {
     MutexAutoLock lock(mFramesMutex);
-    recreateDecoder = !mFrames.HasRedecodeError() && mFrames.MayDiscard();
+    mayDiscard = mFrames.MayDiscard();
   }
 
-  if (recreateDecoder) {
+  if (mayDiscard) {
+    // Recreate the decoder so we can regenerate the frames again.
     mDecoder = DecoderFactory::CloneAnimationDecoder(mDecoder);
     MOZ_ASSERT(mDecoder);
   } else {

image/test/gtest/TestAnimationFrameBuffer.cpp

@@ -143,7 +143,6 @@ TEST_F(ImageAnimationFrameBuffer, FinishUnderBatchAndThreshold)
       EXPECT_FALSE(keepDecoding);
       EXPECT_TRUE(buffer.SizeKnown());
       EXPECT_EQ(size_t(0), buffer.PendingDecode());
-      EXPECT_FALSE(buffer.HasRedecodeError());
     }
 
     EXPECT_FALSE(buffer.MayDiscard());
@@ -221,7 +220,6 @@ TEST_F(ImageAnimationFrameBuffer, FinishMultipleBatchesUnderThreshold)
   EXPECT_TRUE(buffer.SizeKnown());
   EXPECT_EQ(size_t(0), buffer.PendingDecode());
   EXPECT_EQ(size_t(5), frames.Length());
-  EXPECT_FALSE(buffer.HasRedecodeError());
 
   // Finish progressing through the animation.
   for ( ; i < frames.Length(); ++i) {
@@ -332,7 +330,6 @@ TEST_F(ImageAnimationFrameBuffer, MayDiscard)
   EXPECT_TRUE(buffer.SizeKnown());
   EXPECT_EQ(size_t(2), buffer.PendingDecode());
   EXPECT_EQ(size_t(10), frames.Length());
-  EXPECT_FALSE(buffer.HasRedecodeError());
 
   // Use remaining pending room. It shouldn't add new frames, only replace.
   do {
@@ -516,159 +513,3 @@ TEST_F(ImageAnimationFrameBuffer, StartAfterBeginningAndReset)
   EXPECT_EQ(size_t(0), buffer.PendingAdvance());
 }
 
-TEST_F(ImageAnimationFrameBuffer, RedecodeMoreFrames)
-{
-  const size_t kThreshold = 5;
-  const size_t kBatch = 2;
-  AnimationFrameBuffer buffer;
-  buffer.Initialize(kThreshold, kBatch, 0);
-  const auto& frames = buffer.Frames();
-
-  // Add frames until we exceed the threshold.
-  bool keepDecoding;
-  bool restartDecoder;
-  size_t i = 0;
-  do {
-    keepDecoding = buffer.Insert(CreateEmptyFrame());
-    EXPECT_TRUE(keepDecoding);
-    if (i > 0) {
-      restartDecoder = buffer.AdvanceTo(i);
-      EXPECT_FALSE(restartDecoder);
-    }
-    ++i;
-  } while (!buffer.MayDiscard());
-
-  // Should have threshold + 1 frames, and still not complete.
-  EXPECT_EQ(size_t(6), frames.Length());
-  EXPECT_FALSE(buffer.SizeKnown());
-
-  // Now we lock in at 6 frames.
-  keepDecoding = buffer.MarkComplete();
-  EXPECT_TRUE(keepDecoding);
-  EXPECT_TRUE(buffer.SizeKnown());
-  EXPECT_FALSE(buffer.HasRedecodeError());
-
-  // Reinsert 6 frames first.
-  i = 0;
-  do {
-    keepDecoding = buffer.Insert(CreateEmptyFrame());
-    EXPECT_TRUE(keepDecoding);
-    restartDecoder = buffer.AdvanceTo(i);
-    EXPECT_FALSE(restartDecoder);
-    ++i;
-  } while (i < 6);
-
-  // We should now encounter an error and shutdown further decodes.
-  keepDecoding = buffer.Insert(CreateEmptyFrame());
-  EXPECT_FALSE(keepDecoding);
-  EXPECT_EQ(size_t(0), buffer.PendingDecode());
-  EXPECT_TRUE(buffer.HasRedecodeError());
-}
-
-TEST_F(ImageAnimationFrameBuffer, RedecodeFewerFrames)
-{
-  const size_t kThreshold = 5;
-  const size_t kBatch = 2;
-  AnimationFrameBuffer buffer;
-  buffer.Initialize(kThreshold, kBatch, 0);
-  const auto& frames = buffer.Frames();
-
-  // Add frames until we exceed the threshold.
-  bool keepDecoding;
-  bool restartDecoder;
-  size_t i = 0;
-  do {
-    keepDecoding = buffer.Insert(CreateEmptyFrame());
-    EXPECT_TRUE(keepDecoding);
-    if (i > 0) {
-      restartDecoder = buffer.AdvanceTo(i);
-      EXPECT_FALSE(restartDecoder);
-    }
-    ++i;
-  } while (!buffer.MayDiscard());
-
-  // Should have threshold + 1 frames, and still not complete.
-  EXPECT_EQ(size_t(6), frames.Length());
-  EXPECT_FALSE(buffer.SizeKnown());
-
-  // Now we lock in at 6 frames.
-  keepDecoding = buffer.MarkComplete();
-  EXPECT_TRUE(keepDecoding);
-  EXPECT_TRUE(buffer.SizeKnown());
-  EXPECT_FALSE(buffer.HasRedecodeError());
-
-  // Reinsert 5 frames before marking complete.
-  i = 0;
-  do {
-    keepDecoding = buffer.Insert(CreateEmptyFrame());
-    EXPECT_TRUE(keepDecoding);
-    restartDecoder = buffer.AdvanceTo(i);
-    EXPECT_FALSE(restartDecoder);
-    ++i;
-  } while (i < 5);
-
-  // We should now encounter an error and shutdown further decodes.
-  keepDecoding = buffer.MarkComplete();
-  EXPECT_FALSE(keepDecoding);
-  EXPECT_EQ(size_t(0), buffer.PendingDecode());
-  EXPECT_TRUE(buffer.HasRedecodeError());
-}
-
-TEST_F(ImageAnimationFrameBuffer, RedecodeFewerFramesAndBehindAdvancing)
-{
-  const size_t kThreshold = 5;
-  const size_t kBatch = 2;
-  AnimationFrameBuffer buffer;
-  buffer.Initialize(kThreshold, kBatch, 0);
-  const auto& frames = buffer.Frames();
-
-  // Add frames until we exceed the threshold.
-  bool keepDecoding;
-  bool restartDecoder;
-  size_t i = 0;
-  do {
-    keepDecoding = buffer.Insert(CreateEmptyFrame());
-    EXPECT_TRUE(keepDecoding);
-    if (i > 0) {
-      restartDecoder = buffer.AdvanceTo(i);
-      EXPECT_FALSE(restartDecoder);
-    }
-    ++i;
-  } while (!buffer.MayDiscard());
-
-  // Should have threshold + 1 frames, and still not complete.
-  EXPECT_EQ(size_t(6), frames.Length());
-  EXPECT_FALSE(buffer.SizeKnown());
-
-  // Now we lock in at 6 frames.
-  keepDecoding = buffer.MarkComplete();
-  EXPECT_TRUE(keepDecoding);
-  EXPECT_TRUE(buffer.SizeKnown());
-  EXPECT_FALSE(buffer.HasRedecodeError());
-
-  // Reinsert frames without advancing until we exhaust our pending space. This
-  // should be less than the current buffer length by definition.
-  i = 0;
-  do {
-    keepDecoding = buffer.Insert(CreateEmptyFrame());
-    ++i;
-  } while (keepDecoding);
-
-  EXPECT_EQ(size_t(2), i);
-
-  // We should now encounter an error and shutdown further decodes.
-  keepDecoding = buffer.MarkComplete();
-  EXPECT_FALSE(keepDecoding);
-  EXPECT_EQ(size_t(0), buffer.PendingDecode());
-  EXPECT_TRUE(buffer.HasRedecodeError());
-
-  // We should however be able to continue advancing to the last decoded frame
-  // without it requesting the decoder to restart.
-  i = 0;
-  do {
-    restartDecoder = buffer.AdvanceTo(i);
-    EXPECT_FALSE(restartDecoder);
-    ++i;
-  } while (i < 2);
-}
-