Move gif image decoder to SkCodec

third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h

Index: third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
diff --git a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
index e84b1b488dc443a73ab341e658cac98e49b771a7..ba13071acc260e78f5a494679497e2fc4d16f684 100644
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
@@ -26,13 +26,13 @@
 #ifndef GIFImageDecoder_h
 #define GIFImageDecoder_h
 
+#include <algorithm>
+#include <memory>
 #include "platform/image-decoders/ImageDecoder.h"
+#include "third_party/skia/include/codec/SkCodec.h"
+#include "third_party/skia/include/core/SkStream.h"
 #include "wtf/Noncopyable.h"
-#include <memory>
-
-class GIFImageReader;
-
-typedef Vector<unsigned char> GIFRow;
+#include "wtf/RefPtr.h"
 
 namespace blink {
 
@@ -44,56 +44,126 @@ class PLATFORM_EXPORT GIFImageDecoder final : public ImageDecoder {
   GIFImageDecoder(AlphaOption, const ColorBehavior&, size_t maxDecodedBytes);
   ~GIFImageDecoder() override;
 
-  enum GIFParseQuery { GIFSizeQuery, GIFFrameCountQuery };
-
   // ImageDecoder:
   String filenameExtension() const override { return "gif"; }
   void onSetData(SegmentReader* data) override;
   int repetitionCount() const override;
   bool frameIsCompleteAtIndex(size_t) const override;
   float frameDurationAtIndex(size_t) const override;
-  // CAUTION: setFailed() deletes |m_reader|.  Be careful to avoid
-  // accessing deleted memory, especially when calling this from inside
-  // GIFImageReader!
-  bool setFailed() override;
-
-  // Callbacks from the GIF reader.
-  bool haveDecodedRow(size_t frameIndex,
-                      GIFRow::const_iterator rowBegin,
-                      size_t width,
-                      size_t rowNumber,
-                      unsigned repeatCount,
-                      bool writeTransparentPixels);
-  bool frameComplete(size_t frameIndex);
-
-  // For testing.
-  bool parseCompleted() const;
 
  private:
   // ImageDecoder:
-  void clearFrameBuffer(size_t frameIndex) override;
-  virtual void decodeSize() { parse(GIFSizeQuery); }
+  void decodeSize() override {}
   size_t decodeFrameCount() override;
-  void initializeNewFrame(size_t) override;
+  void initializeNewFrame(size_t index) override;
   void decode(size_t) override;
-
-  // Parses as much as is needed to answer the query, ignoring bitmap
-  // data. If parsing fails, sets the "decode failure" flag.
-  void parse(GIFParseQuery);
-
-  // Reset the alpha tracker for this frame. Before calling this method, the
-  // caller must verify that the frame exists.
-  void onInitFrameBuffer(size_t) override;
-
+  bool frameStatusSufficientForSuccessors(size_t index) {
+    DCHECK(index < m_frameBufferCache.size());
+    return m_frameBufferCache[index].getStatus() == ImageFrame::FrameComplete;
+  }
   // When the disposal method of the frame is DisposeOverWritePrevious, the
   // next frame will use the previous frame's buffer as its starting state, so
   // we can't take over the data in that case. Before calling this method, the
   // caller must verify that the frame exists.
   bool canReusePreviousFrameBuffer(size_t) const override;
 
-  bool m_currentBufferSawAlpha;
-  mutable int m_repetitionCount;
-  std::unique_ptr<GIFImageReader> m_reader;
+  class SegmentStream : public SkStream {
+   public:
+    SegmentStream() : m_reader(), m_position(0), m_hasReadAllContents(true) {}
+
+    void setReader(SegmentReader* reader, bool allContentsReceived) {
+      m_reader = reader;
+      if (reader)
+        m_hasReadAllContents = reader->size() == m_position;

[cblume, 2017/03/07 09:00:11]

I need some help here.

With just this, I get this assert:
SkStreamBuffer.cpp:36: fatal error: "assert(bytesRead == bytesToBuffer)"

The problem with this assertion is it happens inside SkStreamBuffer::get(). It
has bytesToBuffer == 1. But when it calls read() it cannot read anything. So
even though it tries to read 1 byte, it reads 0. Hence bytesRead !=
bytesToBuffer.

Makes sense, since we aren't doing anything when reader is null.

So I added an else case to this if branch which sets m_position = 0. Setting
m_hasReadAllContents to either true or false didn't make a difference. But this
gives a different assertion: SkStreamBuffer.cpp:80: fatal error:
"assert(position + length <= fStream->getLength())"

The problem in this case is position and length are parameters passed in.
Position doesn't seem to have gotten reset. So after a few reads, position +
length is past the end of the buffer.

[scroggo_chromium, 2017/03/07 21:35:08]

I don't think I have enough context here. If there's no reader, why are we
calling get()?

[cblume, 2017/03/08 11:36:11]

That is a very good question.
I'll look into this more and try to get an answer. I think this knowledge gap I
have is the cause of a test failing.
$ out/Debug/blink_platform_unittests --test-launcher-bot-mode
--gtest_filter=DeferredImageDecoderTestWoPlatform.mixImagesGif
triggers the behavior I described.

GIFImageDecoder::onSetData with m_segmentStream->setReader(nullptr, false);
caused it.

simply returning (which I used to do before your code review comments) also
caused it.

But when I mistakenly set m_segmentStream = nullptr; the test passed.

I'll investigate tomorrow.

+    }
+
+    size_t read(void* dst, size_t len) {
+      len = std::min(len, m_reader->size() - m_position);
+
+      size_t bytesAdvanced = 0;
+      if (!dst) {  // skipping, not reading
+        bytesAdvanced = len;
+      } else {
+        bytesAdvanced = peek(dst, len);
+      }
+
+      m_position += bytesAdvanced;
+      m_hasReadAllContents = m_position == m_reader->size();
+
+      return bytesAdvanced;
+    }
+
+    size_t peek(void* buffer, size_t size) const override {
+      size = std::min(size, m_reader->size() - m_position);
+
+      size_t peekPosition = m_position;
+      size_t totalBytesPeeked = 0;
+      char* bufferAsCharPtr = reinterpret_cast<char*>(buffer);
+      while (size) {
+        const char* segment = nullptr;
+        size_t bytesPeeked = m_reader->getSomeData(segment, peekPosition);
+        if (!bytesPeeked) {
+          break;
+        }
+        if (bytesPeeked > size) {
+          bytesPeeked = size;
+        }
+
+        memcpy(bufferAsCharPtr, segment, bytesPeeked);
+        bufferAsCharPtr += bytesPeeked;
+        size -= bytesPeeked;
+        totalBytesPeeked += bytesPeeked;
+        peekPosition += bytesPeeked;
+      }
+
+      return totalBytesPeeked;
+    }
+
+    bool isAtEnd() const override { return m_hasReadAllContents; }
+
+    bool rewind() override {
+      m_position = 0;
+      return true;
+    }
+
+    bool hasPosition() const override { return true; }
+    size_t getPosition() const override { return m_position; }
+
+    bool seek(size_t position) override {
+      position = std::min(position, m_reader->size());
+      m_position = position;
+
+      return true;
+    }
+
+    bool move(long offset) override {
+      long absolutePosition = m_position + offset;
+
+      // clamp inside the bounds of the buffer size
+      absolutePosition = std::max(absolutePosition, 0l);
+      absolutePosition =
+          std::min(static_cast<size_t>(absolutePosition), m_reader->size());
+
+      m_position = absolutePosition;
+      m_hasReadAllContents = m_position == m_reader->size();
+
+      return true;
+    }
+
+    bool hasLength() const override { return true; }
+    size_t getLength() const override { return m_reader->size(); }
+
+   private:
+    WTF::RefPtr<SegmentReader> m_reader;
+    size_t m_position;
+    bool m_hasReadAllContents;
+  };
+
+  std::unique_ptr<SkCodec> m_codec;
+  // m_segmentStream is a raw pointer because it passes ownership to
+  // m_codec when m_codec is created. However, we still need the
+  // pointer so we can append more data as it arrives.
+  SegmentStream* m_segmentStream;
 };
 
 }  // namespace blink