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;
+typedef Vector<blink::ImageFrame::PixelData> GIFColorTable;
 
 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);

[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 };
 
     // 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);
+    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;
     virtual void decodeSize() { parse(GIFSizeQuery); }
     size_t decodeFrameCount() override;
     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
 
 #endif