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
authorTimothy Nikkel <tnikkel@gmail.com>
Thu, 22 Dec 2016 13:15:41 -0600
changeset 327057 b50b2a2b2b6f77075d82c01f0aac919946fe2622
parent 327056 a91c57b9a5228fc35430e44a0355f55529d0d78a
child 327058 9c693641a5bafa8cbb61f246f7e410c1eaacbad0
push id31116
push userkwierso@gmail.com
push dateFri, 23 Dec 2016 02:37:16 +0000
treeherdermozilla-central@2785aaf276ba [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaosmond
bugs1325296, 1119774
milestone53.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 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
image/DecodePool.h
image/RasterImage.cpp
image/RasterImage.h
--- a/image/DecodePool.cpp
+++ b/image/DecodePool.cpp
@@ -323,28 +323,29 @@ DecodePool::Observe(nsISupports*, const 
 
 void
 DecodePool::AsyncRun(IDecodingTask* aTask)
 {
   MOZ_ASSERT(aTask);
   mImpl->PushWork(aTask);
 }
 
-void
+bool
 DecodePool::SyncRunIfPreferred(IDecodingTask* aTask)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aTask);
 
   if (aTask->ShouldPreferSyncRun()) {
     aTask->Run();
-    return;
+    return true;
   }
 
   AsyncRun(aTask);
+  return false;
 }
 
 void
 DecodePool::SyncRunIfPossible(IDecodingTask* aTask)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aTask);
   aTask->Run();
--- a/image/DecodePool.h
+++ b/image/DecodePool.h
@@ -57,18 +57,19 @@ public:
   /// Ask the DecodePool to run @aTask asynchronously and return immediately.
   void AsyncRun(IDecodingTask* aTask);
 
   /**
    * Run @aTask synchronously if the task would prefer it. It's up to the task
    * 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
    * synchronously. If, for example, @aTask doesn't yet have the data it needs to
    * run synchronously, it may recover by scheduling an async task to finish up
    * the work when the remaining data is available.
    */
   void SyncRunIfPossible(IDecodingTask* aTask);
--- a/image/RasterImage.cpp
+++ b/image/RasterImage.cpp
@@ -331,20 +331,20 @@ RasterImage::LookupFrame(const IntSize& 
       ((aFlags & FLAG_SYNC_DECODE) && !result)) {
     // We don't have a copy of this frame, and there's no decoder working on
     // one. (Or we're sync decoding and the existing decoder hasn't even started
     // yet.) Trigger decoding so it'll be available next time.
     MOZ_ASSERT(aPlaybackType != PlaybackType::eAnimated ||
                !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 (aFlags & FLAG_SYNC_DECODE) {
+    // If we can or did sync decode, we should already have the frame.
+    if (ranSync || (aFlags & FLAG_SYNC_DECODE)) {
       result = LookupFrameInternal(requestedSize, aFlags, aPlaybackType);
     }
   }
 
   if (!result) {
     // We still weren't able to get a frame. Give up.
     return DrawableSurface();
   }
@@ -1075,61 +1075,61 @@ RasterImage::RequestDecodeForSize(const 
 
   // Perform a frame lookup, which will implicitly start decoding if needed.
   LookupFrame(aSize, flags, mAnimationState ? PlaybackType::eAnimated
                                             : PlaybackType::eStatic);
 
   return NS_OK;
 }
 
-static void
+static bool
 LaunchDecodingTask(IDecodingTask* aTask,
                    RasterImage* aImage,
                    uint32_t aFlags,
                    bool aHaveSourceData)
 {
   if (aHaveSourceData) {
     // If we have all the data, we can sync decode if requested.
     if (aFlags & imgIContainer::FLAG_SYNC_DECODE) {
       PROFILER_LABEL_PRINTF("DecodePool", "SyncRunIfPossible",
         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;
+      return DecodePool::Singleton()->SyncRunIfPreferred(aTask);
     }
   }
 
   // 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)
 {
   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
   // we've decoded at aren't useful anymore. We can allow them to expire from
   // the cache by unlocking them here. When the decode finishes, it will send an
   // invalidation that will cause all instances of this image to redraw. If this
   // image is locked, any surfaces that are still useful will become locked
   // again when LookupFrame touches them, and the remainder will eventually
@@ -1164,24 +1164,23 @@ RasterImage::Decode(const IntSize& aSize
   } else {
     task = DecoderFactory::CreateDecoder(mDecoderType, WrapNotNull(this),
                                          mSourceBuffer, mSize, aSize,
                                          decoderFlags, surfaceFlags);
   }
 
   // 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 NS_OK;
+  return LaunchDecodingTask(task, this, aFlags, mHasSourceData);
 }
 
 NS_IMETHODIMP
 RasterImage::DecodeMetadata(uint32_t aFlags)
 {
   if (mError) {
     return NS_ERROR_FAILURE;
   }
--- a/image/RasterImage.h
+++ b/image/RasterImage.h
@@ -331,20 +331,22 @@ private:
    * Creates and runs a decoder, either synchronously or asynchronously
    * according to @aFlags. Decodes at the provided target size @aSize, using
    * decode flags @aFlags. Performs a single-frame decode of this image unless
    * we know the image is animated *and* @aPlaybackType is
    * PlaybackType::eAnimated.
    *
    * 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,
-                    uint32_t aFlags,
-                    PlaybackType aPlaybackType);
+  bool Decode(const gfx::IntSize& aSize,
+              uint32_t aFlags,
+              PlaybackType aPlaybackType);
 
   /**
    * Creates and runs a metadata decoder, either synchronously or
    * asynchronously according to @aFlags.
    */
   NS_IMETHOD DecodeMetadata(uint32_t aFlags);
 
   /**