Respect colorSpace in DecodingImageGenerator::onGetPixels()

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

 /*
  * Copyright (C) 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 52 matching lines @@
   // this. skbug.com/5485
   if (ctx && !m_allDataReceived)
     return nullptr;
 
   // Other clients are serializers, which want the data even if it requires
   // copying, and even if the data is incomplete. (Otherwise they would
   // potentially need to decode the partial image in order to re-encode it.)
   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())
-    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 (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 (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.

 }
 
 bool DecodingImageGenerator::onQueryYUV8(SkYUVSizeInfo* sizeInfo,
                                          SkYUVColorSpace* colorSpace) const {
   // YUV decoding does not currently support progressive decoding. See comment
   // in ImageFrameGenerator.h.
   if (!m_canYUVDecode || !m_allDataReceived)
     return false;
 
   TRACE_EVENT1("blink", "DecodingImageGenerator::queryYUV8", "sizes",
@@ skipping 34 matching lines @@
       ColorBehavior::transformToGlobalTarget());
   if (!decoder || !decoder->isSizeAvailable())
     return nullptr;
 
   const IntSize size = decoder->size();
   const SkImageInfo info =
       SkImageInfo::MakeN32(size.width(), size.height(), kPremul_SkAlphaType,
                            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;
 
   return new DecodingImageGenerator(frame, info, segmentReader.release(), true,
                                     0);
 }
 
 }  // namespace blink