Bug 1117607 - Make decoders responsible for their own frame allocations. r=tn
☠☠ backed out by d4be320ebecb ☠ ☠
authorSeth Fowler <seth@mozilla.com>
Fri, 09 Jan 2015 15:02:48 -0800
changeset 236133 17fc30214d848a64c87e87b043566d3499dd1c03
parent 236132 eb4ffa05e89c100b85459f149264b3c2cd9b0461
child 236134 bceadf54fee87aebcd295cd7bf1320d5976e0b27
push id389
push usermartin.thomson@gmail.com
push dateFri, 09 Jan 2015 23:59:51 +0000
reviewerstn
bugs1117607
milestone37.0a1
Bug 1117607 - Make decoders responsible for their own frame allocations. r=tn
image/decoders/nsBMPDecoder.cpp
image/decoders/nsGIFDecoder2.cpp
image/decoders/nsGIFDecoder2.h
image/decoders/nsICODecoder.cpp
image/decoders/nsICODecoder.h
image/decoders/nsIconDecoder.cpp
image/decoders/nsJPEGDecoder.cpp
image/decoders/nsPNGDecoder.cpp
image/decoders/nsPNGDecoder.h
image/src/Decoder.cpp
image/src/Decoder.h
image/src/ImageMetadata.h
image/src/RasterImage.cpp
testing/web-platform/meta/2dcontext/drawing-images-to-the-canvas/2d.drawImage.broken.html.ini
--- a/image/decoders/nsBMPDecoder.cpp
+++ b/image/decoders/nsBMPDecoder.cpp
@@ -434,21 +434,26 @@ nsBMPDecoder::WriteInternal(const char* 
         // + 4 because the line is padded to a 4 bit boundary, but
         // I don't want to make exact calculations here, that's unnecessary.
         // Also, it compensates rounding error.
         if (!mRow) {
           PostDataError();
           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
         memset(mImageData, 0, mImageDataLength);
       }
   }
 
   if (mColors && mPos >= mLOH) {
--- a/image/decoders/nsGIFDecoder2.cpp
+++ b/image/decoders/nsGIFDecoder2.cpp
@@ -156,58 +156,52 @@ nsGIFDecoder2::BeginGIF()
   }
 
   mGIFOpen = true;
 
   PostSize(mGIFStruct.screen_width, mGIFStruct.screen_height);
 }
 
 //******************************************************************************
-void
+nsresult
 nsGIFDecoder2::BeginImageFrame(uint16_t aDepth)
 {
   MOZ_ASSERT(HasSize());
 
   gfx::SurfaceFormat format;
   if (mGIFStruct.is_transparent) {
     format = gfx::SurfaceFormat::B8G8R8A8;
     PostHasTransparency();
   } else {
     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
-    NeedNewFrame(mGIFStruct.images_decoded, mGIFStruct.x_offset,
-                 mGIFStruct.y_offset, mGIFStruct.width, mGIFStruct.height,
-                 format, aDepth);
+    // Image data is stored with original depth and palette.
+    rv = AllocateFrame(mGIFStruct.images_decoded, frameRect, format, aDepth);
   } else {
-    nsRefPtr<imgFrame> currentFrame = GetCurrentFrame();
-
-    // 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))) {
-
+    if (!nsIntRect(nsIntPoint(), GetSize()).IsEqualEdges(frameRect)) {
       // 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;
 }
 
 
 //******************************************************************************
 void
 nsGIFDecoder2::EndImageFrame()
 {
   Opacity opacity = Opacity::SOME_TRANSPARENCY;
@@ -952,37 +946,23 @@ nsGIFDecoder2::WriteInternal(const char*
         depth = (q[8]&0x07) + 1;
       }
       uint32_t realDepth = depth;
       while (mGIFStruct.tpixel >= (1 << realDepth) && (realDepth < 8)) {
         realDepth++;
       }
       // Mask to limit the color values within the colormap
       mColorMask = 0xFF >> (8 - realDepth);
-      BeginImageFrame(realDepth);
 
-      if (NeedsNewFrame()) {
-        // We now need a new frame from the decoder framework. We leave all our
-        // data in the buffer as if it wasn't consumed, copy to our hold and
-        // 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
+      if (NS_FAILED(BeginImageFrame(realDepth))) {
+        mGIFStruct.state = gif_error;
+        return;
       }
+
+      // FALL THROUGH
     }
 
     case gif_image_header_continue: {
       // While decoders can reuse frames, we unconditionally increment
       // mGIFStruct.images_decoded when we're done with a frame, so we both can
       // and need to zero out the colormap and image data after every new frame.
       memset(mImageData, 0, mImageDataLength);
       if (mColormap) {
--- a/image/decoders/nsGIFDecoder2.h
+++ b/image/decoders/nsGIFDecoder2.h
@@ -30,17 +30,17 @@ public:
   virtual void FinishInternal() MOZ_OVERRIDE;
   virtual Telemetry::ID SpeedHistogram() MOZ_OVERRIDE;
 
 private:
   // These functions will be called when the decoder has a decoded row,
   // 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);
 
   nsresult  GifWrite(const uint8_t* buf, uint32_t numbytes);
   uint32_t  OutputRow();
   bool      DoLzw(const uint8_t* q);
   bool      SetHold(const uint8_t* buf, uint32_t count,
--- a/image/decoders/nsICODecoder.cpp
+++ b/image/decoders/nsICODecoder.cpp
@@ -77,20 +77,23 @@ nsICODecoder::~nsICODecoder()
 void
 nsICODecoder::FinishInternal()
 {
   // We shouldn't be called in error cases
   NS_ABORT_IF_FALSE(!HasError(), "Shouldn't call FinishInternal after error!");
 
   // 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();
   }
 }
 
 // Returns a buffer filled with the bitmap file header in little endian:
 // Signature 2 bytes 'BM'
 // FileSize      4 bytes File size in bytes
 // reserved      4 bytes unused (=0)
 // DataOffset    4 bytes File offset to Raster Data
@@ -337,19 +340,17 @@ nsICODecoder::WriteInternal(const char* 
     aCount -= toCopy;
     aBuffer += toCopy;
 
     mIsPNG = !memcmp(mSignature, nsPNGDecoder::pngSignatureBytes,
                      PNGSIGNATURESIZE);
     if (mIsPNG) {
       mContainedDecoder = new nsPNGDecoder(mImage);
       mContainedDecoder->SetSizeDecode(IsSizeDecode());
-      mContainedDecoder->InitSharedDecoder(mImageData, mImageDataLength,
-                                           mColormap, mColormapSize,
-                                           Move(mRefForContainedDecoder));
+      mContainedDecoder->Init();
       if (!WriteToContainedDecoder(mSignature, PNGSIGNATURESIZE)) {
         return;
       }
     }
   }
 
   // If we have a PNG, let the PNG decoder do all of the rest of the work
   if (mIsPNG && mContainedDecoder && mPos >= mImageOffset + PNGSIGNATURESIZE) {
@@ -415,19 +416,17 @@ nsICODecoder::WriteInternal(const char* 
 
     // Init the bitmap decoder which will do most of the work for us
     // It will do everything except the AND mask which isn't present in bitmaps
     // bmpDecoder is for local scope ease, it will be freed by mContainedDecoder
     nsBMPDecoder* bmpDecoder = new nsBMPDecoder(mImage);
     mContainedDecoder = bmpDecoder;
     bmpDecoder->SetUseAlphaData(true);
     mContainedDecoder->SetSizeDecode(IsSizeDecode());
-    mContainedDecoder->InitSharedDecoder(mImageData, mImageDataLength,
-                                         mColormap, mColormapSize,
-                                         Move(mRefForContainedDecoder));
+    mContainedDecoder->Init();
 
     // 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
     // generate this header ourselves and feed it to the BMP decoder.
     int8_t bfhBuffer[BMPFILEHEADERSIZE];
     if (!FillBitmapFileHeaderBuffer(bfhBuffer)) {
       PostDataError();
       return;
@@ -620,40 +619,10 @@ nsICODecoder::ProcessDirEntry(IconDirEnt
   aTarget.mBitCount = LittleEndian::readUint16(&aTarget.mBitCount);
   memcpy(&aTarget.mBytesInRes, mDirEntryArray + 8, sizeof(aTarget.mBytesInRes));
   aTarget.mBytesInRes = LittleEndian::readUint32(&aTarget.mBytesInRes);
   memcpy(&aTarget.mImageOffset, mDirEntryArray + 12,
          sizeof(aTarget.mImageOffset));
   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
--- a/image/decoders/nsICODecoder.h
+++ b/image/decoders/nsICODecoder.h
@@ -35,20 +35,16 @@ public:
   // Obtains the height of the icon directory entry
   uint32_t GetRealHeight() const
   {
     return mDirEntry.mHeight == 0 ? 256 : mDirEntry.mHeight;
   }
 
   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
   // Returns true if there are no errors.
   bool WriteToContainedDecoder(const char* aBuffer, uint32_t aCount);
 
   // Processes a single dir entry of the icon resource
   void ProcessDirEntry(IconDirEntry& aTarget);
@@ -78,17 +74,16 @@ private:
   uint16_t mNumIcons; // Stores the number of icons in the ICO file
   uint16_t mCurrIcon; // Stores the current dir entry index we are processing
   uint32_t mImageOffset; // Stores the offset of the image data we want
   uint8_t* mRow;      // Holds one raw line of the image
   int32_t mCurLine;   // Line index of the image that's currently being decoded
   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
   // Holds the potential bytes that can be a PNG signature
   char mSignature[PNGSIGNATURESIZE];
   // Holds the potential bytes for a bitmap information header
   char mBIHraw[40];
   // Stores whether or not the icon file we are processing has type 1 (icon)
--- a/image/decoders/nsIconDecoder.cpp
+++ b/image/decoders/nsIconDecoder.cpp
@@ -68,21 +68,28 @@ nsIconDecoder::WriteInternal(const char*
         }
 
         // If We're doing a size decode, we're done
         if (IsSizeDecode()) {
           mState = iconStateFinished;
           break;
         }
 
-        if (!mImageData) {
-          PostDecoderError(NS_ERROR_OUT_OF_MEMORY);
-          return;
+        {
+          MOZ_ASSERT(!mImageData, "Already have a buffer allocated?");
+          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--;
         mState = iconStateReadPixels;
         break;
 
       case iconStateReadPixels: {
 
--- a/image/decoders/nsJPEGDecoder.cpp
+++ b/image/decoders/nsJPEGDecoder.cpp
@@ -381,24 +381,28 @@ nsJPEGDecoder::WriteInternal(const char*
       }
     }
 
     // Don't allocate a giant and superfluous memory buffer
     // when not doing a progressive decode.
     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",
             mInfo.output_width, mInfo.output_height));
 
     mState = JPEG_START_DECOMPRESS;
   }
 
--- a/image/decoders/nsPNGDecoder.cpp
+++ b/image/decoders/nsPNGDecoder.cpp
@@ -136,62 +136,62 @@ nsPNGDecoder::~nsPNGDecoder()
     // mTransform belongs to us only if mInProfile is non-null
     if (mTransform) {
       qcms_transform_release(mTransform);
     }
   }
 }
 
 // CreateFrame() is used for both simple and animated images
-void nsPNGDecoder::CreateFrame(png_uint_32 x_offset, png_uint_32 y_offset,
-                               int32_t width, int32_t height,
-                               gfx::SurfaceFormat format)
+nsresult
+nsPNGDecoder::CreateFrame(png_uint_32 aXOffset, png_uint_32 aYOffset,
+                          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
-  // infrastructure. Just use it as long as it matches up.
-  nsIntRect neededRect(x_offset, y_offset, width, height);
-  nsRefPtr<imgFrame> currentFrame = GetCurrentFrame();
-  if (!currentFrame->GetRect().IsEqualEdges(neededRect)) {
-    if (mNumFrames == 0) {
-      // 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);
+  nsIntRect frameRect(aXOffset, aYOffset, aWidth, aHeight);
+  if (mNumFrames == 0 &&
+      !nsIntRect(nsIntPoint(), GetSize()).IsEqualEdges(frameRect)) {
+    // 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();
   }
 
-  mFrameRect = neededRect;
+  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
   if (png_get_valid(mPNG, mInfo, PNG_INFO_acTL)) {
     mAnimInfo = AnimFrameInfo(mPNG, mInfo);
 
     if (mAnimInfo.mDispose == DisposalMethod::CLEAR) {
       // We may have to display the background under this image during
       // animation playback, so we regard it as transparent.
       PostHasTransparency();
     }
   }
 #endif
+
+  return NS_OK;
 }
 
 // set timeout and frame disposal method for the current frame
 void
 nsPNGDecoder::EndImageFrame()
 {
   if (mFrameIsHidden) {
     return;
@@ -644,17 +644,21 @@ nsPNGDecoder::info_callback(png_structp 
     png_set_progressive_frame_fn(png_ptr, nsPNGDecoder::frame_info_callback,
                                  nullptr);
   }
 
   if (png_get_first_frame_is_hidden(png_ptr, 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
 
   if (decoder->mTransform &&
       (channels <= 2 || interlace_type == PNG_INTERLACE_ADAM7)) {
     uint32_t bpp[] = { 0, 3, 4, 3, 4 };
     decoder->mCMSLine =
@@ -667,22 +671,16 @@ nsPNGDecoder::info_callback(png_structp 
   if (interlace_type == PNG_INTERLACE_ADAM7) {
     if (height < INT32_MAX / (width * channels)) {
       decoder->interlacebuf = (uint8_t*)moz_malloc(channels * width * height);
     }
     if (!decoder->interlacebuf) {
       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
 nsPNGDecoder::row_callback(png_structp png_ptr, png_bytep new_row,
                            png_uint_32 row_num, int pass)
 {
   /* libpng comments:
    *
@@ -835,23 +833,22 @@ nsPNGDecoder::frame_info_callback(png_st
   // Only the first frame can be hidden, so unhide unconditionally here.
   decoder->mFrameIsHidden = false;
 
   x_offset = png_get_next_frame_x_offset(png_ptr, decoder->mInfo);
   y_offset = png_get_next_frame_y_offset(png_ptr, decoder->mInfo);
   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);
-
-  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);
+  nsresult rv =
+    decoder->CreateFrame(x_offset, y_offset, 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");
 }
 #endif
 
 void
 nsPNGDecoder::end_callback(png_structp png_ptr, png_infop info_ptr)
 {
   /* libpng comments:
    *
--- a/image/decoders/nsPNGDecoder.h
+++ b/image/decoders/nsPNGDecoder.h
@@ -26,19 +26,19 @@ class nsPNGDecoder : public Decoder
 public:
   explicit nsPNGDecoder(RasterImage& aImage);
   virtual ~nsPNGDecoder();
 
   virtual void InitInternal() MOZ_OVERRIDE;
   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,
-                   int32_t width, int32_t height,
-                   gfx::SurfaceFormat format);
+  nsresult CreateFrame(png_uint_32 aXOffset, png_uint_32 aYOffset,
+                       int32_t aWidth, int32_t aHeight,
+                       gfx::SurfaceFormat aFormat);
   void EndImageFrame();
 
   // Check if PNG is valid ICO (32bpp RGBA)
   // http://blogs.msdn.com/b/oldnewthing/archive/2010/10/22/10079192.aspx
   bool IsValidICO() const
   {
     // If there are errors in the call to png_get_IHDR, the error_callback in
     // nsPNGDecoder.cpp is called.  In this error callback we do a longjmp, so
--- a/image/src/Decoder.cpp
+++ b/image/src/Decoder.cpp
@@ -27,18 +27,16 @@ Decoder::Decoder(RasterImage &aImage)
   , mColormap(nullptr)
   , mChunkCount(0)
   , mDecodeFlags(0)
   , mBytesDecoded(0)
   , mDecodeDone(false)
   , mDataError(false)
   , mFrameCount(0)
   , mFailCode(NS_OK)
-  , mNeedsNewFrame(false)
-  , mNeedsToFlushData(false)
   , mInitialized(false)
   , mSizeDecode(false)
   , mInFrame(false)
   , mIsAnimated(false)
 { }
 
 Decoder::~Decoder()
 {
@@ -65,97 +63,48 @@ Decoder::Init()
   }
 
   // Implementation-specific initialization
   InitInternal();
 
   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!");
 
   // Begin recording telemetry data.
   TimeStamp start = TimeStamp::Now();
   mChunkCount++;
 
   // 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;
 
   if (IsSizeDecode() && HasSize()) {
     // More data came in since we found the size. We have nothing to do here.
     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);
 }
 
 void
 Decoder::Finish(ShutdownReason aReason)
 {
   MOZ_ASSERT(NS_IsMainThread());
@@ -220,125 +169,55 @@ Decoder::Finish(ShutdownReason aReason)
   mImageMetadata.SetOnImage(&mImage);
 
   if (mDecodeDone) {
     MOZ_ASSERT(HasError() || mCurrentFrame, "Should have an error or a frame");
     mImage.DecodingComplete(mCurrentFrame.get());
   }
 }
 
-void
-Decoder::FinishSharedDecoder()
+nsresult
+Decoder::AllocateFrame(uint32_t aFrameNum,
+                       const nsIntRect& aFrameRect,
+                       gfx::SurfaceFormat aFormat,
+                       uint8_t aPaletteDepth /* = 0 */)
 {
-  MOZ_ASSERT(NS_IsMainThread());
-
-  if (!HasError()) {
-    FinishInternal();
-  }
-}
-
-nsresult
-Decoder::AllocateFrame()
-{
-  MOZ_ASSERT(mNeedsNewFrame);
-
-  mCurrentFrame = EnsureFrame(mNewFrameData.mFrameNum,
-                              mNewFrameData.mFrameRect,
-                              mDecodeFlags,
-                              mNewFrameData.mFormat,
-                              mNewFrameData.mPaletteDepth,
-                              mCurrentFrame.get());
+  mCurrentFrame = AllocateFrameInternal(aFrameNum, aFrameRect, mDecodeFlags,
+                                        aFormat, aPaletteDepth,
+                                        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,
-                     const nsIntRect& aFrameRect,
-                     uint32_t aDecodeFlags,
-                     SurfaceFormat aFormat,
-                     uint8_t aPaletteDepth,
-                     imgFrame* aPreviousFrame)
+Decoder::AllocateFrameInternal(uint32_t aFrameNum,
+                               const nsIntRect& aFrameRect,
+                               uint32_t aDecodeFlags,
+                               SurfaceFormat aFormat,
+                               uint8_t aPaletteDepth,
+                               imgFrame* aPreviousFrame)
 {
   if (mDataError || NS_FAILED(mFailCode)) {
     return RawAccessFrameRef();
   }
 
-  MOZ_ASSERT(aFrameNum <= mFrameCount, "Invalid frame index!");
-  if (aFrameNum > mFrameCount) {
-    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) {
+  if (aFrameNum != mFrameCount) {
+    MOZ_ASSERT_UNREACHABLE("Allocating frames out of order");
     return RawAccessFrameRef();
   }
 
   MOZ_ASSERT(mImageMetadata.HasSize());
   nsIntSize imageSize(mImageMetadata.GetWidth(), mImageMetadata.GetHeight());
   if (imageSize.width <= 0 || imageSize.height <= 0 ||
       aFrameRect.width <= 0 || aFrameRect.height <= 0) {
     NS_WARNING("Trying to add frame with zero or negative size");
@@ -523,28 +402,10 @@ Decoder::PostDecoderError(nsresult aFail
 
   mFailCode = aFailureCode;
 
   // XXXbholley - we should report the image URI here, but imgContainer
   // needs to know its URI first
   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
--- a/image/src/Decoder.h
+++ b/image/src/Decoder.h
@@ -27,33 +27,18 @@ public:
   /**
    * Initialize an image decoder. Decoders may not be re-initialized.
    *
    * Notifications Sent: TODO
    */
   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
    *
    * Any errors are reported by setting the appropriate state on the decoder.
    *
    * Notifications Sent: TODO
    */
   void Write(const char* aBuffer, uint32_t aCount);
@@ -61,24 +46,16 @@ public:
   /**
    * Informs the decoder that all the data has been written.
    *
    * Notifications Sent: TODO
    */
   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
    * TakeInvalidRect() returns only the invalidation region accumulated since
    * the last call to TakeInvalidRect().
    */
   nsIntRect TakeInvalidRect()
   {
     nsIntRect invalidRect = mInvalidRect;
@@ -151,60 +128,40 @@ public:
   enum DecodeStyle {
       PROGRESSIVE, // produce intermediate frames representing the partial state of the image
       SEQUENTIAL // decode to final image immediately
   };
 
   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();
     return frame.forget();
   }
 
   RawAccessFrameRef GetCurrentFrameRef()
   {
     return mCurrentFrame ? mCurrentFrame->RawAccessRef()
                          : RawAccessFrameRef();
   }
 
 protected:
+  friend class nsICODecoder;
+
   virtual ~Decoder();
 
   /*
    * Internal hooks. Decoder implementations may override these and
    * only these methods.
    */
   virtual void InitInternal();
   virtual void WriteInternal(const char* aBuffer, uint32_t aCount);
@@ -259,42 +216,36 @@ protected:
   // For animated images, specify the loop count. -1 means loop forever, 0
   // means a single iteration, stopping on the last frame.
   void PostDecodeDone(int32_t aLoopCount = 0);
 
   // Data errors are the fault of the source data, decoder errors are our fault
   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
-   * returns a RawAccessFrameRef for that frame.
-   * It is not possible to create sparse frame arrays; you can only append
-   * frames to the current frame array, or if there is only one frame in the
-   * array, replace that frame.
+   * Allocates a new frame, making it our new current frame if successful.
+   *
+   * The @aFrameNum parameter only exists as a sanity check; it's illegal to
+   * create a new frame anywhere but immediately after the existing frames.
+   *
    * If a non-paletted frame is desired, pass 0 for aPaletteDepth.
    */
-  RawAccessFrameRef EnsureFrame(uint32_t aFrameNum,
-                                const nsIntRect& aFrameRect,
-                                uint32_t aDecodeFlags,
-                                gfx::SurfaceFormat aFormat,
-                                uint8_t aPaletteDepth,
-                                imgFrame* aPreviousFrame);
+  nsresult AllocateFrame(uint32_t aFrameNum,
+                         const nsIntRect& aFrameRect,
+                         gfx::SurfaceFormat aFormat,
+                         uint8_t aPaletteDepth = 0);
 
-  RawAccessFrameRef InternalAddFrame(uint32_t aFrameNum,
-                                     const nsIntRect& aFrameRect,
-                                     uint32_t aDecodeFlags,
-                                     gfx::SurfaceFormat aFormat,
-                                     uint8_t aPaletteDepth,
-                                     imgFrame* aPreviousFrame);
+  RawAccessFrameRef AllocateFrameInternal(uint32_t aFrameNum,
+                                          const nsIntRect& aFrameRect,
+                                          uint32_t aDecodeFlags,
+                                          gfx::SurfaceFormat aFormat,
+                                          uint8_t aPaletteDepth,
+                                          imgFrame* aPreviousFrame);
+
   /*
    * Member variables.
    *
    */
   RasterImage &mImage;
   RawAccessFrameRef mCurrentFrame;
   ImageMetadata mImageMetadata;
   nsIntRect mInvalidRect; // Tracks an invalidation region in the current frame.
@@ -314,37 +265,16 @@ protected:
   bool mDecodeDone;
   bool mDataError;
 
 private:
   uint32_t mFrameCount; // Number of frames, including anything in-progress
 
   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;
   bool mIsAnimated;
 };
 
 } // namespace image
 } // namespace mozilla
--- a/image/src/ImageMetadata.h
+++ b/image/src/ImageMetadata.h
@@ -48,16 +48,17 @@ public:
     }
   }
 
   bool HasSize() const { return mSize.isSome(); }
   bool HasOrientation() const { return mOrientation.isSome(); }
 
   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:
   // The hotspot found on cursors, or -1 if none was found.
   int32_t mHotspotX;
   int32_t mHotspotY;
 
   // The loop count for animated images, or -1 for infinite loop.
--- a/image/src/RasterImage.cpp
+++ b/image/src/RasterImage.cpp
@@ -928,19 +928,19 @@ RasterImage::OnAddedFrame(uint32_t aNewF
 {
   if (!NS_IsMainThread()) {
     nsCOMPtr<nsIRunnable> runnable =
       new OnAddedFrameRunnable(this, aNewFrameCount, aNewRefreshArea);
     NS_DispatchToMainThread(runnable);
     return;
   }
 
-  MOZ_ASSERT((mFrameCount == 1 && aNewFrameCount == 1) ||
-             mFrameCount < aNewFrameCount,
-             "Frame count running backwards");
+  MOZ_ASSERT(aNewFrameCount <= mFrameCount ||
+             aNewFrameCount == mFrameCount + 1,
+             "Skipped a frame?");
 
   mFrameCount = aNewFrameCount;
 
   if (aNewFrameCount == 2) {
     // We're becoming animated, so initialize animation stuff.
     MOZ_ASSERT(!mAnim, "Already have animation state?");
     mAnim = MakeUnique<FrameAnimator>(this, mSize.ToIntSize(), mAnimationMode);
 
@@ -1475,25 +1475,16 @@ RasterImage::InitDecoder(bool aDoSizeDec
       break;
     default:
       NS_ABORT_IF_FALSE(0, "Shouldn't get here!");
   }
 
   // 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());
 
   if (!aDoSizeDecode) {
     Telemetry::GetHistogramById(Telemetry::IMAGE_DECODE_COUNT)->Subtract(mDecodeCount);
     mDecodeCount++;
     Telemetry::GetHistogramById(Telemetry::IMAGE_DECODE_COUNT)->Add(mDecodeCount);
 
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/meta/2dcontext/drawing-images-to-the-canvas/2d.drawImage.broken.html.ini
@@ -0,0 +1,5 @@
+[2d.drawImage.broken.html]
+  type: testharness
+  [Canvas test: 2d.drawImage.broken]
+    expected: FAIL
+