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..08774de2b7e4a7895fb458c28af6e0f93961e660 100644
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
@@ -31,6 +31,7 @@
 #include "wtf/OwnPtr.h"
 
 class GIFImageReader;
+struct GIFFrameContext;
 
 typedef Vector<unsigned char> GIFRow;
 
@@ -40,7 +41,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);
     ~GIFImageDecoder() override;
 
     enum GIFParseQuery { GIFSizeQuery, GIFFrameCountQuery };
@@ -58,12 +59,28 @@ 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);
+    bool (GIFImageDecoder::*phaveDecodedRow)(const GIFFrameContext&, GIFRow::const_iterator rowBegin, size_t rowNumber, unsigned repeatCount, bool writeTransparentPixels);

[scroggo_chromium, 2016/01/06 21:50:41]

Does this pointer need to be public?

Also, maybe there should be a typedef? Or I think C++11 "using" matches the
style guide?

Lastly, this name does not match the style guide. Typically, member variables
start with "m_". I'm not sure how Chromium names function pointers. I've seen a
"p" at the start elsewhere, but not in Chromium. Maybe this should be something
like "m_haveDecodedRowFunc"?

[aleksandar.stojiljkovic, 2016/01/18 13:58:50]

Done.
All private, typedef, m_XXXXFunction found e.g. in Timer.h. 
typedef it is - looks less ugly then using with class scope.

+    inline bool haveDecodedRow(const GIFFrameContext& frameContext, GIFRow::const_iterator rowBegin, size_t rowNumber, unsigned repeatCount, bool writeTransparentPixels)
+    {
+        return (this->*phaveDecodedRow)(frameContext, rowBegin, rowNumber, repeatCount, 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);
+
+    // Callback implementations for different output.
+    bool haveDecodedRowN32(const GIFFrameContext&, GIFRow::const_iterator rowBegin, size_t rowNumber, unsigned repeatCount, bool writeTransparentPixels);

[scroggo_chromium, 2016/01/06 21:50:41]

Can these be private?

[aleksandar.stojiljkovic, 2016/01/18 13:58:50]

Done.

+    bool haveDecodedRowIndex8(const GIFFrameContext&, GIFRow::const_iterator rowBegin, size_t rowNumber, unsigned repeatCount, bool writeTransparentPixels);
+
     // 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 +89,28 @@ private:
     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);
 
-    // 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 char getBackgroundIndex(const GIFFrameContext&) const;

[scroggo_chromium, 2016/01/06 21:50:41]

I think you defined this as an unsigned short in GIFImageReader. Why the
difference?

[aleksandar.stojiljkovic, 2016/01/18 13:58:50]

Done. unsigned everywhere.

+    SkColor getBackgroundColor(size_t frameIndex) const;
+    bool initFrameBufferFromPreviousN32(ImageFrame* const buffer, const ImageFrame& previousBuffer);
+    void setupHaveDecodedRowCallbacks(bool isIndex8);
 
     bool m_currentBufferSawAlpha;
     mutable int m_repetitionCount;
     OwnPtr<GIFImageReader> m_reader;
+
+    // Holds information about which decoding output was requested in constructor.

[scroggo_chromium, 2016/01/06 21:50:41]

If this always matches the constructor, I think it should be const and have a
more descriptive name. Maybe m_requestedColorMode?

[aleksandar.stojiljkovic, 2016/01/18 13:58:50]

Done.

+    // N32 color mode is used for testing only.
+    ImageFrame::ColorType m_colorMode;
+
 };
 
 } // namespace blink