Scanline decoding for gifs

src/codec/SkCodec_libgif.cpp

Index: src/codec/SkCodec_libgif.cpp
diff --git a/src/codec/SkCodec_libgif.cpp b/src/codec/SkCodec_libgif.cpp
index b57703115ba132cf9fa8b845cbcf564192a5f9ab..9f8f87b6e0b32ae987a988040042a500217a5bd1 100644
--- a/src/codec/SkCodec_libgif.cpp
+++ b/src/codec/SkCodec_libgif.cpp
@@ -10,6 +10,7 @@
 #include "SkColorPriv.h"
 #include "SkColorTable.h"
 #include "SkGifInterlaceIter.h"
+#include "SkScaledCodec.h"
 #include "SkStream.h"
 #include "SkSwizzler.h"
 #include "SkUtils.h"
@@ -62,28 +63,6 @@ static GifFileType* open_gif(SkStream* stream) {
     return DGifOpen(stream, read_bytes_callback, NULL);
 }
 
- /*
- * This function cleans up the gif object after the decode completes
- * It is used in a SkAutoTCallIProc template
- */
-void SkGifCodec::CloseGif(GifFileType* gif) {
-#if GIFLIB_MAJOR < 5 || (GIFLIB_MAJOR == 5 && GIFLIB_MINOR == 0)
-    DGifCloseFile(gif);
-#else
-    DGifCloseFile(gif, NULL);
-#endif
-}
-
-/*
- * This function free extension data that has been saved to assist the image
- * decoder
- */
-void SkGifCodec::FreeExtension(SavedImage* image) {
-    if (NULL != image->ExtensionBlocks) {
-        GifFreeExtensions(&image->ExtensionBlockCount, &image->ExtensionBlocks);
-    }
-}
-
 /*
  * Check if a there is an index of the color table for a transparent pixel
  */
@@ -100,14 +79,11 @@ static uint32_t find_trans_index(const SavedImage& image) {
         // graphics control extension is always four bytes.  The fourth byte
         // is the transparent index (if it exists), so we need at least four
         // bytes.
-        if (GRAPHICS_EXT_FUNC_CODE == extBlock.Function &&
-                extBlock.ByteCount >= 4) {
-
+        if (GRAPHICS_EXT_FUNC_CODE == extBlock.Function && extBlock.ByteCount >= 4) {
             // Check the transparent color flag which indicates whether a
             // transparent index exists.  It is the least significant bit of
             // the first byte of the extension block.
             if (1 == (extBlock.Bytes[0] & 1)) {
-
                 // Use uint32_t to prevent sign extending
                 return extBlock.Bytes[3];
             }
@@ -122,6 +98,28 @@ static uint32_t find_trans_index(const SavedImage& image) {
     return SK_MaxU32;
 }
 
+ /*

[msarett, 2015/08/24 23:20:13]

These have simply been rearranged.

+ * This function cleans up the gif object after the decode completes
+ * It is used in a SkAutoTCallIProc template
+ */
+void SkGifCodec::CloseGif(GifFileType* gif) {
+#if GIFLIB_MAJOR < 5 || (GIFLIB_MAJOR == 5 && GIFLIB_MINOR == 0)
+    DGifCloseFile(gif);
+#else
+    DGifCloseFile(gif, NULL);
+#endif
+}
+
+/*
+ * This function free extension data that has been saved to assist the image
+ * decoder
+ */
+void SkGifCodec::FreeExtension(SavedImage* image) {
+    if (NULL != image->ExtensionBlocks) {
+        GifFreeExtensions(&image->ExtensionBlockCount, &image->ExtensionBlocks);
+    }
+}
+
 /*
  * Read enough of the stream to initialize the SkGifCodec.
  * Returns a bool representing success or failure.
@@ -200,6 +198,12 @@ SkGifCodec::SkGifCodec(const SkImageInfo& srcInfo, SkStream* stream,
                        GifFileType* gif)
     : INHERITED(srcInfo, stream)
     , fGif(gif)
+    , fSrcBuffer(SkNEW_ARRAY(uint8_t, this->getInfo().width()))

[msarett, 2015/08/24 23:20:12]

This is a behavior change.  We will allocate extra memory when the image frame
width is less than the image header width.  This is rare.

[scroggo, 2015/08/27 16:35:49]

Is this used in onGetPixels, or just in scanline decoding mode?

[msarett, 2015/09/01 17:50:15]

This is shared by both.

+    , fFillIndex(SK_MaxU32)
+    , fFrameDims(SkIRect::MakeEmpty())
+    , fFrameIsSubset(false)
+    , fColorTable(NULL)
+    , fSwizzler(NULL)
 {}
 
 bool SkGifCodec::onRewind() {
@@ -213,32 +217,7 @@ bool SkGifCodec::onRewind() {
     return true;
 }
 
-/*
- * Initiates the gif decode
- */
-SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
-                                        void* dst, size_t dstRowBytes,
-                                        const Options& opts,
-                                        SkPMColor* inputColorPtr,
-                                        int* inputColorCount) {
-    // Rewind if necessary
-    if (!this->rewindIfNeeded()) {
-        return kCouldNotRewind;
-    }
-
-    // Check for valid input parameters
-    if (opts.fSubset) {
-        // Subsets are not supported.
-        return kUnimplemented;
-    }
-    if (dstInfo.dimensions() != this->getInfo().dimensions()) {
-        return gif_error("Scaling not supported.\n", kInvalidScale);
-    }
-    if (!conversion_possible(dstInfo, this->getInfo())) {
-        return gif_error("Cannot convert input type to output type.\n",
-                kInvalidConversion);
-    }
-
+ SkCodec::Result SkGifCodec::readUpToFirstImage(uint32_t* transIndex) {

[msarett, 2015/08/24 23:20:13]

Code moved from original onGetPixels().  We now call find_trans_index() sooner,
and this is safe.

     // Use this as a container to hold information about any gif extension
     // blocks.  This generally stores transparency and animation instructions.
     SavedImage saveExt;
@@ -251,209 +230,15 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
     // We will loop over components of gif images until we find an image.  Once
     // we find an image, we will decode and return it.  While many gif files
     // contain more than one image, we will simply decode the first image.
-    const int32_t width = dstInfo.width();
-    const int32_t height = dstInfo.height();
     GifRecordType recordType;
     do {
         // Get the current record type
         if (GIF_ERROR == DGifGetRecordType(fGif, &recordType)) {
             return gif_error("DGifGetRecordType failed.\n", kInvalidInput);
         }
-
         switch (recordType) {
             case IMAGE_DESC_RECORD_TYPE: {
-                // Read the image descriptor
-                if (GIF_ERROR == DGifGetImageDesc(fGif)) {
-                    return gif_error("DGifGetImageDesc failed.\n",
-                            kInvalidInput);
-                }
-
-                // If reading the image descriptor is successful, the image
-                // count will be incremented
-                SkASSERT(fGif->ImageCount >= 1);
-                SavedImage* image = &fGif->SavedImages[fGif->ImageCount - 1];
-
-                // Process the descriptor
-                const GifImageDesc& desc = image->ImageDesc;
-                int32_t imageLeft = desc.Left;
-                int32_t imageTop = desc.Top;
-                int32_t innerWidth = desc.Width;
-                int32_t innerHeight = desc.Height;
-                // Fail on non-positive dimensions
-                if (innerWidth <= 0 || innerHeight <= 0) {
-                    return gif_error("Invalid dimensions for inner image.\n",
-                            kInvalidInput);
-                }
-                // Treat the following cases as warnings and try to fix
-                if (innerWidth > width) {
-                    gif_warning("Inner image too wide, shrinking.\n");
-                    innerWidth = width;
-                    imageLeft = 0;
-                } else if (imageLeft + innerWidth > width) {
-                    gif_warning("Shifting inner image to left to fit.\n");
-                    imageLeft = width - innerWidth;
-                } else if (imageLeft < 0) {
-                    gif_warning("Shifting image to right to fit\n");
-                    imageLeft = 0;
-                }
-                if (innerHeight > height) {
-                    gif_warning("Inner image too tall, shrinking.\n");
-                    innerHeight = height;
-                    imageTop = 0;
-                } else if (imageTop + innerHeight > height) {
-                    gif_warning("Shifting inner image up to fit.\n");
-                    imageTop = height - innerHeight;
-                } else if (imageTop < 0) {
-                    gif_warning("Shifting image down to fit\n");
-                    imageTop = 0;
-                }
-
-                // Create a color table to store colors the giflib colorMap
-                SkPMColor alternateColorPtr[256];
-                SkPMColor* colorTable;
-                SkColorType dstColorType = dstInfo.colorType();
-                if (kIndex_8_SkColorType == dstColorType) {
-                    SkASSERT(NULL != inputColorPtr);
-                    SkASSERT(NULL != inputColorCount);
-                    colorTable = inputColorPtr;
-                } else {
-                    colorTable = alternateColorPtr;
-                }
-
-                // Set up the color table
-                uint32_t colorCount = 0;
-                // Allocate maximum storage to deal with invalid indices safely
-                const uint32_t maxColors = 256;
-                ColorMapObject* colorMap = fGif->Image.ColorMap;
-                // If there is no local color table, use the global color table
-                if (NULL == colorMap) {
-                    colorMap = fGif->SColorMap;
-                }
-                if (NULL != colorMap) {
-                    colorCount = colorMap->ColorCount;
-                    SkASSERT(colorCount ==
-                            (unsigned) (1 << (colorMap->BitsPerPixel)));
-                    SkASSERT(colorCount <= 256);
-                    for (uint32_t i = 0; i < colorCount; i++) {
-                        colorTable[i] = SkPackARGB32(0xFF,
-                                                     colorMap->Colors[i].Red,
-                                                     colorMap->Colors[i].Green,
-                                                     colorMap->Colors[i].Blue);
-                    }
-                }
-
-                // This is used to fill unspecified pixels in the image data.
-                uint32_t fillIndex = fGif->SBackGroundColor;
-                ZeroInitialized zeroInit = opts.fZeroInitialized;
-
-                // Gifs have the option to specify the color at a single
-                // index of the color table as transparent.
-                {
-                    // Get the transparent index.  If the return value of this
-                    // function is greater than the colorCount, we know that
-                    // there is no valid transparent color in the color table.
-                    // This occurs if there is no graphics control extension or
-                    // if the index specified by the graphics control extension
-                    // is out of range.
-                    uint32_t transIndex = find_trans_index(saveExt);
-
-                    if (transIndex < colorCount) {
-                        colorTable[transIndex] = SK_ColorTRANSPARENT;
-                        // If there is a transparent index, we also use this as
-                        // the fill index.
-                        fillIndex = transIndex;
-                    } else if (fillIndex >= colorCount) {
-                        // If the fill index is invalid, we default to 0.  This
-                        // behavior is unspecified but matches SkImageDecoder.
-                        fillIndex = 0;
-                    }
-                }
-
-                // Fill in the color table for indices greater than color count.
-                // This allows for predictable, safe behavior.
-                for (uint32_t i = colorCount; i < maxColors; i++) {
-                    colorTable[i] = colorTable[fillIndex];
-                } 
-
-                // Check if image is only a subset of the image frame
-                SkAutoTDelete<SkSwizzler> swizzler(NULL);
-                if (innerWidth < width || innerHeight < height) {
-
-                    // Modify the destination info
-                    const SkImageInfo subsetDstInfo =
-                            dstInfo.makeWH(innerWidth, innerHeight);
-
-                    // Fill the destination with the fill color
-                    // FIXME: This may not be the behavior that we want for
-                    //        animated gifs where we draw on top of the
-                    //        previous frame.
-                    SkSwizzler::Fill(dst, dstInfo, dstRowBytes, height,
-                            fillIndex, colorTable, zeroInit);
-
-                    // Modify the dst pointer
-                    const int32_t dstBytesPerPixel =
-                            SkColorTypeBytesPerPixel(dstColorType);
-                    dst = SkTAddOffset<void*>(dst,
-                            dstRowBytes * imageTop +
-                            dstBytesPerPixel * imageLeft);
-
-                    // Create the subset swizzler
-                    swizzler.reset(SkSwizzler::CreateSwizzler(
-                            SkSwizzler::kIndex, colorTable, subsetDstInfo,
-                            zeroInit, this->getInfo()));
-                } else {
-                    // Create the fully dimensional swizzler
-                    swizzler.reset(SkSwizzler::CreateSwizzler(
-                            SkSwizzler::kIndex, colorTable, dstInfo, 
-                            zeroInit, this->getInfo()));
-                }
-
-                // Stores output from dgiflib and input to the swizzler
-                SkAutoTDeleteArray<uint8_t>
-                        buffer(SkNEW_ARRAY(uint8_t, innerWidth));
-
-                // Check the interlace flag and iterate over rows of the input
-                if (fGif->Image.Interlace) {
-                    // In interlace mode, the rows of input are rearranged in
-                    // the output image.  We use an iterator to take care of
-                    // the rearranging.
-                    SkGifInterlaceIter iter(innerHeight);
-                    for (int32_t y = 0; y < innerHeight; y++) {
-                        if (GIF_ERROR == DGifGetLine(fGif, buffer.get(),
-                                innerWidth)) {
-                            // Recover from error by filling remainder of image
-                            memset(buffer.get(), fillIndex, innerWidth);
-                            for (; y < innerHeight; y++) {
-                                void* dstRow = SkTAddOffset<void>(dst,
-                                        dstRowBytes * iter.nextY());
-                                swizzler->swizzle(dstRow, buffer.get());
-                            }
-                            return gif_error(SkStringPrintf(
-                                    "Could not decode line %d of %d.\n",
-                                    y, height - 1).c_str(), kIncompleteInput);
-                        }
-                        void* dstRow = SkTAddOffset<void>(
-                                dst, dstRowBytes * iter.nextY());
-                        swizzler->swizzle(dstRow, buffer.get());
-                    }
-                } else {
-                    // Standard mode
-                    void* dstRow = dst;
-                    for (int32_t y = 0; y < innerHeight; y++) {
-                        if (GIF_ERROR == DGifGetLine(fGif, buffer.get(),
-                                innerWidth)) {
-                            SkSwizzler::Fill(dstRow, dstInfo, dstRowBytes,
-                                    innerHeight - y, fillIndex, colorTable,
-                                    zeroInit);
-                            return gif_error(SkStringPrintf(
-                                    "Could not decode line %d of %d.\n",
-                                    y, height - 1).c_str(), kIncompleteInput);
-                        }
-                        swizzler->swizzle(dstRow, buffer.get());
-                        dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
-                    }
-                }
-
+                *transIndex = find_trans_index(saveExt);
                 // FIXME: Gif files may have multiple images stored in a single
                 //        file.  This is most commonly used to enable
                 //        animations.  Since we are leaving animated gifs as a
@@ -477,7 +262,6 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
                 //        test case that expects this behavior.
                 return kSuccess;
             }
-
             // Extensions are used to specify special properties of the image
             // such as transparency or animation.
             case EXTENSION_RECORD_TYPE:
@@ -500,8 +284,7 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
                     }
                     // Move to the next block
                     if (GIF_ERROR == DGifGetExtensionNext(fGif, &extData)) {
-                        return gif_error("Could not get next extension.\n",
-                                kIncompleteInput);
+                        return gif_error("Could not get next extension.\n", kIncompleteInput);
                     }
                 }
                 break;
@@ -512,12 +295,404 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
 
             default:
                 // giflib returns an error code if the record type is not known.
-                // We should catch this error immediately.
+                // This should not be reached.

[scroggo, 2015/08/27 16:35:49]

So DGifGetRecordType would have returned GIF_ERROR before we reach this switch
statement?

[msarett, 2015/09/01 17:50:15]

Yes I'll improve the comment.

                 SkASSERT(false);
                 break;
         }
     } while (TERMINATE_RECORD_TYPE != recordType);
 
-    return gif_error("Could not find any images to decode in gif file.\n",
-            kInvalidInput);
+    return gif_error("Could not find any images to decode in gif file.\n", kInvalidInput);
+}
+
+/*
+ * A gif may contain many image frames, all of different sizes.
+ * This function checks if the frame dimensions are valid and corrects them if
+ * necessary.
+ */
+bool SkGifCodec::setFrameDimensions(const GifImageDesc& desc) {

[msarett, 2015/08/24 23:20:13]

Code moved from the original onGetPixels().

fFrameDims and fFrameIsSubset are new.

+    // Fail on non-positive dimensions
+    int32_t frameLeft = desc.Left;
+    int32_t frameTop = desc.Top;
+    int32_t frameWidth = desc.Width;
+    int32_t frameHeight = desc.Height;
+    int32_t height = this->getInfo().height();
+    int32_t width = this->getInfo().width();
+    if (frameWidth <= 0 || frameHeight <= 0) {
+        return false;
+    }
+
+    // Treat the following cases as warnings and try to fix
+    if (frameWidth > width) {
+        gif_warning("Image frame too wide, shrinking.\n");
+        frameWidth = width;
+        frameLeft = 0;
+    } else if (frameLeft + frameWidth > width) {
+        gif_warning("Shifting image frame to left to fit.\n");
+        frameLeft = width - frameWidth;
+    } else if (frameLeft < 0) {
+        gif_warning("Shifting image frame to right to fit\n");
+        frameLeft = 0;
+    }
+    if (frameHeight > height) {
+        gif_warning("Image frame too tall, shrinking.\n");
+        frameHeight = height;
+        frameTop = 0;
+    } else if (frameTop + frameHeight > height) {
+        gif_warning("Shifting image frame up to fit.\n");
+        frameTop = height - frameHeight;
+    } else if (frameTop < 0) {
+        gif_warning("Shifting image frame down to fit\n");
+        frameTop = 0;
+    }
+    fFrameDims.setXYWH(frameLeft, frameTop, frameWidth, frameHeight);
+
+    // Indicate if the frame dimensions do not match the header dimensions
+    if (this->getInfo().width() != fFrameDims.width() ||
+            this->getInfo().height() != fFrameDims.height()) {
+        fFrameIsSubset = true;
+    }
+
+    return true;
+}
+
+void SkGifCodec::initializeColorTable(int* inputColorCount, uint32_t transIndex) {

[msarett, 2015/08/24 23:20:13]

Code moved from the original onGetPixels().

+    // Set up our own color table
+    const uint32_t maxColors = 256;
+    SkPMColor colorPtr[256];
+    if (NULL != inputColorCount) {
+        // We set the number of colors to maxColors in order to ensure
+        // safe memory accesses.  Otherwise, an invalid pixel could
+        // access memory outside of our color table array.
+        *inputColorCount = maxColors;
+    }
+
+    // Get local color table
+    ColorMapObject* colorMap = fGif->Image.ColorMap;
+    // If there is no local color table, use the global color table
+    if (NULL == colorMap) {
+        colorMap = fGif->SColorMap;
+    }
+
+    uint32_t colorCount = 0;
+    if (NULL != colorMap) {
+        colorCount = colorMap->ColorCount;
+        SkASSERT(colorCount == (unsigned) (1 << (colorMap->BitsPerPixel)));
+        SkASSERT(colorCount <= 256);

[msarett, 2015/08/24 23:20:13]

Not sure if we should add a Release check here.  giflib is guaranteed to give us
a number <= 256 (I've looked at their code), but maybe we don't want to depend
on that?

[scroggo, 2015/08/27 16:35:49]

I think we should trust giflib in this case. I cannot imagine why that would
change.

I think we have a Release assert, if you'd like to add that: SK_ALWAYSBREAK (it
makes you hit a breakpoint in debug mode).

+        for (uint32_t i = 0; i < colorCount; i++) {
+            colorPtr[i] = SkPackARGB32(0xFF, colorMap->Colors[i].Red,
+                    colorMap->Colors[i].Green, colorMap->Colors[i].Blue);
+        }
+    }
+
+    // This is used to fill unspecified pixels in the image data.
+    uint32_t fillIndex = fGif->SBackGroundColor;
+
+    // Gifs have the option to specify the color at a single index of the color
+    // table as transparent.  If the transparent index is greater than the
+    // colorCount, we know that there is no valid transparent color in the
+    // color table.  This occurs if there is no graphics control extension or
+    // if the index specified by the graphics control extension is out of range.
+    if (transIndex < colorCount) {
+        colorPtr[transIndex] = SK_ColorTRANSPARENT;
+        // If there is a transparent index, we also use this as the fill index.
+        fillIndex = transIndex;
+    } else if (fillIndex >= colorCount) {
+        // If the fill index is invalid, we default to 0.  This behavior is
+        // unspecified but matches SkImageDecoder.
+        fillIndex = 0;
+    }
+
+    // Fill in the color table for indices greater than color count.
+    // This allows for predictable, safe behavior.
+    for (uint32_t i = colorCount; i < maxColors; i++) {
+        colorPtr[i] = colorPtr[fillIndex];
+    }
+
+    fColorTable.reset(SkNEW_ARGS(SkColorTable, (colorPtr, maxColors)));
+    fFillIndex = fillIndex;
+}
+
+SkCodec::Result SkGifCodec::initializeSwizzler(const SkImageInfo& dstInfo,
+        ZeroInitialized zeroInit) {
+    const SkPMColor* colorPtr = get_color_ptr(fColorTable.get());
+    fSwizzler.reset(SkSwizzler::CreateSwizzler(SkSwizzler::kIndex,
+            colorPtr, dstInfo, zeroInit, this->getInfo()));
+    if (NULL != fSwizzler.get()) {
+        return kSuccess;
+    }
+    return kUnimplemented;
+}
+
+SkCodec::Result SkGifCodec::readRow() {
+    if (GIF_ERROR == DGifGetLine(fGif, fSrcBuffer.get(), fFrameDims.width())) {
+        return kIncompleteInput;
+    }
+    return kSuccess;
+}
+
+/*
+ * Initiates the gif decode
+ */
+SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
+                                        void* dst, size_t dstRowBytes,
+                                        const Options& opts,
+                                        SkPMColor* inputColorPtr,
+                                        int* inputColorCount) {
+    // Rewind if necessary
+    if (!this->rewindIfNeeded()) {
+        return kCouldNotRewind;
+    }
+
+    // Check for valid input parameters
+    if (opts.fSubset) {
+        // Subsets are not supported.
+        return kUnimplemented;
+    }
+    if (dstInfo.dimensions() != this->getInfo().dimensions()) {
+        return gif_error("Scaling not supported.\n", kInvalidScale);
+    }
+    if (!conversion_possible(dstInfo, this->getInfo())) {
+        return gif_error("Cannot convert input type to output type.\n",
+                kInvalidConversion);
+    }
+
+    // Read through gif extensions to get to the image data.  Set the
+    // transparent index based on the extension data.
+    uint32_t transIndex;
+    SkCodec::Result result = this->readUpToFirstImage(&transIndex);
+    if (kSuccess != result){
+        return result;
+    }
+
+    // Read the image descriptor
+    if (GIF_ERROR == DGifGetImageDesc(fGif)) {
+        return gif_error("DGifGetImageDesc failed.\n", kInvalidInput);
+    }
+
+    // If reading the image descriptor is successful, the image count will be
+    // incremented
+    SkASSERT(fGif->ImageCount >= 1);

[msarett, 2015/08/24 23:20:13]

Looks like we also rely on giflib behaving properly here.

If we need a Release check in the other case, we probably also need one here.

+    SavedImage* image = &fGif->SavedImages[fGif->ImageCount - 1];
+    const GifImageDesc& desc = image->ImageDesc;
+
+    // Check that the frame dimensions are valid and set them
+    if(!this->setFrameDimensions(desc)) {
+        return gif_error("Invalid dimensions for image frame.\n", kInvalidInput);
+    }
+
+    // Initialize color table and copy to the client if necessary
+    this->initializeColorTable(inputColorCount, transIndex);
+    copy_color_table(dstInfo, this->fColorTable, inputColorPtr, inputColorCount);
+
+    // Initialize the swizzler
+    if (fFrameIsSubset) {
+        const SkImageInfo subsetDstInfo = dstInfo.makeWH(fFrameDims.width(), fFrameDims.height());
+        if (kSuccess != this->initializeSwizzler(subsetDstInfo, opts.fZeroInitialized)) {
+            return gif_error("Could not initialize swizzler.\n", kUnimplemented);
+        }
+
+        // Fill the background
+        const SkPMColor* colorPtr = get_color_ptr(fColorTable.get());
+        SkSwizzler::Fill(dst, dstInfo, dstRowBytes, this->getInfo().height(),
+                fFillIndex, colorPtr, opts.fZeroInitialized);
+
+        // Modify the dst pointer
+        const int32_t dstBytesPerPixel = SkColorTypeBytesPerPixel(dstInfo.colorType());
+        dst = SkTAddOffset<void*>(dst, dstRowBytes * fFrameDims.top() +
+                dstBytesPerPixel * fFrameDims.left());
+    } else {
+        if (kSuccess != this->initializeSwizzler(dstInfo, opts.fZeroInitialized)) {
+            return gif_error("Could not initialize swizzler.\n", kUnimplemented);
+        }
+    }
+
+    // Check the interlace flag and iterate over rows of the input
+    if (fGif->Image.Interlace) {
+        // In interlace mode, the rows of input are rearranged in
+        // the output image.  We use an iterator to take care of
+        // the rearranging.
+        SkGifInterlaceIter iter(fFrameDims.height());
+        for (int32_t y = 0; y < fFrameDims.height(); y++) {
+            if (kSuccess != this->readRow()) {
+                // Recover from error by filling remainder of image
+                memset(fSrcBuffer.get(), fFillIndex, fFrameDims.width());
+                for (; y < fFrameDims.height(); y++) {
+                    void* dstRow = SkTAddOffset<void>(dst, dstRowBytes * iter.nextY());
+                    fSwizzler->swizzle(dstRow, fSrcBuffer.get());
+                }
+                return gif_error("Could not decode line.\n", kIncompleteInput);
+            }
+            void* dstRow = SkTAddOffset<void>(dst, dstRowBytes * iter.nextY());
+            fSwizzler->swizzle(dstRow, fSrcBuffer.get());
+        }
+    } else {
+        // Standard mode
+        void* dstRow = dst;
+        for (int32_t y = 0; y < fFrameDims.height(); y++) {
+            if (kSuccess != this->readRow()) {
+                const SkPMColor* colorPtr = get_color_ptr(fColorTable.get());
+                SkSwizzler::Fill(dstRow, dstInfo, dstRowBytes,
+                        fFrameDims.height() - y, fFillIndex, colorPtr,
+                        opts.fZeroInitialized);
+                return gif_error("Could not decode line\n", kIncompleteInput);
+            }
+            fSwizzler->swizzle(dstRow, fSrcBuffer.get());
+            dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
+        }
+    }
+    return kSuccess;
+}
+
+class SkGifScanlineDecoder : public SkScanlineDecoder {
+public:
+    SkGifScanlineDecoder(const SkImageInfo& srcInfo, SkGifCodec* codec)
+        : INHERITED(srcInfo)
+        , fCodec(codec)
+        , fIter(-1)
+    {}
+
+    SkEncodedFormat onGetEncodedFormat() const override {
+        return kGIF_SkEncodedFormat;
+    }
+
+    SkCodec::Result onStart(const SkImageInfo& dstInfo, const SkCodec::Options& opts,
+                            SkPMColor inputColorPtr[], int* inputColorCount) override {
+
+        // Rewind if necessary
+        if (!fCodec->rewindIfNeeded()) {
+            return SkCodec::kCouldNotRewind;
+        }
+        // Check for valid input parameters
+        if (opts.fSubset) {
+            // Subsets are not supported.
+            return SkCodec::kUnimplemented;
+        }
+        // Check to see if scaling was requested.
+        if (dstInfo.dimensions() != this->getInfo().dimensions()) {
+            if (!SkScaledCodec::DimensionsSupportedForSampling(this->getInfo(), dstInfo)) {
+                return gif_error("Scaling not supported.\n", SkCodec::kInvalidScale);
+            }
+        }
+        if (!conversion_possible(dstInfo, this->getInfo())) {
+            return gif_error("Cannot convert input type to output type.\n",
+                    SkCodec::kInvalidConversion);
+        }
+
+        // Read through gif extensions to get to the image data
+        uint32_t transIndex;
+        SkCodec::Result result = fCodec->readUpToFirstImage(&transIndex);
+        if (SkCodec::kSuccess != result){
+            return result;
+        }
+
+        // Read the image descriptor
+        if (GIF_ERROR == DGifGetImageDesc(fCodec->fGif)) {
+            return gif_error("DGifGetImageDesc failed.\n", SkCodec::kInvalidInput);
+        }
+
+        // If reading the image descriptor is successful, the image count will be
+        // incremented
+        SkASSERT(fCodec->fGif->ImageCount >= 1);
+        SavedImage* image = &fCodec->fGif->SavedImages[fCodec->fGif->ImageCount - 1];
+        const GifImageDesc& desc = image->ImageDesc;
+
+        // Check that the frame dimensions are valid and set them
+        if(!fCodec->setFrameDimensions(desc)) {
+            return gif_error("Invalid dimensions for image frame.\n", SkCodec::kInvalidInput);
+        }
+        SkGifInterlaceIter iter(fCodec->fFrameDims.height());
+        fIter = iter;

[msarett, 2015/08/24 23:20:13]

This is unfortunate.  We don't know fCodec->fFrameDims.height() in the
constructor so we can't initialize fIter until we get here.

And (unless my C++ knowledge is letting me down), we need a copy constructor to
perform this assignment.  And if we want to use the default copy constructor,
SkGifInterlaceIter can't have a const field.

[scroggo, 2015/08/27 16:35:49]

What is the problem? It seems like the constructors are fairly cheap (a few
assignments).
We actually need an operator= function.
Ah, so here's the problem - you had to make its fields no longer const and make
it no longer noncopyable. I think that's fine.

Alternatively, you could introduce a new method:

 reset(int height)

You could even make it so the constructor does nothing, and reset() needs to be
called before you use it (we have other classes/structs that behave this way).
Then you would skip the step of setting the variables with a -1 height.

[msarett, 2015/09/01 17:50:15]

This was fixed in the rebase - we now deal with interlaced gifs without an
iterator.

+
+        // Initialize color table and copy to the client if necessary
+        fCodec->initializeColorTable(inputColorCount, transIndex);
+        copy_color_table(dstInfo, fCodec->fColorTable, inputColorPtr, inputColorCount);
+
+        // Initialize the swizzler
+        if (fCodec->fFrameIsSubset) {
+            int sampleX;
+            SkScaledCodec::ComputeSampleSize(dstInfo, fCodec->getInfo(), &sampleX, NULL);
+            const SkImageInfo subsetDstInfo = dstInfo.makeWH(
+                    SkScaledCodec::GetScaledDimension(fCodec->fFrameDims.width(), sampleX),
+                    fCodec->fFrameDims.height());
+            if (SkCodec::kSuccess != fCodec->initializeSwizzler(subsetDstInfo, opts.fZeroInitialized)) {

[scroggo, 2015/08/27 16:35:49]

nit: line too long

[msarett, 2015/09/01 17:50:15]

Done.

+                return gif_error("Could not initialize swizzler.\n", SkCodec::kUnimplemented);
+            }
+        } else {
+            if (SkCodec::kSuccess != fCodec->initializeSwizzler(dstInfo, opts.fZeroInitialized)) {
+                return gif_error("Could not initialize swizzler.\n", SkCodec::kUnimplemented);
+            }
+        }
+
+        return SkCodec::kSuccess;
+    }
+
+    SkCodec::Result onGetScanlines(void* dst, int count, size_t rowBytes) override {
+        if (fCodec->fFrameIsSubset) {
+            // Fill the requested rows
+            const SkPMColor* colorPtr = get_color_ptr(fCodec->fColorTable.get());
+            SkSwizzler::Fill(dst, this->dstInfo(), rowBytes, count, fCodec->fFillIndex, colorPtr, this->options().fZeroInitialized);

[scroggo, 2015/08/27 16:35:49]

nit: line too long

[msarett, 2015/09/01 17:50:15]

Done.

+
+            // Do nothing for rows before the image frame
+            int rowsBeforeFrame = fCodec->fFrameDims.top() - INHERITED::getY();
+            if (rowsBeforeFrame > 0) {
+                count = SkTMin(0, count - rowsBeforeFrame);
+                dst = SkTAddOffset<void>(dst, rowBytes * rowsBeforeFrame);
+            }
+
+            // Do nothing for rows after the image frame
+            int rowsAfterFrame = INHERITED::getY() + count - fCodec->fFrameDims.bottom();
+            if (rowsAfterFrame > 0) {
+                count = SkTMin(0, count - rowsAfterFrame);
+            }
+
+            // Adjust dst pointer for left offset
+            dst = SkTAddOffset<void>(dst, SkColorTypeBytesPerPixel(this->dstInfo().colorType()) * fCodec->fFrameDims.left());

[scroggo, 2015/08/27 16:35:49]

nit line too long

[msarett, 2015/09/01 17:50:15]

Done.

+        }
+
+        for (int i = 0; i < count; i++) {
+            if (SkCodec::kSuccess != fCodec->readRow()) {
+                const SkPMColor* colorPtr = get_color_ptr(fCodec->fColorTable.get());
+                SkSwizzler::Fill(dst, this->dstInfo(), rowBytes,
+                        fCodec->fFrameDims.height() - i, fCodec->fFillIndex, colorPtr,
+                        this->options().fZeroInitialized);
+                return gif_error("Could not decode line\n", SkCodec::kIncompleteInput);
+            }
+            fCodec->fSwizzler->swizzle(dst, fCodec->fSrcBuffer.get());
+            dst = SkTAddOffset<void>(dst, rowBytes);
+        }
+        return SkCodec::kSuccess;
+    }
+
+    SkScanlineOrder onGetScanlineOrder() const override {
+        if (fCodec->fGif->Image.Interlace) {
+            return kOutOfOrder_SkScanlineOrder;
+        } else {
+            return kTopDown_SkScanlineOrder;
+        }
+    }
+
+    int onGetY() override {
+        if (fCodec->fGif->Image.Interlace) {
+            return fIter.nextY();

[scroggo, 2015/08/27 16:35:49]

This seems a little weird - it seems like we're depending on the client to call
getY() in order for fIter to advance? Maybe that's a reasonable requirement.

If we want getY() to remain const, we could split up SkGifIterator::nextY() into
two functions: one that returns the next, and another that advances it. (It is
already partially split - we can make prepareY() public, and change nextY() to
no longer call it. (Maybe we'll want different names for them.)

[msarett, 2015/09/01 17:50:15]

Acknowledged.  We may end up using this approach for performance reasons.

For now, we use an alternative to the iterator.

+        } else {
+            return INHERITED::getY();
+        }
+    }
+
+private:
+    SkAutoTDelete<SkGifCodec> fCodec;
+    SkGifInterlaceIter        fIter;
+
+    typedef SkScanlineDecoder INHERITED;
+};
+
+SkScanlineDecoder* SkGifCodec::NewSDFromStream(SkStream* stream) {
+    SkAutoTDelete<SkGifCodec> codec (static_cast<SkGifCodec*>(SkGifCodec::NewFromStream(stream)));
+    if (!codec) {
+        return NULL;
+    }
+
+    const SkImageInfo& srcInfo = codec->getInfo();
+
+    return SkNEW_ARGS(SkGifScanlineDecoder, (srcInfo, codec.detach()));
 }