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 0f68eb0e58d9711a1da64cd48cd0f5cc12e83b03..fab7420ce310b1ad670c3cb4427fde3f75310129 100644
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
@@ -26,14 +26,12 @@
 #ifndef GIFImageDecoder_h
 #define GIFImageDecoder_h
 
+#include "codec/SkCodec.h"

[scroggo_chromium, 2016/12/13 16:57:37]

I thought these were typically included as

#include "third_party/skia/include/codec/SkCodec.h"

[cblume, 2016/12/14 09:16:56]

Done.

+#include "core/SkStream.h"
 #include "platform/image-decoders/ImageDecoder.h"
 #include "wtf/Noncopyable.h"
 #include <memory>
 
-class GIFImageReader;
-
-typedef Vector<unsigned char> GIFRow;
-
 namespace blink {
 
 // This class decodes the GIF image format.
@@ -47,56 +45,67 @@ class PLATFORM_EXPORT GIFImageDecoder final : public ImageDecoder {
                   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 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;
-
   // 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 {

[scroggo_chromium, 2016/12/13 16:57:37]

Add a constructor to initialize m_isDone and m_position?

[cblume, 2016/12/14 09:16:57]

Done.

+   public:
+    void setReader(SegmentReader* reader, bool isDone) {
+      m_reader.reset(reader);
+      m_isDone = isDone;
+    }
+    size_t read(void* dst, size_t len) {
+      if (!dst) {  // skipping, not reading
+        m_position += len;

[scroggo_chromium, 2016/12/13 16:57:37]

I think this should only advance to the end of the reader. e.g.

  const size_t bytesSkipped = std::min(len, m_reader->size());
  m_position += bytesSkipped;
  return bytesSkipped;

(Currently SkGifCodec does not do any skipping, but I have an in progress CL
that could. Looking back at it, I call SkStream::move() instead, but it could
easily call read(nullptr, len).)

[cblume, 2016/12/14 09:16:57]

I agree. In fact, I should probably clamp len to always be <= m_reader->size() -
m_position (not just when skipping).

I can also use this to set m_isDone, which should reflect the read hitting the
end (not just the end of the buffer coming in).

+        return len;
+      }
+
+      size_t totalBytesRead = 0;
+      intptr_t destAsInt = reinterpret_cast<intptr_t>(dst);
+      while (len) {
+        const char* segment;
+        size_t bytesRead = m_reader->getSomeData(segment, len);
+        if (!bytesRead) {
+          return totalBytesRead;  // or break;
+        }
+        if (bytesRead > len) {
+          bytesRead = len;
+        }

[cblume, 2016/12/14 09:16:57]

I'm not sure we need to check if getSomeData() returned more than we asked for.
It seems like the contract is if we ask for more than is available it returns 0.

[scroggo_chromium, 2016/12/14 17:49:23]

getSomeData returning more than we asked for is the opposite of asking for more
than is available. SegmentReader is based on SharedBuffer, and the SharedBuffer
implementation forwards this call directly to its SharedBuffer. Both classes are
designed to have blocks of data that can be peered into. So the size returned
corresponds to how much data is contiguous starting from the position you asked
for.

Which made me look at your call to getSomeData - you want to pass m_position as
the second parameter. Once you do that, you'll see that the SegmentReader does
not even know how many bytes you wanted. So you definitely need to make this
check.

[cblume, 2016/12/16 02:57:10]

Done.

+        memcpy(reinterpret_cast<void*>(destAsInt), segment, bytesRead);
+        destAsInt += bytesRead;
+        len -= bytesRead;
+        totalBytesRead += bytesRead;
+      }
+      return totalBytesRead;
+    }
+
+    bool isAtEnd() const override { return m_isDone; }

[scroggo_chromium, 2016/12/13 16:57:37]

What does isDone mean? The fact that it is passed in with the SegmentReader
makes it sound like it tells us whether allDataIsReceived. But isAtEnd() means
we've actually read all the way to the end, and the next read will fail because
there's no more data.

(As it happens, SkCodec never checks isAtEnd.)

[cblume, 2016/12/14 09:16:57]

Thank you for catching this.
I've fixed it to correctly reflect whether the data has all been read.

+
+   private:
+    std::unique_ptr<SegmentReader> m_reader;

[scroggo_chromium, 2016/12/13 16:57:37]

SegmentReader is a ref counted class. Don't you want to make this a RefPtr?

[cblume, 2016/12/14 09:16:56]

Done.

+    size_t m_position;
+    bool m_isDone;
+  };
+
+  std::unique_ptr<SkCodec> m_codec;
+  std::unique_ptr<SegmentStream> m_segmentStream;
 };
 
 }  // namespace blink