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..fabe263e66328b49a1163dc543710b66ab633993 100644
--- a/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
+++ b/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
@@ -70,34 +70,89 @@ SkData* DecodingImageGenerator::onRefEncodedData(GrContext* ctx) {
   return m_data->getAsSkData().release();
 }
 
-bool DecodingImageGenerator::onGetPixels(const SkImageInfo& info,
-                                         void* pixels,
-                                         size_t rowBytes,
-                                         SkPMColor table[],
-                                         int* tableCount) {
+bool DecodingImageGenerator::onGetPixels(const SkImageInfo& dstInfo,
+                                         void* dstPixels,
+                                         size_t dstRowBytes,
+                                         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.
-    return false;
+  sk_sp<SkColorSpace> decodeColorSpace = getInfo().refColorSpace();
+  switch (dstInfo.colorType()) {
+    case kN32_SkColorType:
+      break;
+    case kRGBA_F16_SkColorType:
+      if (!decodeColorSpace || !dstInfo.colorSpace() ||
+          !dstInfo.colorSpace()->gammaIsLinear()) {
+        return false;
+      }
+      break;
+    default:
+      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.
+
+  void* decodePixels = dstPixels;
+  size_t decodeRowBytes = dstRowBytes;
+  std::unique_ptr<uint32_t[]> tmp = nullptr;
+  if (kRGBA_F16_SkColorType == dstInfo.colorType()) {
+    // Since the decoder does not support F16, we'll need to decode into a
+    // temporary buffer, then convert that to F16.
+    tmp.reset(new uint32_t[dstInfo.width() * dstInfo.height()]);
+    decodePixels = tmp.get();
+    decodeRowBytes = sizeof(uint32_t) * dstInfo.width();
+  }
+
+  // Might as well pass kN32 and decodeColorSpace to the decoder.  That's what
+  // the output is always going to be anyway.
+  SkImageInfo decodeInfo =
+      dstInfo.makeColorType(kN32_SkColorType).makeColorSpace(decodeColorSpace);
   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, decodePixels,
+      decodeRowBytes);
   PlatformInstrumentation::didDecodeLazyPixelRef();
 
-  return decoded;
+  if (!decoded) {
+    return false;
+  }
+
+  if (kRGBA_F16_SkColorType == dstInfo.colorType() ||
+      (decodeColorSpace && dstInfo.colorSpace() &&
+       !SkColorSpace::Equals(decodeColorSpace.get(), dstInfo.colorSpace()))) {

[f(malita), 2017/03/31 19:52:07]

It reads a bit strange for this conditional to be different from the one setting
up the temp buffer above - looks like we can land here with decodePixels ==
dstPixels, is that ok?

I think it would be easier to follow if we made the decision once (bool
requiresColorSpaceConversion = ...) and then stick to it.

[msarett1, 2017/03/31 21:47:30]

Yeah I can see how this is confusing.  I'm dropping F16 support from this CL
altogether... I think I was getting a little greedy :).

+    // FIXME:

[f(malita), 2017/03/31 19:52:07]

Nit: Chromium style is // TODO(msarett): ...

[msarett1, 2017/03/31 21:47:30]

Done.

+    // Why can't we take this out of the "if" condition and just assert it?
+    // DCHECK(dstInfo.colorSpace());
+
+    SkPixmap dst(dstInfo, dstPixels, dstRowBytes);
+    SkPixmap src(decodeInfo, decodePixels, decodeRowBytes);
+
+    // FIXME:

[f(malita), 2017/03/31 19:52:07]

Nit: ditto

[msarett1, 2017/03/31 21:47:30]

Done.

+    // In the case of images with alpha, the decoder will premultiply them in
+    // the |src| color space.  In order to do this xform correctly, we will need
+    // to unpremultiply in |src| color space, and then premultiply again in the
+    // |dst| color space.  The Skia API for this conversion (non-linear

[msarett1, 2017/03/31 19:05:28]

I'll see if I can make this public.

Regardless, this is a performance problem.  Potential fixes:
(1) Support kUnpremul decodes in ImageDecoder
(2) Pass the dstColorSpace through to ImageDecoder and respect it.
(3) Switch to SkCodec.

(3) is the least throw-away work because that's the plan anyway.  It will also
probably take the longest.  SkCodec also helps with the linear premultiply vs.
non-linear premultiply problem because it supports both.

[ccameron, 2017/03/31 19:46:08]

Doing (2) may be possible ... I'm looking to see about the order of creation of
things.

Also, would it be possible to just run an SkColorSpaceXform directly over the
pixels here?

[msarett1, 2017/03/31 19:59:10]

I looked at (2) first, then got scared and backed off.  I'll probably talk with
Leon (or someone who knows this code better).  That may be a better approach.
Yeah that would be fine (except it doesn't support unpremul then re-premul).

That's pretty much what SkPixmap::readPixels() will do anyway.

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

FWIW, kUnpremul in ImageDecoder is already supported (see
ImageDecoder::AlphaNotPremultiplied). That means you do not need to
unpremultiply first (and then I'm guessing you need to premultiply after
conversion since the client wants premultiplied?).

I can believe (2) is scary, and it becomes throw-away with (3). This approach
seems like the fastest way to implement the correct behavior, though it's
probably not as fast as (2).

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

Thanks for the unpremul tip, not sure how I missed that.

+    // premultiplies and unpremultiplies) is not public. In the use case where
+    // we want linear blending (which means linear premultiplies), this works
+    // correctly as is.  Of course, it will assume that the premultiply
+    // performed by the image decoder was linear (which it was not).
+    bool xformed = src.readPixels(dst);
+    DCHECK(xformed);

[ccameron, 2017/03/31 19:46:08]

This DCHECK fires for the test image attached at
https://bugs.chromium.org/p/chromium/issues/detail?id=706613#c6

I'll start digging in to see why.

[f(malita), 2017/03/31 19:52:07]

Nit: return xformed?

[msarett1, 2017/03/31 19:59:10]

SkPixmap::readPixels() has some restrictions on transfer functions that I forgot
about.  I could make it more flexible, but this might not be the right API to
use here.

[msarett1, 2017/03/31 21:47:30]

I still like the DCHECK, but yeah I think so.

+  }
+
+  return true;
 }
 
 bool DecodingImageGenerator::onQueryYUV8(SkYUVSizeInfo* sizeInfo,