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

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

Index: third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h
diff --git a/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h b/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h
index 05388ef8f7154c63814a8deec632e04155820730..dfee0fe16c9172da9366fd348b80d7ab150b6702 100644
--- a/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h
+++ b/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h
@@ -168,7 +168,7 @@ public:
 
     // Decodes as much of the requested frame as possible, and returns an
     // ImageDecoder-owned pointer.
-    ImageFrame* frameBufferAtIndex(size_t);
+    ImageFrame* frameBufferAtIndex(size_t, ImageFrame::ColorType = ImageFrame::N32);

[scroggo_chromium, 2015/12/03 21:47:21]

It seems odd to me that the client asks for a particular color type. Does that
mean this might do a copy? (i.e. what happens if it already decoded that frame
to a different type?) Why doesn't the ImageDecoder just return the frame that it
has?

[aleksandar.stojiljkovic, 2015/12/04 00:07:34]

1st Reason is in discussion we had here:
https://codereview.chromium.org/1495693003/#msg8

Main reason though, once 1st reason is fixed, is for testing - wanted to keep
rgb decode having previous functionality and 1) keep it working and 2)compare
rgba-index8 in tests.

There is no copy, as you pointed out - the use case with frame decoded to one
color type and then asked in another is not existing, even in tests. I just
didn't add checks / asserts there. test use one decoder for all frames to rgba
or to "index9 + unsupported index8 frames in rgba)

Splitting to test method would help and some of the decode to this / can decode
to that would go away after changing skia API.

 
     // Whether the requested frame has alpha.
     virtual bool frameHasAlphaAtIndex(size_t) const;
@@ -292,6 +292,10 @@ public:
     virtual bool canDecodeToYUV() { return false; }
     virtual bool decodeToYUV() { return false; }
     virtual void setImagePlanes(PassOwnPtr<ImagePlanes>) { }
+    virtual bool canDecodeTo(size_t index, ImageFrame::ColorType outputType)
+    {
+        return outputType == ImageFrame::N32;
+    }
 
 protected:
     // Calculates the most recent frame whose image data may be needed in
@@ -328,6 +332,15 @@ protected:
 
     // Decodes the requested frame.
     virtual void decode(size_t) = 0;
+    // Decodes the requested frame to specified color type output.
+    virtual void decodeTo(size_t index, ImageFrame::ColorType outputColor)

[scroggo_chromium, 2015/12/03 21:47:21]

If we need to pass outputColor, why not merge this with decode?

[aleksandar.stojiljkovic, 2015/12/04 00:07:34]

Didn't want to change code in other decoders, that is why it is just wrapping
them for N32 (currently only supported output).

+    {
+        if (!canDecodeTo(index, outputColor))

[scroggo_chromium, 2015/12/03 21:47:21]

So the subclass can override decodeTo and not call canDecodeTo?

[aleksandar.stojiljkovic, 2015/12/04 00:07:34]

This would keep the existing decoders unchanged, just change it for gif decoder.

+            return;

[scroggo_chromium, 2015/12/03 21:47:21]

So this will silently fail?

[aleksandar.stojiljkovic, 2015/12/04 00:07:34]

If there is no frame in cache, ImageFrameGenerator::decode calling this would
return false since the frame is empty.
If there is frame in cache of wrong type it should release assert if frame is
already cached and of wrong type - and not here but in
ImageDecoder::frameBufferAtIndex since it is from there calling this only if
frame is not complete.

+        // this is the default implementation that is just wrapping decode().
+        // subclasses need to override this and canDecodeTo for supported colorTypes.
+        decode(index);
+    }
 
     RefPtr<SharedBuffer> m_data; // The encoded data.
     Vector<ImageFrame, 1> m_frameBufferCache;