color: Enable color conversion in image decoder caches

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 78 matching lines @@
     // request for opaque, that is ok even if our initial alpha type was not
     // opaque.
     return false;
   }
 
   PlatformInstrumentation::willDecodeLazyPixelRef(uniqueID());
   bool decoded = m_frameGenerator->decodeAndScale(
       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,
                                          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 44 matching lines @@
       ImageFrameGenerator::create(SkISize::Make(size.width(), size.height()),
                                   false, decoder->colorBehavior());
   if (!frame)
     return nullptr;
 
   return new DecodingImageGenerator(frame, info, segmentReader.release(), true,
                                     0);
 }
 
 }  // namespace blink