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..5433ca8c9bab4ef12dcbcb51ef5d94ff8143c930 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() ||

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

nit: you can call dimensions() for both, cutting this down to one line.

[msarett1, 2017/04/10 21:34:26]

Done.

+      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 decodeColorSpace to the decoder.  That is what we can expect the
+  // output to be.
+  sk_sp<SkColorSpace> decodeColorSpace = getInfo().refColorSpace();

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

nit: Why do you call refColorSpace (and store in an sk_sp) instead of colorSpace
(and store in a raw pointer)? I think the SkImageGenerator will hang on to it
for the lifetime of this method, so there is no need for you to take a ref?

I see that makeColorSpace takes an sk_sp, rather than a raw pointer, so if you
were to use a raw pointer, you would have to call

  dstInfo.makeColorSpace(sk_ref_sp(decodeColorSpace));

IIUC, that would save a call to ref/unref (although this probably won't have a
big impact on performance?).

[msarett1, 2017/04/10 21:34:26]

You're right, done.

+  SkImageInfo decodeInfo = dstInfo.makeColorSpace(decodeColorSpace);
+
+  const bool needsColorXform =
+      decodeColorSpace && dstInfo.colorSpace() &&
+      !SkColorSpace::Equals(decodeColorSpace.get(), dstInfo.colorSpace());
+  ImageDecoder::AlphaOption alphaOption = ImageDecoder::AlphaPremultiplied;
+  if (needsColorXform && !decodeInfo.isOpaque()) {
+    alphaOption = ImageDecoder::AlphaNotPremultiplied;
+    decodeInfo = decodeInfo.makeAlphaType(kUnpremul_SkAlphaType);
+  }
+
   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, decodeInfo, pixels,
+      rowBytes, alphaOption);
   PlatformInstrumentation::didDecodeLazyPixelRef();
 
-  return decoded;
+  if (!decoded) {
+    return false;
+  }
+
+  if (needsColorXform) {

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

nit/suggestion: If you changed the above if to

  if (!decoded || !needsColorXform) {
    return decoded;
  }

you can save a level of indentation, which might allow you to put more code on
one line.

[msarett1, 2017/04/10 21:34:26]

Done.

+    std::unique_ptr<SkColorSpaceXform> xform =
+        SkColorSpaceXform::New(decodeColorSpace.get(), dstInfo.colorSpace());
+
+    uint32_t* row = (uint32_t*)pixels;

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

C++ style cast. Same down below.

[msarett1, 2017/04/10 21:34:26]

Done.

+    for (int y = 0; y < dstInfo.height(); y++) {
+      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 xformed =
+          xform->apply(format, row, format, row, dstInfo.width(), alphaType);
+      DCHECK(xformed);
+
+      // 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 true;
 }
 
 bool DecodingImageGenerator::onQueryYUV8(SkYUVSizeInfo* sizeInfo,