diff --git a/image/AnimationFrameBuffer.cpp b/image/AnimationFrameBuffer.cpp index 5d8b672f6665..31ce5b103e50 100644 --- a/image/AnimationFrameBuffer.cpp +++ b/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()); - - // 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; - } + MOZ_ASSERT(mInsertIndex < mFrames.Length()); 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; } diff --git a/image/AnimationFrameBuffer.h b/image/AnimationFrameBuffer.h index aa23327e9186..0ad94aebe57f 100644 --- a/image/AnimationFrameBuffer.h +++ b/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 diff --git a/image/AnimationSurfaceProvider.cpp b/image/AnimationSurfaceProvider.cpp index 4eeac1d852f2..4ff055e57534 100644 --- a/image/AnimationSurfaceProvider.cpp +++ b/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 - // 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. - if (!mDecoder || !continueDecoding || - DecodePool::Singleton()->IsShuttingDown()) { - return; + // ahead of the current. + if (continueDecoding) { + MOZ_ASSERT(mDecoder); + continue; } + + 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 - // 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. + // more for the moment we can break out of the loop. MOZ_ASSERT(result == LexerResult(Yield::OUTPUT_AVAILABLE)); - if (!CheckForNewFrameAtYield() || - DecodePool::Singleton()->IsShuttingDown()) { + if (!CheckForNewFrameAtYield()) { 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 - // frames and need to loop back to the beginning. - bool recreateDecoder; + // Destroy our decoder; we don't need it anymore. + bool mayDiscard; { 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 { diff --git a/image/test/gtest/TestAnimationFrameBuffer.cpp b/image/test/gtest/TestAnimationFrameBuffer.cpp index e8fc495cc6da..55743ebdb1e7 100644 --- a/image/test/gtest/TestAnimationFrameBuffer.cpp +++ b/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); -} -