Bug 1503935 - Fix some WebP decoder implementation bugs. r=tnikkel

First we did not handle the SourceBufferIterator::WAITING state which
can happen when we get woken up but there is no data to read from the
SourceBufferIterator. StreamingLexer handled this properly by yielding
with NEED_MORE_DATA, and properly scheduling the decoder to resume. This
patch does the same in the WebP decoder.

Second nsWebPDecoder::GetType was not implemented. This meant it would
return DecoderType::UNKNOWN, and would fail to recreate the decoder if
we are discarding frames and need to restart from the beginning. In
addition to implementing that method, this patch also corrects an assert
in DecoderFactory::CloneAnimationDecoder which failed to check for WebP
as a supported animated decoder.

This patch also modestly improves the logging output and library method
checks.

Differential Revision: https://phabricator.services.mozilla.com/D10624

Author: aosmond

image/DecoderFactory.cpp

@@ -246,7 +246,8 @@ DecoderFactory::CloneAnimationDecoder(Decoder* aDecoder)
   // get scheduled yet, or it has only decoded the first frame and has yet to
   // rediscover it is animated).
   DecoderType type = aDecoder->GetType();
-  MOZ_ASSERT(type == DecoderType::GIF || type == DecoderType::PNG,
+  MOZ_ASSERT(type == DecoderType::GIF || type == DecoderType::PNG ||
+             type == DecoderType::WEBP,
              "Calling CloneAnimationDecoder for non-animating DecoderType");
 
   RefPtr<Decoder> decoder = GetDecoder(type, nullptr, /* aIsRedecode = */ true);

image/IDecodingTask.cpp

@@ -185,8 +185,10 @@ MetadataDecodingTask::Run()
 // AnonymousDecodingTask implementation.
 ///////////////////////////////////////////////////////////////////////////////
 
-AnonymousDecodingTask::AnonymousDecodingTask(NotNull<Decoder*> aDecoder)
+AnonymousDecodingTask::AnonymousDecodingTask(NotNull<Decoder*> aDecoder,
+                                             bool aResumable)
   : mDecoder(aDecoder)
+  , mResumable(aResumable)
 { }
 
 void
@@ -211,5 +213,20 @@ AnonymousDecodingTask::Run()
   }
 }
 
+void
+AnonymousDecodingTask::Resume()
+{
+  // Anonymous decoders normally get all their data at once. We have tests
+  // where they don't; typically in these situations, the test re-runs them
+  // manually. However some tests want to verify Resume works, so they will
+  // explicitly request this behaviour.
+  if (mResumable) {
+    RefPtr<AnonymousDecodingTask> self(this);
+    NS_DispatchToMainThread(NS_NewRunnableFunction(
+      "image::AnonymousDecodingTask::Resume",
+      [self]() -> void { self->Run(); }));
+  }
+}
+
 } // namespace image
 } // namespace mozilla

image/IDecodingTask.h

@@ -109,22 +109,21 @@ class AnonymousDecodingTask final : public IDecodingTask
 public:
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AnonymousDecodingTask, override)
 
-  explicit AnonymousDecodingTask(NotNull<Decoder*> aDecoder);
+  explicit AnonymousDecodingTask(NotNull<Decoder*> aDecoder,
+                                 bool aResumable);
 
   void Run() override;
 
   bool ShouldPreferSyncRun() const override { return true; }
   TaskPriority Priority() const override { return TaskPriority::eLow; }
 
-  // Anonymous decoders normally get all their data at once. We have tests where
-  // they don't; in these situations, the test re-runs them manually. So no
-  // matter what, we don't want to resume by posting a task to the DecodePool.
-  void Resume() override { }
+  void Resume() override;
 
 private:
   virtual ~AnonymousDecodingTask() { }
 
   NotNull<RefPtr<Decoder>> mDecoder;
+  bool mResumable;
 };
 
 } // namespace image

image/ImageOps.cpp

@@ -179,7 +179,8 @@ ImageOps::DecodeMetadata(ImageBuffer* aBuffer,
   }
 
   // Run the decoder synchronously.
-  RefPtr<IDecodingTask> task = new AnonymousDecodingTask(WrapNotNull(decoder));
+  RefPtr<IDecodingTask> task =
+    new AnonymousDecodingTask(WrapNotNull(decoder), /* aResumable */ false);
   task->Run();
   if (!decoder->GetDecodeDone() || decoder->HasError()) {
     return NS_ERROR_FAILURE;
@@ -233,7 +234,8 @@ ImageOps::DecodeToSurface(ImageBuffer* aBuffer,
   }
 
   // Run the decoder synchronously.
-  RefPtr<IDecodingTask> task = new AnonymousDecodingTask(WrapNotNull(decoder));
+  RefPtr<IDecodingTask> task =
+    new AnonymousDecodingTask(WrapNotNull(decoder), /* aResumable */ false);
   task->Run();
   if (!decoder->GetDecodeDone() || decoder->HasError()) {
     return nullptr;

image/decoders/nsWebPDecoder.cpp

@@ -110,7 +110,7 @@ nsWebPDecoder::DoDecode(SourceBufferIterator& aIterator, IResumable* aOnResume)
 
   SourceBufferIterator::State state = SourceBufferIterator::COMPLETE;
   if (!mIteratorComplete) {
-    state = aIterator.Advance(SIZE_MAX);
+    state = aIterator.AdvanceOrScheduleResume(SIZE_MAX, aOnResume);
 
     // We need to remember since we can't advance a complete iterator.
     mIteratorComplete = state == SourceBufferIterator::COMPLETE;
@@ -133,6 +133,8 @@ nsWebPDecoder::DoDecode(SourceBufferIterator& aIterator, IResumable* aOnResume)
       return ReadData();
     case SourceBufferIterator::COMPLETE:
       return ReadData();
+    case SourceBufferIterator::WAITING:
+      return LexerResult(Yield::NEED_MORE_DATA);
     default:
       MOZ_LOG(sWebPLog, LogLevel::Error,
           ("[this=%p] nsWebPDecoder::DoDecode -- bad state\n", this));
@@ -180,8 +182,16 @@ nsWebPDecoder::CreateFrame(const nsIntRect& aFrameRect)
   MOZ_ASSERT(!mDecoder);
 
   MOZ_LOG(sWebPLog, LogLevel::Debug,
-      ("[this=%p] nsWebPDecoder::CreateFrame -- frame %u, %d x %d\n",
-       this, mCurrentFrame, aFrameRect.width, aFrameRect.height));
+      ("[this=%p] nsWebPDecoder::CreateFrame -- frame %u, (%d, %d) %d x %d\n",
+       this, mCurrentFrame, aFrameRect.x, aFrameRect.y,
+       aFrameRect.width, aFrameRect.height));
+
+  if (aFrameRect.width <= 0 || aFrameRect.height <= 0) {
+    MOZ_LOG(sWebPLog, LogLevel::Error,
+        ("[this=%p] nsWebPDecoder::CreateFrame -- bad frame rect\n",
+         this));
+    return NS_ERROR_FAILURE;
+  }
 
   // If this is our first frame in an animation and it doesn't cover the
   // full frame, then we are transparent even if there is no alpha
@@ -220,6 +230,7 @@ nsWebPDecoder::CreateFrame(const nsIntRect& aFrameRect)
     return NS_ERROR_FAILURE;
   }
 
+  mFrameRect = aFrameRect;
   mPipe = std::move(*pipe);
   return NS_OK;
 }
@@ -419,7 +430,9 @@ nsWebPDecoder::ReadSingle(const uint8_t* aData, size_t aLength, const IntRect& a
     return LexerResult(Yield::NEED_MORE_DATA);
   }
 
-  if (width <= 0 || height <= 0 || stride <= 0) {
+  if (width != mFrameRect.width || height != mFrameRect.height ||
+      stride < mFrameRect.width * 4 ||
+      lastRow > mFrameRect.height) {
     MOZ_LOG(sWebPLog, LogLevel::Error,
         ("[this=%p] nsWebPDecoder::ReadSingle -- bad (w,h,s) = (%d, %d, %d)\n",
          this, width, height, stride));

image/decoders/nsWebPDecoder.h

@@ -21,6 +21,8 @@ class nsWebPDecoder final : public Decoder
 public:
   virtual ~nsWebPDecoder();
 
+  DecoderType GetType() const override { return DecoderType::WEBP; }
+
 protected:
   LexerResult DoDecode(SourceBufferIterator& aIterator,
                        IResumable* aOnResume) override;
@@ -77,6 +79,9 @@ class nsWebPDecoder final : public Decoder
   /// Surface format for the current frame.
   gfx::SurfaceFormat mFormat;
 
+  /// Frame rect for the current frame.
+  IntRect mFrameRect;
+
   /// The last row of decoded pixels written to mPipe.
   int mLastRow;
 

image/test/gtest/Common.cpp

@@ -55,15 +55,7 @@ AutoInitializeImageLib::AutoInitializeImageLib()
   // Depending on initialization order, it is possible that our pref changes
   // have not taken effect yet because there are pending gfx-related events on
   // the main thread.
-  nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
-  EXPECT_TRUE(mainThread != nullptr);
-
-  bool processed;
-  do {
-    processed = false;
-    nsresult rv = mainThread->ProcessNextEvent(false, &processed);
-    EXPECT_TRUE(NS_SUCCEEDED(rv));
-  } while (processed);
+  SpinPendingEvents();
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -102,6 +94,20 @@ AutoInitializeImageLib::AutoInitializeImageLib()
     return rv;                        \
   }
 
+void
+SpinPendingEvents()
+{
+  nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
+  EXPECT_TRUE(mainThread != nullptr);
+
+  bool processed;
+  do {
+    processed = false;
+    nsresult rv = mainThread->ProcessNextEvent(false, &processed);
+    EXPECT_TRUE(NS_SUCCEEDED(rv));
+  } while (processed);
+}
+
 already_AddRefed<nsIInputStream>
 LoadFile(const char* aRelativePath)
 {

image/test/gtest/Common.h

@@ -133,6 +133,9 @@ class AutoInitializeImageLib
   AutoInitializeImageLib();
 };
 
+/// Spins on the main thread to process any pending events.
+void SpinPendingEvents();
+
 /// Loads a file from the current directory. @return an nsIInputStream for it.
 already_AddRefed<nsIInputStream> LoadFile(const char* aRelativePath);
 

image/test/gtest/TestDecoders.cpp

@@ -34,6 +34,11 @@ using namespace mozilla::image;
 static already_AddRefed<SourceSurface>
 CheckDecoderState(const ImageTestCase& aTestCase, Decoder* aDecoder)
 {
+  // Decoder should match what we asked for in the MIME type.
+  EXPECT_NE(aDecoder->GetType(), DecoderType::UNKNOWN);
+  EXPECT_EQ(aDecoder->GetType(),
+            DecoderFactory::GetDecoderType(aTestCase.mMimeType));
+
   EXPECT_TRUE(aDecoder->GetDecodeDone());
   EXPECT_EQ(bool(aTestCase.mFlags & TEST_CASE_HAS_ERROR),
             aDecoder->HasError());
@@ -119,7 +124,8 @@ void WithSingleChunkDecode(const ImageTestCase& aTestCase,
                                            DecoderFlags::FIRST_FRAME_ONLY,
                                            DefaultSurfaceFlags());
   ASSERT_TRUE(decoder != nullptr);
-  RefPtr<IDecodingTask> task = new AnonymousDecodingTask(WrapNotNull(decoder));
+  RefPtr<IDecodingTask> task =
+    new AnonymousDecodingTask(WrapNotNull(decoder), /* aResumable */ false);
 
   // Run the full decoder synchronously.
   task->Run();
@@ -136,6 +142,58 @@ CheckDecoderSingleChunk(const ImageTestCase& aTestCase)
   });
 }
 
+template <typename Func>
+void WithDelayedChunkDecode(const ImageTestCase& aTestCase,
+                           const Maybe<IntSize>& aOutputSize,
+                           Func aResultChecker)
+{
+  nsCOMPtr<nsIInputStream> inputStream = LoadFile(aTestCase.mPath);
+  ASSERT_TRUE(inputStream != nullptr);
+
+  // Figure out how much data we have.
+  uint64_t length;
+  nsresult rv = inputStream->Available(&length);
+  ASSERT_TRUE(NS_SUCCEEDED(rv));
+
+  // Prepare an empty SourceBuffer.
+  auto sourceBuffer = MakeNotNull<RefPtr<SourceBuffer>>();
+
+  // Create a decoder.
+  DecoderType decoderType =
+    DecoderFactory::GetDecoderType(aTestCase.mMimeType);
+  RefPtr<Decoder> decoder =
+    DecoderFactory::CreateAnonymousDecoder(decoderType, sourceBuffer, aOutputSize,
+                                           DecoderFlags::FIRST_FRAME_ONLY,
+                                           DefaultSurfaceFlags());
+  ASSERT_TRUE(decoder != nullptr);
+  RefPtr<IDecodingTask> task =
+    new AnonymousDecodingTask(WrapNotNull(decoder), /* aResumable */ true);
+
+  // Run the full decoder synchronously. It should now be waiting on
+  // the iterator to yield some data since we haven't written anything yet.
+  task->Run();
+
+  // Writing all of the data should wake up the decoder to complete.
+  sourceBuffer->ExpectLength(length);
+  rv = sourceBuffer->AppendFromInputStream(inputStream, length);
+  ASSERT_TRUE(NS_SUCCEEDED(rv));
+  sourceBuffer->Complete(NS_OK);
+
+  // It would have gotten posted to the main thread to avoid mutex contention.
+  SpinPendingEvents();
+
+  // Call the lambda to verify the expected results.
+  aResultChecker(decoder);
+}
+
+static void
+CheckDecoderDelayedChunk(const ImageTestCase& aTestCase)
+{
+  WithDelayedChunkDecode(aTestCase, Nothing(), [&](Decoder* aDecoder) {
+    CheckDecoderResults(aTestCase, aDecoder);
+  });
+}
+
 static void
 CheckDecoderMultiChunk(const ImageTestCase& aTestCase)
 {
@@ -157,7 +215,8 @@ CheckDecoderMultiChunk(const ImageTestCase& aTestCase)
                                            DecoderFlags::FIRST_FRAME_ONLY,
                                            DefaultSurfaceFlags());
   ASSERT_TRUE(decoder != nullptr);
-  RefPtr<IDecodingTask> task = new AnonymousDecodingTask(WrapNotNull(decoder));
+  RefPtr<IDecodingTask> task =
+    new AnonymousDecodingTask(WrapNotNull(decoder), /* aResumable */ false);
 
   for (uint64_t read = 0; read < length ; ++read) {
     uint64_t available = 0;
@@ -581,6 +640,11 @@ TEST_F(ImageDecoders, PNGSingleChunk)
   CheckDecoderSingleChunk(GreenPNGTestCase());
 }
 
+TEST_F(ImageDecoders, PNGDelayedChunk)
+{
+  CheckDecoderDelayedChunk(GreenPNGTestCase());
+}
+
 TEST_F(ImageDecoders, PNGMultiChunk)
 {
   CheckDecoderMultiChunk(GreenPNGTestCase());
@@ -596,6 +660,11 @@ TEST_F(ImageDecoders, GIFSingleChunk)
   CheckDecoderSingleChunk(GreenGIFTestCase());
 }
 
+TEST_F(ImageDecoders, GIFDelayedChunk)
+{
+  CheckDecoderDelayedChunk(GreenGIFTestCase());
+}
+
 TEST_F(ImageDecoders, GIFMultiChunk)
 {
   CheckDecoderMultiChunk(GreenGIFTestCase());
@@ -611,6 +680,11 @@ TEST_F(ImageDecoders, JPGSingleChunk)
   CheckDecoderSingleChunk(GreenJPGTestCase());
 }
 
+TEST_F(ImageDecoders, JPGDelayedChunk)
+{
+  CheckDecoderDelayedChunk(GreenJPGTestCase());
+}
+
 TEST_F(ImageDecoders, JPGMultiChunk)
 {
   CheckDecoderMultiChunk(GreenJPGTestCase());
@@ -626,6 +700,11 @@ TEST_F(ImageDecoders, BMPSingleChunk)
   CheckDecoderSingleChunk(GreenBMPTestCase());
 }
 
+TEST_F(ImageDecoders, BMPDelayedChunk)
+{
+  CheckDecoderDelayedChunk(GreenBMPTestCase());
+}
+
 TEST_F(ImageDecoders, BMPMultiChunk)
 {
   CheckDecoderMultiChunk(GreenBMPTestCase());
@@ -641,6 +720,11 @@ TEST_F(ImageDecoders, ICOSingleChunk)
   CheckDecoderSingleChunk(GreenICOTestCase());
 }
 
+TEST_F(ImageDecoders, ICODelayedChunk)
+{
+  CheckDecoderDelayedChunk(GreenICOTestCase());
+}
+
 TEST_F(ImageDecoders, ICOMultiChunk)
 {
   CheckDecoderMultiChunk(GreenICOTestCase());
@@ -661,6 +745,11 @@ TEST_F(ImageDecoders, IconSingleChunk)
   CheckDecoderSingleChunk(GreenIconTestCase());
 }
 
+TEST_F(ImageDecoders, IconDelayedChunk)
+{
+  CheckDecoderDelayedChunk(GreenIconTestCase());
+}
+
 TEST_F(ImageDecoders, IconMultiChunk)
 {
   CheckDecoderMultiChunk(GreenIconTestCase());
@@ -676,6 +765,11 @@ TEST_F(ImageDecoders, WebPSingleChunk)
   CheckDecoderSingleChunk(GreenWebPTestCase());
 }
 
+TEST_F(ImageDecoders, WebPDelayedChunk)
+{
+  CheckDecoderDelayedChunk(GreenWebPTestCase());
+}
+
 TEST_F(ImageDecoders, WebPMultiChunk)
 {
   CheckDecoderMultiChunk(GreenWebPTestCase());