Bug 1325296. RasterImage::LookupFrame does not return a surface if it was created as a result of a sync decode from with the FLAG_SYNC_DECODE_IF_FAST flag. r=aosmond

The Decode call may result in synchronously creating the surface, but we only check again if the surface is there for FLAG_SYNC_DECODE, not FLAG_SYNC_DECODE_IF_FAST.

All of the decoding we do during painting is of the type FLAG_SYNC_DECODE_IF_FAST, which means it would be useless to do that decoding synchronously during painting because the paint doesn't benefit from the result of that decoding.

Looking at the history of this code it looks like https://hg.mozilla.org/mozilla-central/rev/435df926eb10 (part 6 of bug 1119774) was where this bug was introduced. Before that changeset we always did another LookupFrameInternal call after the Decode (called WantDecodedFrames back then). But that changeset changed it to only be done for standard sync decodes, not "sync decode if fast".

image/DecodePool.cpp

@@ -328,7 +328,7 @@ DecodePool::AsyncRun(IDecodingTask* aTask)
   mImpl->PushWork(aTask);
 }
 
-void
+bool
 DecodePool::SyncRunIfPreferred(IDecodingTask* aTask)
 {
   MOZ_ASSERT(NS_IsMainThread());
@@ -336,10 +336,11 @@ DecodePool::SyncRunIfPreferred(IDecodingTask* aTask)
 
   if (aTask->ShouldPreferSyncRun()) {
     aTask->Run();
-    return;
+    return true;
   }
 
   AsyncRun(aTask);
+  return false;
 }
 
 void

image/DecodePool.h

@@ -62,8 +62,9 @@ public:
    * itself to make this decision; @see IDecodingTask::ShouldPreferSyncRun(). If
    * @aTask doesn't prefer it, just run @aTask asynchronously and return
    * immediately.
+   * @return true if the task was run sync, false otherwise.
    */
-  void SyncRunIfPreferred(IDecodingTask* aTask);
+  bool SyncRunIfPreferred(IDecodingTask* aTask);
 
   /**
    * Run @aTask synchronously. This does not guarantee that @aTask will complete

image/RasterImage.cpp

@@ -336,10 +336,10 @@ RasterImage::LookupFrame(const IntSize& aSize,
                !mAnimationState || mAnimationState->KnownFrameCount() < 1,
                "Animated frames should be locked");
 
-    Decode(requestedSize, aFlags, aPlaybackType);
+    bool ranSync = Decode(requestedSize, aFlags, aPlaybackType);
 
-    // If we can sync decode, we should already have the frame.
+    // If we can or did sync decode, we should already have the frame.
-    if (aFlags & FLAG_SYNC_DECODE) {
+    if (ranSync || (aFlags & FLAG_SYNC_DECODE)) {
       result = LookupFrameInternal(requestedSize, aFlags, aPlaybackType);
     }
   }
@@ -1080,7 +1080,7 @@ RasterImage::RequestDecodeForSize(const IntSize& aSize, uint32_t aFlags)
   return NS_OK;
 }
 
-static void
+static bool
 LaunchDecodingTask(IDecodingTask* aTask,
                    RasterImage* aImage,
                    uint32_t aFlags,
@@ -1093,24 +1093,24 @@ LaunchDecodingTask(IDecodingTask* aTask,
         js::ProfileEntry::Category::GRAPHICS,
         "%s", aImage->GetURIString().get());
       DecodePool::Singleton()->SyncRunIfPossible(aTask);
-      return;
+      return true;
     }
 
     if (aFlags & imgIContainer::FLAG_SYNC_DECODE_IF_FAST) {
       PROFILER_LABEL_PRINTF("DecodePool", "SyncRunIfPreferred",
         js::ProfileEntry::Category::GRAPHICS,
         "%s", aImage->GetURIString().get());
-      DecodePool::Singleton()->SyncRunIfPreferred(aTask);
+      return DecodePool::Singleton()->SyncRunIfPreferred(aTask);
-      return;
     }
   }
 
   // Perform an async decode. We also take this path if we don't have all the
   // source data yet, since sync decoding is impossible in that situation.
   DecodePool::Singleton()->AsyncRun(aTask);
+  return false;
 }
 
-NS_IMETHODIMP
+bool
 RasterImage::Decode(const IntSize& aSize,
                     uint32_t aFlags,
                     PlaybackType aPlaybackType)
@@ -1118,13 +1118,13 @@ RasterImage::Decode(const IntSize& aSize,
   MOZ_ASSERT(NS_IsMainThread());
 
   if (mError) {
-    return NS_ERROR_FAILURE;
+    return false;
   }
 
   // If we don't have a size yet, we can't do any other decoding.
   if (!mHasSize) {
     mWantFullDecode = true;
-    return NS_OK;
+    return false;
   }
 
   // We're about to decode again, which may mean that some of the previous sizes
@@ -1169,14 +1169,13 @@ RasterImage::Decode(const IntSize& aSize,
 
   // Make sure DecoderFactory was able to create a decoder successfully.
   if (!task) {
-    return NS_ERROR_FAILURE;
+    return false;
   }
 
   mDecodeCount++;
 
   // We're ready to decode; start the decoder.
-  LaunchDecodingTask(task, this, aFlags, mHasSourceData);
+  return LaunchDecodingTask(task, this, aFlags, mHasSourceData);
-  return NS_OK;
 }
 
 NS_IMETHODIMP

image/RasterImage.h

@@ -336,10 +336,12 @@ private:
    *
    * It's an error to call Decode() before this image's intrinsic size is
    * available. A metadata decode must successfully complete first.
+   *
+   * Returns true of the decode was run synchronously.
    */
-  NS_IMETHOD Decode(const gfx::IntSize& aSize,
+  bool Decode(const gfx::IntSize& aSize,
-                    uint32_t aFlags,
+              uint32_t aFlags,
-                    PlaybackType aPlaybackType);
+              PlaybackType aPlaybackType);
 
   /**
    * Creates and runs a metadata decoder, either synchronously or