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

third_party/WebKit/Source/platform/image-decoders/ImageFrame.h

Index: third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
diff --git a/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h b/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
index b5d1490fa484e330e0b3ce1084a75bf873d456e1..a9a8d6c2b06160522af663331ee061950e22a2d1 100644
--- a/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
+++ b/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
@@ -30,6 +30,7 @@
 #include "platform/PlatformExport.h"
 #include "platform/geometry/IntRect.h"
 #include "third_party/skia/include/core/SkBitmap.h"
+#include "third_party/skia/include/core/SkColorPriv.h"

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

Why is this needed? I don't think you're using anything from SkColorPriv (and
you shouldn't be, ideally - "Priv" is short for "private").

 #include "wtf/Allocator.h"
 #include "wtf/Assertions.h"
 #include "wtf/PassRefPtr.h"
@@ -68,7 +69,12 @@ public:
         // overwrite the corresponding pixels).
         BlendAtopBgcolor,
     };
+    enum ColorType {
+        N32 = kN32_SkColorType,
+        Index8 = kIndex_8_SkColorType
+    };
     typedef uint32_t PixelData;
+    typedef uint8_t PixelData8;

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

nit: 8 just tells me that this is 8 bit, but not that it is an index. Maybe call
this PixelDataIndex8?

 
     ImageFrame();
 
@@ -87,31 +93,19 @@ public:
     void clearPixelData();
     void zeroFillPixelData();
     void zeroFillFrameRect(const IntRect&);
+    void zeroFillFrameRectIndex8(const IntRect&, unsigned char transparentIndex);

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

I'm not a big fan of this name. Although the intent is to clear to transparent,
arguably this could be used (and potentially is, as far as I can tell) to clear
to a non-zero color. Maybe it would be better named

fillFrameRectIndex8(const IntRect&, unsigned char index);

 
     // Makes this frame have an independent copy of the provided image's
     // pixel data, so that modifications in one frame are not reflected in
     // the other.  Returns whether the copy succeeded.
     bool copyBitmapData(const ImageFrame&);
-
-    // Copies the pixel data at [(startX, startY), (endX, startY)) to the
-    // same X-coordinates on each subsequent row up to but not including
-    // endY.
-    void copyRowNTimes(int startX, int endX, int startY, int endY)
-    {
-        ASSERT(startX < width());
-        ASSERT(endX <= width());
-        ASSERT(startY < height());
-        ASSERT(endY <= height());
-        const int rowBytes = (endX - startX) * sizeof(PixelData);
-        const PixelData* const startAddr = getAddr(startX, startY);
-        for (int destY = startY + 1; destY < endY; ++destY)
-            memcpy(getAddr(startX, destY), startAddr, rowBytes);
-    }
+    bool copyBitmapData(const ImageFrame&, ImageFrame::ColorType);
 
     // Allocates space for the pixel data.  Must be called before any pixels
     // are written.  Must only be called once.  Returns whether allocation
     // succeeded.
-    bool setSize(int newWidth, int newHeight);
+    bool setSize(int newWidth, int newHeight, SkColor background = 0);

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

I think it would be clearer to use SK_ColorTRANSPARENT (a macro for zero) as the
default value.

+    bool setSizeIndex8(int newWidth, int newHeight, const Vector<blink::ImageFrame::PixelData>& colorMap, unsigned backgroundIndex, bool backgroundIsTransparent = true);
 
     // Returns a caller-owned pointer to the underlying native image data.
     // (Actual use: This pointer will be owned by BitmapImage and freed in
@@ -148,6 +142,11 @@ public:
         return m_bitmap.getAddr32(x, y);
     }
 
+    inline PixelData8* getAddr8(int x, int y)
+    {
+        return m_bitmap.getAddr8(x, y);
+    }
+
     inline void setRGBA(int x, int y, unsigned r, unsigned g, unsigned b, unsigned a)
     {
         setRGBA(getAddr(x, y), r, g, b, a);