GIF decoding to Index8, unit tests and misusing unit test as benchmark

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 3ce2da9d3bbc71324c81ccc2e0559ece8a61b519..9a358cc335a296b2fd0bf3f2cdadfacaa80a9dda 100644
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
@@ -31,8 +31,10 @@
 #include "wtf/OwnPtr.h"
 
 class GIFImageReader;
+struct GIFFrameContext;
 
 typedef Vector<unsigned char> GIFRow;
+typedef Vector<blink::ImageFrame::PixelData> GIFColorTable;
 
 namespace blink {
 
@@ -40,7 +42,7 @@ namespace blink {
 class PLATFORM_EXPORT GIFImageDecoder final : public ImageDecoder {
     WTF_MAKE_NONCOPYABLE(GIFImageDecoder);
 public:
-    GIFImageDecoder(AlphaOption, GammaAndColorProfileOption, size_t maxDecodedBytes);
+    GIFImageDecoder(AlphaOption, GammaAndColorProfileOption, size_t maxDecodedBytes, ImageFrame::ColorType decodeTo = ImageFrame::Index8);

[scroggo_chromium, 2016/04/29 19:48:15]

Since this is always Index8 except for testing, why not use a factory function
with a name ending in ForTesting which sets it to N32? (Normal constructor will
still set to Index8) I think that would be clearer.

     ~GIFImageDecoder() override;
 
     enum GIFParseQuery { GIFSizeQuery, GIFFrameCountQuery };
@@ -58,12 +60,20 @@ public:
     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);
+    void haveDecodedRow(const GIFFrameContext&, GIFRow::const_iterator rowBegin, size_t rowNumber, unsigned repeatCount, bool writeTransparentPixels);
     bool frameComplete(size_t frameIndex);
 
+    // Called to initialize the frame buffer with the given index, based on
+    // the previous frame's disposal method. Returns true on success. On
+    // failure, this will mark the image as failed.
+    bool initFrameBuffer(size_t frameIndex);
+
     // For testing.
     bool parseCompleted() const;
 
+    // For supporting both N32 and Index8 output.
+    bool canDecodeTo(size_t index, ImageFrame::ColorType outputType) override;
+
 private:
     // ImageDecoder:
     void clearFrameBuffer(size_t frameIndex) override;
@@ -72,18 +82,37 @@ private:
     void initializeNewFrame(size_t) override;
     void decode(size_t) override;
 
+

[scroggo_chromium, 2016/04/29 19:48:15]

nit: Please remove this extra line.

     // Parses as much as is needed to answer the query, ignoring bitmap
     // data. If parsing fails, sets the "decode failure" flag.
     void parse(GIFParseQuery);
 
-    // Called to initialize the frame buffer with the given index, based on
-    // the previous frame's disposal method. Returns true on success. On
-    // failure, this will mark the image as failed.
-    bool initFrameBuffer(size_t frameIndex);
+    bool initFrameBufferN32(size_t frameIndex);
+
+    bool isIndex8Applicable(const GIFFrameContext&, size_t requiredPreviousFrameIndex) const;
+    void updateRequiredPreviousFrame(ImageFrame*, const GIFFrameContext&);
+    unsigned getBackgroundIndex(const GIFFrameContext&) const;
+    SkColor getBackgroundColor(size_t frameIndex) const;
+    bool initFrameBufferFromPrevious(ImageFrame* buffer, const ImageFrame& previousBuffer, ImageFrame::ColorType, size_t transparentIndex = 0xFF);
+    void setupHaveDecodedRowCallbacks(bool isIndex8);
+
+    // Callback implementations for different output.
+    void writeRowN32(const GIFFrameContext&, ImageFrame&, const GIFColorTable&, GIFRow::const_iterator, GIFRow::const_iterator, int, int, bool);
+    void writeRowIndex8(const GIFFrameContext&, ImageFrame&, const GIFColorTable&, GIFRow::const_iterator, GIFRow::const_iterator, int, int, bool);
+
+    typedef void (GIFImageDecoder::*WriteRowFunction)(const GIFFrameContext&, ImageFrame&, const GIFColorTable&, GIFRow::const_iterator, GIFRow::const_iterator, int, int, bool);

[scroggo_chromium, 2016/04/29 19:48:15]

Please add parameter names where they are not obvious. (e.g. GIFFrameContext
needs no parameter name, since it will provide no new information. But looking
at this function declaration I have no idea what int, int, bool are)

Same for function pointer on the next line, and for the methods they point to
(e.g. writeRow... above).

Also, should this use "using" instead of typedef?

+    typedef void (*CopyRowNTimesFunction)(ImageFrame&, int, int, int, int);
+
+    WriteRowFunction m_writeRowFunction;
+    CopyRowNTimesFunction m_copyRowNTimesFunction;
 
     bool m_currentBufferSawAlpha;
     mutable int m_repetitionCount;
     OwnPtr<GIFImageReader> m_reader;
+
+    // Important for testing only - it is always Index8 mode except in some tests
+    // where we also use N32 mode to compare with Index8.
+    ImageFrame::ColorType m_requestedColorMode;

[scroggo_chromium, 2016/04/29 19:48:15]

Could be const?

 };
 
 } // namespace blink