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

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

 /*
  * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 13 matching lines @@
  */
 
 #ifndef GIFImageDecoder_h
 #define GIFImageDecoder_h
 
 #include "platform/image-decoders/ImageDecoder.h"
 #include "wtf/Noncopyable.h"
 #include "wtf/OwnPtr.h"
 
 class GIFImageReader;
+struct GIFFrameContext;
 
 typedef Vector<unsigned char> GIFRow;
 
 namespace blink {
 
 // This class decodes the GIF image format.
 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 };
 
     // ImageDecoder:
     String filenameExtension() const override { return "gif"; }
     void onSetData(SharedBuffer* data) override;
     int repetitionCount() const override;
     bool frameIsCompleteAtIndex(size_t) const override;
     float frameDurationAtIndex(size_t) const override;
     size_t clearCacheExceptFrame(size_t) 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 (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;
     virtual void decodeSize() { parse(GIFSizeQuery); }
     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);
 
-    // 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
 
 #endif