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..cd22c2e2da256de80d0a8f3a4594adad33262c97 100644
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
@@ -27,13 +27,13 @@
 #define GIFImageDecoder_h
 
 #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 "wtf/RefPtr.h"
+#include <algorithm>
 #include <memory>
 
-class GIFImageReader;
-
-typedef Vector<unsigned char> GIFRow;
-
 namespace blink {
 
 // This class decodes the GIF image format.
@@ -47,56 +47,83 @@ 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 {
+   public:
+    SegmentStream()
+        : m_reader(),
+          m_position(0),
+          m_hasReadAllContents(true) {}
+
+    void setReader(SegmentReader* reader, bool allContentsReceived) {
+      m_reader = reader;
+      m_position = 0;

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

Why does this reset the position? You'll be receiving a new SegmentReader which
has the full data from the start, so you want to keep reading from the same
position.

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

Yeah, that was a mistake. I removed it.

+      m_hasReadAllContents = false;

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

This assumes you will never be given a SegmentReader of the same size you've
seen before. That may be true, but I don't think it's guaranteed.
ImageFrameGenerator::decode pulls an ImageDecoder out of its cache and sets the
data. I could imagine that we are re-decoding an earlier frame after receiving
all the data, in which case we might already be at the end.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

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

We might be thinking of different things. Perhaps I have the wrong mental model
here.

I intended to treat this differently from isAllDataReceived().
I was intending this to be used to reflect how much of the received data has
actually been read (decoded).

In that case, would a new image decoder with reused data still need to start
from the beginning while decoding? I imagine so.

[scroggo_chromium, 2016/12/16 15:03:48]

Yes, but that would be determined by m_position, which should be zero in that
case. I think rather than setting this to false you should check:

  m_hasReadAllContents = reader->size() == m_position

[cblume, 2016/12/16 17:43:41]

Done.

+    }
+
+    size_t read(void* dst, size_t len) {
+      len = std::min(len, m_reader->size());
+
+      size_t bytesAdvanced = 0;
+      if (!dst) {  // skipping, not reading
+        bytesAdvanced = len;
+      } else {
+        size_t totalBytesRead = 0;
+        intptr_t destAsInt = reinterpret_cast<intptr_t>(dst);

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

I haven't seen this pattern before, and do not see it anywhere in chromium. (The
only instance I see of "intptr_t" is here [1].)

According to [2], intptr_t may not be defined anyway.

[1]
https://cs.chromium.org/chromium/src/sandbox/win/src/internal_types.h?dr=CSs&...
[2] http://www.cplusplus.com/reference/cstdint/

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

Hrmmm you are right, it is optional.
We had used the unsigned version before:
https://cs.chromium.org/chromium/src/third_party/skia/src/image/SkImage_Gpu.c...

It is a way to avoid pointer aliasing and being able to do math on the pointer.
But I can use char* instead.
C/C++ have special rules that char* does not alias and I can do pointer
arithmetic on it.

+        while (len) {
+          const char* segment = nullptr;
+          size_t bytesRead = m_reader->getSomeData(segment, len);

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

This came up while I was reviewing the first patch set, but I wanted to note it
here - you want to use m_position here.

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

Done.

+          if (!bytesRead) {
+            break;
+          }
+
+          memcpy(reinterpret_cast<void*>(destAsInt), segment, bytesRead);
+          destAsInt += bytesRead;
+          len -= bytesRead;
+          totalBytesRead += bytesRead;
+        }
+
+        bytesAdvanced = totalBytesRead;
+      }
+
+      m_position += bytesAdvanced;
+      m_hasReadAllContents = m_position == m_reader->size();
+
+      return bytesAdvanced;
+    }
+
+    bool isAtEnd() const override { return m_hasReadAllContents; }
+
+   private:
+    WTF::RefPtr<SegmentReader> m_reader;
+    size_t m_position;
+    bool m_hasReadAllContents;
+  };
+
+  std::unique_ptr<SkCodec> m_codec;
+  SegmentStream* m_segmentStream;
+  // m_segmentStream is a raw pointer because it passes ownership to
+  // m_codec when m_codec is created. However, we still need the
+  // reference so we can append more data as it arrives.
 };
 
 }  // namespace blink