color: Enable color conversion in image decoder caches

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..412b7f7f331f0695e3187fb4e7d2642ca2956701 100644
--- a/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
+++ b/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
@@ -96,8 +96,46 @@ bool DecodingImageGenerator::onGetPixels(const SkImageInfo& info,
       m_data.get(), m_allDataReceived, m_frameIndex, getInfo(), pixels,
       rowBytes);
   PlatformInstrumentation::didDecodeLazyPixelRef();
+  if (!decoded)
+    return false;
 
-  return decoded;
+  // Apply color space transformation if requested. Do not fail the decode if

[msarett1, 2017/04/10 15:27:21]

Why not?  When is it ok to fail?

[ccameron, 2017/04/10 20:04:06]

This is to match existing behavior -- see for instance this one
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...

Or any of the other places we call ColorTransform() in the decoders -- we treat
"couldn't parse the color space" the same as "didn't have a color space" and we
also treat "couldn't apply the xform" as "nothing bad happened".

(that's what's happening in your https://codereview.chromium.org/2787053004/).

[msarett1, 2017/04/10 21:02:53]

Acknowledged.

+  // the color transform fails.
+  if (info.colorSpace() &&
+      !SkColorSpace::Equals(info.colorSpace(), getInfo().colorSpace())) {
+    SkColorSpaceXform::ColorFormat xformColorFormat =
+        SkColorSpaceXform::kRGBA_8888_ColorFormat;
+    switch (info.colorType()) {
+      case kRGBA_8888_SkColorType:
+        xformColorFormat = SkColorSpaceXform::kRGBA_8888_ColorFormat;
+        break;
+      case kBGRA_8888_SkColorType:
+        xformColorFormat = SkColorSpaceXform::kBGRA_8888_ColorFormat;
+        break;
+      case kRGBA_F16_SkColorType:

[msarett1, 2017/04/10 15:27:21]

This case feels a little strange here.  Supporting 8888->F16 might make more
sense because the decoder can currently output 8888.

But it may be best to leave this out.

[ccameron, 2017/04/10 20:04:06]

(rebased on your patch instead)

+        xformColorFormat = SkColorSpaceXform::kRGBA_F16_ColorFormat;
+        break;
+      case kRGB_565_SkColorType:

[msarett1, 2017/04/10 15:27:21]

Let's not support 565 in Chrome.  This ColorFormat exists as a concession to
Android, not because it's a good idea to use 565 for color correct rendering (or
any rendering).

[ccameron, 2017/04/10 20:04:06]

(rebased on your patch instead)

+        xformColorFormat = SkColorSpaceXform::kBGR_565_ColorFormat;
+        break;
+      default:
+        // Not supported.
+        return true;
+    }
+    std::unique_ptr<SkColorSpaceXform> xform =
+        SkColorSpaceXform::New(getInfo().colorSpace(), info.colorSpace());
+    if (!xform)
+      return true;
+    uint8_t* row = reinterpret_cast<uint8_t*>(pixels);
+    for (int y = 0; y < info.height(); y++) {
+      bool applyResult = xform->apply(xformColorFormat, row, xformColorFormat,
+                                      row, info.width(), info.alphaType());

[msarett1, 2017/04/10 15:27:21]

This won't handle alpha correctly.  kOpaque will be fine.

Passing kPremul here says: "Do a premultiply".  This doesn't make sense because:
(1) The image is already premultiplied.
(2) This will premultiply in a linear space.

[ccameron, 2017/04/10 20:04:06]

(rebased on your patch instead)

+      DCHECK(applyResult);
+      row += rowBytes;
+    }
+  }
+
+  return true;
 }
 
 bool DecodingImageGenerator::onQueryYUV8(SkYUVSizeInfo* sizeInfo,