Bug 1194059 (Part 2) - Always detect IS_ANIMATED during the metadata decode. r=tn

image/Decoder.cpp

@@ -37,10 +37,8 @@ Decoder::Decoder(RasterImage* aImage)
   , mMetadataDecode(false)
   , mSendPartialInvalidations(false)
   , mImageIsTransient(false)
-  , mImageIsLocked(false)
   , mFirstFrameDecode(false)
   , mInFrame(false)
-  , mIsAnimated(false)
   , mDataDone(false)
   , mDecodeDone(false)
   , mDataError(false)
@@ -237,7 +235,7 @@ Decoder::CompleteDecode()
     // If this image wasn't animated and isn't a transient image, mark its frame
     // as optimizable. We don't support optimizing animated images and
     // optimizing transient images isn't worth it.
-    if (!mIsAnimated && !mImageIsTransient && mCurrentFrame) {
+    if (!HasAnimation() && !mImageIsTransient && mCurrentFrame) {
       mCurrentFrame->SetOptimizable();
     }
   }
@@ -260,7 +258,12 @@ Decoder::AllocateFrame(uint32_t aFrameNum,
     mCurrentFrame->GetPaletteData(&mColormap, &mColormapSize);
 
     if (aFrameNum + 1 == mFrameCount) {
-      PostFrameStart();
+      // If we're past the first frame, PostIsAnimated() should've been called.
+      MOZ_ASSERT_IF(mFrameCount > 1, HasAnimation());
+
+      // Update our state to reflect the new frame
+      MOZ_ASSERT(!mInFrame, "Starting new frame but not done with old one!");
+      mInFrame = true;
     }
   } else {
     PostDataError();
@@ -406,19 +409,11 @@ Decoder::PostHasTransparency()
 }
 
 void
-Decoder::PostFrameStart()
+Decoder::PostIsAnimated(int32_t aFirstFrameTimeout)
 {
-  // We shouldn't already be mid-frame
+  mProgress |= FLAG_IS_ANIMATED;
-  MOZ_ASSERT(!mInFrame, "Starting new frame but not done with old one!");
+  mImageMetadata.SetHasAnimation();
-
+  mImageMetadata.SetFirstFrameTimeout(aFirstFrameTimeout);
-  // Update our state to reflect the new frame
-  mInFrame = true;
-
-  // If we just became animated, record that fact.
-  if (mFrameCount > 1) {
-    mIsAnimated = true;
-    mProgress |= FLAG_IS_ANIMATED;
-  }
 }
 
 void
@@ -442,7 +437,7 @@ Decoder::PostFrameStop(Opacity aFrameOpacity    /* = Opacity::TRANSPARENT */,
 
   // If we're not sending partial invalidations, then we send an invalidation
   // here when the first frame is complete.
-  if (!mSendPartialInvalidations && !mIsAnimated) {
+  if (!mSendPartialInvalidations && !HasAnimation()) {
     mInvalidRect.UnionRect(mInvalidRect,
                            gfx::IntRect(gfx::IntPoint(0, 0), GetSize()));
   }
@@ -459,7 +454,7 @@ Decoder::PostInvalidation(const nsIntRect& aRect,
 
   // Record this invalidation, unless we're not sending partial invalidations
   // or we're past the first frame.
-  if (mSendPartialInvalidations && !mIsAnimated) {
+  if (mSendPartialInvalidations && !HasAnimation()) {
     mInvalidRect.UnionRect(mInvalidRect, aRect);
     mCurrentFrame->ImageUpdated(aRectAtTargetSize.valueOr(aRect));
   }

image/Decoder.h

@@ -181,20 +181,6 @@ public:
     mImageIsTransient = aIsTransient;
   }
 
-  /**
-   * Set whether the image is locked for the lifetime of this decoder. We lock
-   * the image during our initial decode to ensure that we don't evict any
-   * surfaces before we realize that the image is animated.
-   */
-  void SetImageIsLocked()
-  {
-    MOZ_ASSERT(!mInitialized, "Shouldn't be initialized yet");
-    mImageIsLocked = true;
-  }
-
-  bool ImageIsLocked() const { return mImageIsLocked; }
-
-
   /**
    * Set whether we should stop decoding after the first frame.
    */
@@ -225,7 +211,7 @@ public:
   }
 
   // Did we discover that the image we're decoding is animated?
-  bool HasAnimation() const { return mIsAnimated; }
+  bool HasAnimation() const { return mImageMetadata.HasAnimation(); }
 
   // Error tracking
   bool HasError() const { return HasDataError() || HasDecoderError(); }
@@ -344,9 +330,11 @@ protected:
   // actual contents of the frame and give a more accurate result.
   void PostHasTransparency();
 
-  // Called by decoders when they begin a frame. Informs the image, sends
+  // Called by decoders if they determine that the image is animated.
-  // notifications, and does internal book-keeping.
+  //
-  void PostFrameStart();
+  // @param aTimeout The time for which the first frame should be shown before
+  //                 we advance to the next frame.
+  void PostIsAnimated(int32_t aFirstFrameTimeout);
 
   // Called by decoders when they end a frame. Informs the image, sends
   // notifications, and does internal book-keeping.
@@ -451,10 +439,8 @@ private:
   bool mMetadataDecode : 1;
   bool mSendPartialInvalidations : 1;
   bool mImageIsTransient : 1;
-  bool mImageIsLocked : 1;
   bool mFirstFrameDecode : 1;
   bool mInFrame : 1;
-  bool mIsAnimated : 1;
   bool mDataDone : 1;
   bool mDecodeDone : 1;
   bool mDataError : 1;

image/DecoderFactory.cpp

@@ -113,8 +113,7 @@ DecoderFactory::CreateDecoder(DecoderType aType,
                               int aSampleSize,
                               const IntSize& aResolution,
                               bool aIsRedecode,
-                              bool aImageIsTransient,
+                              bool aImageIsTransient)
-                              bool aImageIsLocked)
 {
   if (aType == DecoderType::UNKNOWN) {
     return nullptr;
@@ -131,10 +130,7 @@ DecoderFactory::CreateDecoder(DecoderType aType,
   decoder->SetResolution(aResolution);
   decoder->SetSendPartialInvalidations(!aIsRedecode);
   decoder->SetImageIsTransient(aImageIsTransient);
-
+  decoder->SetIsFirstFrameDecode();
-  if (aImageIsLocked) {
-    decoder->SetImageIsLocked();
-  }
 
   // Set a target size for downscale-during-decode if applicable.
   if (aTargetSize) {
@@ -152,6 +148,39 @@ DecoderFactory::CreateDecoder(DecoderType aType,
   return decoder.forget();
 }
 
+/* static */ already_AddRefed<Decoder>
+DecoderFactory::CreateAnimationDecoder(DecoderType aType,
+                                       RasterImage* aImage,
+                                       SourceBuffer* aSourceBuffer,
+                                       uint32_t aFlags,
+                                       const IntSize& aResolution)
+{
+  if (aType == DecoderType::UNKNOWN) {
+    return nullptr;
+  }
+
+  MOZ_ASSERT(aType == DecoderType::GIF || aType == DecoderType::PNG,
+             "Calling CreateAnimationDecoder for non-animating DecoderType");
+
+  nsRefPtr<Decoder> decoder =
+    GetDecoder(aType, aImage, /* aIsRedecode = */ true);
+  MOZ_ASSERT(decoder, "Should have a decoder now");
+
+  // Initialize the decoder.
+  decoder->SetMetadataDecode(false);
+  decoder->SetIterator(aSourceBuffer->Iterator());
+  decoder->SetFlags(aFlags);
+  decoder->SetResolution(aResolution);
+  decoder->SetSendPartialInvalidations(false);
+
+  decoder->Init();
+  if (NS_FAILED(decoder->GetDecoderError())) {
+    return nullptr;
+  }
+
+  return decoder.forget();
+}
+
 /* static */ already_AddRefed<Decoder>
 DecoderFactory::CreateMetadataDecoder(DecoderType aType,
                                       RasterImage* aImage,

image/DecoderFactory.h

@@ -38,12 +38,13 @@ public:
   static DecoderType GetDecoderType(const char* aMimeType);
 
   /**
-   * Creates and initializes a decoder of type @aType. The decoder will send
+   * Creates and initializes a decoder for non-animated images of type @aType.
-   * notifications to @aImage.
+   * (If the image *is* animated, only the first frame will be decoded.) The
+   * decoder will send notifications to @aImage.
    *
-   * XXX(seth): @aIsRedecode, @aImageIsTransient, and @aImageIsLocked should
+   * XXX(seth): @aIsRedecode and @aImageIsTransient should really be part of
-   * really be part of @aFlags. This requires changes to the way that decoder
+   * @aFlags. This requires changes to the way that decoder flags work, though.
-   * flags work, though. See bug 1185800.
+   * See bug 1185800.
    *
    * @param aType Which type of decoder to create - JPEG, PNG, etc.
    * @param aImage The image will own the decoder and which should receive
@@ -62,9 +63,6 @@ public:
    *                    empty rect if none).
    * @param aIsRedecode Specify 'true' if this image has been decoded before.
    * @param aImageIsTransient Specify 'true' if this image is transient.
-   * @param aImageIsLocked Specify 'true' if this image is locked for the
-   *                       lifetime of this decoder, and should be unlocked
-   *                       when the decoder finishes.
    */
   static already_AddRefed<Decoder>
   CreateDecoder(DecoderType aType,
@@ -75,8 +73,28 @@ public:
                 int aSampleSize,
                 const gfx::IntSize& aResolution,
                 bool aIsRedecode,
-                bool aImageIsTransient,
+                bool aImageIsTransient);
-                bool aImageIsLocked);
+
+  /**
+   * Creates and initializes a decoder for animated images of type @aType.
+   * The decoder will send notifications to @aImage.
+   *
+   * @param aType Which type of decoder to create - JPEG, PNG, etc.
+   * @param aImage The image will own the decoder and which should receive
+   *               notifications as decoding progresses.
+   * @param aSourceBuffer The SourceBuffer which the decoder will read its data
+   *                      from.
+   * @param aFlags Flags specifying what type of output the decoder should
+   *               produce; see GetDecodeFlags() in RasterImage.h.
+   * @param aResolution The resolution requested using #-moz-resolution (or an
+   *                    empty rect if none).
+   */
+  static already_AddRefed<Decoder>
+  CreateAnimationDecoder(DecoderType aType,
+                         RasterImage* aImage,
+                         SourceBuffer* aSourceBuffer,
+                         uint32_t aFlags,
+                         const gfx::IntSize& aResolution);
 
   /**
    * Creates and initializes a metadata decoder of type @aType. This decoder

image/FrameAnimator.cpp

@@ -291,14 +291,19 @@ FrameAnimator::GetCompositedFrame(uint32_t aFrameNum)
 int32_t
 FrameAnimator::GetTimeoutForFrame(uint32_t aFrameNum) const
 {
+  int32_t rawTimeout = 0;
+
   RawAccessFrameRef frame = GetRawFrame(aFrameNum);
-  if (!frame) {
+  if (frame) {
+    AnimationData data = frame->GetAnimationData();
+    rawTimeout = data.mRawTimeout;
+  } else if (aFrameNum == 0) {
+    rawTimeout = mFirstFrameTimeout;
+  } else {
     NS_WARNING("No frame; called GetTimeoutForFrame too early?");
     return 100;
   }
 
-  AnimationData data = frame->GetAnimationData();
-
   // Ensure a minimal time between updates so we don't throttle the UI thread.
   // consider 0 == unspecified and make it fast but not too fast.  Unless we
   // have a single loop GIF. See bug 890743, bug 125137, bug 139677, and bug
@@ -312,11 +317,11 @@ FrameAnimator::GetTimeoutForFrame(uint32_t aFrameNum) const
   // It seems that there are broken tools out there that set a 0ms or 10ms
   // timeout when they really want a "default" one.  So munge values in that
   // range.
-  if (data.mRawTimeout >= 0 && data.mRawTimeout <= 10) {
+  if (rawTimeout >= 0 && rawTimeout <= 10) {
     return 100;
   }
 
-  return data.mRawTimeout;
+  return rawTimeout;
 }
 
 static void

image/FrameAnimator.h

@@ -33,6 +33,7 @@ public:
     , mLoopRemainingCount(-1)
     , mLastCompositedFrameIndex(-1)
     , mLoopCount(-1)
+    , mFirstFrameTimeout(0)
     , mAnimationMode(aAnimationMode)
     , mDoneDecoding(false)
   { }
@@ -148,6 +149,12 @@ public:
   void SetLoopCount(int32_t aLoopCount) { mLoopCount = aLoopCount; }
   int32_t LoopCount() const { return mLoopCount; }
 
+  /*
+   * Set the timeout for the first frame. This is used to allow animation
+   * scheduling even before a full decode runs for this image.
+   */
+  void SetFirstFrameTimeout(int32_t aTimeout) { mFirstFrameTimeout = aTimeout; }
+
   /**
    * Collect an accounting of the memory occupied by the compositing surfaces we
    * use during animation playback. All of the actual animation frames are
@@ -277,6 +284,9 @@ private: // data
   //! The total number of loops for the image.
   int32_t mLoopCount;
 
+  //! The timeout for the first frame of this image.
+  int32_t mFirstFrameTimeout;
+
   //! The animation mode of this image. Constants defined in imgIContainer.
   uint16_t mAnimationMode;
 

image/ImageMetadata.cpp

@@ -1,44 +0,0 @@
-/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
- *
- * This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-#include "ImageMetadata.h"
-
-#include "RasterImage.h"
-#include "nsComponentManagerUtils.h"
-#include "nsISupportsPrimitives.h"
-#include "nsXPCOMCID.h"
-
-namespace mozilla {
-namespace image {
-
-nsresult
-ImageMetadata::SetOnImage(RasterImage* aImage)
-{
-  nsresult rv = NS_OK;
-
-  if (mHotspotX != -1 && mHotspotY != -1) {
-    nsCOMPtr<nsISupportsPRUint32> intwrapx =
-      do_CreateInstance(NS_SUPPORTS_PRUINT32_CONTRACTID);
-    nsCOMPtr<nsISupportsPRUint32> intwrapy =
-      do_CreateInstance(NS_SUPPORTS_PRUINT32_CONTRACTID);
-    intwrapx->SetData(mHotspotX);
-    intwrapy->SetData(mHotspotY);
-    aImage->Set("hotspotX", intwrapx);
-    aImage->Set("hotspotY", intwrapy);
-  }
-
-  aImage->SetLoopCount(mLoopCount);
-
-  if (HasSize()) {
-    MOZ_ASSERT(HasOrientation(), "Should have orientation");
-    rv = aImage->SetSize(GetWidth(), GetHeight(), GetOrientation());
-  }
-
-  return rv;
-}
-
-} // namespace image
-} // namespace mozilla

image/ImageMetadata.h

@@ -22,23 +22,26 @@ class ImageMetadata
 {
 public:
   ImageMetadata()
-    : mHotspotX(-1)
+    : mLoopCount(-1)
-    , mHotspotY(-1)
+    , mFirstFrameTimeout(0)
-    , mLoopCount(-1)
+    , mHasAnimation(false)
   { }
 
-  // Set the metadata this object represents on an image.
+  void SetHotspot(uint16_t aHotspotX, uint16_t aHotspotY)
-  nsresult SetOnImage(RasterImage* aImage);
-
-  void SetHotspot(uint16_t hotspotx, uint16_t hotspoty)
   {
-    mHotspotX = hotspotx;
+    mHotspot = Some(gfx::IntPoint(aHotspotX, aHotspotY));
-    mHotspotY = hotspoty;
   }
+  gfx::IntPoint GetHotspot() const { return *mHotspot; }
+  bool HasHotspot() const { return mHotspot.isSome(); }
+
   void SetLoopCount(int32_t loopcount)
   {
     mLoopCount = loopcount;
   }
+  int32_t GetLoopCount() const { return mLoopCount; }
+
+  void SetFirstFrameTimeout(int32_t aTimeout) { mFirstFrameTimeout = aTimeout; }
+  int32_t GetFirstFrameTimeout() const { return mFirstFrameTimeout; }
 
   void SetSize(int32_t width, int32_t height, Orientation orientation)
   {
@@ -47,25 +50,28 @@ public:
       mOrientation.emplace(orientation);
     }
   }
-
+  nsIntSize GetSize() const { return *mSize; }
+  Orientation GetOrientation() const { return *mOrientation; }
   bool HasSize() const { return mSize.isSome(); }
   bool HasOrientation() const { return mOrientation.isSome(); }
 
-  int32_t GetWidth() const { return mSize->width; }
+  void SetHasAnimation() { mHasAnimation = true; }
-  int32_t GetHeight() const { return mSize->height; }
+  bool HasAnimation() const { return mHasAnimation; }
-  nsIntSize GetSize() const { return *mSize; }
-  Orientation GetOrientation() const { return *mOrientation; }
 
 private:
-  // The hotspot found on cursors, or -1 if none was found.
+  /// The hotspot found on cursors, if present.
-  int32_t mHotspotX;
+  Maybe<gfx::IntPoint> mHotspot;
-  int32_t mHotspotY;
 
-  // The loop count for animated images, or -1 for infinite loop.
+  /// The loop count for animated images, or -1 for infinite loop.
   int32_t mLoopCount;
 
+  /// The timeout of an animated image's first frame.
+  int32_t mFirstFrameTimeout;
+
   Maybe<nsIntSize> mSize;
   Maybe<Orientation> mOrientation;
+
+  bool mHasAnimation : 1;
 };
 
 } // namespace image

image/RasterImage.cpp

@@ -24,6 +24,7 @@
 #include "nsIConsoleService.h"
 #include "nsIInputStream.h"
 #include "nsIScriptError.h"
+#include "nsISupportsPrimitives.h"
 #include "nsPresContext.h"
 #include "SourceBuffer.h"
 #include "SurfaceCache.h"
@@ -478,7 +479,7 @@ RasterImage::LookupFrame(uint32_t aFrameNum,
     // 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(!mAnim, "Animated frames should be locked");
+    MOZ_ASSERT(!mAnim || GetNumFrames() < 1, "Animated frames should be locked");
 
     Decode(requestedSize, aFlags);
 
@@ -529,7 +530,7 @@ RasterImage::GetRequestedFrameIndex(uint32_t aWhichFrame) const
 IntRect
 RasterImage::GetFirstFrameRect()
 {
-  if (mAnim) {
+  if (mAnim && mHasBeenDecoded) {
     return mAnim->GetFirstFrameRefreshArea();
   }
 
@@ -582,7 +583,10 @@ RasterImage::GetAnimated(bool* aAnimated)
   }
 
   // Otherwise, we need to have been decoded to know for sure, since if we were
-  // decoded at least once mAnim would have been created for animated images
+  // decoded at least once mAnim would have been created for animated images.
+  // This is true even though we check for animation during the metadata decode,
+  // because we may still discover animation only during the full decode for
+  // corrupt images.
   if (!mHasBeenDecoded) {
     return NS_ERROR_NOT_AVAILABLE;
   }
@@ -921,21 +925,9 @@ RasterImage::OnAddedFrame(uint32_t aNewFrameCount,
     mFrameCount = aNewFrameCount;
 
     if (aNewFrameCount == 2) {
-      // We're becoming animated, so initialize animation stuff.
+      MOZ_ASSERT(mAnim, "Should already have animation state");
-      MOZ_ASSERT(!mAnim, "Already have animation state?");
-      mAnim = MakeUnique<FrameAnimator>(this, mSize, mAnimationMode);
-
-      // We don't support discarding animated images (See bug 414259).
-      // Lock the image and throw away the key.
-      //
-      // Note that this is inefficient, since we could get rid of the source
-      // data too. However, doing this is actually hard, because we're probably
-      // mid-decode, and thus we're decoding out of the source buffer. Since
-      // we're going to fix this anyway later, and since we didn't kill the
-      // source data in the old world either, locking is acceptable for the
-      // moment.
-      LockImage();
 
+      // We may be able to start animating.
       if (mPendingAnimation && ShouldAnimate()) {
         StartAnimation();
       }
@@ -947,7 +939,8 @@ RasterImage::OnAddedFrame(uint32_t aNewFrameCount,
 }
 
 nsresult
-RasterImage::SetSize(int32_t aWidth, int32_t aHeight, Orientation aOrientation)
+RasterImage::SetMetadata(const ImageMetadata& aMetadata,
+                         bool aFromMetadataDecode)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
@@ -955,26 +948,64 @@ RasterImage::SetSize(int32_t aWidth, int32_t aHeight, Orientation aOrientation)
     return NS_ERROR_FAILURE;
   }
 
-  // Ensure that we have positive values
+  if (aMetadata.HasSize()) {
-  // XXX - Why isn't the size unsigned? Should this be changed?
+    IntSize size = aMetadata.GetSize();
-  if ((aWidth < 0) || (aHeight < 0)) {
+    if (size.width < 0 || size.height < 0) {
-    return NS_ERROR_INVALID_ARG;
+      return NS_ERROR_INVALID_ARG;
+    }
+
+    MOZ_ASSERT(aMetadata.HasOrientation());
+    Orientation orientation = aMetadata.GetOrientation();
+
+    // If we already have a size, check the new size against the old one.
+    if (mHasSize && (size != mSize || orientation != mOrientation)) {
+      NS_WARNING("Image changed size or orientation on redecode! "
+                 "This should not happen!");
+      DoError();
+      return NS_ERROR_UNEXPECTED;
+    }
+
+    // Set the size and flag that we have it.
+    mSize = size;
+    mOrientation = orientation;
+    mHasSize = true;
   }
 
-  // if we already have a size, check the new size against the old one
+  if (mHasSize && aMetadata.HasAnimation() && !mAnim) {
-  if (mHasSize &&
+    // We're becoming animated, so initialize animation stuff.
-      ((aWidth != mSize.width) ||
+    mAnim = MakeUnique<FrameAnimator>(this, mSize, mAnimationMode);
-       (aHeight != mSize.height) ||
+
-       (aOrientation != mOrientation))) {
+    // We don't support discarding animated images (See bug 414259).
-    NS_WARNING("Image changed size on redecode! This should not happen!");
+    // Lock the image and throw away the key.
-    DoError();
+    LockImage();
-    return NS_ERROR_UNEXPECTED;
+
+    if (!aFromMetadataDecode) {
+      // The metadata decode reported that this image isn't animated, but we
+      // discovered that it actually was during the full decode. This is a
+      // rare failure that only occurs for corrupt images. To recover, we need
+      // to discard all existing surfaces and redecode.
+      RecoverFromLossOfFrames(mSize, DECODE_FLAGS_DEFAULT);
+    }
   }
 
-  // Set the size and flag that we have it
+  if (mAnim) {
-  mSize.SizeTo(aWidth, aHeight);
+    mAnim->SetLoopCount(aMetadata.GetLoopCount());
-  mOrientation = aOrientation;
+    mAnim->SetFirstFrameTimeout(aMetadata.GetFirstFrameTimeout());
-  mHasSize = true;
+  }
+
+  if (aMetadata.HasHotspot()) {
+    IntPoint hotspot = aMetadata.GetHotspot();
+
+    nsCOMPtr<nsISupportsPRUint32> intwrapx =
+      do_CreateInstance(NS_SUPPORTS_PRUINT32_CONTRACTID);
+    nsCOMPtr<nsISupportsPRUint32> intwrapy =
+      do_CreateInstance(NS_SUPPORTS_PRUINT32_CONTRACTID);
+    intwrapx->SetData(hotspot.x);
+    intwrapy->SetData(hotspot.y);
+
+    Set("hotspotX", intwrapx);
+    Set("hotspotY", intwrapy);
+  }
 
   return NS_OK;
 }
@@ -999,10 +1030,9 @@ RasterImage::StartAnimation()
 
   MOZ_ASSERT(ShouldAnimate(), "Should not animate!");
 
-  // If we don't have mAnim yet, then we're not ready to animate.  Setting
+  // If we're not ready to animate, then set mPendingAnimation, which will cause
-  // mPendingAnimation will cause us to start animating as soon as we have a
+  // us to start animating if and when we do become ready.
-  // second frame, which causes mAnim to be constructed.
+  mPendingAnimation = !mAnim || GetNumFrames() < 2;
-  mPendingAnimation = !mAnim;
   if (mPendingAnimation) {
     return NS_OK;
   }
@@ -1091,19 +1121,6 @@ RasterImage::GetFrameIndex(uint32_t aWhichFrame)
          : mAnim->GetCurrentAnimationFrameIndex();
 }
 
-void
-RasterImage::SetLoopCount(int32_t aLoopCount)
-{
-  if (mError) {
-    return;
-  }
-
-  // No need to set this if we're not an animation.
-  if (mAnim) {
-    mAnim->SetLoopCount(aLoopCount);
-  }
-}
-
 NS_IMETHODIMP_(IntRect)
 RasterImage::GetImageSpaceInvalidationRect(const IntRect& aRect)
 {
@@ -1391,20 +1408,19 @@ RasterImage::Decode(const IntSize& aSize, uint32_t aFlags)
 
   Maybe<IntSize> targetSize = mSize != aSize ? Some(aSize) : Nothing();
 
-  bool imageIsLocked = false;
-  if (!mHasBeenDecoded) {
-    // Lock the image while we're decoding, so that it doesn't get evicted from
-    // the SurfaceCache before we have a chance to realize that it's animated.
-    // The corresponding unlock happens in FinalizeDecoder.
-    LockImage();
-    imageIsLocked = true;
-  }
-
   // Create a decoder.
-  nsRefPtr<Decoder> decoder =
+  nsRefPtr<Decoder> decoder;
-    DecoderFactory::CreateDecoder(mDecoderType, this, mSourceBuffer, targetSize,
+  if (mAnim) {
-                                  aFlags, mRequestedSampleSize, mRequestedResolution,
+    decoder = DecoderFactory::CreateAnimationDecoder(mDecoderType, this,
-                                  mHasBeenDecoded, mTransient, imageIsLocked);
+                                                     mSourceBuffer, aFlags,
+                                                     mRequestedResolution);
+  } else {
+    decoder = DecoderFactory::CreateDecoder(mDecoderType, this, mSourceBuffer,
+                                            targetSize, aFlags,
+                                            mRequestedSampleSize,
+                                            mRequestedResolution,
+                                            mHasBeenDecoded, mTransient);
+  }
 
   // Make sure DecoderFactory was able to create a decoder successfully.
   if (!decoder) {
@@ -1953,7 +1969,8 @@ RasterImage::FinalizeDecoder(Decoder* aDecoder)
   }
 
   // Record all the metadata the decoder gathered about this image.
-  nsresult rv = aDecoder->GetImageMetadata().SetOnImage(this);
+  nsresult rv = SetMetadata(aDecoder->GetImageMetadata(),
+                            aDecoder->IsMetadataDecode());
   if (NS_FAILED(rv)) {
     aDecoder->PostResizeError();
   }
@@ -1964,17 +1981,8 @@ RasterImage::FinalizeDecoder(Decoder* aDecoder)
   if (aDecoder->GetDecodeTotallyDone() && !mError) {
     // Flag that we've been decoded before.
     mHasBeenDecoded = true;
-
+    if (mAnim) {
-    if (aDecoder->HasAnimation()) {
+      mAnim->SetDoneDecoding(true);
-      if (mAnim) {
-        mAnim->SetDoneDecoding(true);
-      } else {
-        // The OnAddedFrame event that will create mAnim is still in the event
-        // queue. Wait for it.
-        nsCOMPtr<nsIRunnable> runnable =
-          NS_NewRunnableMethod(this, &RasterImage::MarkAnimationDecoded);
-        NS_DispatchToMainThread(runnable);
-      }
     }
   }
 
@@ -2022,11 +2030,6 @@ RasterImage::FinalizeDecoder(Decoder* aDecoder)
     }
   }
 
-  if (aDecoder->ImageIsLocked()) {
-    // Unlock the image, balancing the LockImage call we made in CreateDecoder.
-    UnlockImage();
-  }
-
   // If we were a metadata decode and a full decode was requested, do it.
   if (done && wasMetadata && mWantFullDecode) {
     mWantFullDecode = false;
@@ -2034,17 +2037,6 @@ RasterImage::FinalizeDecoder(Decoder* aDecoder)
   }
 }
 
-void
-RasterImage::MarkAnimationDecoded()
-{
-  MOZ_ASSERT(mAnim, "Should have an animation now");
-  if (!mAnim) {
-    return;
-  }
-
-  mAnim->SetDoneDecoding(true);
-}
-
 void
 RasterImage::ReportDecoderError(Decoder* aDecoder)
 {

image/RasterImage.h

@@ -132,6 +132,7 @@ namespace image {
 
 class Decoder;
 class FrameAnimator;
+class ImageMetadata;
 class SourceBuffer;
 
 /**
@@ -188,18 +189,6 @@ public:
 
   void OnAddedFrame(uint32_t aNewFrameCount, const nsIntRect& aNewRefreshArea);
 
-  /** Sets the size and inherent orientation of the container. This should only
-   * be called by the decoder. This function may be called multiple times, but
-   * will throw an error if subsequent calls do not match the first.
-   */
-  nsresult SetSize(int32_t aWidth, int32_t aHeight, Orientation aOrientation);
-
-  /**
-   * Number of times to loop the image.
-   * @note -1 means forever.
-   */
-  void     SetLoopCount(int32_t aLoopCount);
-
   /**
    * Sends the provided progress notifications to ProgressTracker.
    *
@@ -222,8 +211,7 @@ public:
    */
   void FinalizeDecoder(Decoder* aDecoder);
 
-  // Helper methods for FinalizeDecoder.
+  // Helper method for FinalizeDecoder.
-  void MarkAnimationDecoded();
   void ReportDecoderError(Decoder* aDecoder);
 
 
@@ -339,6 +327,19 @@ private:
    */
   NS_IMETHOD DecodeMetadata(uint32_t aFlags);
 
+  /**
+   * Sets the size, inherent orientation, animation metadata, and other
+   * information about the image gathered during decoding.
+   *
+   * This function may be called multiple times, but will throw an error if
+   * subsequent calls do not match the first.
+   *
+   * @param aMetadata The metadata to set on this image.
+   * @param aFromMetadataDecode True if this metadata came from a metadata
+   *                            decode; false if it came from a full decode.
+   */
+  nsresult SetMetadata(const ImageMetadata& aMetadata, bool aFromMetadataDecode);
+
   /**
    * In catastrophic circumstances like a GPU driver crash, we may lose our
    * frames even if they're locked. RecoverFromLossOfFrames discards all

image/decoders/nsGIFDecoder2.cpp

@@ -857,6 +857,11 @@ nsGIFDecoder2::WriteInternal(const char* aBuffer, uint32_t aCount)
       }
 
       mGIFStruct.delay_time = GETINT16(q + 1) * 10;
+
+      if (mGIFStruct.delay_time > 0) {
+        PostIsAnimated(mGIFStruct.delay_time);
+      }
+
       GETN(1, gif_consume_block);
       break;
 
@@ -921,11 +926,20 @@ nsGIFDecoder2::WriteInternal(const char* aBuffer, uint32_t aCount)
       break;
 
     case gif_image_header: {
-      if (mGIFStruct.images_decoded > 0 && IsFirstFrameDecode()) {
+      if (mGIFStruct.images_decoded == 1) {
-        // We're about to get a second frame, but we only want the first. Stop
+        if (!HasAnimation()) {
-        // decoding now.
+          // We should've already called PostIsAnimated(); this must be a
-        mGIFStruct.state = gif_done;
+          // corrupt animated image with a first frame timeout of zero. Signal
-        break;
+          // that we're animated now, before the first-frame decode early exit
+          // below, so that RasterImage can detect that this happened.
+          PostIsAnimated(/* aFirstFrameTimeout = */ 0);
+        }
+        if (IsFirstFrameDecode()) {
+          // We're about to get a second frame, but we only want the first. Stop
+          // decoding now.
+          mGIFStruct.state = gif_done;
+          break;
+        }
       }
 
       // Get image offsets, with respect to the screen origin

image/decoders/nsICODecoder.cpp

@@ -378,8 +378,8 @@ nsICODecoder::WriteInternal(const char* aBuffer, uint32_t aCount)
     }
 
     if (!HasSize() && mContainedDecoder->HasSize()) {
-      PostSize(mContainedDecoder->GetImageMetadata().GetWidth(),
+      nsIntSize size = mContainedDecoder->GetSize();
-               mContainedDecoder->GetImageMetadata().GetHeight());
+      PostSize(size.width, size.height);
     }
 
     mPos += aCount;
@@ -479,8 +479,8 @@ nsICODecoder::WriteInternal(const char* aBuffer, uint32_t aCount)
       return;
     }
 
-    PostSize(mContainedDecoder->GetImageMetadata().GetWidth(),
+    nsIntSize size = mContainedDecoder->GetSize();
-             mContainedDecoder->GetImageMetadata().GetHeight());
+    PostSize(size.width, size.height);
 
     // We have the size. If we're doing a metadata decode, we're done.
     if (IsMetadataDecode()) {

image/decoders/nsPNGDecoder.cpp

@@ -56,32 +56,33 @@ nsPNGDecoder::AnimFrameInfo::AnimFrameInfo()
 { }
 
 #ifdef PNG_APNG_SUPPORTED
+
+int32_t GetNextFrameDelay(png_structp aPNG, png_infop aInfo)
+{
+  // Delay, in seconds, is delayNum / delayDen.
+  png_uint_16 delayNum = png_get_next_frame_delay_num(aPNG, aInfo);
+  png_uint_16 delayDen = png_get_next_frame_delay_den(aPNG, aInfo);
+
+  if (delayNum == 0) {
+    return 0; // SetFrameTimeout() will set to a minimum.
+  }
+
+  if (delayDen == 0) {
+    delayDen = 100; // So says the APNG spec.
+  }
+
+  // Need to cast delay_num to float to have a proper division and
+  // the result to int to avoid a compiler warning.
+  return static_cast<int32_t>(static_cast<double>(delayNum) * 1000 / delayDen);
+}
+
 nsPNGDecoder::AnimFrameInfo::AnimFrameInfo(png_structp aPNG, png_infop aInfo)
  : mDispose(DisposalMethod::KEEP)
  , mBlend(BlendMethod::OVER)
  , mTimeout(0)
 {
-  png_uint_16 delay_num, delay_den;
+  png_byte dispose_op = png_get_next_frame_dispose_op(aPNG, aInfo);
-  // delay, in seconds is delay_num/delay_den
+  png_byte blend_op = png_get_next_frame_blend_op(aPNG, aInfo);
-  png_byte dispose_op;
-  png_byte blend_op;
-  delay_num = png_get_next_frame_delay_num(aPNG, aInfo);
-  delay_den = png_get_next_frame_delay_den(aPNG, aInfo);
-  dispose_op = png_get_next_frame_dispose_op(aPNG, aInfo);
-  blend_op = png_get_next_frame_blend_op(aPNG, aInfo);
-
-  if (delay_num == 0) {
-    mTimeout = 0; // SetFrameTimeout() will set to a minimum
-  } else {
-    if (delay_den == 0) {
-      delay_den = 100; // so says the APNG spec
-    }
-
-    // Need to cast delay_num to float to have a proper division and
-    // the result to int to avoid compiler warning
-    mTimeout = static_cast<int32_t>(static_cast<double>(delay_num) *
-                                    1000 / delay_den);
-  }
 
   if (dispose_op == PNG_DISPOSE_OP_PREVIOUS) {
     mDispose = DisposalMethod::RESTORE_PREVIOUS;
@@ -96,6 +97,8 @@ nsPNGDecoder::AnimFrameInfo::AnimFrameInfo(png_structp aPNG, png_infop aInfo)
   } else {
     mBlend = BlendMethod::OVER;
   }
+
+  mTimeout = GetNextFrameDelay(aPNG, aInfo);
 }
 #endif
 
@@ -595,18 +598,25 @@ nsPNGDecoder::info_callback(png_structp png_ptr, png_infop info_ptr)
     png_longjmp(decoder->mPNG, 1); // invalid number of channels
   }
 
+#ifdef PNG_APNG_SUPPORTED
+  bool isAnimated = png_get_valid(png_ptr, info_ptr, PNG_INFO_acTL);
+  if (isAnimated) {
+    decoder->PostIsAnimated(GetNextFrameDelay(png_ptr, info_ptr));
+  }
+#endif
+
   if (decoder->IsMetadataDecode()) {
     decoder->CheckForTransparency(decoder->format,
                                   IntRect(0, 0, width, height));
 
-    // We have the size and transparency information we're looking for, so we
+    // We have the metadata we're looking for, so we don't need to decode any
-    // don't need to decode any further.
+    // further.
     decoder->mSuccessfulEarlyFinish = true;
     png_longjmp(decoder->mPNG, 1);
   }
 
 #ifdef PNG_APNG_SUPPORTED
-  if (png_get_valid(png_ptr, info_ptr, PNG_INFO_acTL)) {
+  if (isAnimated) {
     png_set_progressive_frame_fn(png_ptr, nsPNGDecoder::frame_info_callback,
                                  nullptr);
   }

image/moz.build

@@ -59,7 +59,6 @@ UNIFIED_SOURCES += [
     'Image.cpp',
     'ImageCacheKey.cpp',
     'ImageFactory.cpp',
-    'ImageMetadata.cpp',
     'ImageOps.cpp',
     'ImageWrapper.cpp',
     'imgFrame.cpp',