Respect colorSpace in DecodingImageGenerator::onGetPixels()

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 52 matching lines @@
   // this. skbug.com/5485
   if (ctx && !m_allDataReceived)
     return nullptr;
 
   // Other clients are serializers, which want the data even if it requires
   // copying, and even if the data is incomplete. (Otherwise they would
   // potentially need to decode the partial image in order to re-encode it.)
   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())
-    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 (dstInfo.width() != getInfo().width() ||
+      dstInfo.height() != getInfo().height()) {
     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,
                                          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