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,
+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,
                                          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