Bug 1287691 (Part 1) - Expose yielding to decoding tasks. r=edwin
authorSeth Fowler <mark.seth.fowler@gmail.com>
Mon, 18 Jul 2016 23:46:35 -0700
changeset 305572 77a8e7ea0fe57ebc175e6765075ecda3bb5e9793
parent 305571 402ae3d3dec6f6b6dddca251adfb4457598bd8bc
child 305573 1bd6857c728d4345f870ecc9ba896c3568690d71
push id79641
push usermfowler@mozilla.com
push dateWed, 20 Jul 2016 00:15:18 +0000
treeherdermozilla-inbound@fe259a08d225 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersedwin
bugs1287691
milestone50.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 1287691 (Part 1) - Expose yielding to decoding tasks. r=edwin
image/Decoder.cpp
image/Decoder.h
image/IDecodingTask.cpp
image/decoders/nsICODecoder.cpp
--- a/image/Decoder.cpp
+++ b/image/Decoder.cpp
@@ -101,56 +101,57 @@ Decoder::Init()
   // Implementation-specific initialization.
   nsresult rv = InitInternal();
 
   mInitialized = true;
 
   return rv;
 }
 
-nsresult
+LexerResult
 Decoder::Decode(IResumable* aOnResume /* = nullptr */)
 {
   MOZ_ASSERT(mInitialized, "Should be initialized here");
   MOZ_ASSERT(mIterator, "Should have a SourceBufferIterator");
 
   // If we're already done, don't attempt to keep decoding.
   if (GetDecodeDone()) {
-    return HasError() ? NS_ERROR_FAILURE : NS_OK;
+    return LexerResult(HasError() ? TerminalState::FAILURE
+                                  : TerminalState::SUCCESS);
   }
 
   LexerResult lexerResult(TerminalState::FAILURE);
   {
     PROFILER_LABEL("ImageDecoder", "Decode", js::ProfileEntry::Category::GRAPHICS);
     AutoRecordDecoderTelemetry telemetry(this);
 
     lexerResult =  DoDecode(*mIterator, aOnResume);
   };
 
   if (lexerResult.is<Yield>()) {
-    // We need more data to continue. If @aOnResume was non-null, the
-    // SourceBufferIterator will automatically reschedule us. Otherwise, it's up
-    // to the caller.
-    MOZ_ASSERT(lexerResult.as<Yield>() == Yield::NEED_MORE_DATA);
-    return NS_OK;
+    // We either need more data to continue (in which case either @aOnResume or
+    // the caller will reschedule us to run again later), or the decoder is
+    // yielding to allow the caller access to some intermediate output.
+    return lexerResult;
   }
 
   // We reached a terminal state; we're now done decoding.
   MOZ_ASSERT(lexerResult.is<TerminalState>());
   mReachedTerminalState = true;
 
   // If decoding failed, record that fact.
   if (lexerResult.as<TerminalState>() == TerminalState::FAILURE) {
     PostError();
   }
 
   // Perform final cleanup.
   CompleteDecode();
 
-  return HasError() ? NS_ERROR_FAILURE : NS_OK;
+  return LexerResult(HasError() ? TerminalState::FAILURE
+                                : TerminalState::SUCCESS);
 }
 
 bool
 Decoder::ShouldSyncDecode(size_t aByteLimit)
 {
   MOZ_ASSERT(aByteLimit > 0);
   MOZ_ASSERT(mIterator, "Should have a SourceBufferIterator");
 
--- a/image/Decoder.h
+++ b/image/Decoder.h
@@ -43,19 +43,24 @@ public:
   nsresult Init();
 
   /**
    * Decodes, reading all data currently available in the SourceBuffer.
    *
    * If more data is needed and @aOnResume is non-null, Decode() will schedule
    * @aOnResume to be called when more data is available.
    *
-   * Any errors are reported by setting the appropriate state on the decoder.
+   * @return a LexerResult which may indicate:
+   *   - the image has been successfully decoded (TerminalState::SUCCESS), or
+   *   - the image has failed to decode (TerminalState::FAILURE), or
+   *   - the decoder is yielding until it gets more data (Yield::NEED_MORE_DATA), or
+   *   - the decoder is yielding to allow the caller to access intermediate
+   *     output (Yield::OUTPUT_AVAILABLE).
    */
-  nsresult Decode(IResumable* aOnResume = nullptr);
+  LexerResult Decode(IResumable* aOnResume = nullptr);
 
   /**
    * Given a maximum number of bytes we're willing to decode, @aByteLimit,
    * returns true if we should attempt to run this decoder synchronously.
    */
   bool ShouldSyncDecode(size_t aByteLimit);
 
   /**
--- a/image/IDecodingTask.cpp
+++ b/image/IDecodingTask.cpp
@@ -80,30 +80,41 @@ DecodingTask::DecodingTask(NotNull<Decod
 {
   MOZ_ASSERT(!mDecoder->IsMetadataDecode(),
              "Use MetadataDecodingTask for metadata decodes");
 }
 
 void
 DecodingTask::Run()
 {
-  nsresult rv = mDecoder->Decode(WrapNotNull(this));
+  while (true) {
+    LexerResult result = mDecoder->Decode(WrapNotNull(this));
 
-  if (NS_SUCCEEDED(rv) && !mDecoder->GetDecodeDone()) {
+    if (result.is<TerminalState>()) {
+      NotifyDecodeComplete(mDecoder);
+      return;  // We're done.
+    }
+
+    MOZ_ASSERT(result.is<Yield>());
+
     // Notify for the progress we've made so far.
     if (mDecoder->HasProgress()) {
       NotifyProgress(mDecoder);
     }
 
-    // We don't need to do anything else for this case. The decoder itself will
-    // ensure that we get reenqueued when more data is available.
-    return;
+    if (result == LexerResult(Yield::NEED_MORE_DATA)) {
+      // We can't make any more progress right now. The decoder itself will
+      // ensure that we get reenqueued when more data is available; just return
+      // for now.
+      return;
+    }
+
+    // Right now we don't do anything special for other kinds of yields, so just
+    // keep working.
   }
-
-  NotifyDecodeComplete(mDecoder);
 }
 
 bool
 DecodingTask::ShouldPreferSyncRun() const
 {
   return mDecoder->ShouldSyncDecode(gfxPrefs::ImageMemDecodeBytesAtATime());
 }
 
@@ -117,38 +128,59 @@ MetadataDecodingTask::MetadataDecodingTa
 {
   MOZ_ASSERT(mDecoder->IsMetadataDecode(),
              "Use DecodingTask for non-metadata decodes");
 }
 
 void
 MetadataDecodingTask::Run()
 {
-  nsresult rv = mDecoder->Decode(WrapNotNull(this));
+  LexerResult result = mDecoder->Decode(WrapNotNull(this));
 
-  if (NS_SUCCEEDED(rv) && !mDecoder->GetDecodeDone()) {
-    // It's important that metadata decode results are delivered atomically, so
-    // we'll wait until NotifyDecodeComplete() to report any progress.  We don't
-    // need to do anything else for this case. The decoder itself will ensure
-    // that we get reenqueued when more data is available.
+  if (result.is<TerminalState>()) {
+    NotifyDecodeComplete(mDecoder);
+    return;  // We're done.
+  }
+
+  if (result == LexerResult(Yield::NEED_MORE_DATA)) {
+    // We can't make any more progress right now. We also don't want to report
+    // any progress, because it's important that metadata decode results are
+    // delivered atomically. The decoder itself will ensure that we get
+    // reenqueued when more data is available; just return for now.
     return;
   }
 
-  NotifyDecodeComplete(mDecoder);
+  MOZ_ASSERT_UNREACHABLE("Metadata decode yielded for an unexpected reason");
 }
 
 
 ///////////////////////////////////////////////////////////////////////////////
 // AnonymousDecodingTask implementation.
 ///////////////////////////////////////////////////////////////////////////////
 
 AnonymousDecodingTask::AnonymousDecodingTask(NotNull<Decoder*> aDecoder)
   : mDecoder(aDecoder)
 { }
 
 void
 AnonymousDecodingTask::Run()
 {
-  mDecoder->Decode(WrapNotNull(this));
+  while (true) {
+    LexerResult result = mDecoder->Decode(WrapNotNull(this));
+
+    if (result.is<TerminalState>()) {
+      return;  // We're done.
+    }
+
+    if (result == LexerResult(Yield::NEED_MORE_DATA)) {
+      // We can't make any more progress right now. Let the caller decide how to
+      // handle it.
+      return;
+    }
+
+    // Right now we don't do anything special for other kinds of yields, so just
+    // keep working.
+    MOZ_ASSERT(result.is<Yield>());
+  }
 }
 
 } // namespace image
 } // namespace mozilla
--- a/image/decoders/nsICODecoder.cpp
+++ b/image/decoders/nsICODecoder.cpp
@@ -640,20 +640,24 @@ nsICODecoder::WriteToContainedDecoder(co
   mContainedSourceBuffer->Append(aBuffer, aCount);
 
   bool succeeded = true;
 
   // Write to the contained decoder. If we run out of data, the ICO decoder will
   // get resumed when there's more data available, as usual, so we don't need
   // the contained decoder to get resumed too. To avoid that, we provide an
   // IResumable which just does nothing.
-  if (NS_FAILED(mContainedDecoder->Decode())) {
+  LexerResult result = mContainedDecoder->Decode();
+  if (result == LexerResult(TerminalState::FAILURE)) {
     succeeded = false;
   }
 
+  MOZ_ASSERT(result != LexerResult(Yield::OUTPUT_AVAILABLE),
+             "Unexpected yield");
+
   // Make our state the same as the state of the contained decoder, and
   // propagate errors.
   mProgress |= mContainedDecoder->TakeProgress();
   mInvalidRect.UnionRect(mInvalidRect, mContainedDecoder->TakeInvalidRect());
   if (mContainedDecoder->HasError()) {
     succeeded = false;
   }