Fix decoding GIF to 565

565 cannot take the !writeTransparentPixels path, so disable it for
cases where we might have to take that path.

This only affects frames beyond the first. If the first frame has
a transparent pixel, it will be marked as non-opaque, so we cannot
decode to 565 anyway.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2441833002

Review-Url: https://codereview.chromium.org/2441833002

4 files changed, 75 insertions, 18 deletions

diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp
index 30185a2ab1..4b297c1e10 100644
--- a/dm/DMSrcSink.cpp
+++ b/dm/DMSrcSink.cpp
@@ -473,6 +473,13 @@ Error CodecSrc::draw(SkCanvas* canvas) const {
                         }
                         break;
                     }
+                    case SkCodec::kInvalidConversion:
+                        if (i > 0 && (decodeInfo.colorType() == kRGB_565_SkColorType
+                                      || decodeInfo.colorType() == kIndex_8_SkColorType)) {
+                            return Error::Nonfatal(SkStringPrintf(
+                                "Cannot decode frame %i to 565/Index8 (%s).", i, fPath.c_str()));
+                        }
+                        // Fall through.
                     default:
                         return SkStringPrintf("Couldn't getPixels for frame %i in %s.",
                                               i, fPath.c_str());
diff --git a/src/codec/SkGifCodec.cpp b/src/codec/SkGifCodec.cpp
index 876d71f553..cb36cbf464 100644
--- a/src/codec/SkGifCodec.cpp
+++ b/src/codec/SkGifCodec.cpp
@@ -175,15 +175,37 @@ SkCodec::Result SkGifCodec::prepareToDecode(const SkImageInfo& dstInfo, SkPMColo
     }
 
     const size_t frameIndex = opts.fFrameIndex;
-    if (frameIndex > 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
-        // 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, but
-        // we do not (currently) need to support later frames as index8.
-        return gif_error("Cannot decode multiframe gif (except frame 0) as index 8.\n",
-                         kInvalidConversion);
+    if (frameIndex > 0) {
+        switch (dstInfo.colorType()) {
+            case kIndex_8_SkColorType:
+                // FIXME: It is possible that a later frame can be decoded to index8, if it does one
+                // of the 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,
+                // but we do not (currently) need to support later frames as index8.
+                return gif_error("Cannot decode multiframe gif (except frame 0) as index 8.\n",
+                                 kInvalidConversion);
+            case kRGB_565_SkColorType:
+                // FIXME: In theory, we might be able to support this, but it's not clear that it
+                // is necessary (Chromium does not decode to 565, and Android does not decode
+                // frames beyond the first). Disabling it because it is somewhat difficult:
+                // - If there is a transparent pixel, and this frame draws on top of another frame
+                //   (if the frame is independent with a transparent pixel, we should not decode to
+                //   565 anyway, since it is not opaque), we need to skip drawing the transparent
+                //   pixels (see writeTransparentPixels in haveDecodedRow). We currently do this by
+                //   first swizzling into temporary memory, then copying into the destination. (We
+                //   let the swizzler handle it first because it may need to sample.) After
+                //   swizzling to 565, we do not know which pixels in our temporary memory
+                //   correspond to the transparent pixel, so we do not know what to skip. We could
+                //   special case the non-sampled case (no need to swizzle), but as this is
+                //   currently unused we can just not support it.
+                return gif_error("Cannot decode multiframe gif (except frame 0) as 565.\n",
+                                 kInvalidConversion);
+            default:
+                break;
+        }
     }
 
     fReader->parse((SkGifImageReader::SkGIFParseQuery) frameIndex);
@@ -489,7 +511,7 @@ bool SkGifCodec::haveDecodedRow(size_t frameIndex, const unsigned char* rowBegin
     // we must write these for passes beyond the first, or the initial passes
     // will "show through" the later ones.
     const auto dstInfo = this->dstInfo();
-    if (writeTransparentPixels || dstInfo.colorType() == kRGB_565_SkColorType) {
+    if (writeTransparentPixels) {
         fSwizzler->swizzle(dstLine, rowBegin);
     } else {
         // We cannot swizzle directly into the dst, since that will write the transparent pixels.
diff --git a/third_party/gif/SkGifImageReader.cpp b/third_party/gif/SkGifImageReader.cpp
index c40c27203e..eeaee68c1d 100644
--- a/third_party/gif/SkGifImageReader.cpp
+++ b/third_party/gif/SkGifImageReader.cpp
@@ -361,7 +361,7 @@ sk_sp<SkColorTable> SkGifImageReader::getColorTable(SkColorType colorType, size_
 // Perform decoding for this frame. frameComplete will be true if the entire frame is decoded.
 // Returns false if a decoding error occurred. This is a fatal error and causes the SkGifImageReader to set the "decode failed" flag.
 // Otherwise, either not enough data is available to decode further than before, or the new data has been decoded successfully; returns true in this case.
-bool SkGIFFrameContext::decode(SkGifCodec* client, bool* frameComplete)
+bool SkGIFFrameContext::decode(SkGifCodec* client, const SkGIFColorMap& globalMap, bool* frameComplete)
 {
     *frameComplete = false;
     if (!m_lzwContext) {
@@ -370,7 +370,7 @@ bool SkGIFFrameContext::decode(SkGifCodec* client, bool* frameComplete)
             return true;
 
         m_lzwContext.reset(new SkGIFLZWContext(client, this));
-        if (!m_lzwContext->prepareToDecode()) {
+        if (!m_lzwContext->prepareToDecode(globalMap)) {
             m_lzwContext.reset();
             return false;
         }
@@ -403,7 +403,7 @@ bool SkGifImageReader::decode(size_t frameIndex, bool* frameComplete)
 {
     SkGIFFrameContext* currentFrame = m_frames[frameIndex].get();
 
-    return currentFrame->decode(m_client, frameComplete);
+    return currentFrame->decode(m_client, m_globalColorMap, frameComplete);
 }
 
 // Parse incoming GIF data stream into internal data structures.
@@ -890,7 +890,7 @@ void SkGifImageReader::addFrameIfNecessary()
 }
 
 // FIXME: Move this method to close to doLZW().
-bool SkGIFLZWContext::prepareToDecode()
+bool SkGIFLZWContext::prepareToDecode(const SkGIFColorMap& globalMap)
 {
     SkASSERT(m_frameContext->isDataSizeDefined() && m_frameContext->isHeaderDefined());
 
@@ -906,8 +906,35 @@ bool SkGIFLZWContext::prepareToDecode()
     datum = bits = 0;
     ipass = m_frameContext->interlaced() ? 1 : 0;
     irow = 0;
-    alwaysWriteTransparentPixels = !m_frameContext->interlaced()
-            && m_frameContext->getRequiredFrame() == SkCodec::kNone;
+    alwaysWriteTransparentPixels = false;
+    if (m_frameContext->getRequiredFrame() == SkCodec::kNone) {
+        if (!m_frameContext->interlaced()) {
+            alwaysWriteTransparentPixels = true;
+        } else {
+            // The frame is interlaced, so we do not want to write transparent
+            // pixels. But if there are no transparent pixels anyway, there is
+            // no harm in taking the alwaysWriteTransparentPixels path, which
+            // is faster, and it also supports 565.
+            // Since the frame is independent, it does not matter whether the
+            // frame is subset (nothing behind it needs to show through). So we
+            // only need to know whether there is a valid transparent pixel.
+            // This is a little counterintuitive - we want to "always write
+            // transparent pixels" if there ARE NO transparent pixels, so we
+            // check to see whether the pixel index is >= numColors.
+            const auto& localMap = m_frameContext->localColorMap();
+            const auto trans = m_frameContext->transparentPixel();
+            if (localMap.isDefined()) {
+                alwaysWriteTransparentPixels = trans >= localMap.numColors();
+            } else {
+                // Note that if the map is not defined, the value of
+                // alwaysWriteTransparentPixels is meaningless, since without
+                // any color table, we will skip drawing entirely.
+                // FIXME: We could even skip calling prepareToDecode in that
+                // case, meaning we can SkASSERT(globalMap.isDefined())
+                alwaysWriteTransparentPixels = trans >= globalMap.numColors();
+            }
+        }
+    }
 
     // We want to know the longest sequence encodable by a dictionary with
     // SK_MAX_DICTIONARY_ENTRIES entries. If we ignore the need to encode the base
diff --git a/third_party/gif/SkGifImageReader.h b/third_party/gif/SkGifImageReader.h
index fee1a5f3c7..5936350681 100644
--- a/third_party/gif/SkGifImageReader.h
+++ b/third_party/gif/SkGifImageReader.h
@@ -86,6 +86,7 @@ enum SkGIFState {
 };
 
 struct SkGIFFrameContext;
+class SkGIFColorMap;
 
 // LZW decoder state machine.
 class SkGIFLZWContext final : public SkNoncopyable {
@@ -108,7 +109,7 @@ public:
         , m_frameContext(frameContext)
     { }
 
-    bool prepareToDecode();
+    bool prepareToDecode(const SkGIFColorMap& globalMap);
     bool outputRow(const unsigned char* rowBegin);
     bool doLZW(const unsigned char* block, size_t bytesInBlock);
     bool hasRemainingRows() { return SkToBool(rowsRemaining); }
@@ -210,7 +211,7 @@ public:
         m_lzwBlocks.push_back(SkData::MakeWithCopy(data, size));
     }
 
-    bool decode(SkGifCodec* client, bool* frameDecoded);
+    bool decode(SkGifCodec* client, const SkGIFColorMap& globalMap, bool* frameDecoded);
 
     int frameId() const { return m_frameId; }
     void setRect(unsigned x, unsigned y, unsigned width, unsigned height)