Treat invalid indices as transparent in gifs

Other browsers (including old Chrome) treat invalid palette indices as
transparent for gifs. And there are gifs in the wild which rely on this.

As an example, if the palette only has 64 entries (0-63) then index 64
is treated as transparent.

BUG=skia:7069

Change-Id: I15e8919a953387506c9ac5945c3ae6a2b90189ab
Reviewed-on: https://skia-review.googlesource.com/51100
Reviewed-by: Leon Scroggins <scroggo@google.com>
Commit-Queue: Chris Blume <cblume@google.com>

2 files changed, 12 insertions, 38 deletions

diff --git a/third_party/gif/SkGifImageReader.cpp b/third_party/gif/SkGifImageReader.cpp
index 990da5618b..658e08348f 100644
--- a/third_party/gif/SkGifImageReader.cpp
+++ b/third_party/gif/SkGifImageReader.cpp
@@ -96,6 +96,16 @@ mailing address.
 // Get a 16-bit value stored in little-endian format.
 #define GETINT16(p)   ((p)[1]<<8|(p)[0])
 
+namespace {
+    bool is_palette_index_valid(int transparentIndex) {
+        // -1 is a signal that there is no transparent index.
+        // Otherwise, it is encoded in 8 bits, and all 256 values are considered
+        // valid since a GIF may use an index outside of the palette to be
+        // transparent.
+        return transparentIndex >= 0;
+    }
+} // anonymous namespace
+
 // Send the data to the display front-end.
 void SkGIFLZWContext::outputRow(const unsigned char* rowBegin)
 {
@@ -752,9 +762,7 @@ SkCodec::Result SkGifImageReader::parse(SkGifImageReader::SkGIFParseQuery query)
             if (currentFrameIsFirstFrame()) {
                 const int transPix = m_frames.empty() ? SkGIFColorMap::kNotFound
                                                       : m_frames[0]->transparentPixel();
-                if (this->hasTransparency(transPix,
-                        isLocalColormapDefined, numColors))
-                {
+                if (is_palette_index_valid(transPix)) {
                     m_firstFrameHasAlpha = true;
                 } else {
                     const bool frameIsSubset = xOffset > 0 || yOffset > 0
@@ -846,30 +854,6 @@ SkCodec::Result SkGifImageReader::parse(SkGifImageReader::SkGIFParseQuery query)
     }
 }
 
-bool SkGifImageReader::hasTransparency(int transparentPixel, bool isLocalColormapDefined,
-                                       int localColors) const {
-    const int globalColors = m_globalColorMap.numColors();
-    if (!isLocalColormapDefined && globalColors == 0) {
-        // No color table for this frame, so it is completely transparent.
-        return true;
-    }
-
-    if (transparentPixel < 0) {
-        SkASSERT(SkGIFColorMap::kNotFound == transparentPixel);
-        return false;
-    }
-
-    if (isLocalColormapDefined) {
-        return transparentPixel < localColors;
-    }
-
-    // If there is a global color table, it will be parsed before reaching
-    // here. If its numColors is set, it will be defined.
-    SkASSERT(globalColors > 0);
-    SkASSERT(m_globalColorMap.isDefined());
-    return transparentPixel < globalColors;
-}
-
 void SkGifImageReader::addFrameIfNecessary()
 {
     if (m_frames.empty() || m_frames.back()->isComplete()) {
@@ -899,8 +883,7 @@ bool SkGIFFrameContext::onReportsAlpha() const {
     // Note: We could correct these after decoding - i.e. some frames may turn out to be
     // independent and opaque if they do not use the transparent pixel, but that would require
     // checking whether each pixel used the transparent index.
-    return m_owner->hasTransparency(this->transparentPixel(),
-            m_localColorMap.isDefined(), m_localColorMap.numColors());
+    return is_palette_index_valid(this->transparentPixel());
 }
 
 void SkFrameHolder::setAlphaAndRequiredFrame(SkFrame* frame) {
diff --git a/third_party/gif/SkGifImageReader.h b/third_party/gif/SkGifImageReader.h
index 5b9cdd5ec8..d1063dd843 100644
--- a/third_party/gif/SkGifImageReader.h
+++ b/third_party/gif/SkGifImageReader.h
@@ -198,7 +198,6 @@ class SkGIFFrameContext : public SkFrame {
 public:
     SkGIFFrameContext(SkGifImageReader* reader, int id)
         : INHERITED(id)
-        , m_owner(reader)
         , m_transparentPixel(SkGIFColorMap::kNotFound)
         , m_dataSize(0)
         , m_progressiveDisplay(false)
@@ -251,9 +250,6 @@ protected:
     bool onReportsAlpha() const override;
 
 private:
-    // Unowned pointer to the object that owns this frame.
-    const SkGifImageReader* m_owner;
-
     int m_transparentPixel; // Index of transparent pixel. Value is kNotFound if there is no transparent pixel.
     int m_dataSize;
 
@@ -361,11 +357,6 @@ public:
 
     bool firstFrameHasAlpha() const { return m_firstFrameHasAlpha; }
 
-    // Helper function that returns whether an SkGIFFrameContext has transparency.
-    // This method is sometimes called before creating one/parsing its color map,
-    // so it cannot rely on SkGIFFrameContext::transparentPixel or ::localColorMap().
-    bool hasTransparency(int transPix, bool hasLocalColorMap, int localMapColors) const;
-
 protected:
     const SkFrame* onGetFrame(int i) const override {
         return static_cast<const SkFrame*>(this->frameContext(i));