Bug 1293472 (Part 3) - Store animated images in the surface cache as a sequence of frames, rather than each frame getting its own cache entry. r=dholbert,edwin,njn

Author: sethfowler

image/AnimationSurfaceProvider.cpp

@@ -115,6 +115,21 @@ AnimationSurfaceProvider::LogicalSizeInBytes() const
   return 3 * size.width * size.height * sizeof(uint32_t);
 }
 
+void
+AnimationSurfaceProvider::AddSizeOfExcludingThis(MallocSizeOf aMallocSizeOf,
+                                                 size_t& aHeapSizeOut,
+                                                 size_t& aNonHeapSizeOut)
+{
+  // Note that the surface cache lock is already held here, and then we acquire
+  // mFramesMutex. For this method, this ordering is unavoidable, which means
+  // that we must be careful to always use the same ordering elsewhere.
+  MutexAutoLock lock(mFramesMutex);
+
+  for (const RawAccessFrameRef& frame : mFrames) {
+    frame->AddSizeOfExcludingThis(aMallocSizeOf, aHeapSizeOut, aNonHeapSizeOut);
+  }
+}
+
 void
 AnimationSurfaceProvider::Run()
 {
@@ -233,7 +248,11 @@ AnimationSurfaceProvider::AnnounceSurfaceAvailable()
   mFramesMutex.AssertNotCurrentThreadOwns();
   MOZ_ASSERT(mImage);
 
-  // We just got the first frame; let the surface cache know.
+  // We just got the first frame; let the surface cache know. We deliberately do
+  // this outside of mFramesMutex to avoid a potential deadlock with
+  // AddSizeOfExcludingThis(), since otherwise we'd be acquiring mFramesMutex
+  // and then the surface cache lock, while the memory reporting code would
+  // acquire the surface cache lock and then mFramesMutex.
   SurfaceCache::SurfaceAvailable(WrapNotNull(this),
                                  ImageKey(mImage.get()),
                                  mSurfaceKey);

image/AnimationSurfaceProvider.h

@@ -45,6 +45,9 @@ class AnimationSurfaceProvider final
 
   bool IsFinished() const override;
   size_t LogicalSizeInBytes() const override;
+  void AddSizeOfExcludingThis(MallocSizeOf aMallocSizeOf,
+                              size_t& aHeapSizeOut,
+                              size_t& aNonHeapSizeOut) override;
 
 protected:
   DrawableFrameRef DrawableRef(size_t aFrame) override;

image/DecodedSurfaceProvider.cpp

@@ -27,7 +27,7 @@ DecodedSurfaceProvider::DecodedSurfaceProvider(NotNull<RasterImage*> aImage,
   MOZ_ASSERT(!mDecoder->IsMetadataDecode(),
              "Use MetadataDecodingTask for metadata decodes");
   MOZ_ASSERT(mDecoder->IsFirstFrameDecode(),
-             "Use AnimationDecodingTask for animation decodes");
+             "Use AnimationSurfaceProvider for animation decodes");
 }
 
 DecodedSurfaceProvider::~DecodedSurfaceProvider()

image/Decoder.cpp

@@ -98,10 +98,9 @@ Decoder::Init()
   // Metadata decoders must not set an output size.
   MOZ_ASSERT_IF(mMetadataDecode, !mHaveExplicitOutputSize);
 
-  // It doesn't make sense to decode anything but the first frame if we can't
-  // store anything in the SurfaceCache, since only the last frame we decode
-  // will be retrievable.
-  MOZ_ASSERT(ShouldUseSurfaceCache() || IsFirstFrameDecode());
+  // All decoders must be anonymous except for metadata decoders.
+  // XXX(seth): Soon that exception will be removed.
+  MOZ_ASSERT_IF(mImage, IsMetadataDecode());
 
   // Implementation-specific initialization.
   nsresult rv = InitInternal();
@@ -341,13 +340,6 @@ Decoder::AllocateFrameInternal(uint32_t aFrameNum,
     return RawAccessFrameRef();
   }
 
-  const uint32_t bytesPerPixel = aPaletteDepth == 0 ? 4 : 1;
-  if (ShouldUseSurfaceCache() &&
-      !SurfaceCache::CanHold(aFrameRect.Size(), bytesPerPixel)) {
-    NS_WARNING("Trying to add frame that's too large for the SurfaceCache");
-    return RawAccessFrameRef();
-  }
-
   NotNull<RefPtr<imgFrame>> frame = WrapNotNull(new imgFrame());
   bool nonPremult = bool(mSurfaceFlags & SurfaceFlags::NO_PREMULTIPLY_ALPHA);
   if (NS_FAILED(frame->InitForDecoder(aOutputSize, aFrameRect, aFormat,
@@ -362,29 +354,6 @@ Decoder::AllocateFrameInternal(uint32_t aFrameNum,
     return RawAccessFrameRef();
   }
 
-  if (ShouldUseSurfaceCache()) {
-    NotNull<RefPtr<ISurfaceProvider>> provider =
-      WrapNotNull(new SimpleSurfaceProvider(frame));
-    InsertOutcome outcome =
-      SurfaceCache::Insert(provider, ImageKey(mImage.get()),
-                           RasterSurfaceKey(aOutputSize,
-                                            mSurfaceFlags,
-                                            aFrameNum));
-    if (outcome == InsertOutcome::FAILURE) {
-      // We couldn't insert the surface, almost certainly due to low memory. We
-      // treat this as a permanent error to help the system recover; otherwise,
-      // we might just end up attempting to decode this image again immediately.
-      ref->Abort();
-      return RawAccessFrameRef();
-    } else if (outcome == InsertOutcome::FAILURE_ALREADY_PRESENT) {
-      // Another decoder beat us to decoding this frame. We abort this decoder
-      // rather than treat this as a real error.
-      mDecodeAborted = true;
-      ref->Abort();
-      return RawAccessFrameRef();
-    }
-  }
-
   if (aFrameNum == 1) {
     MOZ_ASSERT(aPreviousFrame, "Must provide a previous frame when animated");
     aPreviousFrame->SetRawAccessOnly();

image/Decoder.h

@@ -287,9 +287,6 @@ class Decoder
   /// Are we in the middle of a frame right now? Used for assertions only.
   bool InFrame() const { return mInFrame; }
 
-  /// Should we store surfaces created by this decoder in the SurfaceCache?
-  bool ShouldUseSurfaceCache() const { return bool(mImage); }
-
   /**
    * Returns true if this decoder was aborted.
    *

image/DecoderFactory.cpp

@@ -8,6 +8,7 @@
 #include "nsMimeTypes.h"
 #include "mozilla/RefPtr.h"
 
+#include "AnimationSurfaceProvider.h"
 #include "Decoder.h"
 #include "IDecodingTask.h"
 #include "nsPNGDecoder.h"
@@ -141,7 +142,7 @@ DecoderFactory::CreateDecoder(DecoderType aType,
   // Create a DecodedSurfaceProvider which will manage the decoding process and
   // make this decoder's output available in the surface cache.
   SurfaceKey surfaceKey =
-    RasterSurfaceKey(aOutputSize, aSurfaceFlags, /* aFrameNum = */ 0);
+    RasterSurfaceKey(aOutputSize, aSurfaceFlags, PlaybackType::eStatic);
   NotNull<RefPtr<DecodedSurfaceProvider>> provider =
     WrapNotNull(new DecodedSurfaceProvider(aImage,
                                            WrapNotNull(decoder),
@@ -175,8 +176,9 @@ DecoderFactory::CreateAnimationDecoder(DecoderType aType,
   MOZ_ASSERT(aType == DecoderType::GIF || aType == DecoderType::PNG,
              "Calling CreateAnimationDecoder for non-animating DecoderType");
 
-  RefPtr<Decoder> decoder =
-    GetDecoder(aType, aImage, /* aIsRedecode = */ true);
+  // Create an anonymous decoder. Interaction with the SurfaceCache and the
+  // owning RasterImage will be mediated by AnimationSurfaceProvider.
+  RefPtr<Decoder> decoder = GetDecoder(aType, nullptr, /* aIsRedecode = */ true);
   MOZ_ASSERT(decoder, "Should have a decoder now");
 
   // Initialize the decoder.
@@ -189,17 +191,25 @@ DecoderFactory::CreateAnimationDecoder(DecoderType aType,
     return nullptr;
   }
 
-  // Add a placeholder for the first frame to the SurfaceCache so we won't
-  // trigger any more decoders with the same parameters.
+  // Create an AnimationSurfaceProvider which will manage the decoding process
+  // and make this decoder's output available in the surface cache.
   SurfaceKey surfaceKey =
-    RasterSurfaceKey(aIntrinsicSize, aSurfaceFlags, /* aFrameNum = */ 0);
+    RasterSurfaceKey(aIntrinsicSize, aSurfaceFlags, PlaybackType::eAnimated);
+  NotNull<RefPtr<AnimationSurfaceProvider>> provider =
+    WrapNotNull(new AnimationSurfaceProvider(aImage,
+                                             WrapNotNull(decoder),
+                                             surfaceKey));
+
+  // Attempt to insert the surface provider into the surface cache right away so
+  // we won't trigger any more decoders with the same parameters.
   InsertOutcome outcome =
-    SurfaceCache::InsertPlaceholder(ImageKey(aImage.get()), surfaceKey);
+    SurfaceCache::Insert(provider, ImageKey(aImage.get()), surfaceKey);
   if (outcome != InsertOutcome::SUCCESS) {
     return nullptr;
   }
 
-  RefPtr<IDecodingTask> task = new AnimationDecodingTask(WrapNotNull(decoder));
+  // Return the surface provider in its IDecodingTask guise.
+  RefPtr<IDecodingTask> task = provider.get();
   return task.forget();
 }
 

image/FrameAnimator.cpp

@@ -297,8 +297,6 @@ FrameAnimator::RequestRefresh(AnimationState& aState, const TimeStamp& aTime)
 LookupResult
 FrameAnimator::GetCompositedFrame(uint32_t aFrameNum)
 {
-  MOZ_ASSERT(aFrameNum != 0, "First frame is never composited");
-
   // If we have a composited version of this frame, return that.
   if (mLastCompositedFrameIndex == int32_t(aFrameNum)) {
     return LookupResult(DrawableSurface(mCompositingFrame->DrawableRef()),
@@ -311,9 +309,20 @@ FrameAnimator::GetCompositedFrame(uint32_t aFrameNum)
     SurfaceCache::Lookup(ImageKey(mImage),
                          RasterSurfaceKey(mSize,
                                           DefaultSurfaceFlags(),
-                                          aFrameNum));
-  MOZ_ASSERT(!result || !result.Surface()->GetIsPaletted(),
+                                          PlaybackType::eAnimated));
+  if (!result) {
+    return result;
+  }
+
+  // Seek to the appropriate frame. If seeking fails, it means that we couldn't
+  // get the frame we're looking for; treat this as if the lookup failed.
+  if (NS_FAILED(result.Surface().Seek(aFrameNum))) {
+    return LookupResult(MatchType::NOT_FOUND);
+  }
+
+  MOZ_ASSERT(!result.Surface()->GetIsPaletted(),
              "About to return a paletted frame");
+
   return result;
 }
 
@@ -339,7 +348,7 @@ DoCollectSizeOfCompositingSurfaces(const RawAccessFrameRef& aSurface,
   // Concoct a SurfaceKey for this surface.
   SurfaceKey key = RasterSurfaceKey(aSurface->GetImageSize(),
                                     DefaultSurfaceFlags(),
-                                    /* aFrameNum = */ 0);
+                                    PlaybackType::eStatic);
 
   // Create a counter for this surface.
   SurfaceMemoryCounter counter(key, /* aIsLocked = */ true, aType);
@@ -381,9 +390,19 @@ FrameAnimator::GetRawFrame(uint32_t aFrameNum) const
     SurfaceCache::Lookup(ImageKey(mImage),
                          RasterSurfaceKey(mSize,
                                           DefaultSurfaceFlags(),
-                                          aFrameNum));
-  return result ? result.Surface()->RawAccessRef()
-                : RawAccessFrameRef();
+                                          PlaybackType::eAnimated));
+  if (!result) {
+    return RawAccessFrameRef();
+  }
+
+  // Seek to the frame we want. If seeking fails, it means we couldn't get the
+  // frame we're looking for, so we bail here to avoid returning the wrong frame
+  // to the caller.
+  if (NS_FAILED(result.Surface().Seek(aFrameNum))) {
+    return RawAccessFrameRef();  // Not available yet.
+  }
+
+  return result.Surface()->RawAccessRef();
 }
 
 //******************************************************************************

image/IDecodingTask.cpp

@@ -38,21 +38,21 @@ IDecodingTask::NotifyProgress(NotNull<RasterImage*> aImage,
   Progress progress = aDecoder->TakeProgress();
   IntRect invalidRect = aDecoder->TakeInvalidRect();
   Maybe<uint32_t> frameCount = aDecoder->TakeCompleteFrameCount();
+  DecoderFlags decoderFlags = aDecoder->GetDecoderFlags();
   SurfaceFlags surfaceFlags = aDecoder->GetSurfaceFlags();
 
   // Synchronously notify if we can.
-  if (NS_IsMainThread() &&
-      !(aDecoder->GetDecoderFlags() & DecoderFlags::ASYNC_NOTIFY)) {
-    aImage->NotifyProgress(progress, invalidRect,
-                           frameCount, surfaceFlags);
+  if (NS_IsMainThread() && !(decoderFlags & DecoderFlags::ASYNC_NOTIFY)) {
+    aImage->NotifyProgress(progress, invalidRect, frameCount,
+                           decoderFlags, surfaceFlags);
     return;
   }
 
   // We're forced to notify asynchronously.
   NotNull<RefPtr<RasterImage>> image = aImage;
   NS_DispatchToMainThread(NS_NewRunnableFunction([=]() -> void {
-    image->NotifyProgress(progress, invalidRect,
-                          frameCount, surfaceFlags);
+    image->NotifyProgress(progress, invalidRect, frameCount,
+                          decoderFlags, surfaceFlags);
   }));
 }
 
@@ -70,21 +70,23 @@ IDecodingTask::NotifyDecodeComplete(NotNull<RasterImage*> aImage,
   Progress progress = aDecoder->TakeProgress();
   IntRect invalidRect = aDecoder->TakeInvalidRect();
   Maybe<uint32_t> frameCount = aDecoder->TakeCompleteFrameCount();
+  DecoderFlags decoderFlags = aDecoder->GetDecoderFlags();
   SurfaceFlags surfaceFlags = aDecoder->GetSurfaceFlags();
 
   // Synchronously notify if we can.
-  if (NS_IsMainThread() &&
-      !(aDecoder->GetDecoderFlags() & DecoderFlags::ASYNC_NOTIFY)) {
+  if (NS_IsMainThread() && !(decoderFlags & DecoderFlags::ASYNC_NOTIFY)) {
     aImage->NotifyDecodeComplete(finalStatus, metadata, telemetry, progress,
-                                 invalidRect, frameCount, surfaceFlags);
+                                 invalidRect, frameCount, decoderFlags,
+                                 surfaceFlags);
     return;
   }
 
   // We're forced to notify asynchronously.
   NotNull<RefPtr<RasterImage>> image = aImage;
   NS_DispatchToMainThread(NS_NewRunnableFunction([=]() -> void {
     image->NotifyDecodeComplete(finalStatus, metadata, telemetry, progress,
-                                invalidRect, frameCount, surfaceFlags);
+                                invalidRect, frameCount, decoderFlags,
+                                surfaceFlags);
   }));
 }
 
@@ -100,59 +102,6 @@ IDecodingTask::Resume()
 }
 
 
-///////////////////////////////////////////////////////////////////////////////
-// AnimationDecodingTask implementation.
-///////////////////////////////////////////////////////////////////////////////
-
-AnimationDecodingTask::AnimationDecodingTask(NotNull<Decoder*> aDecoder)
-  : mMutex("mozilla::image::AnimationDecodingTask")
-  , mDecoder(aDecoder)
-{
-  MOZ_ASSERT(!mDecoder->IsMetadataDecode(),
-             "Use MetadataDecodingTask for metadata decodes");
-  MOZ_ASSERT(!mDecoder->IsFirstFrameDecode(),
-             "Use DecodingTask for single-frame image decodes");
-}
-
-void
-AnimationDecodingTask::Run()
-{
-  MutexAutoLock lock(mMutex);
-
-  while (true) {
-    LexerResult result = mDecoder->Decode(WrapNotNull(this));
-
-    if (result.is<TerminalState>()) {
-      NotifyDecodeComplete(mDecoder->GetImage(), mDecoder);
-      return;  // We're done.
-    }
-
-    MOZ_ASSERT(result.is<Yield>());
-
-    // Notify for the progress we've made so far.
-    if (mDecoder->HasProgress()) {
-      NotifyProgress(mDecoder->GetImage(), mDecoder);
-    }
-
-    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.
-  }
-}
-
-bool
-AnimationDecodingTask::ShouldPreferSyncRun() const
-{
-  return mDecoder->ShouldSyncDecode(gfxPrefs::ImageMemDecodeBytesAtATime());
-}
-
-
 ///////////////////////////////////////////////////////////////////////////////
 // MetadataDecodingTask implementation.
 ///////////////////////////////////////////////////////////////////////////////

image/IDecodingTask.h

@@ -62,33 +62,6 @@ class IDecodingTask : public IResumable
 };
 
 
-/**
- * An IDecodingTask implementation for full decodes of animated images.
- */
-class AnimationDecodingTask final : public IDecodingTask
-{
-public:
-  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AnimationDecodingTask, override)
-
-  explicit AnimationDecodingTask(NotNull<Decoder*> aDecoder);
-
-  void Run() override;
-  bool ShouldPreferSyncRun() const override;
-
-  // Full decodes are low priority compared to metadata decodes because they
-  // don't block layout or page load.
-  TaskPriority Priority() const override { return TaskPriority::eLow; }
-
-private:
-  virtual ~AnimationDecodingTask() { }
-
-  /// Mutex protecting access to mDecoder.
-  Mutex mMutex;
-
-  NotNull<RefPtr<Decoder>> mDecoder;
-};
-
-
 /**
  * An IDecodingTask implementation for metadata decodes of images.
  */