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 db42c9e42fbad562c22abf726963f7fbac6fcff6..a2fbaec6990a89adb0520ac5deb355bd9256493e 100644
--- a/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
+++ b/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
@@ -70,34 +70,84 @@ SkData* DecodingImageGenerator::onRefEncodedData(GrContext* ctx) {
   return data_->GetAsSkData().release();
 }
 
-bool DecodingImageGenerator::onGetPixels(const SkImageInfo& info,
+bool DecodingImageGenerator::onGetPixels(const SkImageInfo& dst_info,
                                          void* pixels,
                                          size_t row_bytes,
-                                         SkPMColor table[],
-                                         int* table_count) {
+                                         SkPMColor*,
+                                         int*) {
   TRACE_EVENT1("blink", "DecodingImageGenerator::getPixels", "frame index",
                static_cast<int>(frame_index_));
 
   // 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 (dst_info.dimensions() != getInfo().dimensions()) {
     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 != dst_info.colorType()) {

[Nico, 2017/04/11 15:41:05]

no yoda conditionals please

[msarett1, 2017/04/11 20:52:05]

Done.

     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.
+  SkColorSpace* decode_color_space = getInfo().colorSpace();
+  SkImageInfo decode_info =
+      dst_info.makeColorSpace(sk_ref_sp(decode_color_space));
+
+  const bool needs_color_xform =
+      decode_color_space && dst_info.colorSpace() &&

[ccameron, 2017/04/11 19:03:10]

This will have no functional changes because dst_info.colorSpace() is nullptr
unless --enable-color-correct-rendering is specified.

[Nico, 2017/04/11 19:07:48]

Pasting this bit from above: """I think the process also applies for changes
happening behind flags. You
normally send an "intent to implement" very early on, then implement your thing
behind a flag (usually behind a single global 'give me new web stuff' flag), and
then you "intent to ship" when you're ready to move it out from behind a flag.
This is designed so that not a lot of work is spent implementing something that
isn't web-friendly, and also to give web devs a chance to chime in before a lot
of implementation work has happened. I think in this case it's a pretty simple
feature, but it should still go through the process as far as I understand."""

+      !SkColorSpace::Equals(decode_color_space, dst_info.colorSpace());
+  ImageDecoder::AlphaOption alpha_option = ImageDecoder::kAlphaPremultiplied;
+  if (needs_color_xform && !decode_info.isOpaque()) {
+    alpha_option = ImageDecoder::kAlphaNotPremultiplied;
+    decode_info = decode_info.makeAlphaType(kUnpremul_SkAlphaType);
+  }
+
   PlatformInstrumentation::WillDecodeLazyPixelRef(uniqueID());
   bool decoded = frame_generator_->DecodeAndScale(
-      data_.Get(), all_data_received_, frame_index_, getInfo(), pixels,
-      row_bytes);
+      data_.Get(), all_data_received_, frame_index_, decode_info, pixels,
+      row_bytes, alpha_option);
   PlatformInstrumentation::DidDecodeLazyPixelRef();
 
-  return decoded;
+  if (!decoded || !needs_color_xform) {
+    return decoded;
+  }
+
+  std::unique_ptr<SkColorSpaceXform> xform =
+      SkColorSpaceXform::New(decode_color_space, dst_info.colorSpace());
+
+  uint32_t* row = reinterpret_cast<uint32_t*>(pixels);
+  for (int y = 0; y < dst_info.height(); y++) {

[Nico, 2017/04/11 15:41:05]

This function is getting very long. Consider extracting the loop into a
well-named helper. (Guideline: Every chunk in a function should operate on the
same level of abstraction, and a loop over pixels feels lower-level than the
set-up for it)

[msarett1, 2017/04/11 20:52:05]

Done.  I think you're right, this works better as a helper.

+    SkColorSpaceXform::ColorFormat format =
+        SkColorSpaceXform::kRGBA_8888_ColorFormat;
+    if (kN32_SkColorType == kBGRA_8888_SkColorType) {
+      format = SkColorSpaceXform::kBGRA_8888_ColorFormat;
+    }
+    SkAlphaType alpha_type =
+        dst_info.isOpaque() ? kOpaque_SkAlphaType : kUnpremul_SkAlphaType;
+    bool xformed =
+        xform->apply(format, row, format, row, dst_info.width(), alpha_type);
+    DCHECK(xformed);
+
+    // To be compatible with dst space blending, premultiply in the dst space.
+    if (kPremul_SkAlphaType == dst_info.alphaType()) {
+      for (int x = 0; x < dst_info.width(); x++) {
+        row[x] =
+            SkPreMultiplyARGB(SkGetPackedA32(row[x]), SkGetPackedR32(row[x]),
+                              SkGetPackedG32(row[x]), SkGetPackedB32(row[x]));
+      }
+    }
+
+    row = reinterpret_cast<uint32_t*>(
+        (reinterpret_cast<uint8_t*>(row) + row_bytes));
+  }
+
+  return true;
 }
 
 bool DecodingImageGenerator::onQueryYUV8(SkYUVSizeInfo* size_info,