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..3787634ae1425383879e9e190426e25eca318196 100644
--- a/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
+++ b/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
@@ -70,34 +70,85 @@ 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() ||
+      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()) {
     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.
+
+  // Pass kN32 and decodeColorSpace to the decoder.  That is what we can expect
+  // output to be.
+  sk_sp<SkColorSpace> decodeColorSpace = getInfo().refColorSpace();
+  SkImageInfo decodeInfo =
+      dstInfo.makeColorType(kN32_SkColorType).makeColorSpace(decodeColorSpace);

[scroggo_chromium, 2017/04/04 20:52:45]

You can drop the makeColorType piece, since it will always be kN32. (OTOH, I
suppose it is needed once you add back in F16 support.)

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

Thanks, removing.

   PlatformInstrumentation::willDecodeLazyPixelRef(uniqueID());
-  bool decoded = m_frameGenerator->decodeAndScale(
-      m_data.get(), m_allDataReceived, m_frameIndex, getInfo(), pixels,
+  bool success = m_frameGenerator->decodeAndScale(
+      m_data.get(), m_allDataReceived, m_frameIndex, decodeInfo, pixels,
       rowBytes);
   PlatformInstrumentation::didDecodeLazyPixelRef();
 
-  return decoded;
+  if (!success) {
+    return false;
+  }
+
+  if (decodeColorSpace && dstInfo.colorSpace() &&
+      !SkColorSpace::Equals(decodeColorSpace.get(), dstInfo.colorSpace())) {
+    std::unique_ptr<SkColorSpaceXform> xform =
+        SkColorSpaceXform::New(decodeColorSpace.get(), dstInfo.colorSpace());
+
+    uint32_t* row = (uint32_t*)pixels;
+    for (int y = 0; y < dstInfo.height(); y++) {
+      // Any premultiplication by the decoder was in the src color space.  Which
+      // means that we must start by unpremultiplying in the src color space.
+      if (kPremul_SkAlphaType == dstInfo.alphaType()) {
+        for (int x = 0; x < dstInfo.width(); x++) {
+          row[x] = SkUnPreMultiply::UnPreMultiplyPreservingByteOrder(row[x]);
+        }
+      }
+
+      SkColorSpaceXform::ColorFormat format =
+          SkColorSpaceXform::kRGBA_8888_ColorFormat;
+      if (kN32_SkColorType == kBGRA_8888_SkColorType) {
+        format = SkColorSpaceXform::kBGRA_8888_ColorFormat;
+      }
+      SkAlphaType alphaType =
+          dstInfo.isOpaque() ? kOpaque_SkAlphaType : kUnpremul_SkAlphaType;
+      bool success =

[scroggo_chromium, 2017/04/04 20:52:45]

This shadows a variable in the larger scope, and is only used for debugging.
Maybe there is a better way to do this?

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

Done.

+          xform->apply(format, row, format, row, dstInfo.width(), alphaType);
+      DCHECK(success);
+
+      // To be compatible with dst space blending, premultiply in the dst space.
+      if (kPremul_SkAlphaType == dstInfo.alphaType()) {
+        for (int x = 0; x < dstInfo.width(); x++) {
+          row[x] =
+              SkPreMultiplyARGB(SkGetPackedA32(row[x]), SkGetPackedR32(row[x]),
+                                SkGetPackedG32(row[x]), SkGetPackedB32(row[x]));
+        }
+      }
+
+      row = (uint32_t*)(((uint8_t*)row) + rowBytes);
+    }
+  }
+
+  return success;

[scroggo_chromium, 2017/04/04 20:52:45]

I think this can be

  return true;

If success was false, we would have already returned above (before the color
xform conversion).

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

Done.  "return success" gives us "false" on a failed color xform (when DCHECKS
are off).  I think this is fine though.

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

I don't think so. The nested variable success gets set to false on a failed
color xform, but the shadowed version will remain unchanged.

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

Ack.  I meant to not redeclare it...  I think as it is now is better.

 }
 
 bool DecodingImageGenerator::onQueryYUV8(SkYUVSizeInfo* sizeInfo,