Add support for multiple frames in SkCodec

src/codec/SkGifCodec.cpp

Index: src/codec/SkGifCodec.cpp
diff --git a/src/codec/SkGifCodec.cpp b/src/codec/SkGifCodec.cpp
index c35cd24ae498f5d12f62c39a687a95a57b9fcb08..cbe87938b1bb15ac9a748dc41f465faa5f84825e 100644
--- a/src/codec/SkGifCodec.cpp
+++ b/src/codec/SkGifCodec.cpp
@@ -5,6 +5,9 @@
  * found in the LICENSE file.
  */
 
+#include "SkBitmap.h"
+#include "SkCanvas.h"
+#include "SkCodecAnimation.h"
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
 #include "SkColorTable.h"
@@ -59,9 +62,11 @@ static GifFileType* open_gif(SkStream* stream) {
 }
 
 /*
- * Check if a there is an index of the color table for a transparent pixel
+ * Read the graphics extension and report info from it. Returns false if there is none.
  */
-static uint32_t find_trans_index(const SavedImage& image) {
+static bool read_graphics_extension(const SavedImage& image, uint32_t* transIndex,
+                                    size_t* duration,
+                                    SkCodecAnimation::DisposalMethod* disposalMethod) {
     // If there is a transparent index specified, it will be contained in an
     // extension block.  We will loop through extension blocks in reverse order
     // to check the most recent extension blocks first.
@@ -78,44 +83,45 @@ static uint32_t find_trans_index(const SavedImage& image) {
             // 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];
+            if (transIndex) {
+                if (1 == (extBlock.Bytes[0] & 1)) {
+                    // Use uint32_t to prevent sign extending
+                    *transIndex = extBlock.Bytes[3];
+                } else {
+                    // Use maximum unsigned int (surely an invalid index) to indicate that a valid
+                    // index was not found.
+                    *transIndex = SK_MaxU32;
+                }
+            }
+
+            // The second and third bytes represent the duration.
+            if (duration) {
+                *duration = extBlock.Bytes[2] << 8 | extBlock.Bytes[1];
+            }
+
+            if (disposalMethod) {
+                int rawDisposalMethod = (extBlock.Bytes[0] >> 2) & 7;
+                switch (rawDisposalMethod) {
+                    case 1:
+                    case 2:
+                    case 3:
+                        *disposalMethod = (SkCodecAnimation::DisposalMethod) rawDisposalMethod;
+                        break;
+                    case 4:
+                        *disposalMethod = SkCodecAnimation::RestorePrevious_DisposalMethod;
+                        break;
+                    default:
+                        *disposalMethod = SkCodecAnimation::Keep_DisposalMethod;
+                        break;
+                }
             }
 
             // There should only be one graphics control extension for the image frame
-            break;
+            return true;
         }
     }
 
-    // Use maximum unsigned int (surely an invalid index) to indicate that a valid
-    // index was not found.
-    return SK_MaxU32;
-}
-
-inline uint32_t ceil_div(uint32_t a, uint32_t b) {
-    return (a + b - 1) / b;
-}
-
-/*
- * Gets the output row corresponding to the encoded row for interlaced gifs
- */
-inline uint32_t get_output_row_interlaced(uint32_t encodedRow, uint32_t height) {
-    SkASSERT(encodedRow < height);
-    // First pass
-    if (encodedRow * 8 < height) {
-        return encodedRow * 8;
-    }
-    // Second pass
-    if (encodedRow * 4 < height) {
-        return 4 + 8 * (encodedRow - ceil_div(height, 8));
-    }
-    // Third pass
-    if (encodedRow * 2 < height) {
-        return 2 + 4 * (encodedRow - ceil_div(height, 4));
-    }
-    // Fourth pass
-    return 1 + 2 * (encodedRow - ceil_div(height, 2));
+    return false;
 }
 
 /*
@@ -174,31 +180,21 @@ bool SkGifCodec::ReadHeader(SkStream* stream, SkCodec** codecOut, GifFileType**
         return false;
     }
 
-    // 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 = ReadUpToFirstImage(gif, &transIndex);
-    if (kSuccess != result){
+    // Read the whole file.
+    if (GIF_ERROR == DGifSlurp(gif) || gif->ImageCount == 0) {
         return false;
     }
 
-    // Read the image descriptor
-    if (GIF_ERROR == DGifGetImageDesc(gif)) {
+    SkISize size;
+    if (!GetDimensions(gif, &size)) {
+        gif_error("Invalid gif size.\n");
         return false;
     }
-    // If reading the image descriptor is successful, the image count will be
-    // incremented.
-    SkASSERT(gif->ImageCount >= 1);
 
-    if (nullptr != codecOut) {
-        SkISize size;
-        SkIRect frameRect;
-        if (!GetDimensions(gif, &size, &frameRect)) {
-            gif_error("Invalid gif size.\n");
-            return false;
-        }
-        bool frameIsSubset = (size != frameRect.size());
+    uint32_t transIndex = SK_MaxU32;
+    read_graphics_extension(gif->SavedImages[0], &transIndex, nullptr, nullptr);
 
+    if (nullptr != codecOut) {
         // Determine the encoded alpha type.  The transIndex might be valid if it less
         // than 256.  We are not certain that the index is valid until we process the color
         // table, since some gifs have color tables with less than 256 colors.  If
@@ -215,7 +211,7 @@ bool SkGifCodec::ReadHeader(SkStream* stream, SkCodec** codecOut, GifFileType**
         // FIXME: Gifs can actually be encoded with 4-bits per pixel.  Can we support this?
         SkEncodedInfo info = SkEncodedInfo::Make(SkEncodedInfo::kPalette_Color, alpha, 8);
         *codecOut = new SkGifCodec(size.width(), size.height(), info, streamDeleter.release(),
-                gif.release(), transIndex, frameRect, frameIsSubset);
+                gif.release(), transIndex);
     } else {
         SkASSERT(nullptr != gifOut);
         streamDeleter.release();
@@ -232,134 +228,105 @@ bool SkGifCodec::ReadHeader(SkStream* stream, SkCodec** codecOut, GifFileType**
 SkCodec* SkGifCodec::NewFromStream(SkStream* stream) {
     SkCodec* codec = nullptr;
     if (ReadHeader(stream, &codec, nullptr)) {
+        SkASSERT(codec);
         return codec;
     }
     return nullptr;
 }
 
+static SkColorTable* create_color_table(const ColorMapObject& colorMap, uint32_t transIndex,
+                                        uint32_t backgroundIndex);
 SkGifCodec::SkGifCodec(int width, int height, const SkEncodedInfo& info, SkStream* stream,
-        GifFileType* gif, uint32_t transIndex, const SkIRect& frameRect, bool frameIsSubset)
+        GifFileType* gif, uint32_t transIndex)
     : INHERITED(width, height, info, stream)
     , fGif(gif)
     , fSrcBuffer(new uint8_t[this->getInfo().width()])
-    , fFrameRect(frameRect)
-    // If it is valid, fTransIndex will be used to set fFillIndex.  We don't know if
-    // fTransIndex is valid until we process the color table, since fTransIndex may
-    // be greater than the size of the color table.
-    , fTransIndex(transIndex)
-    // Default fFillIndex is 0.  We will overwrite this if fTransIndex is valid, or if
-    // there is a valid background color.
     , fFillIndex(0)
-    , fFrameIsSubset(frameIsSubset)
     , fSwizzler(NULL)
     , fColorTable(NULL)
-{}
-
-bool SkGifCodec::onRewind() {
-    GifFileType* gifOut = nullptr;
-    if (!ReadHeader(this->stream(), nullptr, &gifOut)) {
-        return false;
+    , fFrameInfos(gif->ImageCount)
+{
+    if (gif->SColorMap) {
+        // FIXME: Note - this is the global color table.
+        fColorTable.reset(create_color_table(*gif->SColorMap, transIndex, gif->SBackGroundColor));

[msarett, 2016/09/22 22:54:35]

So if there is a global color table we always use it to set fColorTable - I'm
guessing this is the FIXME?

[scroggo, 2016/09/23 15:53:15]

Not sure. Most of this code is throw-away/proof-of-concept.

+    } else {
+        // Read the first image's local color table, for now.
+        const SavedImage* image = &gif->SavedImages[0];
+        const GifImageDesc& desc = image->ImageDesc;
+
+        if (desc.ColorMap) {
+            // I'm guessing the background index only makes sense for the global color table.

[msarett, 2016/09/22 22:54:35]

I have no strong feelings about using or not using the background color.  Would
be interesting how many gifs have frames that actually don't fill the entire
bounds.

[scroggo, 2016/09/23 15:53:15]

My guess would be a lot of them. Even if they fill the bounds, they might have
transparency. We can determine this with some UMA histograms, but I know I've
seen some e.g. emoji-like images that are characters that don't fill the bounds.

The tricky thing is that if we do not use the background color, we've changed
the behavior for Android. But if we do, we've changed the behavior for Chrome.

+            // A lot of users (including Chromium!) ignore the background color, but we (currently)
+            // use it to fill in the color table if it's less than 256 colors.
+            fColorTable.reset(create_color_table(*desc.ColorMap, transIndex, SK_MaxU32));
+        }
     }
 
-    SkASSERT(nullptr != gifOut);
-    fGif.reset(gifOut);
-    return true;
-}
-
-SkCodec::Result SkGifCodec::ReadUpToFirstImage(GifFileType* gif, uint32_t* transIndex) {
-    // Use this as a container to hold information about any gif extension
-    // blocks.  This generally stores transparency and animation instructions.
-    SavedImage saveExt;
-    SkAutoTCallVProc<SavedImage, FreeExtension> autoFreeExt(&saveExt);
-    saveExt.ExtensionBlocks = nullptr;
-    saveExt.ExtensionBlockCount = 0;
-    GifByteType* extData;
-    int32_t extFunction;
-
-    // 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.
-    GifRecordType recordType;
-    do {
-        // Get the current record type
-        if (GIF_ERROR == DGifGetRecordType(gif, &recordType)) {
-            return gif_error("DGifGetRecordType failed.\n", kInvalidInput);
+    for (int i = 0; i < gif->ImageCount; i++) {
+        const SavedImage* image = &gif->SavedImages[i];
+        const GifImageDesc& desc = image->ImageDesc;
+
+        auto& frame = fFrameInfos.push_back();
+        // FIXME: Probably want to intersect this with the bounds, just in case.

[msarett, 2016/09/22 22:54:35]

I think I made a bunch of test gifs to exercise some of these cases - should be
on the bots.

How does Chrome handle this case?  I'm wondering if we shouldn't just "crop" the
parts of the frame outside of the image dimensions.  I think that's what you're
suggesting here.

[scroggo, 2016/09/23 15:53:15]

Yes, that is what they do. But I didn't want to handle that with this dummy
implementation.

+        // But then we'll need to make sure we draw the right thing...
+        // i.e. if the top is cut off (desc.Top < 0), we want to start off drawing line -desc.Top
+        // (probably?)
+        // Or if we're cut off width-wise, we need to ensure we use the original row bytes
+        // (desc.Width) but the corrected width.
+        frame.fFrameRect = SkIRect::MakeXYWH(desc.Left, desc.Top, desc.Width, desc.Height);
+        if (!read_graphics_extension(*image, &frame.fTransIndex, &frame.fDuration,
+                                     &frame.fDisposalMethod)) {
+            frame.fDisposalMethod = SkCodecAnimation::Keep_DisposalMethod;
+            frame.fDuration = 0;
+            frame.fTransIndex = SK_MaxU32;
         }
-        switch (recordType) {
-            case IMAGE_DESC_RECORD_TYPE: {
-                *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
-                //        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 (GIF_ERROR == DGifGetExtension(gif, &extFunction, &extData)) {
-                    return gif_error("Could not get extension.\n", kIncompleteInput);
-                }
-
-                // Create an extension block with our data
-                while (nullptr != extData) {
-                    // Add a single block
 
-#if GIFLIB_MAJOR < 5
-                    if (AddExtensionBlock(&saveExt, extData[0],
-                                          &extData[1]) == GIF_ERROR) {
-#else
-                    if (GIF_ERROR == GifAddExtensionBlock(&saveExt.ExtensionBlockCount,
-                                                          &saveExt.ExtensionBlocks,
-                                                          extFunction, extData[0], &extData[1])) {
-#endif
-                        return gif_error("Could not add extension block.\n", kIncompleteInput);
+        if (0 == i) {
+            frame.fRequiredFrame = kIndependentFrame;
+        } else {
+            // FIXME: We could correct these after decoding (i.e. some frames may turn out to be

[msarett, 2016/09/22 22:54:34]

Seems like an optimization to consider later...

[scroggo, 2016/09/23 15:53:16]

Yeah, this is what we talked about yesterday (e.g. looking at whether a frame
used the transparent index at all.

+            // independent although we did not determine that here).
+            const SkCodecAnimation::FrameInfo& prevFrame = fFrameInfos[i-1];
+            switch (prevFrame.fDisposalMethod) {
+                case SkCodecAnimation::Keep_DisposalMethod:
+                    frame.fRequiredFrame = i - 1;
+                    break;
+                case SkCodecAnimation::RestorePrevious_DisposalMethod:
+                    frame.fRequiredFrame = prevFrame.fRequiredFrame;
+                    break;
+                case SkCodecAnimation::RestoreBGColor_DisposalMethod:

[msarett, 2016/09/22 22:54:34]

I'm not sure I completely understand the logic in this case...

[scroggo, 2016/09/23 15:53:16]

Yeah, this is a confusing one. As I understand it, if frame i-1 is marked as
RestoreBGColor_DisposalMethod, it means that i will start with i-1, except the
space taken up by i-1's frameRect is cleared to the background color (which
Chrome interprets as transparent).

If i-1 fills the dimensions, then we'll be erasing the whole frame anyway, so i
is effectively independent. That is the first half of the or conjunction.

If i-1 is independent, then that means it only occupies the frameRect anyway, so
outside of the frameRect is transparent, and we clear inside the frameRect to
transparent, too, so i is independent. This is the second half of the or.

Otherwise, we need to depend on i - 1. When i is drawn, we'll start with i-1,
erase the frameRect, and see the "border" - i.e. whatever was drawn by i - 2
minus the frameRect for i - 1.

+                    if (prevFrame.fFrameRect == SkIRect::MakeWH(width, height)
+                            || prevFrame.fRequiredFrame == kIndependentFrame) {
+                        frame.fRequiredFrame = kIndependentFrame;
+                    } else {
+                        frame.fRequiredFrame = i - 1;
                     }
-                    // Move to the next block
-                    if (GIF_ERROR == DGifGetExtensionNext(gif, &extData)) {
-                        return gif_error("Could not get next extension.\n", kIncompleteInput);
-                    }
-                }
-                break;
-
-            // Signals the end of the gif file
-            case TERMINATE_RECORD_TYPE:
-                break;
-
-            default:
-                // DGifGetRecordType returns an error if the record type does
-                // not match one of the above cases.  This should not be
-                // reached.
-                SkASSERT(false);
-                break;
+                    break;
+            }
         }
-    } while (TERMINATE_RECORD_TYPE != recordType);
+    }
+}
 
-    return gif_error("Could not find any images to decode in gif file.\n", kInvalidInput);
+size_t SkGifCodec::onGetRequiredFrame(size_t index) {
+    if ((int) index >= fFrameInfos.count()) {

[msarett, 2016/09/22 22:54:36]

Can we assert this?  Seems like client should be smart enough to not pass us an
invalid index?

Edit: Maybe I'm wrong here - see below.

[scroggo, 2016/09/23 15:53:15]

I lean towards returning a failure for a public API. But I could be convinced
(assuming we allow querying the count up front, which I think we have not
settled on).

+        return kIndependentFrame;
+    }
+
+    return fFrameInfos[index].fRequiredFrame;
+}
+
+size_t SkGifCodec::onGetFrameDuration(size_t index) {
+    if ((int) index >= fFrameInfos.count()) {

[msarett, 2016/09/22 22:54:35]

Ditto.

+        return 0;
+    }
+
+    return fFrameInfos[index].fDuration;
 }
 
-bool SkGifCodec::GetDimensions(GifFileType* gif, SkISize* size, SkIRect* frameRect) {
-    // Get the encoded dimension values
-    SavedImage* image = &gif->SavedImages[gif->ImageCount - 1];
+bool SkGifCodec::GetDimensions(GifFileType* gif, SkISize* size) {
+    // Get the encoded dimension values for the first frame. Some GIFs have frames that are bigger

[msarett, 2016/09/22 22:54:36]

I remember when we decided to do this...

But now I kind of think that decision doesn't make sense anymore.  I'd rather
totally respect the bounds reported by the gif header.  And then we can treat
frames that fall outside these bounds "consistently".

Not sure whether the answer is shifting or cropping though.  I would vote
cropping - in some cases, if we shift, we might still have to crop anyway.

[scroggo, 2016/09/23 15:53:15]

I think we should stick with the behavior that Chromium currently has, and
consider changing that later on. Chrome expands based on the first frame. I'm
not sure whether they shift.

+    // than the screen width and height. For the first frame only, we will expand in that case.
+    SavedImage* image = &gif->SavedImages[0];
     const GifImageDesc& desc = image->ImageDesc;
     int frameLeft = desc.Left;
     int frameTop = desc.Top;
@@ -380,41 +347,36 @@ bool SkGifCodec::GetDimensions(GifFileType* gif, SkISize* size, SkIRect* frameRe
         return false;
     }
 
-    frameRect->setXYWH(frameLeft, frameTop, frameWidth, frameHeight);
     size->set(width, height);
     return true;
 }
 
+constexpr uint32_t kMaxColors = 256;
+
 void SkGifCodec::initializeColorTable(const SkImageInfo& dstInfo, SkPMColor* inputColorPtr,
         int* inputColorCount) {
-    // 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;
+    // FIXME: Use the correct one for the frame.
+    SkASSERT(fColorTable);
+    if (inputColorCount) {
+        *inputColorCount = fColorTable->count();
     }
 
-    // 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;
-    }
+    copy_color_table(dstInfo, fColorTable, inputColorPtr, inputColorCount);
+}
 
-    uint32_t colorCount = 0;
-    if (NULL != colorMap) {
-        colorCount = colorMap->ColorCount;
-        // giflib guarantees these properties
-        SkASSERT(colorCount == (unsigned) (1 << (colorMap->BitsPerPixel)));
-        SkASSERT(colorCount <= 256);
-        PackColorProc proc = choose_pack_color_proc(false, dstInfo.colorType());
-        for (uint32_t i = 0; i < colorCount; i++) {
-            colorPtr[i] = proc(0xFF, colorMap->Colors[i].Red,
-                    colorMap->Colors[i].Green, colorMap->Colors[i].Blue);
-        }
+static SkColorTable* create_color_table(const ColorMapObject& colorMap, uint32_t transIndex,
+                                        uint32_t backgroundIndex) {
+    SkPMColor colorPtr[kMaxColors];
+    const uint32_t colorCount = colorMap.ColorCount;
+    // giflib guarantees these properties
+    SkASSERT(colorCount == (unsigned) (1 << (colorMap.BitsPerPixel)));
+    SkASSERT(colorCount <= 256);
+
+    // FIXME: We may be creating this at the wrong time - we don't know the dst color type.
+    PackColorProc proc = choose_pack_color_proc(false, kIndex_8_SkColorType);
+    for (uint32_t i = 0; i < colorCount; i++) {
+        const GifColorType& color = colorMap.Colors[i];
+        colorPtr[i] = proc(0xFF, color.Red, color.Green, color.Blue);
     }
 
     // Fill in the color table for indices greater than color count.
@@ -428,23 +390,24 @@ void SkGifCodec::initializeColorTable(const SkImageInfo& dstInfo, SkPMColor* inp
         // valid, we will let fFillIndex default to 0 (it is set to zero in the
         // constructor).  This behavior is not specified but matches
         // SkImageDecoder_libgif.
-        uint32_t backgroundIndex = fGif->SBackGroundColor;
-        if (fTransIndex < colorCount) {
-            colorPtr[fTransIndex] = SK_ColorTRANSPARENT;
-            fFillIndex = fTransIndex;
+        uint32_t fillIndex;
+        if (transIndex < colorCount) {
+            colorPtr[transIndex] = SK_ColorTRANSPARENT;
+            fillIndex = transIndex;
         } else if (backgroundIndex < colorCount) {
-            fFillIndex = backgroundIndex;
+            fillIndex = backgroundIndex;
+        } else {
+            fillIndex = 0;
         }
 
-        for (uint32_t i = colorCount; i < maxColors; i++) {
-            colorPtr[i] = colorPtr[fFillIndex];
+        for (uint32_t i = colorCount; i < kMaxColors; i++) {
+            colorPtr[i] = colorPtr[fillIndex];
         }
     } else {
-        sk_memset32(colorPtr, 0xFF000000, maxColors);
+        sk_memset32(colorPtr, 0xFF000000, kMaxColors);
     }
 
-    fColorTable.reset(new SkColorTable(colorPtr, maxColors));
-    copy_color_table(dstInfo, this->fColorTable, inputColorPtr, inputColorCount);
+    return new SkColorTable(colorPtr, kMaxColors);
 }
 
 SkCodec::Result SkGifCodec::prepareToDecode(const SkImageInfo& dstInfo, SkPMColor* inputColorPtr,
@@ -454,30 +417,31 @@ SkCodec::Result SkGifCodec::prepareToDecode(const SkImageInfo& dstInfo, SkPMColo
         return gif_error("Cannot convert input type to output type.\n", kInvalidConversion);
     }
 
+    if ((opts.fFrameOptions && opts.fFrameOptions->fIndex > 0)
+            && dstInfo.colorType() == kIndex_8_SkColorType) {
+        // FIXME: It is possible that a later frame can be decoded to index8, if it does one of the

[msarett, 2016/09/22 22:54:35]

Good questions, probably a good place to follow up.

+        // following:
+        // - Covers the entire previous frame
+        // - Shares a color table (and transparent index) with any prior frames that are showing.
+        // We must support index8 for the first frame to be backwards compatible on Android.
+        return gif_error("Cannot decode multiframe gif (except frame 0) as index 8.\n",
+                         kInvalidConversion);
+    }
+
+    if (opts.fFrameOptions && (int) opts.fFrameOptions->fIndex > fFrameInfos.count()) {
+        return gif_error("frame index out of range!\n", kInvalidParameters);

[msarett, 2016/09/22 22:54:34]

Hmmm ok...  We are set up to fail nicely here on invalid parameters.  Maybe I'm
wrong above - we should try to be consistent.

[scroggo, 2016/09/23 15:53:16]

Again I lean towards failing gracefully in public APIs, and if we remove the
option to know how many frames there are up front, we need to be able to fail
gracefully for sure.

+    }
+
     // Initialize color table and copy to the client if necessary
     this->initializeColorTable(dstInfo, inputColorPtr, inputColorCount);
-
-    this->initializeSwizzler(dstInfo, opts);
     return kSuccess;
 }
 
-void SkGifCodec::initializeSwizzler(const SkImageInfo& dstInfo, const Options& opts) {
-    const SkPMColor* colorPtr = get_color_ptr(fColorTable.get());
-    const SkIRect* frameRect = fFrameIsSubset ? &fFrameRect : nullptr;
-    fSwizzler.reset(SkSwizzler::CreateSwizzler(this->getEncodedInfo(), colorPtr, dstInfo, opts,
-            frameRect));
-    SkASSERT(fSwizzler);
-}
-
-bool SkGifCodec::readRow() {
-    return GIF_ERROR != DGifGetLine(fGif, fSrcBuffer.get(), fFrameRect.width());
-}
-
 /*
  * Initiates the gif decode
  */
 SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
-                                        void* dst, size_t dstRowBytes,
+                                        void* pixels, size_t dstRowBytes,
                                         const Options& opts,
                                         SkPMColor* inputColorPtr,
                                         int* inputColorCount,
@@ -491,117 +455,105 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
         return gif_error("Scaling not supported.\n", kInvalidScale);
     }
 
-    // Initialize the swizzler
-    if (fFrameIsSubset) {
-        // Fill the background
-        SkSampler::Fill(dstInfo, dst, dstRowBytes, this->getFillValue(dstInfo),
-                opts.fZeroInitialized);
-    }
-
-    // Iterate over rows of the input
-    for (int y = fFrameRect.top(); y < fFrameRect.bottom(); y++) {
-        if (!this->readRow()) {
-            *rowsDecoded = y;
-            return gif_error("Could not decode line.\n", kIncompleteInput);
-        }
-        void* dstRow = SkTAddOffset<void>(dst, dstRowBytes * this->outputScanline(y));
-        fSwizzler->swizzle(dstRow, fSrcBuffer.get());
-    }
+    this->decodeFrame(dstInfo, pixels, dstRowBytes, opts);
     return kSuccess;
 }
 
-// FIXME: This is similar to the implementation for bmp and png.  Can we share more code or
-//        possibly make this non-virtual?
-uint64_t SkGifCodec::onGetFillValue(const SkImageInfo& dstInfo) const {
-    const SkPMColor* colorPtr = get_color_ptr(fColorTable.get());
-    return get_color_table_fill_value(dstInfo.colorType(), dstInfo.alphaType(), colorPtr,
-                                      fFillIndex, nullptr);
-}
-
-SkCodec::Result SkGifCodec::onStartScanlineDecode(const SkImageInfo& dstInfo,
-        const SkCodec::Options& opts, SkPMColor inputColorPtr[], int* inputColorCount) {
-    return this->prepareToDecode(dstInfo, inputColorPtr, inputColorCount, opts);
-}
-
-void SkGifCodec::handleScanlineFrame(int count, int* rowsBeforeFrame, int* rowsInFrame) {
-    if (fFrameIsSubset) {
-        const int currRow = this->currScanline();
-
-        // The number of rows that remain to be skipped before reaching rows that we
-        // actually must decode into.
-        // This must be at least zero.  We also make sure that it is less than or
-        // equal to count, since we will skip at most count rows.
-        *rowsBeforeFrame = SkTMin(count, SkTMax(0, fFrameRect.top() - currRow));
-
-        // Rows left to decode once we reach the start of the frame.
-        const int rowsLeft = count - *rowsBeforeFrame;
-
-        // Count the number of that extend beyond the bottom of the frame.  We do not
-        // need to decode into these rows.
-        const int rowsAfterFrame = SkTMax(0, currRow + rowsLeft - fFrameRect.bottom());
+// FIXME: This should be SkCodec::Result, probably?
+void SkGifCodec::decodeFrame(const SkImageInfo& dstInfo, void* pixels, size_t dstRowBytes,
+                             const Options& opts) {
+    SkBitmap tmpBm;
+    void* dst = pixels;
+
+    const size_t frameIndex = opts.fFrameOptions ? opts.fFrameOptions->fIndex : 0;
+    SkASSERT((int) frameIndex < fFrameInfos.count());

[scroggo, 2016/09/23 21:15:00]

D'oh! After all my talk about failing gracefully I have this assert here...

+    const auto& frameInfo = fFrameInfos[frameIndex];
+    const SkIRect& frameRect = frameInfo.fFrameRect;
+    const bool independent = frameInfo.fRequiredFrame == kIndependentFrame;
+    if (!independent) {
+        if (opts.fFrameOptions && !opts.fFrameOptions->fHasPriorFrame) {
+            // Decode that frame into pixels.
+            Options prevFrameOpts(opts);
+            MultiFrameOptions prevFrameMultiOpts;
+            prevFrameMultiOpts.fIndex = frameInfo.fRequiredFrame;
+            prevFrameMultiOpts.fHasPriorFrame = false;
+            prevFrameOpts.fFrameOptions = &prevFrameMultiOpts;
+            this->decodeFrame(dstInfo, pixels, dstRowBytes, prevFrameOpts);
+        }
+        const auto& prevFrame = fFrameInfos[frameInfo.fRequiredFrame];
+        if (prevFrame.fDisposalMethod == SkCodecAnimation::RestoreBGColor_DisposalMethod) {
+            const SkIRect& prevRect = prevFrame.fFrameRect;
+            void* const eraseDst = SkTAddOffset<void>(pixels, prevRect.fTop * dstRowBytes
+                    + prevRect.fLeft * SkColorTypeBytesPerPixel(dstInfo.colorType()));
+            // FIXME: Is this the right color?

[msarett, 2016/09/22 22:54:34]

Is this what Chrome does?

[scroggo, 2016/09/23 15:53:15]

Yes. But my concern is that if we're decoding to index 8, this will be the 0th
color, which may not be transparent.

+            SkSampler::Fill(dstInfo.makeWH(prevRect.width(), prevRect.height()), eraseDst,
+                            dstRowBytes, SK_ColorTRANSPARENT, SkCodec::kNo_ZeroInitialized);
+        }
 
-        // Set the actual number of source rows that we need to decode.
-        *rowsInFrame = rowsLeft - rowsAfterFrame;
+        // Now we need to swizzle to a temporary bitmap. That way we do not overwrite pixels
+        // with transparent. Later we'll draw to pixels
+        const int frameWidth = frameRect.width();
+        const int frameHeight = frameRect.height();
+        SkImageInfo tmpInfo = dstInfo.makeWH(frameWidth, frameHeight);
+        // FIXME: Could be more agressive than this (i.e. it could be out of the actual range of
+        // the color map).
+        if (frameInfo.fTransIndex < 256) {
+            // Need alpha to blend with prior frame.
+            tmpInfo = tmpInfo.makeAlphaType(kUnpremul_SkAlphaType);
+        }
+        tmpBm.allocPixels(tmpInfo);
+        dst = tmpBm.getPixels();
     } else {
-        *rowsBeforeFrame = 0;
-        *rowsInFrame = count;
-    }
-}
-
-int SkGifCodec::onGetScanlines(void* dst, int count, size_t rowBytes) {
-    int rowsBeforeFrame;
-    int rowsInFrame;
-    this->handleScanlineFrame(count, &rowsBeforeFrame, &rowsInFrame);
-
-    if (fFrameIsSubset) {
-        // Fill the requested rows
-        SkImageInfo fillInfo = this->dstInfo().makeWH(this->dstInfo().width(), count);
-        uint64_t fillValue = this->onGetFillValue(this->dstInfo());
-        fSwizzler->fill(fillInfo, dst, rowBytes, fillValue, this->options().fZeroInitialized);
-
-        // Start to write pixels at the start of the image frame
-        dst = SkTAddOffset<void>(dst, rowBytes * rowsBeforeFrame);
-    }
-
-    for (int i = 0; i < rowsInFrame; i++) {
-        if (!this->readRow()) {
-            return i + rowsBeforeFrame;
+        // This frame is independent
+        if (frameRect != dstInfo.bounds()) {
+            // Fill the background
+            // FIXME: Android may want the BG color, but Chromium wants transparent!
+            SkSampler::Fill(dstInfo, pixels, dstRowBytes, SK_ColorTRANSPARENT,
+                    opts.fZeroInitialized);
+
+            // Now offset dst to draw the frame:
+            dst = SkTAddOffset<void>(dst, frameRect.fTop * dstRowBytes
+                    + frameRect.fLeft * SkColorTypeBytesPerPixel(dstInfo.colorType()));
         }
-        fSwizzler->swizzle(dst, fSrcBuffer.get());
-        dst = SkTAddOffset<void>(dst, rowBytes);
     }
 
-    return count;
-}
+    SkASSERT((int) frameIndex < fGif->ImageCount);
+    const SavedImage* image = &fGif->SavedImages[frameIndex];
+    const GifImageDesc& desc = image->ImageDesc;
 
-bool SkGifCodec::onSkipScanlines(int count) {
-    int rowsBeforeFrame;
-    int rowsInFrame;
-    this->handleScanlineFrame(count, &rowsBeforeFrame, &rowsInFrame);
+    ColorMapObject* cmap = desc.ColorMap ? desc.ColorMap : fGif->SColorMap;
+    sk_sp<SkColorTable> ct(create_color_table(*cmap, frameInfo.fTransIndex, SK_MaxU32));
 
-    for (int i = 0; i < rowsInFrame; i++) {
-        if (!this->readRow()) {
-            return false;
-        }
-    }
+    std::unique_ptr<SkSwizzler> swizzler(SkSwizzler::CreateSwizzler(this->getEncodedInfo(),
+            ct->readColors(), dstInfo, opts));
+    SkASSERT(swizzler);
 
-    return true;
-}
+    // Iterate over rows of the input
+    void* dstRow = dst;
+    GifByteType* src = image->RasterBits;
+    for (int y = 0; y < desc.Height; y++) {
+        swizzler->swizzle(dstRow, src);
+        dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
+        src = SkTAddOffset<GifByteType>(src, desc.Width);
+    }
 
-SkCodec::SkScanlineOrder SkGifCodec::onGetScanlineOrder() const {
-    if (fGif->Image.Interlace) {
-        return kOutOfOrder_SkScanlineOrder;
+    if (!independent) {
+        // We drew to a temporary bitmap. Now we need to draw it to pixels.
+        SkASSERT(tmpBm.getPixels() == dst);
+        SkBitmap dstBm;
+        // FIXME: What to do if this fails?
+        dstBm.installPixels(dstInfo, pixels, dstRowBytes, fColorTable, nullptr, nullptr);
+        SkCanvas canvas(dstBm);
+        const SkScalar left = SkIntToScalar(frameRect.left());
+        const SkScalar top = SkIntToScalar(frameRect.top());
+        canvas.drawBitmap(tmpBm, left, top);
     }
-    return kTopDown_SkScanlineOrder;
 }
 
-int SkGifCodec::onOutputScanline(int inputScanline) const {
-    if (fGif->Image.Interlace) {
-        if (inputScanline < fFrameRect.top() || inputScanline >= fFrameRect.bottom()) {
-            return inputScanline;
-        }
-        return get_output_row_interlaced(inputScanline - fFrameRect.top(), fFrameRect.height()) +
-                fFrameRect.top();
-    }
-    return inputScanline;
+// FIXME: This is similar to the implementation for bmp and png.  Can we share more code or
+//        possibly make this non-virtual?
+uint64_t SkGifCodec::onGetFillValue(const SkImageInfo& dstInfo) const {
+    const SkPMColor* colorPtr = get_color_ptr(fColorTable.get());
+    return get_color_table_fill_value(dstInfo.colorType(), dstInfo.alphaType(), colorPtr,
+                                      fFillIndex, nullptr);
 }