Creating a new wrapper for gif decoder

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
new file mode 100644
index 0000000000000000000000000000000000000000..56cc5bc2eb16071f3f2c5f9e9863234f21d8fdb7
--- /dev/null
+++ b/src/codec/SkCodec_libgif.cpp
@@ -0,0 +1,471 @@
+/*
+ * Copyright 2015 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "SkCodec_libgif.h"
+#include "SkCodecPriv.h"
+#include "SkColorPriv.h"
+#include "SkColorTable.h"
+#include "SkGifInterlaceIter.h"
+#include "SkStream.h"
+#include "SkSwizzler.h"
+#include "SkUtils.h"
+
+/*
+ * Checks the start of the stream to see if the image is a gif
+ */
+bool SkGifCodec::IsGif(SkStream* stream) {
+    char buf[GIF_STAMP_LEN];
+    if (stream->read(buf, GIF_STAMP_LEN) == GIF_STAMP_LEN) {
+        if (memcmp(GIF_STAMP,   buf, GIF_STAMP_LEN) == 0 ||
+                memcmp(GIF87_STAMP, buf, GIF_STAMP_LEN) == 0 ||
+                memcmp(GIF89_STAMP, buf, GIF_STAMP_LEN) == 0) {
+            return true;
+        }
+    }
+    return false;
+}
+
+/*
+ * Error reporting function
+ * TODO: Add option to turn off printing of error
+ */
+ static void gif_error(const char* msg) {
+    SkDebugf("Gif Error: %s\n", msg);
+ }
+
+ /*
+ * This function cleans up the gif object after the decode completes
+ * It is used in a SkAutoTCallIProc template
+ */
+int32_t SkGifCodec::close_gif(GifFileType* gif) {

[scroggo, 2015/03/25 19:44:48]

It looks like this returns an int in the header?

[msarett, 2015/03/26 19:15:57]

I'll make it uniform.  Is there a difference between using int and int32_t?

[scroggo, 2015/03/26 20:03:52]

int is platform dependent, and may be 2 or 4 bytes (possibly other sizes?).
int32_t is guaranteed 4 bytes.

+#if GIFLIB_MAJOR < 5 || (GIFLIB_MAJOR == 5 && GIFLIB_MINOR == 0)
+    return DGifCloseFile(gif);
+#else
+    return DGifCloseFile(gif, NULL);
+#endif
+}
+
+/*
+ * This function free extension data that has been saved to assist the image
+ * decoder
+ */
+void free_extension(SavedImage* image) {
+    if (NULL != image->ExtensionBlocks) {
+#if GIFLIB_MAJOR < 5
+        FreeExtension(image);
+#else
+        GifFreeExtensions(&image->ExtensionBlockCount, &image->ExtensionBlocks);
+#endif
+    }
+}
+
+/*
+ * Read function that will be passed to gif_lib
+ */
+static int read_bytes_callback(GifFileType* fileType, GifByteType* out,
+        int size) {
+    SkStream* stream = (SkStream*) fileType->UserData;
+    return (int) stream->read(out, size);
+}
+
+/*
+ * Check if a there is an index of the color table for a transparent pixel
+ */
+static int find_trans_index(const SavedImage& image, uint32_t colorCount) {
+    int transIndex = -1;
+    for (int32_t i = 0; i < image.ExtensionBlockCount; i++) {
+        const ExtensionBlock* eb = image.ExtensionBlocks + i;

[scroggo, 2015/03/25 19:44:49]

Is this an array? i.e. Can this be

  const ExtensionBlock* eb = image.ExtensionBlocks[i];

?

Also, I don't find "eb" to be a very informative name. Maybe extBlock? What is
an extension block, after all?

[msarett, 2015/03/26 19:15:56]

Yes this is an array.  Sorry will fix up this helper function.

+        if (GRAPHICS_EXT_FUNC_CODE == eb->Function && 4 == eb->ByteCount) {

[scroggo, 2015/03/25 19:44:48]

Can you add some comments to this function? I'm having trouble following what's
going on.

[msarett, 2015/03/26 19:15:56]

Done.

[scroggo, 2015/03/26 20:03:53]

This is helpful. Thanks!

+            if (eb->Bytes[0] & 1) {
+                transIndex = (uint8_t) eb->Bytes[3];
+                if (transIndex >= (signed) colorCount) {

[scroggo, 2015/03/25 19:44:49]

I'm confused:

transIndex is declared as an int.
When you assign to it, you cast to an unsigned type.
When you compare it to another unsigned type, you cast that one to signed.

Can we stick with a type and avoid (at least some of) this casting?

[msarett, 2015/03/26 19:15:56]

Done.

+                    transIndex = -1;
+                }
+                break;

[scroggo, 2015/03/25 19:44:48]

This might be a matter of preference, but what if we returned transIndex here
(and declared it in this block), and then returned -1 at the end?

[msarett, 2015/03/26 19:15:56]

Done.

+            }
+        }
+    }
+    return transIndex;
+}
+
+/*
+ * Assumes IsGif was called and returned true
+ * Creates a gif decoder
+ * Reads enough of the stream to determine the image format
+ */
+SkCodec* SkGifCodec::NewFromStream(SkStream* stream) {
+    // Read gif header, logical screen descriptor, and global color table
+#if GIFLIB_MAJOR < 5
+    GifFileType* gif = DGifOpen(stream, read_bytes_callback);
+#else
+    GifFileType* gif = DGifOpen(stream, read_bytes_callback, NULL);
+#endif
+    if (NULL == gif) {
+        gif_error("DGifOpen failed.\n");
+        return NULL;
+    }
+
+    // Get fields from header
+    const int width = gif->SWidth;
+    const int height = gif->SHeight;
+
+    // Return the codec
+    // FIXME: Gifs are naturally stored as kIndex.  We should default to this

[scroggo, 2015/03/25 19:44:49]

Go ahead and return kIndex. I do think it's a little weird that we don't support
the color type that we report, but this better matches the data.

[msarett, 2015/03/26 19:15:56]

Done.

[scroggo, 2015/03/26 20:03:52]

So I've realized that in order to return kIndex8, we need to support that as an
output option, in order for clients of SkImageGenerator to work properly. So we
need to implement the swizzler for index 8, which should not be hard.

+    //        color type, but currently we only support kN32.  Many gifs
+    //        specify a color table index for transparent pixels, so we guess
+    //        that they are stored as kUnpremul.  Gifs without this index are

[scroggo, 2015/03/25 19:44:49]

Refresh my memory: the colors besides the transparent color are guaranteed to be
opaque, right?

This comment seems a little misleading - I think you mean we're guessing that
it's not opaque, since it *might* have a transparent color, as opposed to
whether the colors are premultiplied or not.

[msarett, 2015/03/26 19:15:57]

Yes, colors that are not the transparent color are guaranteed to be opaque.  And
yes, the comment is unclear, I meant that we are guessing that it is not opaque.

Thinking through it again, it makes the most sense to mark kPremul, since pixels
will either be completely opaque or all components will be zeroed.

[scroggo, 2015/03/26 20:03:53]

To be fair, ARGB 0 0 0 0 is a valid premul AND unpremul color.

[msarett, 2015/03/26 22:26:36]

Agreed.  I was just thinking that if it could be considered premul or unpremul,
it's best to consider it premul because it saves us work later?

[scroggo, 2015/03/27 13:09:36]

Good thinking.

+    //        opaque, but we do not have a way of knowing this before decoding.
+    const SkImageInfo& imageInfo = SkImageInfo::Make(width, height,
+            kN32_SkColorType, kUnpremul_SkAlphaType);
+    return SkNEW_ARGS(SkGifCodec, (imageInfo, stream, gif));
+}
+
+SkGifCodec::SkGifCodec(const SkImageInfo& srcInfo, SkStream* stream,
+                       GifFileType* gif)
+    : INHERITED(srcInfo, stream)
+    , fGif(gif)
+    , fAutoClose(gif)
+{}
+
+/*
+ * Checks if the conversion between the input image and the requested output
+ * image has been implemented
+ */
+static bool conversion_possible(const SkImageInfo& dst,
+                                const SkImageInfo& src) {
+    // Ensure that the profile type is unchanged
+    if (dst.profileType() != src.profileType()) {
+        return false;
+    }
+
+    // Check for supported color and alpha types
+    switch (dst.colorType()) {
+        case kN32_SkColorType:
+            return src.alphaType() == dst.alphaType() ||
+                    (kPremul_SkAlphaType == dst.alphaType() &&
+                    kUnpremul_SkAlphaType == src.alphaType());
+        default:
+            return false;
+    }
+}
+
+/*
+ * Initiates the gif decode
+ */
+SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
+                                        void* dst, size_t dstRowBytes,
+                                        const Options&, SkPMColor*, int*) {
+    if (!this->rewindIfNeeded()) {
+        return kCouldNotRewind;
+    }
+    if (dstInfo.dimensions() != this->getInfo().dimensions()) {
+        SkDebugf("Error: scaling not supported.\n");

[scroggo, 2015/03/25 19:44:48]

gif_error?

[msarett, 2015/03/26 19:15:57]

Done.

+        return kInvalidScale;
+    }
+    if (!conversion_possible(dstInfo, this->getInfo())) {
+        SkDebugf("Error: cannot convert input type to output type.\n");

[scroggo, 2015/03/25 19:44:48]

gif_error?

[msarett, 2015/03/26 19:15:56]

Done.

+        return kInvalidConversion;
+    }
+
+    // Use this as a container to hold information about any gif extension
+    // blocks.  This generally stores transparency and animation instructions.
+    SavedImage saveExt;

[scroggo, 2015/03/25 19:44:48]

It looks like this is only needed if GIFLIB_MAJOR < 5?

[msarett, 2015/03/26 19:15:56]

It's actually needed for both.

[scroggo, 2015/03/26 20:03:52]

But it looks like you've deleted it?

Oh, you've moved it up to the top. Why? Its comment is still down here though.

[msarett, 2015/03/26 22:26:36]

I was grouping auto calls together.  It's better down here, you're right. 
Especially because I refactored the auto calls again anyway.

[scroggo, 2015/03/27 13:09:36]

In general, I would say to declare variables as late as you can (i.e. right
before they're used).

+    saveExt.ExtensionBlocks = NULL;
+    saveExt.ExtensionBlockCount = 0;
+    GifByteType* extData;
+#if GIFLIB_MAJOR >= 5
+    int extFunction;
+#endif
+
+    // This is the index used to fill unspecified pixels in the image data.
+    uint32_t fillIndex = fGif->SBackGroundColor;

[scroggo, 2015/03/25 19:44:48]

Can you move this variable closer to where it's used?

[msarett, 2015/03/26 19:15:56]

Done.

+
+    // 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.
+    int width = dstInfo.width();

[scroggo, 2015/03/25 19:44:48]

Can these be const?

[msarett, 2015/03/26 19:15:55]

Yes.

+    int height = dstInfo.height();
+    GifRecordType recordType = UNDEFINED_RECORD_TYPE;
+    while (TERMINATE_RECORD_TYPE != recordType) {
+        // Get the current record type
+        if (GIF_ERROR == DGifGetRecordType(fGif, &recordType)) {
+            gif_error("DGifGetRecordType failed.\n");
+            return kInvalidInput;
+        }
+
+        switch (recordType) {
+            case IMAGE_DESC_RECORD_TYPE: {
+                // Read the image descriptor
+                if (GIF_ERROR == DGifGetImageDesc(fGif)) {
+                    gif_error("DGifGetImageDesc failed.\n");
+                    return 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;
+                int imageLeft = desc.Left;
+                int imageTop = desc.Top;
+                int innerWidth = desc.Width;
+                int innerHeight = desc.Height;
+                // Fail on non-positive dimensions
+                if (innerWidth <= 0 || innerHeight <= 0) {
+                    gif_error("Invalid dimensions for inner image.\n");
+                    return kInvalidInput;
+                }
+                // Treat the following cases as warnings and try to fix
+                if (innerWidth > width) {
+                    gif_error("Inner image too wide, shrinking.\n");

[scroggo, 2015/03/25 19:44:48]

Should this be a gif_warning?

[msarett, 2015/03/26 19:15:57]

gif_error, does not return it only prints a message.  I should probably change
the name to make that clear.  I use it for errors and warnings.

[scroggo, 2015/03/26 20:03:52]

We could also have a separate one for errors vs warnings.

+                    // TODO: The original gif decoder expands the overall image
+                    //       dimensions.  Here we shrink the inner image
+                    //       because we can't change the requested dimensions.
+                    //       What are the implications of this decision?

[scroggo, 2015/03/25 19:44:48]

I think this is the right decision. At this point, the caller has already
allocated their memory, so we don't have a way to correct that.

[msarett, 2015/03/26 19:15:56]

Done.

+                    innerWidth = width;
+                    imageLeft = 0;
+                } else if (imageLeft + innerWidth > width) {
+                    gif_error("Shifting inner image to left to fit.\n");

[scroggo, 2015/03/25 19:44:49]

gif_warning?

[msarett, 2015/03/26 19:15:56]

Acknowledged.

+                    imageLeft = width - innerWidth;
+                } else if (imageLeft < 0) {
+                    gif_error("Shifting image to right to fit\n");
+                    imageLeft = 0;
+                }
+                if (innerHeight > height) {
+                    gif_error("Inner image too tall, shrinking.\n");

[scroggo, 2015/03/25 19:44:48]

gif_warning?

[msarett, 2015/03/26 19:15:56]

Acknowledged.

+                    // TODO: The original gif decoder expands the overall image
+                    //       dimensions.  Here we shrink the inner image
+                    //       because we can't change the requested dimensions.
+                    //       What are the implications of this decision?
+                    innerHeight = height;
+                    imageTop = 0;
+                } else if (imageTop + innerHeight > height) {
+                    gif_error("Shifting inner image up to fit.\n");
+                    imageTop = height - innerHeight;
+                } else if (imageTop < 0) {
+                    gif_error("Shifting image down to fit\n");
+                    imageTop = 0;
+                }
+
+                // Set up the color table
+                uint32_t colorCount = 0;
+                // Allocate maximum storage to deal with invalid indices safely
+                const uint32_t maxColors = 256;
+                SkPMColor colorPtr[maxColors];
+                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);
+                    uint32_t i;
+                    for (i = 0; i < colorCount; i++) {
+                        colorPtr[i] = SkPackARGB32(0xFF,
+                                                   colorMap->Colors[i].Red,
+                                                   colorMap->Colors[i].Green,
+                                                   colorMap->Colors[i].Blue);
+                    }
+                }
+
+                // Gifs have the option to specify the color at a single
+                // index of the color table as transparent.
+                int transIndex = find_trans_index(saveExt, colorCount);

[scroggo, 2015/03/25 19:44:49]

Maybe add a scope here, since transIndex is only used here?

[msarett, 2015/03/26 19:15:57]

Done.  What is the benefit of adding this type of scope?

+                if (transIndex >= 0) {
+                    colorPtr[transIndex] = SK_ColorTRANSPARENT;
+                    fillIndex = transIndex;
+                } else if (fillIndex >= colorCount) {
+                    fillIndex = 0;

[scroggo, 2015/03/25 19:44:49]

How did you choose 0? Can you add a comment explaining this decision?

[msarett, 2015/03/26 19:15:56]

I don't really have a great reason.  This only occurs on invalid inputs. 
Basically it is the same way ImageDecoder deals with this case.

+                }
+
+                // 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];
+                } 
+
+                SkAutoTDelete<SkColorTable> colorTable(
+                        SkNEW_ARGS(SkColorTable, (colorPtr, colorCount)));

[scroggo, 2015/03/25 19:44:48]

I don't think you need an SkColorTable here. It would come in handy if you
wanted to keep it outside this function, but since you only use it here (I
think), it seems like you can just use colorPtr directly.

(That said, if we *do* decide to support kIndex8, we will need an SkColorTable,
but we can add this if/when we add that support.)

[msarett, 2015/03/26 19:15:56]

Done.

+
+                // 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.
+                    // FIXME: This code is kN32 specific.  We should also
+                    //        support kIndex because this is the most logical
+                    //        destination color type for gifs.  If we want to
+                    //        support more than these, that may require
+                    //        additional cases.  There is also an option to add
+                    //        this functionality to the swizzler, but I
+                    //        currently think the complexity makes more sense
+                    //        here.
+                    SkColorType dstColorType = dstInfo.colorType();
+                    switch (dstColorType) {
+                        case kN32_SkColorType:
+                            sk_memset32((SkPMColor*) dst,
+                                colorTable->operator[](fillIndex),

[scroggo, 2015/03/25 19:44:48]

It's possible that the color you're filling with is zero. If so, and
Options.fYes_ZeroInitialized, you can skip this.

[msarett, 2015/03/26 19:15:56]

Done.

+                                dstRowBytes * height / sizeof(SkPMColor));
+                            break;
+                        // case kIndex_SkColorType:

[scroggo, 2015/03/25 19:44:48]

I'm fine with supporting kIndex. Android *might* decide they want it after all.

One downside is that we don't have a good way to test - DM checks the
destination color type in order to decide which type to try for, and it will
never be kIndex, since we cannot draw to kIndex.

[msarett, 2015/03/26 19:15:56]

That's interesting, I wasn't aware of that.  My thinking is to put this on hold
until we are finished supporting kN32.  Then hopefully if we want to support
other color types the changes will almost all be in the swizzler.  I mainly want
to draw attention to color type specific code outside of the swizzler.

Does that seem reasonable?

[scroggo, 2015/03/26 20:03:53]

Sure. Go ahead and ignore my other comments about supporting kIndex8 for now.
But change the default to kN32 with a TODO that it should be kIndex8 once we
support that. (Sorry for flip flopping...)

+                        //     memset(dst, fillIndex, dstRowBytes * height);
+                        //     break;
+                        default:
+                            SkASSERT(false);
+                            break;
+                    }
+
+                    // Modify the dst pointer
+                    const int32_t dstBytesPerPixel =
+                            SkColorTypeBytesPerPixel(dstColorType);
+                    void* subsetDst = SkTAddOffset<void*>(dst,
+                            dstRowBytes * imageTop +
+                            dstBytesPerPixel * imageLeft);
+
+                    // Create the subset swizzler
+                    swizzler.reset(SkSwizzler::CreateSwizzler(
+                        SkSwizzler::kIndex, colorTable->readColors(),
+                        subsetDstInfo, subsetDst, dstRowBytes,
+                        SkImageGenerator::kNo_ZeroInitialized));

[scroggo, 2015/03/25 19:44:49]

Why did you use this instead of the value in Options?

[msarett, 2015/03/26 19:15:56]

No idea.  This is clearly wrong, thanks.

+                } else {
+                    // Create the fully dimensional swizzler
+                    swizzler.reset(SkSwizzler::CreateSwizzler(
+                        SkSwizzler::kIndex, colorTable->readColors(), dstInfo,
+                        dst, dstRowBytes,
+                        SkImageGenerator::kNo_ZeroInitialized));
+                }
+
+                // 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)) {
+                            gif_error("Could not decode line.\n");

[scroggo, 2015/03/25 19:44:49]

Maybe specify what line you're on?

[msarett, 2015/03/26 19:15:56]

Good idea.  I know that information is useful because I printed it out when I
was debugging.  Seems to makes sense to make it permanent.

+                            // Recover from error by filling remainder of image
+                            memset(buffer.get(), fillIndex, innerWidth);
+                            for (; y < innerHeight; y++) {

[scroggo, 2015/03/25 19:44:49]

This isn't quite right, is it? You have switched the version of next that is
called.

This image is interlaced, so you could have failed on any arbitrary line. Maybe
you want to call next with iter.nextY()?

Also, in both cases, if the fillIndex is 0, this is unnecessary if
kYes_ZeroInitialized.

[msarett, 2015/03/26 19:15:55]

Nice catch.

+                                swizzler->next(buffer.get());
+                            }
+                            return kIncompleteInput;
+                        }
+                        swizzler->next(buffer.get(), iter.nextY());
+                    }
+                } else {
+                    // Standard mode
+                    for (int32_t y = 0; y < innerHeight; y++) {
+                        if (GIF_ERROR == DGifGetLine(fGif, buffer.get(),
+                                innerWidth)) {
+                            gif_error("Could not decode line.\n");
+                            memset(buffer.get(), fillIndex, innerWidth);
+                            for (; y < innerHeight; y++) {
+                                swizzler->next(buffer.get());

[scroggo, 2015/03/25 19:44:48]

Is the swizzler overkill here? Could we just use memset/memset32?

[msarett, 2015/03/26 19:15:57]

Done.

+                            }
+                            return kIncompleteInput;
+                        }
+                        swizzler->next(buffer.get());
+                    }
+                }
+
+                // 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
+                //        TODO, we will return kSuccess after decoding the
+                //        first image in the file.  This is the same behavior
+                //        as SkImageDecoder_libgif.
+                //
+                //        Most times this works pretty well, but sometimes it
+                //        doesn't.  For example, I have an animated test image
+                //        where the first image in the file is 1x1, but the
+                //        subsequent images are meaningful.  This currently
+                //        displays the 1x1 image, which is not ideal.  Right
+                //        now I am leaving this as an issue that will be
+                //        addressed when we implement animated gifs.
+                //
+                //        It is also possible (not explicitly disallowed in the
+                //        specification) that gif files provide multiple
+                //        images in a single file that are all meant to be
+                //        displayed in the same frame together.  I will
+                //        currently leave this unimplemented until I find a
+                //        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:
+                // Read extension data
+#if GIFLIB_MAJOR < 5
+                if (GIF_ERROR ==
+                        DGifGetExtension(fGif, &saveExt.Function, &extData)) {
+#else
+                if (GIF_ERROR ==
+                        DGifGetExtension(fGif, &extFunction, &extData)) {
+#endif
+                    gif_error("Could not get extension.\n");
+                    return kInvalidInput;
+                }
+
+                // Create an extension block with our data
+                while (NULL != extData) {
+                    // Add a single block
+#if GIFLIB_MAJOR < 5
+                    if (GIF_ERROR == AddExtensionBlock(&saveExt, extData[0],
+                            &extData[1])) {
+#else
+                    if (GIF_ERROR ==
+                            GifAddExtensionBlock(&saveExt.ExtensionBlockCount,
+                            &saveExt.ExtensionBlocks, extFunction, extData[0],
+                            &extData[1])) {
+#endif
+                        gif_error("Could not add extension block.\n");
+                        return kInvalidInput;
+                    }
+                    // Move to the next block
+                    if (GIF_ERROR == DGifGetExtensionNext(fGif, &extData)) {
+                        gif_error("Could not get next extension.\n");
+                        return kInvalidInput;
+                    }
+#if GIFLIB_MAJOR < 5
+                    saveExt.Function = 0;
+#endif
+                }
+                break;
+
+            // Signals the end of the gif file
+            case TERMINATE_RECORD_TYPE:

[scroggo, 2015/03/25 19:44:49]

Handle skbug.com/3534?

[msarett, 2015/03/26 19:15:56]

I added a check for 0x0 images in NewFromStream.  Additionally, if we break out
of the loop, it means that we have not decoded an image.  I now return failure
in this case.

+                break;
+
+            default:
+                break;
+        }
+    }
+
+    // Clean up memory
+    free_extension(&saveExt);

[scroggo, 2015/03/25 19:44:48]

Can you put this in an SkAutoCallVProc?

[msarett, 2015/03/26 19:15:56]

Yes definitely.  The way it is set up now seems to leak every time.

[scroggo, 2015/03/26 20:03:53]

Oh, you mean before updating? And now it's fixed? Good.

+
+    return kSuccess;

[scroggo, 2015/03/25 19:56:58]

This function is over 300 lines. Can it be broken up?

[msarett, 2015/03/26 19:15:56]

This is unfortunate.

I can't find a good way to break it up.  I thought about refactoring all of the
innerWidth/Height vs width/height checks but we may make modifications to these
values and use them later.

I actually wrote a SkPMColor* createColorTable() function, but it caused issues.
 We end up needing to pass around fill/transIndex information that needs access
to the colorCount and also is used later.

I think it's best to keep it together, but I'm sure it is actually possible to
break it up, and I'm happy to do it if you feel strongly.

[scroggo, 2015/03/26 20:03:53]

Only break it up if there's a clear way to do it. I'm fine with it as is.

+}