Bug 1117607 - Make decoders responsible for their own frame allocations. r=tn

image/decoders/nsBMPDecoder.cpp

@@ -439,11 +439,16 @@ nsBMPDecoder::WriteInternal(const char* aBuffer, uint32_t aCount)
           return;
         }
       }
-      if (!mImageData) {
+
-        PostDecoderError(NS_ERROR_FAILURE);
+      MOZ_ASSERT(!mImageData, "Already have a buffer allocated?");
+      nsresult rv = AllocateFrame(0, nsIntRect(nsIntPoint(), GetSize()),
+                                  gfx::SurfaceFormat::B8G8R8A8);
+      if (NS_FAILED(rv)) {
         return;
       }
 
+      MOZ_ASSERT(mImageData, "Should have a buffer now");
+
       // Prepare for transparency
       if ((mBIH.compression == BI_RLE8) || (mBIH.compression == BI_RLE4)) {
         // Clear the image, as the RLE may jump over areas

image/decoders/nsGIFDecoder2.cpp

@@ -161,7 +161,7 @@ nsGIFDecoder2::BeginGIF()
 }
 
 //******************************************************************************
-void
+nsresult
 nsGIFDecoder2::BeginImageFrame(uint16_t aDepth)
 {
   MOZ_ASSERT(HasSize());
@@ -174,35 +174,29 @@ nsGIFDecoder2::BeginImageFrame(uint16_t aDepth)
     format = gfx::SurfaceFormat::B8G8R8X8;
   }
 
+  nsIntRect frameRect(mGIFStruct.x_offset, mGIFStruct.y_offset,
+                      mGIFStruct.width, mGIFStruct.height);
+
   // Use correct format, RGB for first frame, PAL for following frames
   // and include transparency to allow for optimization of opaque images
+  nsresult rv = NS_OK;
   if (mGIFStruct.images_decoded) {
-    // Image data is stored with original depth and palette
+    // Image data is stored with original depth and palette.
-    NeedNewFrame(mGIFStruct.images_decoded, mGIFStruct.x_offset,
+    rv = AllocateFrame(mGIFStruct.images_decoded, frameRect, format, aDepth);
-                 mGIFStruct.y_offset, mGIFStruct.width, mGIFStruct.height,
-                 format, aDepth);
   } else {
-    nsRefPtr<imgFrame> currentFrame = GetCurrentFrame();
+    if (!nsIntRect(nsIntPoint(), GetSize()).IsEqualEdges(frameRect)) {
-
-    // Our first full frame is automatically created by the image decoding
-    // infrastructure. Just use it as long as it matches up.
-    if (!currentFrame->GetRect().IsEqualEdges(nsIntRect(mGIFStruct.x_offset,
-                                                        mGIFStruct.y_offset,
-                                                        mGIFStruct.width,
-                                                        mGIFStruct.height))) {
-
       // We need padding on the first frame, which means that we don't draw into
       // part of the image at all. Report that as transparency.
       PostHasTransparency();
-
-      // Regardless of depth of input, image is decoded into 24bit RGB
-      NeedNewFrame(mGIFStruct.images_decoded, mGIFStruct.x_offset,
-                   mGIFStruct.y_offset, mGIFStruct.width, mGIFStruct.height,
-                   format);
     }
+
+    // Regardless of depth of input, the first frame is decoded into 24bit RGB.
+    rv = AllocateFrame(mGIFStruct.images_decoded, frameRect, format);
   }
 
   mCurrentFrameIndex = mGIFStruct.images_decoded;
+
+  return rv;
 }
 
 
@@ -957,27 +951,13 @@ nsGIFDecoder2::WriteInternal(const char* aBuffer, uint32_t aCount)
       }
       // Mask to limit the color values within the colormap
       mColorMask = 0xFF >> (8 - realDepth);
-      BeginImageFrame(realDepth);
 
-      if (NeedsNewFrame()) {
+      if (NS_FAILED(BeginImageFrame(realDepth))) {
-        // We now need a new frame from the decoder framework. We leave all our
+        mGIFStruct.state = gif_error;
-        // data in the buffer as if it wasn't consumed, copy to our hold and
+        return;
-        // return to the decoder framework.
-        uint32_t size =
-          len + mGIFStruct.bytes_to_consume + mGIFStruct.bytes_in_hold;
-        if (size) {
-          if (SetHold(q,
-                      mGIFStruct.bytes_to_consume + mGIFStruct.bytes_in_hold,
-                      buf, len)) {
-            // Back into the decoder infrastructure so we can get called again.
-            GETN(9, gif_image_header_continue);
-            return;
-          }
-        }
-        break;
-      } else {
-        // FALL THROUGH
       }
+
+      // FALL THROUGH
     }
 
     case gif_image_header_continue: {

image/decoders/nsGIFDecoder2.h

@@ -35,7 +35,7 @@ private:
   // frame size information, etc.
 
   void      BeginGIF();
-  void      BeginImageFrame(uint16_t aDepth);
+  nsresult  BeginImageFrame(uint16_t aDepth);
   void      EndImageFrame();
   void      FlushImageData();
   void      FlushImageData(uint32_t fromRow, uint32_t rows);

image/decoders/nsICODecoder.cpp

@@ -82,10 +82,13 @@ nsICODecoder::FinishInternal()
 
   // Finish the internally used decoder as well
   if (mContainedDecoder) {
-    mContainedDecoder->FinishSharedDecoder();
+    if (!mContainedDecoder->HasError()) {
+      mContainedDecoder->FinishInternal();
+    }
     mDecodeDone = mContainedDecoder->GetDecodeDone();
     mProgress |= mContainedDecoder->TakeProgress();
     mInvalidRect.Union(mContainedDecoder->TakeInvalidRect());
+    mCurrentFrame = mContainedDecoder->GetCurrentFrameRef();
   }
 }
 
@@ -342,9 +345,7 @@ nsICODecoder::WriteInternal(const char* aBuffer, uint32_t aCount)
     if (mIsPNG) {
       mContainedDecoder = new nsPNGDecoder(mImage);
       mContainedDecoder->SetSizeDecode(IsSizeDecode());
-      mContainedDecoder->InitSharedDecoder(mImageData, mImageDataLength,
+      mContainedDecoder->Init();
-                                           mColormap, mColormapSize,
-                                           Move(mRefForContainedDecoder));
       if (!WriteToContainedDecoder(mSignature, PNGSIGNATURESIZE)) {
         return;
       }
@@ -420,9 +421,7 @@ nsICODecoder::WriteInternal(const char* aBuffer, uint32_t aCount)
     mContainedDecoder = bmpDecoder;
     bmpDecoder->SetUseAlphaData(true);
     mContainedDecoder->SetSizeDecode(IsSizeDecode());
-    mContainedDecoder->InitSharedDecoder(mImageData, mImageDataLength,
+    mContainedDecoder->Init();
-                                         mColormap, mColormapSize,
-                                         Move(mRefForContainedDecoder));
 
     // The ICO format when containing a BMP does not include the 14 byte
     // bitmap file header. To use the code of the BMP decoder we need to
@@ -625,35 +624,5 @@ nsICODecoder::ProcessDirEntry(IconDirEntry& aTarget)
   aTarget.mImageOffset = LittleEndian::readUint32(&aTarget.mImageOffset);
 }
 
-bool
-nsICODecoder::NeedsNewFrame() const
-{
-  if (mContainedDecoder) {
-    return mContainedDecoder->NeedsNewFrame();
-  }
-
-  return Decoder::NeedsNewFrame();
-}
-
-nsresult
-nsICODecoder::AllocateFrame()
-{
-  nsresult rv;
-
-  if (mContainedDecoder) {
-    rv = mContainedDecoder->AllocateFrame();
-    mCurrentFrame = mContainedDecoder->GetCurrentFrameRef();
-    mProgress |= mContainedDecoder->TakeProgress();
-    mInvalidRect.Union(mContainedDecoder->TakeInvalidRect());
-    return rv;
-  }
-
-  // Grab a strong ref that we'll later hand over to the contained decoder. This
-  // lets us avoid creating a RawAccessFrameRef off-main-thread.
-  rv = Decoder::AllocateFrame();
-  mRefForContainedDecoder = GetCurrentFrameRef();
-  return rv;
-}
-
 } // namespace image
 } // namespace mozilla

image/decoders/nsICODecoder.h

@@ -40,10 +40,6 @@ public:
 
   virtual void WriteInternal(const char* aBuffer, uint32_t aCount) MOZ_OVERRIDE;
   virtual void FinishInternal() MOZ_OVERRIDE;
-  virtual nsresult AllocateFrame() MOZ_OVERRIDE;
-
-protected:
-  virtual bool NeedsNewFrame() const MOZ_OVERRIDE;
 
 private:
   // Writes to the contained decoder and sets the appropriate errors
@@ -83,7 +79,6 @@ private:
   uint32_t mRowBytes; // How many bytes of the row were already received
   int32_t mOldLine;   // Previous index of the line
   nsRefPtr<Decoder> mContainedDecoder; // Contains either a BMP or PNG resource
-  RawAccessFrameRef mRefForContainedDecoder; // Avoid locking off-main-thread
 
   char mDirEntryArray[ICODIRENTRYSIZE]; // Holds the current dir entry buffer
   IconDirEntry mDirEntry; // Holds a decoded dir entry

image/decoders/nsIconDecoder.cpp

@@ -73,11 +73,18 @@ nsIconDecoder::WriteInternal(const char* aBuffer, uint32_t aCount)
           break;
         }
 
-        if (!mImageData) {
+        {
-          PostDecoderError(NS_ERROR_OUT_OF_MEMORY);
+          MOZ_ASSERT(!mImageData, "Already have a buffer allocated?");
-          return;
+          nsresult rv = AllocateFrame(0, nsIntRect(nsIntPoint(), GetSize()),
+                                      gfx::SurfaceFormat::B8G8R8A8);
+          if (NS_FAILED(rv)) {
+            mState = iconStateFinished;
+            return;
+          }
         }
 
+        MOZ_ASSERT(mImageData, "Should have a buffer now");
+
         // Book Keeping
         aBuffer++;
         aCount--;

image/decoders/nsJPEGDecoder.cpp

@@ -386,14 +386,18 @@ nsJPEGDecoder::WriteInternal(const char* aBuffer, uint32_t aCount)
     mInfo.buffered_image = mDecodeStyle == PROGRESSIVE &&
                            jpeg_has_multiple_scans(&mInfo);
 
-    if (!mImageData) {
+    MOZ_ASSERT(!mImageData, "Already have a buffer allocated?");
+    nsresult rv = AllocateFrame(0, nsIntRect(nsIntPoint(), GetSize()),
+                                gfx::SurfaceFormat::B8G8R8A8);
+    if (NS_FAILED(rv)) {
       mState = JPEG_ERROR;
-      PostDecoderError(NS_ERROR_OUT_OF_MEMORY);
       PR_LOG(GetJPEGDecoderAccountingLog(), PR_LOG_DEBUG,
              ("} (could not initialize image frame)"));
       return;
     }
 
+    MOZ_ASSERT(mImageData, "Should have a buffer now");
+
     PR_LOG(GetJPEGDecoderAccountingLog(), PR_LOG_DEBUG,
            ("        JPEGDecoderAccounting: nsJPEGDecoder::"
             "Write -- created image frame with %ux%u pixels",

image/decoders/nsPNGDecoder.cpp

@@ -141,39 +141,44 @@ nsPNGDecoder::~nsPNGDecoder()
 }
 
 // CreateFrame() is used for both simple and animated images
-void nsPNGDecoder::CreateFrame(png_uint_32 x_offset, png_uint_32 y_offset,
+nsresult
-                               int32_t width, int32_t height,
+nsPNGDecoder::CreateFrame(png_uint_32 aXOffset, png_uint_32 aYOffset,
-                               gfx::SurfaceFormat format)
+                          int32_t aWidth, int32_t aHeight,
+                          gfx::SurfaceFormat aFormat)
 {
   MOZ_ASSERT(HasSize());
 
-  if (format == gfx::SurfaceFormat::B8G8R8A8) {
+  if (aFormat == gfx::SurfaceFormat::B8G8R8A8) {
     PostHasTransparency();
   }
 
-  // Our first full frame is automatically created by the image decoding
+  nsIntRect frameRect(aXOffset, aYOffset, aWidth, aHeight);
-  // infrastructure. Just use it as long as it matches up.
+  if (mNumFrames == 0 &&
-  nsIntRect neededRect(x_offset, y_offset, width, height);
+      !nsIntRect(nsIntPoint(), GetSize()).IsEqualEdges(frameRect)) {
-  nsRefPtr<imgFrame> currentFrame = GetCurrentFrame();
+    // We need padding on the first frame, which means that we don't draw into
-  if (!currentFrame->GetRect().IsEqualEdges(neededRect)) {
+    // part of the image at all. Report that as transparency.
-    if (mNumFrames == 0) {
+    PostHasTransparency();
-      // We need padding on the first frame, which means that we don't draw into
-      // part of the image at all. Report that as transparency.
-      PostHasTransparency();
-    }
-
-    NeedNewFrame(mNumFrames, x_offset, y_offset, width, height, format);
-  } else if (mNumFrames != 0) {
-    NeedNewFrame(mNumFrames, x_offset, y_offset, width, height, format);
   }
 
-  mFrameRect = neededRect;
+  // XXX(seth): Some tests depend on the first frame of PNGs being B8G8R8A8.
+  // This is something we should fix.
+  gfx::SurfaceFormat format = aFormat;
+  if (mNumFrames == 0) {
+    format = gfx::SurfaceFormat::B8G8R8A8;
+  }
+
+  nsresult rv = AllocateFrame(mNumFrames, frameRect, format);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  mFrameRect = frameRect;
   mFrameHasNoAlpha = true;
 
   PR_LOG(GetPNGDecoderAccountingLog(), PR_LOG_DEBUG,
          ("PNGDecoderAccounting: nsPNGDecoder::CreateFrame -- created "
           "image frame with %dx%d pixels in container %p",
-          width, height,
+          aWidth, aHeight,
           &mImage));
 
 #ifdef PNG_APNG_SUPPORTED
@@ -187,6 +192,8 @@ void nsPNGDecoder::CreateFrame(png_uint_32 x_offset, png_uint_32 y_offset,
     }
   }
 #endif
+
+  return NS_OK;
 }
 
 // set timeout and frame disposal method for the current frame
@@ -649,7 +656,11 @@ nsPNGDecoder::info_callback(png_structp png_ptr, png_infop info_ptr)
     decoder->mFrameIsHidden = true;
   } else {
 #endif
-    decoder->CreateFrame(0, 0, width, height, decoder->format);
+    nsresult rv = decoder->CreateFrame(0, 0, width, height, decoder->format);
+    if (NS_FAILED(rv)) {
+      png_longjmp(decoder->mPNG, 5); // NS_ERROR_OUT_OF_MEMORY
+    }
+    MOZ_ASSERT(decoder->mImageData, "Should have a buffer now");
 #ifdef PNG_APNG_SUPPORTED
   }
 #endif
@@ -672,12 +683,6 @@ nsPNGDecoder::info_callback(png_structp png_ptr, png_infop info_ptr)
       png_longjmp(decoder->mPNG, 5); // NS_ERROR_OUT_OF_MEMORY
     }
   }
-
-  if (decoder->NeedsNewFrame()) {
-    // We know that we need a new frame, so pause input so the decoder
-    // infrastructure can give it to us.
-    png_process_data_pause(png_ptr, /* save = */ 1);
-  }
 }
 
 void
@@ -840,13 +845,12 @@ nsPNGDecoder::frame_info_callback(png_structp png_ptr, png_uint_32 frame_num)
   width = png_get_next_frame_width(png_ptr, decoder->mInfo);
   height = png_get_next_frame_height(png_ptr, decoder->mInfo);
 
-  decoder->CreateFrame(x_offset, y_offset, width, height, decoder->format);
+  nsresult rv =
-
+    decoder->CreateFrame(x_offset, y_offset, width, height, decoder->format);
-  if (decoder->NeedsNewFrame()) {
+  if (NS_FAILED(rv)) {
-    // We know that we need a new frame, so pause input so the decoder
+    png_longjmp(decoder->mPNG, 5); // NS_ERROR_OUT_OF_MEMORY
-    // infrastructure can give it to us.
-    png_process_data_pause(png_ptr, /* save = */ 1);
   }
+  MOZ_ASSERT(decoder->mImageData, "Should have a buffer now");
 }
 #endif
 

image/decoders/nsPNGDecoder.h

@@ -31,9 +31,9 @@ public:
   virtual void WriteInternal(const char* aBuffer, uint32_t aCount) MOZ_OVERRIDE;
   virtual Telemetry::ID SpeedHistogram() MOZ_OVERRIDE;
 
-  void CreateFrame(png_uint_32 x_offset, png_uint_32 y_offset,
+  nsresult CreateFrame(png_uint_32 aXOffset, png_uint_32 aYOffset,
-                   int32_t width, int32_t height,
+                       int32_t aWidth, int32_t aHeight,
-                   gfx::SurfaceFormat format);
+                       gfx::SurfaceFormat aFormat);
   void EndImageFrame();
 
   // Check if PNG is valid ICO (32bpp RGBA)

image/src/Decoder.cpp

@@ -32,8 +32,6 @@ Decoder::Decoder(RasterImage &aImage)
   , mDataError(false)
   , mFrameCount(0)
   , mFailCode(NS_OK)
-  , mNeedsNewFrame(false)
-  , mNeedsToFlushData(false)
   , mInitialized(false)
   , mSizeDecode(false)
   , mInFrame(false)
@@ -70,39 +68,15 @@ Decoder::Init()
   mInitialized = true;
 }
 
-// Initializes a decoder whose image and observer is already being used by a
-// parent decoder
-void
-Decoder::InitSharedDecoder(uint8_t* aImageData, uint32_t aImageDataLength,
-                           uint32_t* aColormap, uint32_t aColormapSize,
-                           RawAccessFrameRef&& aFrameRef)
-{
-  // No re-initializing
-  NS_ABORT_IF_FALSE(!mInitialized, "Can't re-initialize a decoder!");
-
-  mImageData = aImageData;
-  mImageDataLength = aImageDataLength;
-  mColormap = aColormap;
-  mColormapSize = aColormapSize;
-  mCurrentFrame = Move(aFrameRef);
-
-  // We have all the frame data, so we've started the frame.
-  if (!IsSizeDecode()) {
-    mFrameCount++;
-    PostFrameStart();
-  }
-
-  // Implementation-specific initialization
-  InitInternal();
-  mInitialized = true;
-}
-
 void
 Decoder::Write(const char* aBuffer, uint32_t aCount)
 {
   PROFILER_LABEL("ImageDecoder", "Write",
     js::ProfileEntry::Category::GRAPHICS);
 
+  MOZ_ASSERT(aBuffer, "Should have a buffer");
+  MOZ_ASSERT(aCount > 0, "Should have a non-zero count");
+
   // We're strict about decoder errors
   MOZ_ASSERT(!HasDecoderError(),
              "Not allowed to make more decoder calls after error!");
@@ -114,12 +88,6 @@ Decoder::Write(const char* aBuffer, uint32_t aCount)
   // Keep track of the total number of bytes written.
   mBytesDecoded += aCount;
 
-  // If we're flushing data, clear the flag.
-  if (aBuffer == nullptr && aCount == 0) {
-    MOZ_ASSERT(mNeedsToFlushData, "Flushing when we don't need to");
-    mNeedsToFlushData = false;
-  }
-
   // If a data error occured, just ignore future data.
   if (HasDataError())
     return;
@@ -129,28 +97,9 @@ Decoder::Write(const char* aBuffer, uint32_t aCount)
     return;
   }
 
-  MOZ_ASSERT(!NeedsNewFrame() || HasDataError(),
-             "Should not need a new frame before writing anything");
-  MOZ_ASSERT(!NeedsToFlushData() || HasDataError(),
-             "Should not need to flush data before writing anything");
-
   // Pass the data along to the implementation.
   WriteInternal(aBuffer, aCount);
 
-  // If we need a new frame to proceed, let's create one and call it again.
-  while (NeedsNewFrame() && !HasDataError()) {
-    MOZ_ASSERT(!IsSizeDecode(), "Shouldn't need new frame for size decode");
-
-    nsresult rv = AllocateFrame();
-
-    if (NS_SUCCEEDED(rv)) {
-      // Use the data we saved when we asked for a new frame.
-      WriteInternal(nullptr, 0);
-    }
-
-    mNeedsToFlushData = false;
-  }
-
   // Finish telemetry.
   mDecodeTime += (TimeStamp::Now() - start);
 }
@@ -221,119 +170,49 @@ Decoder::Finish(ShutdownReason aReason)
 
   if (mDecodeDone) {
     MOZ_ASSERT(HasError() || mCurrentFrame, "Should have an error or a frame");
-    mImage.DecodingComplete(mCurrentFrame.get());
+    mImage.DecodingComplete(mCurrentFrame.get(), mIsAnimated);
-  }
-}
-
-void
-Decoder::FinishSharedDecoder()
-{
-  MOZ_ASSERT(NS_IsMainThread());
-
-  if (!HasError()) {
-    FinishInternal();
   }
 }
 
 nsresult
-Decoder::AllocateFrame()
+Decoder::AllocateFrame(uint32_t aFrameNum,
+                       const nsIntRect& aFrameRect,
+                       gfx::SurfaceFormat aFormat,
+                       uint8_t aPaletteDepth /* = 0 */)
 {
-  MOZ_ASSERT(mNeedsNewFrame);
+  mCurrentFrame = AllocateFrameInternal(aFrameNum, aFrameRect, mDecodeFlags,
-
+                                        aFormat, aPaletteDepth,
-  mCurrentFrame = EnsureFrame(mNewFrameData.mFrameNum,
+                                        mCurrentFrame.get());
-                              mNewFrameData.mFrameRect,
-                              mDecodeFlags,
-                              mNewFrameData.mFormat,
-                              mNewFrameData.mPaletteDepth,
-                              mCurrentFrame.get());
 
   if (mCurrentFrame) {
     // Gather the raw pointers the decoders will use.
     mCurrentFrame->GetImageData(&mImageData, &mImageDataLength);
     mCurrentFrame->GetPaletteData(&mColormap, &mColormapSize);
 
-    if (mNewFrameData.mFrameNum + 1 == mFrameCount) {
+    if (aFrameNum + 1 == mFrameCount) {
       PostFrameStart();
     }
   } else {
     PostDataError();
   }
 
-  // Mark ourselves as not needing another frame before talking to anyone else
-  // so they can tell us if they need yet another.
-  mNeedsNewFrame = false;
-
-  // If we've received any data at all, we may have pending data that needs to
-  // be flushed now that we have a frame to decode into.
-  if (mBytesDecoded > 0) {
-    mNeedsToFlushData = true;
-  }
-
   return mCurrentFrame ? NS_OK : NS_ERROR_FAILURE;
 }
 
 RawAccessFrameRef
-Decoder::EnsureFrame(uint32_t aFrameNum,
+Decoder::AllocateFrameInternal(uint32_t aFrameNum,
-                     const nsIntRect& aFrameRect,
+                               const nsIntRect& aFrameRect,
-                     uint32_t aDecodeFlags,
+                               uint32_t aDecodeFlags,
-                     SurfaceFormat aFormat,
+                               SurfaceFormat aFormat,
-                     uint8_t aPaletteDepth,
+                               uint8_t aPaletteDepth,
-                     imgFrame* aPreviousFrame)
+                               imgFrame* aPreviousFrame)
 {
   if (mDataError || NS_FAILED(mFailCode)) {
     return RawAccessFrameRef();
   }
 
-  MOZ_ASSERT(aFrameNum <= mFrameCount, "Invalid frame index!");
+  if (aFrameNum != mFrameCount) {
-  if (aFrameNum > mFrameCount) {
+    MOZ_ASSERT_UNREACHABLE("Allocating frames out of order");
-    return RawAccessFrameRef();
-  }
-
-  // Adding a frame that doesn't already exist. This is the normal case.
-  if (aFrameNum == mFrameCount) {
-    return InternalAddFrame(aFrameNum, aFrameRect, aDecodeFlags, aFormat,
-                            aPaletteDepth, aPreviousFrame);
-  }
-
-  // We're replacing a frame. It must be the first frame; there's no reason to
-  // ever replace any other frame, since the first frame is the only one we
-  // speculatively allocate without knowing what the decoder really needs.
-  // XXX(seth): I'm not convinced there's any reason to support this at all. We
-  // should figure out how to avoid triggering this and rip it out.
-  MOZ_ASSERT(aFrameNum == 0, "Replacing a frame other than the first?");
-  MOZ_ASSERT(mFrameCount == 1, "Should have only one frame");
-  MOZ_ASSERT(aPreviousFrame, "Need the previous frame to replace");
-  if (aFrameNum != 0 || !aPreviousFrame || mFrameCount != 1) {
-    return RawAccessFrameRef();
-  }
-
-  MOZ_ASSERT(!aPreviousFrame->GetRect().IsEqualEdges(aFrameRect) ||
-             aPreviousFrame->GetFormat() != aFormat ||
-             aPreviousFrame->GetPaletteDepth() != aPaletteDepth,
-             "Replacing first frame with the same kind of frame?");
-
-  // Remove the old frame from the SurfaceCache.
-  IntSize prevFrameSize = aPreviousFrame->GetImageSize();
-  SurfaceCache::RemoveSurface(ImageKey(&mImage),
-                              RasterSurfaceKey(prevFrameSize, aDecodeFlags, 0));
-  mFrameCount = 0;
-  mInFrame = false;
-
-  // Add the new frame as usual.
-  return InternalAddFrame(aFrameNum, aFrameRect, aDecodeFlags, aFormat,
-                          aPaletteDepth, nullptr);
-}
-
-RawAccessFrameRef
-Decoder::InternalAddFrame(uint32_t aFrameNum,
-                          const nsIntRect& aFrameRect,
-                          uint32_t aDecodeFlags,
-                          SurfaceFormat aFormat,
-                          uint8_t aPaletteDepth,
-                          imgFrame* aPreviousFrame)
-{
-  MOZ_ASSERT(aFrameNum <= mFrameCount, "Invalid frame index!");
-  if (aFrameNum > mFrameCount) {
     return RawAccessFrameRef();
   }
 
@@ -528,23 +407,5 @@ Decoder::PostDecoderError(nsresult aFailureCode)
   NS_WARNING("Image decoding error - This is probably a bug!");
 }
 
-void
-Decoder::NeedNewFrame(uint32_t framenum, uint32_t x_offset, uint32_t y_offset,
-                      uint32_t width, uint32_t height,
-                      gfx::SurfaceFormat format,
-                      uint8_t palette_depth /* = 0 */)
-{
-  // Decoders should never call NeedNewFrame without yielding back to Write().
-  MOZ_ASSERT(!mNeedsNewFrame);
-
-  // We don't want images going back in time or skipping frames.
-  MOZ_ASSERT(framenum == mFrameCount || framenum == (mFrameCount - 1));
-
-  mNewFrameData = NewFrameData(framenum,
-                               nsIntRect(x_offset, y_offset, width, height),
-                               format, palette_depth);
-  mNeedsNewFrame = true;
-}
-
 } // namespace image
 } // namespace mozilla

image/src/Decoder.h

@@ -31,24 +31,9 @@ public:
    */
   void Init();
 
-  /**
-   * Initializes a decoder whose image and observer is already being used by a
-   * parent decoder. Decoders may not be re-initialized.
-   *
-   * Notifications Sent: TODO
-   */
-  void InitSharedDecoder(uint8_t* aImageData, uint32_t aImageDataLength,
-                         uint32_t* aColormap, uint32_t aColormapSize,
-                         RawAccessFrameRef&& aFrameRef);
-
   /**
    * Writes data to the decoder.
    *
-   * If aBuffer is null and aCount is 0, Write() flushes any buffered data to
-   * the decoder. Data is buffered if the decoder wasn't able to completely
-   * decode it because it needed a new frame.  If it's necessary to flush data,
-   * NeedsToFlushData() will return true.
-   *
    * @param aBuffer buffer containing the data to be written
    * @param aCount the number of bytes to write
    *
@@ -65,14 +50,6 @@ public:
    */
   void Finish(ShutdownReason aReason);
 
-  /**
-   * Informs the shared decoder that all the data has been written.
-   * Should only be used if InitSharedDecoder was useed
-   *
-   * Notifications Sent: TODO
-   */
-  void FinishSharedDecoder();
-
   /**
    * Gets the invalidation region accumulated by the decoder so far, and clears
    * the decoder's invalidation region. This means that each call to
@@ -156,37 +133,15 @@ public:
   void SetDecodeFlags(uint32_t aFlags) { mDecodeFlags = aFlags; }
   uint32_t GetDecodeFlags() { return mDecodeFlags; }
 
+  nsIntSize GetSize() const { return mImageMetadata.GetSize(); }
   bool HasSize() const { return mImageMetadata.HasSize(); }
   void SetSizeOnImage();
 
-  void SetSize(const nsIntSize& aSize, const Orientation& aOrientation)
-  {
-    PostSize(aSize.width, aSize.height, aOrientation);
-  }
-
   // Use HistogramCount as an invalid Histogram ID
   virtual Telemetry::ID SpeedHistogram() { return Telemetry::HistogramCount; }
 
   ImageMetadata& GetImageMetadata() { return mImageMetadata; }
 
-  // Tell the decoder infrastructure to allocate a frame. By default, frame 0
-  // is created as an ARGB frame with no offset and with size width * height.
-  // If decoders need something different, they must ask for it.
-  // This is called by decoders when they need a new frame. These decoders
-  // must then save the data they have been sent but not yet processed and
-  // return from WriteInternal. When the new frame is created, WriteInternal
-  // will be called again with nullptr and 0 as arguments.
-  void NeedNewFrame(uint32_t frameNum, uint32_t x_offset, uint32_t y_offset,
-                    uint32_t width, uint32_t height,
-                    gfx::SurfaceFormat format,
-                    uint8_t palette_depth = 0);
-  virtual bool NeedsNewFrame() const { return mNeedsNewFrame; }
-
-
-  // Try to allocate a frame as described in mNewFrameData and return the
-  // status code from that attempt. Clears mNewFrameData.
-  virtual nsresult AllocateFrame();
-
   already_AddRefed<imgFrame> GetCurrentFrame()
   {
     nsRefPtr<imgFrame> frame = mCurrentFrame.get();
@@ -200,6 +155,8 @@ public:
   }
 
 protected:
+  friend class nsICODecoder;
+
   virtual ~Decoder();
 
   /*
@@ -264,32 +221,26 @@ protected:
   void PostDataError();
   void PostDecoderError(nsresult aFailCode);
 
-  // Returns true if we may have stored data that we need to flush now that we
-  // have a new frame to decode into. Callers can use Write() to actually
-  // flush the data; see the documentation for that method.
-  bool NeedsToFlushData() const { return mNeedsToFlushData; }
-
   /**
-   * Ensures that a given frame number exists with the given parameters, and
+   * Allocates a new frame, making it our new current frame if successful.
-   * returns a RawAccessFrameRef for that frame.
+   *
-   * It is not possible to create sparse frame arrays; you can only append
+   * The @aFrameNum parameter only exists as a sanity check; it's illegal to
-   * frames to the current frame array, or if there is only one frame in the
+   * create a new frame anywhere but immediately after the existing frames.
-   * array, replace that frame.
+   *
    * If a non-paletted frame is desired, pass 0 for aPaletteDepth.
    */
-  RawAccessFrameRef EnsureFrame(uint32_t aFrameNum,
+  nsresult AllocateFrame(uint32_t aFrameNum,
-                                const nsIntRect& aFrameRect,
+                         const nsIntRect& aFrameRect,
-                                uint32_t aDecodeFlags,
+                         gfx::SurfaceFormat aFormat,
-                                gfx::SurfaceFormat aFormat,
+                         uint8_t aPaletteDepth = 0);
-                                uint8_t aPaletteDepth,
+
-                                imgFrame* aPreviousFrame);
+  RawAccessFrameRef AllocateFrameInternal(uint32_t aFrameNum,
+                                          const nsIntRect& aFrameRect,
+                                          uint32_t aDecodeFlags,
+                                          gfx::SurfaceFormat aFormat,
+                                          uint8_t aPaletteDepth,
+                                          imgFrame* aPreviousFrame);
 
-  RawAccessFrameRef InternalAddFrame(uint32_t aFrameNum,
-                                     const nsIntRect& aFrameRect,
-                                     uint32_t aDecodeFlags,
-                                     gfx::SurfaceFormat aFormat,
-                                     uint8_t aPaletteDepth,
-                                     imgFrame* aPreviousFrame);
   /*
    * Member variables.
    *
@@ -319,27 +270,6 @@ private:
 
   nsresult mFailCode;
 
-  struct NewFrameData
-  {
-    NewFrameData() { }
-
-    NewFrameData(uint32_t aFrameNum, const nsIntRect& aFrameRect,
-                 gfx::SurfaceFormat aFormat, uint8_t aPaletteDepth)
-      : mFrameNum(aFrameNum)
-      , mFrameRect(aFrameRect)
-      , mFormat(aFormat)
-      , mPaletteDepth(aPaletteDepth)
-    { }
-
-    uint32_t mFrameNum;
-    nsIntRect mFrameRect;
-    gfx::SurfaceFormat mFormat;
-    uint8_t mPaletteDepth;
-  };
-
-  NewFrameData mNewFrameData;
-  bool mNeedsNewFrame;
-  bool mNeedsToFlushData;
   bool mInitialized;
   bool mSizeDecode;
   bool mInFrame;

image/src/ImageMetadata.h

@@ -53,6 +53,7 @@ public:
 
   int32_t GetWidth() const { return mSize->width; }
   int32_t GetHeight() const { return mSize->height; }
+  nsIntSize GetSize() const { return *mSize; }
   Orientation GetOrientation() const { return *mOrientation; }
 
 private:

image/src/RasterImage.cpp

@@ -933,9 +933,9 @@ RasterImage::OnAddedFrame(uint32_t aNewFrameCount,
     return;
   }
 
-  MOZ_ASSERT((mFrameCount == 1 && aNewFrameCount == 1) ||
+  MOZ_ASSERT(aNewFrameCount <= mFrameCount ||
-             mFrameCount < aNewFrameCount,
+             aNewFrameCount == mFrameCount + 1,
-             "Frame count running backwards");
+             "Skipped a frame?");
 
   mFrameCount = aNewFrameCount;
 
@@ -1003,7 +1003,7 @@ RasterImage::SetSize(int32_t aWidth, int32_t aHeight, Orientation aOrientation)
 }
 
 void
-RasterImage::DecodingComplete(imgFrame* aFinalFrame)
+RasterImage::DecodingComplete(imgFrame* aFinalFrame, bool aIsAnimated)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
@@ -1019,15 +1019,31 @@ RasterImage::DecodingComplete(imgFrame* aFinalFrame)
 
   // If there's only 1 frame, mark it as optimizable. Optimizing animated images
   // is not supported. Optimizing transient images isn't worth it.
-  if (GetNumFrames() == 1 && !mTransient && aFinalFrame) {
+  if (!aIsAnimated && !mTransient && aFinalFrame) {
     aFinalFrame->SetOptimizable();
   }
 
-  if (mAnim) {
+  if (aIsAnimated) {
-    mAnim->SetDoneDecoding(true);
+    if (mAnim) {
+      mAnim->SetDoneDecoding(true);
+    } else {
+      NS_DispatchToMainThread(
+        NS_NewRunnableMethod(this, &RasterImage::MarkAnimationDecoded));
+    }
   }
 }
 
+void
+RasterImage::MarkAnimationDecoded()
+{
+  NS_ASSERTION(mAnim, "No FrameAnimator in MarkAnimationDecoded - bad event order");
+  if (!mAnim) {
+    return;
+  }
+
+  mAnim->SetDoneDecoding(true);
+}
+
 NS_IMETHODIMP
 RasterImage::SetAnimationMode(uint16_t aAnimationMode)
 {
@@ -1480,15 +1496,6 @@ RasterImage::InitDecoder(bool aDoSizeDecode)
   // Initialize the decoder
   mDecoder->SetSizeDecode(aDoSizeDecode);
   mDecoder->SetDecodeFlags(mFrameDecodeFlags);
-  if (!aDoSizeDecode) {
-    // We already have the size; tell the decoder so it can preallocate a
-    // frame.  By default, we create an ARGB frame with no offset. If decoders
-    // need a different type, they need to ask for it themselves.
-    mDecoder->SetSize(mSize, mOrientation);
-    mDecoder->NeedNewFrame(0, 0, 0, mSize.width, mSize.height,
-                           SurfaceFormat::B8G8R8A8);
-    mDecoder->AllocateFrame();
-  }
   mDecoder->Init();
   CONTAINER_ENSURE_SUCCESS(mDecoder->GetDecoderError());
 

image/src/RasterImage.h

@@ -197,7 +197,8 @@ public:
   void     SetLoopCount(int32_t aLoopCount);
 
   /* notification that the entire image has been decoded */
-  void DecodingComplete(imgFrame* aFinalFrame);
+  void DecodingComplete(imgFrame* aFinalFrame, bool aIsAnimated);
+  void MarkAnimationDecoded();
 
 
   //////////////////////////////////////////////////////////////////////////////

image/test/reftest/gif/reftest.list

@@ -17,7 +17,8 @@
 random == delaytest.html?transparent-animation.gif transparent-animation-finalframe.gif # incorrect timing dependence (bug 558678)
 
 # test for bug 641198
-skip-if(B2G) random-if(Android) == test_bug641198.html animation2a-finalframe.gif # bug 773482
+# Marked random because of increased failure frequency after bug 1117607.
+skip == test_bug641198.html animation2a-finalframe.gif # bug 773482
 
 # Bug 1062886: a gif with a single color and an offset
 == one-color-offset.gif one-color-offset-ref.gif