Use correct color table for prior GIF frames

While investigating skbug.com/5883 I noticed that we use the color
table for the current frame even while recursively decoding frames that
the current frame depends on.

This CL updates fCurrColorTable, fCurrColorTableIsReal and fSwizzler
before decoding prior frames, and then sets them back afterwards.

Move telling the client about the color table into prepareToDecode,
since the other callers do not need to do so. (That is only necessary
for decoding to index 8, which is unsupported for frames with
dependencies.)

Add a test that exposes the bug. colorTables.gif has a local color
table in its second frame that does not match the global table used by
the first frame.

GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=4208

Change-Id: Id2dc9e3283adfd92801d2f38726afa74574b1955
Reviewed-on: https://skia-review.googlesource.com/4208
Reviewed-by: Joost Ouwerling <joostouwerling@google.com>
Reviewed-by: Matt Sarett <msarett@google.com>
Commit-Queue: Leon Scroggins <scroggo@google.com>

4 files changed, 21 insertions, 23 deletions

diff --git a/resources/colorTables.gif b/resources/colorTables.gif
new file mode 100644
index 0000000000..f25d13cfaa
--- /dev/null
+++ b/resources/colorTables.gif
diff --git a/src/codec/SkGifCodec.cpp b/src/codec/SkGifCodec.cpp
index fe0a2b6623..13223960c8 100644
--- a/src/codec/SkGifCodec.cpp
+++ b/src/codec/SkGifCodec.cpp
@@ -139,8 +139,7 @@ std::vector<SkCodec::FrameInfo> SkGifCodec::onGetFrameInfo() {
     return result;
 }
 
-void SkGifCodec::initializeColorTable(const SkImageInfo& dstInfo, size_t frameIndex,
-        SkPMColor* inputColorPtr, int* inputColorCount) {
+void SkGifCodec::initializeColorTable(const SkImageInfo& dstInfo, size_t frameIndex) {
     fCurrColorTable = fReader->getColorTable(dstInfo.colorType(), frameIndex);
     fCurrColorTableIsReal = fCurrColorTable;
     if (!fCurrColorTable) {
@@ -148,12 +147,6 @@ void SkGifCodec::initializeColorTable(const SkImageInfo& dstInfo, size_t frameIn
         SkPMColor color = SK_ColorTRANSPARENT;
         fCurrColorTable.reset(new SkColorTable(&color, 1));
     }
-
-    if (inputColorCount) {
-        *inputColorCount = fCurrColorTable->count();
-    }
-
-    copy_color_table(dstInfo, fCurrColorTable.get(), inputColorPtr, inputColorCount);
 }
 
 
@@ -215,9 +208,15 @@ SkCodec::Result SkGifCodec::prepareToDecode(const SkImageInfo& dstInfo, SkPMColo
 
     fTmpBuffer.reset(new uint8_t[dstInfo.minRowBytes()]);
 
-    // Initialize color table and copy to the client if necessary
-    this->initializeColorTable(dstInfo, frameIndex, inputColorPtr, inputColorCount);
+    this->initializeColorTable(dstInfo, frameIndex);
     this->initializeSwizzler(dstInfo, frameIndex);
+
+    SkASSERT(fCurrColorTable);
+    if (inputColorCount) {
+        *inputColorCount = fCurrColorTable->count();
+    }
+    copy_color_table(dstInfo, fCurrColorTable.get(), inputColorPtr, inputColorCount);
+
     return kSuccess;
 }
 
@@ -337,6 +336,11 @@ SkCodec::Result SkGifCodec::decodeFrame(bool firstAttempt, const Options& opts,
                 Options prevFrameOpts(opts);
                 prevFrameOpts.fFrameIndex = frameContext->getRequiredFrame();
                 prevFrameOpts.fHasPriorFrame = false;
+                // The prior frame may have a different color table, so update it and the
+                // swizzler.
+                this->initializeColorTable(dstInfo, prevFrameOpts.fFrameIndex);
+                this->initializeSwizzler(dstInfo, prevFrameOpts.fFrameIndex);
+
                 const Result prevResult = this->decodeFrame(true, prevFrameOpts, nullptr);
                 switch (prevResult) {
                     case kSuccess:
@@ -348,6 +352,10 @@ SkCodec::Result SkGifCodec::decodeFrame(bool firstAttempt, const Options& opts,
                     default:
                         return prevResult;
                 }
+
+                // Go back to using the correct color table for this frame.
+                this->initializeColorTable(dstInfo, frameIndex);
+                this->initializeSwizzler(dstInfo, frameIndex);
             }
             const auto* prevFrame = fReader->frameContext(frameContext->getRequiredFrame());
             if (prevFrame->getDisposalMethod() == SkCodecAnimation::RestoreBGColor_DisposalMethod) {
diff --git a/src/codec/SkGifCodec.h b/src/codec/SkGifCodec.h
index b5f8f7ce0c..9d8e0e46bf 100644
--- a/src/codec/SkGifCodec.h
+++ b/src/codec/SkGifCodec.h
@@ -62,19 +62,8 @@ private:
      *
      * @param dstInfo         Contains the requested dst color type.
      * @param frameIndex      Frame whose color table to use.
-     * @param inputColorPtr   Copies the encoded color table to the client's
-     *                        input color table if the client requests kIndex8.
-     * @param inputColorCount If the client requests kIndex8, this will be set
-     *                        to the number of colors in the array that
-     *                        inputColorPtr now points to. This will typically
-     *                        be 256. Since gifs may have up to 8-bit indices,
-     *                        using a 256-entry table means a pixel will never
-     *                        be out of range. This will be set to 1 if there
-     *                        is no color table, since that will be a
-     *                        transparent frame.
      */
-    void initializeColorTable(const SkImageInfo& dstInfo, size_t frameIndex,
-            SkPMColor* colorPtr, int* inputColorCount);
+    void initializeColorTable(const SkImageInfo& dstInfo, size_t frameIndex);
 
    /*
     * Does necessary setup, including setting up the color table and swizzler,
diff --git a/tests/CodecAnimTest.cpp b/tests/CodecAnimTest.cpp
index fee6879a90..de36ce65d0 100644
--- a/tests/CodecAnimTest.cpp
+++ b/tests/CodecAnimTest.cpp
@@ -29,6 +29,7 @@ DEF_TEST(Codec_frames, r) {
         { "box.gif", 1, {}, {} },
         { "color_wheel.gif", 1, {}, {} },
         { "test640x479.gif", 4, { 0, 1, 2 }, { 200, 200, 200, 200 } },
+        { "colorTables.gif", 2, { 0 }, { 1000, 1000 } },
 
         { "arrow.png",  1, {}, {} },
         { "google_chrome.ico", 1, {}, {} },
@@ -67,7 +68,7 @@ DEF_TEST(Codec_frames, r) {
 
         if (rec.fRequiredFrames.size() + 1 != expected) {
             ERRORF(r, "'%s' has wrong number entries in fRequiredFrames; expected: %i\tactual: %i",
-                   rec.fName, expected, rec.fRequiredFrames.size());
+                   rec.fName, expected, rec.fRequiredFrames.size() + 1);
             continue;
         }