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

third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp

Index: third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp b/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
index 09f931bb74b691ffd32a4543fee80a1a121818ab..93655a241feb43d383d36604b06951f56239a043 100644
--- a/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
+++ b/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
@@ -105,6 +105,7 @@ ImageFrameGenerator::ImageFrameGenerator(const SkISize& fullSize, PassRefPtr<Sha
     , m_decodeFailed(false)
     , m_frameCount(0)
     , m_encodedData(nullptr)
+    , m_decoderCanDecodeToIndex8(false)
 {
     setData(data.get(), allDataReceived);
 }
@@ -172,7 +173,30 @@ SkData* ImageFrameGenerator::refEncodedData()
     return m_encodedData;
 }
 
-bool ImageFrameGenerator::decodeAndScale(size_t index, const SkImageInfo& info, void* pixels, size_t rowBytes)
+static bool copyIndex8PixelsTo(const SkBitmap& bitmap, const SkImageInfo& info, void* pixels, size_t rowBytes)

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

Various thoughts:

- I think it would be helpful to name these how they are used, src versus dst.
e.g.

(const SkBitmap& src, const SkImageInfo& dstInfo, void* dstPixels, size_t
dstRowBytes)

You could also wrap the dst into an SkPixmap, so it would be

copyIndex8PixelsTo(const SkBitmap&src, SkPixmap& dst)

- You actually never use info. It might be useful for error checking (must be
N32, must have the same width and height as the bitmap). My preference would be
to drop it and use clear comments/names to indicate how the function is to be
called, but I'm okay with using it in asserts, too. (I assume this method is
intended to mimic SkBitmap::copyPixelsTo, which does not take an SkImageInfo
argument, FWIW.)

- This isn't really a copy. I suggest calling this convertIndex8ToN32. (Another
confusing part about the current name - it sounds like pixels are the source,
but they are the destination.)

+{
+    // This method is called for GIF animations, only for frames that, in first animation loop, get
+    // decoded to N32, but then are decoded to Index8 in later animation loop. It is needed to return
+    // N32 pixels but internally GIF decoder produces and caches Index8 bitmap.
+    if (bitmap.colorType() != kIndex_8_SkColorType)

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

I think this should be an assert statement, rather than an if, since it will
only be called with index8.

Then this method can return void.

+        return false;
+    // GIF decoder always produce colortable with 256 entries.
+    ASSERT(bitmap.getColorTable() && bitmap.getColorTable()->count() == 256);
+
+    const SkPMColor* colors = bitmap.getColorTable()->readColors();
+    uint8_t* destination8 = (uint8_t*)pixels;
+    for (int j = 0; j < bitmap.height(); ++j) {
+        uint32_t* destination32 = (uint32_t*)destination8;
+        const uint8_t* source = bitmap.getAddr8(0, j);
+        const uint8_t* sourceEnd = source + bitmap.width();
+        while (source != sourceEnd)
+            *destination32++ = colors[*source++];
+        destination8 += rowBytes;
+    }
+    return true;
+}
+
+bool ImageFrameGenerator::decodeAndScale(size_t index, const SkImageInfo& info, void* pixels, size_t rowBytes, SkPMColor ctable[], int* ctableCount)
 {
     // Prevent concurrent decode or scale operations on the same image data.
     MutexLocker lock(m_decodeMutex);
@@ -200,10 +224,31 @@ bool ImageFrameGenerator::decodeAndScale(size_t index, const SkImageInfo& info,
     // provided. If not, make a copy.
     ASSERT(bitmap.width() == scaledSize.width());
     ASSERT(bitmap.height() == scaledSize.height());
+
+    // If decoder cannot produce requested color type, return false.
+    if (bitmap.colorType() != info.colorType() && bitmap.colorType() != kIndex_8_SkColorType)
+        return false;
+
+    bool result = true;

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

I don't think this variable is necessary. When you would have set it to false
(only when returning from bitmap.copyPixelsTo; as I suggest above,
copyIndex8PixelsTo should never return false) you can just return false (and
skip checking whether we need to memcpy/set the ctableCount).

     SkAutoLockPixels bitmapLock(bitmap);
-    if (bitmap.getPixels() != pixels)
-        return bitmap.copyPixelsTo(pixels, rowBytes * info.height(), rowBytes);
-    return true;
+    if (bitmap.getPixels() != pixels) {
+        if (bitmap.colorType() == info.colorType()) {
+            result = bitmap.copyPixelsTo(pixels, rowBytes * info.height(), rowBytes);
+        } else {
+            ASSERT(bitmap.colorType() == kIndex_8_SkColorType && info.colorType() == kN32_SkColorType);
+            result = copyIndex8PixelsTo(bitmap, info, pixels, rowBytes);
+        }
+    }
+    if (info.colorType() == kIndex_8_SkColorType && ctable && bitmap.getColorTable()) {
+        memcpy(ctable, bitmap.getColorTable()->readColors(), bitmap.getColorTable()->count() * sizeof(SkPMColor));
+    }
+    if (ctableCount) {
+        // Based on SkImageGenerator API spec, ctableCount needs to be set to 0 for other types.
+        *ctableCount = (info.colorType() == kIndex_8_SkColorType && bitmap.getColorTable())
+            ? bitmap.getColorTable()->count()
+            : 0;
+    }
+    return result;
 }
 
 bool ImageFrameGenerator::decodeToYUV(size_t index, SkISize componentSizes[3], void* planes[3], size_t rowBytes[3])
@@ -419,4 +464,37 @@ bool ImageFrameGenerator::getYUVComponentSizes(SkISize componentSizes[3])
     return updateYUVComponentSizes(decoder.get(), componentSizes, ImageDecoder::SizeForMemoryAllocation);
 }
 
+bool ImageFrameGenerator::canDecodeTo(size_t index, SkColorType outputType)
+{
+    if (!m_decoderCanDecodeToIndex8)
+        return (outputType == kN32_SkColorType);
+
+    // Prevents concurrent decode or scale operations on the same image data.
+    MutexLocker lock(m_decodeMutex);
+    ImageDecoder* decoder = 0;
+    const bool decoderExist = ImageDecodingStore::instance().lockDecoder(this, m_fullSize, &decoder);
+    ASSERT(!decoderExist || decoder);
+    SharedBuffer* data = 0;
+    bool allDataReceived = false;
+    m_data->data(&data, &allDataReceived);
+    if (!decoderExist) {
+        if (m_imageDecoderFactory)
+            decoder = m_imageDecoderFactory->create().leakPtr();
+        if (!decoder)
+            decoder = ImageDecoder::create(*data, ImageDecoder::AlphaPremultiplied, ImageDecoder::GammaAndColorProfileApplied).leakPtr();
+        if (!decoder)
+            return (outputType == kIndex_8_SkColorType);
+    }
+    decoder->setData(data, allDataReceived);
+    bool canDecodeToIndex8 = decoder->canDecodeTo(index, static_cast<ImageFrame::ColorType>(outputType));
+    decoder->setData(0, false); // Unref SharedBuffer from ImageDecoder.
+    if (decoderExist) {
+        ImageDecodingStore::instance().unlockDecoder(this, decoder);
+    } else {
+        ImageDecodingStore::instance().insertDecoder(this, PassOwnPtr<blink::ImageDecoder>(adoptPtr(decoder)));
+    }
+    return (canDecodeToIndex8 && (outputType == kIndex_8_SkColorType));
+}
+
+
 } // namespace blink