Add support for multiple frames in SkCodec

src/codec/SkGifCodec.cpp

 /*
  * 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 "SkBitmap.h"
+#include "SkCanvas.h"
+#include "SkCodecAnimation.h"
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
 #include "SkColorTable.h"
 #include "SkGifCodec.h"
 #include "SkStream.h"
 #include "SkSwizzler.h"
 #include "SkUtils.h"
 
 #include "gif_lib.h"
 
@@ skipping 34 matching lines @@
  */
 static GifFileType* open_gif(SkStream* stream) {
 #if GIFLIB_MAJOR < 5
     return DGifOpen(stream, read_bytes_callback);
 #else
     return DGifOpen(stream, read_bytes_callback, nullptr);
 #endif
 }
 
 /*
- * 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.
     for (int32_t i = image.ExtensionBlockCount - 1; i >= 0; i--) {
         // Get an extension block
         const ExtensionBlock& extBlock = image.ExtensionBlocks[i];
 
         // Specifically, we need to check for a graphics control extension,
         // which may contain transparency information.  Also, note that a valid
         // 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) {
             // 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;
+    return false;
 }
 
 /*
- * 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));
-}
-
-/*
  * 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, nullptr);
 #endif
 }
@@ skipping 35 matching lines @@
     SkAutoTDelete<SkStream> streamDeleter(stream);
 
     // Read gif header, logical screen descriptor, and global color table
     SkAutoTCallVProc<GifFileType, CloseGif> gif(open_gif(stream));
 
     if (nullptr == gif) {
         gif_error("DGifOpen failed.\n");
         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);
+
+    uint32_t transIndex = SK_MaxU32;
+    read_graphics_extension(gif->SavedImages[0], &transIndex, nullptr, nullptr);
 
     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());
-
         // 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
         // there might be a valid transparent index, we must indicate that the image has
         // alpha.
         // In the case where we must support alpha, we indicate kBinary, since every
         // pixel will either be fully opaque or fully transparent.
         SkEncodedInfo::Alpha alpha = (transIndex < 256) ? SkEncodedInfo::kBinary_Alpha :
                 SkEncodedInfo::kOpaque_Alpha;
 
         // Return the codec
         // Use kPalette since Gifs are encoded with a color table.
         // Use 8-bits per component, since this is the output we get from giflib.
         // 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();
         *gifOut = gif.release();
     }
     return true;
 }
 
 /*
  * 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) {
     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)
-{}
+    , 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;
 
-bool SkGifCodec::onRewind() {
-    GifFileType* gifOut = nullptr;
-    if (!ReadHeader(this->stream(), nullptr, &gifOut)) {
-        return false;
+        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;
+    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;
+        }
+
+        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;
+                    }
+                    break;
+            }
+        }
+    }
 }
 
-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;
+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;
+    }
 
-    // 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);
-        }
-        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);
-                    }
-                    // 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;
-        }
-    } while (TERMINATE_RECORD_TYPE != recordType);
-
-    return gif_error("Could not find any images to decode in gif file.\n", kInvalidInput);
+    return fFrameInfos[index].fRequiredFrame;
 }
 
-bool SkGifCodec::GetDimensions(GifFileType* gif, SkISize* size, SkIRect* frameRect) {
-    // Get the encoded dimension values
-    SavedImage* image = &gif->SavedImages[gif->ImageCount - 1];
+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) {
+    // 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;
     int frameWidth = desc.Width;
     int frameHeight = desc.Height;
     int width = gif->SWidth;
     int height = gif->SHeight;
 
     // Ensure that the decode dimensions are large enough to contain the frame
     width = SkTMax(width, frameWidth + frameLeft);
     height = SkTMax(height, frameHeight + frameTop);
 
     // All of these dimensions should be positive, as they are encoded as unsigned 16-bit integers.
     // It is unclear why giflib casts them to ints.  We will go ahead and check that they are
     // in fact positive.
     if (frameLeft < 0 || frameTop < 0 || frameWidth < 0 || frameHeight < 0 || width <= 0 ||
             height <= 0) {
         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.
     // This allows for predictable, safe behavior.
     if (colorCount > 0) {
         // Gifs have the option to specify the color at a single index of the color

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

I know Chrome always fills incomplete images with transparent pixels...  How do
they handle out of range indices?

This is complicated but seems to make a bit of sense.

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

Do you mean a transparent pixel that is out of range? They treat that as not
having a transparent pixel.

For other colors, if they are out of range, they skip that pixel (which is okay,
since they already cleared to transparent). There might be ways to optimize
that:
- For an 8 bit GIF, the values to be read out of the GIF are guaranteed to be <=
256, right? So the only way for the index to be out of range is for the table to
have less than 256 colors. So we can fill in up to 256 colors with transparent
(I think we might already do that?). That might be faster than the if statement,
not sure.
- I'm guessing this would be similarly true for 4 bit colors.

         // 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.  If there is not valid transparent index, we will try to use the
         // backgroundIndex as the fill index.  If the backgroundIndex is also not
         // 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,
         int* inputColorCount, const Options& opts) {
     // Check for valid input parameters
     if (!conversion_possible_ignore_color_space(dstInfo, this->getInfo())) {
         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,
                                         int* rowsDecoded) {
     Result result = this->prepareToDecode(dstInfo, inputColorPtr, inputColorCount, opts);
     if (kSuccess != result) {
         return result;
     }
 
     if (dstInfo.dimensions() != this->getInfo().dimensions()) {
         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);
+    this->decodeFrame(dstInfo, pixels, dstRowBytes, opts);
+    return kSuccess;
+}
+
+// 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);
+        }
+
+        // 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 {
+        // 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()));
+        }
     }
 
+    SkASSERT((int) frameIndex < fGif->ImageCount);
+    const SavedImage* image = &fGif->SavedImages[frameIndex];
+    const GifImageDesc& desc = image->ImageDesc;
+
+    ColorMapObject* cmap = desc.ColorMap ? desc.ColorMap : fGif->SColorMap;
+    sk_sp<SkColorTable> ct(create_color_table(*cmap, frameInfo.fTransIndex, SK_MaxU32));
+
+    std::unique_ptr<SkSwizzler> swizzler(SkSwizzler::CreateSwizzler(this->getEncodedInfo(),
+            ct->readColors(), dstInfo, opts));
+    SkASSERT(swizzler);
+
     // 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());
+    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);
     }
-    return kSuccess;
+
+    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);
+    }
 }
 
 // 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());
-
-        // Set the actual number of source rows that we need to decode.
-        *rowsInFrame = rowsLeft - rowsAfterFrame;
-    } 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;
-        }
-        fSwizzler->swizzle(dst, fSrcBuffer.get());
-        dst = SkTAddOffset<void>(dst, rowBytes);
-    }
-
-    return count;
-}
-
-bool SkGifCodec::onSkipScanlines(int count) {
-    int rowsBeforeFrame;
-    int rowsInFrame;
-    this->handleScanlineFrame(count, &rowsBeforeFrame, &rowsInFrame);
-
-    for (int i = 0; i < rowsInFrame; i++) {
-        if (!this->readRow()) {
-            return false;
-        }
-    }
-
-    return true;
-}
-
-SkCodec::SkScanlineOrder SkGifCodec::onGetScanlineOrder() const {
-    if (fGif->Image.Interlace) {
-        return kOutOfOrder_SkScanlineOrder;
-    }
-    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;
-}