Respect colorSpace in DecodingImageGenerator::onGetPixels()

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 caff5e4cca0f89744ffc0e7a61b000555e3b248f..2fa4597ec567fb37836b1ad97553bd318a1e706f 100644
--- a/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
+++ b/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
@@ -111,7 +111,8 @@ ImageFrameGenerator::ImageFrameGenerator(const SkISize& fullSize,
       m_isMultiFrame(isMultiFrame),
       m_decodeFailed(false),
       m_yuvDecodingFailed(false),
-      m_frameCount(0) {}
+      m_frameCount(0),
+      m_alphaOption(ImageDecoder::AlphaPremultiplied) {}
 
 ImageFrameGenerator::~ImageFrameGenerator() {
   ImageDecodingStore::instance().removeCacheIndexedByGenerator(this);
@@ -295,8 +296,7 @@ bool ImageFrameGenerator::decode(SegmentReader* data,
       *decoder = m_imageDecoderFactory->create().release();
 
     if (!*decoder) {
-      *decoder = ImageDecoder::create(data, allDataReceived,
-                                      ImageDecoder::AlphaPremultiplied,
+      *decoder = ImageDecoder::create(data, allDataReceived, m_alphaOption,
                                       m_decoderColorBehavior)
                      .release();
       // The newly created decoder just grabbed the data.  No need to reset it.
@@ -307,6 +307,8 @@ bool ImageFrameGenerator::decode(SegmentReader* data,
       return false;
   }
 
+  DCHECK((*decoder)->frameBufferAtIndex(index)->premultiplyAlpha() ==

[msarett1, 2017/04/04 23:02:36]

I'm expecting the DCHECK() to warn us if we've already created a decoder with
the wrong alpha behavior.

[scroggo_chromium, 2017/04/05 19:59:01]

frameBufferAtIndex may trigger a decode, so you don't want to call it inside
DCHECK. (And you don't want to trigger the decode until later, either.) And
frameBufferAtIndex might return null, for example in the case where we haven't
set the data yet.

Instead, perhaps calling setAlphaOption should tell the ImageFrameGenerator to
use a different ImageDecoder? Maybe the AlphaOption should be added to the key
for the cache of ImageDecoder in ImageDecodingStore::lockDecoder?

(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...)

Come to think of it, would it make sense to move code you're adding here instead
of in DecodingImageGenerator? You already pass an SkImageInfo to the
ImageFrameGenerator inside decodeAndScale. It uses the original SkImageInfo
instead of the dstInfo, but maybe it could use the dstInfo and
ImageFrameGenerator can decide how to handle the conversion?

[msarett1, 2017/04/06 22:05:29]

This all sgtm.  Thanks for your thoughts.

+         (m_alphaOption == ImageDecoder::AlphaPremultiplied));
 
   if (shouldCallSetData)
     (*decoder)->setData(data, allDataReceived);