Respect colorSpace in DecodingImageGenerator::onGetPixels()

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

Index: third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp b/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
index 78dea66626650db17f55dd143817340770044d42..0ea2d9ebb1434cf1069a011a45bd318e2e1c0be2 100644
--- a/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
+++ b/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
@@ -70,31 +70,33 @@ SkData* DecodingImageGenerator::onRefEncodedData(GrContext* ctx) {
   return m_data->getAsSkData().release();
 }
 
-bool DecodingImageGenerator::onGetPixels(const SkImageInfo& info,
+bool DecodingImageGenerator::onGetPixels(const SkImageInfo& dstInfo,
                                          void* pixels,
                                          size_t rowBytes,
-                                         SkPMColor table[],
-                                         int* tableCount) {
+                                         SkPMColor*,
+                                         int*) {
   TRACE_EVENT1("blink", "DecodingImageGenerator::getPixels", "frame index",
                static_cast<int>(m_frameIndex));
 
   // Implementation doesn't support scaling yet, so make sure we're not given a
   // different size.
-  if (info.width() != getInfo().width() || info.height() != getInfo().height())
+  if (dstInfo.width() != getInfo().width() ||

[scroggo_chromium, 2017/04/07 18:06:06]

nit: I think you originally changed "info" to "dstInfo" since you had another
SkImageInfo in this method. Now that you don't, I suggest you leave this code as
it was. (Either way is fine, in my view, but leaving it unchanged makes git
blame more useful, and the change isn't necessary.)

[msarett1, 2017/04/10 14:42:45]

Acknowledged.

Moving the xform more logic to this function, so I'll leave as is.

+      dstInfo.height() != getInfo().height()) {
     return false;
+  }
 
-  if (info.colorType() != getInfo().colorType()) {
-    // blink::ImageFrame may have changed the owning SkBitmap to
-    // kOpaque_SkAlphaType after fully decoding the image frame, so if we see a
-    // request for opaque, that is ok even if our initial alpha type was not
-    // opaque.
+  if (kN32_SkColorType != dstInfo.colorType()) {

[scroggo_chromium, 2017/04/07 18:06:05]

This change also appears to be unnecessary, I'm guessing. I think
getInfo().colorType() will always be kN32?

[msarett1, 2017/04/10 14:42:45]

Yes, but I think this is more clear.  Regardless of whether or not all
DecodingImageGenerators are created with kN32 as the colorType, this function
only succeeds if kN32 is the dstColorType.

[scroggo_chromium, 2017/04/10 17:31:23]

Fair enough.

     return false;
   }
 
+  // Skip the check for alphaType.  blink::ImageFrame may have changed the
+  // owning SkBitmap to kOpaque_SkAlphaType after fully decoding the image
+  // frame, so if we see a request for opaque, that is ok even if our initial
+  // alpha type was not opaque.
+
   PlatformInstrumentation::willDecodeLazyPixelRef(uniqueID());
   bool decoded = m_frameGenerator->decodeAndScale(
-      m_data.get(), m_allDataReceived, m_frameIndex, getInfo(), pixels,
-      rowBytes);
+      m_data.get(), m_allDataReceived, m_frameIndex, dstInfo, pixels, rowBytes);
   PlatformInstrumentation::didDecodeLazyPixelRef();
 
   return decoded;

[scroggo_chromium, 2017/04/07 18:06:06]

One difference between doing the conversion here versus inside the
ImageFrameGenerator is that in the prior patch set the conversion did not count
towards the time to decode in the trace, but it does in this patch set. Not sure
whether it should count.

[msarett1, 2017/04/10 14:42:45]

I think it should not (for now).  We are typically seeing the conversion being
triggered by code that is drawing SkImage.  Right now it seems more logically to
be part of the draw.

@@ -152,8 +154,7 @@ SkImageGenerator* DecodingImageGenerator::create(SkData* data) {
                            decoder->colorSpaceForSkImages());
 
   RefPtr<ImageFrameGenerator> frame =
-      ImageFrameGenerator::create(SkISize::Make(size.width(), size.height()),
-                                  false, decoder->colorBehavior());
+      ImageFrameGenerator::create(info, false, decoder->colorBehavior());
   if (!frame)
     return nullptr;