Color correct ImageBitmap(HTMLImageElement*) constructor

third_party/WebKit/Source/core/frame/ImageBitmap.cpp

Index: third_party/WebKit/Source/core/frame/ImageBitmap.cpp
diff --git a/third_party/WebKit/Source/core/frame/ImageBitmap.cpp b/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
index 776800c8b50edcf2a1e08e937bc39e32d2611268..6d5d1e07bd965a58c4eb79a4d762211a97967d0c 100644
--- a/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
+++ b/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
@@ -10,6 +10,7 @@
 #include "platform/graphics/skia/SkiaUtils.h"
 #include "platform/image-decoders/ImageDecoder.h"
 #include "third_party/skia/include/core/SkCanvas.h"
+#include "third_party/skia/include/core/SkImageInfo.h"
 #include "third_party/skia/include/core/SkSurface.h"
 #include "wtf/CheckedNumeric.h"
 #include "wtf/PtrUtil.h"
@@ -31,12 +32,16 @@ struct ParsedOptions {
   unsigned resizeHeight = 0;
   IntRect cropRect;
   SkFilterQuality resizeQuality = kLow_SkFilterQuality;
-  // This value should be changed in the future when we support
-  // createImageBitmap with higher bit depth, in the parseOptions() function.
-  // For now, it is always 4.
-  int bytesPerPixel = 4;
+  sk_sp<SkColorSpace> dstColorSpace = nullptr;
+  sk_sp<SkColorSpace> latestColorSpace = nullptr;
+  SkColorType dstColorType = kN32_SkColorType;
+  SkColorType latestColorType = kN32_SkColorType;
 };
 
+ParsedOptions defaultOptions() {
+  return ParsedOptions();
+}
+
 // The following two functions are helpers used in cropImage
 static inline IntRect normalizeRect(const IntRect& rect) {
   return IntRect(std::min(rect.x(), rect.maxX()),
@@ -63,6 +68,36 @@ ParsedOptions parseOptions(const ImageBitmapOptions& options,
            options.premultiplyAlpha() == "premultiply");
   }
 
+  if (options.colorSpaceConversion() != "none") {
+    if (!RuntimeEnabledFeatures::experimentalCanvasFeaturesEnabled() ||
+        !RuntimeEnabledFeatures::colorCorrectRenderingEnabled()) {
+      DCHECK_EQ(options.colorSpaceConversion(), "default");
+      // TODO(zakerinasab): Replace sRGB with a call to
+      // ImageDecoder::globalTargetColorSpace() when the crash problem on Mac
+      // is fixed. crbug.com/668546.
+      if (RuntimeEnabledFeatures::colorCorrectRenderingDefaultModeEnabled()) {
+        parsedOptions.dstColorSpace =
+            SkColorSpace::MakeNamed(SkColorSpace::kSRGB_Named);
+        parsedOptions.dstColorType = SkColorType::kN32_SkColorType;
+      }
+    } else {
+      if (options.colorSpaceConversion() == "default" ||
+          options.colorSpaceConversion() == "srgb") {
+        parsedOptions.dstColorSpace =
+            SkColorSpace::MakeNamed(SkColorSpace::kSRGB_Named);
+        parsedOptions.dstColorType = SkColorType::kN32_SkColorType;
+      } else if (options.colorSpaceConversion() == "linear-rgb") {
+        parsedOptions.dstColorSpace =
+            SkColorSpace::MakeNamed(SkColorSpace::kSRGBLinear_Named);
+        parsedOptions.dstColorType = SkColorType::kRGBA_F16_SkColorType;
+      } else {
+        NOTREACHED()
+            << "Invalid ImageBitmap creation attribute colorSpaceConversion: "
+            << options.colorSpaceConversion();
+      }
+    }
+  }
+
   int sourceWidth = sourceSize.width();
   int sourceHeight = sourceSize.height();
   if (!cropRect) {
@@ -110,7 +145,7 @@ ParsedOptions parseOptions(const ImageBitmapOptions& options,
 bool dstBufferSizeHasOverflow(ParsedOptions options) {

[dcheng, 2016/12/11 20:11:58]

Nit: not introduced by this CL, but this should be const ParsedOptions&

[zakerinasab1, 2016/12/12 18:35:21]

Done.

   CheckedNumeric<unsigned> totalBytes = options.cropRect.width();
   totalBytes *= options.cropRect.height();
-  totalBytes *= options.bytesPerPixel;
+  totalBytes *= SkColorTypeBytesPerPixel(options.latestColorType);
   if (!totalBytes.IsValid())
     return true;
 
@@ -118,7 +153,7 @@ bool dstBufferSizeHasOverflow(ParsedOptions options) {
     return false;
   totalBytes = options.resizeWidth;
   totalBytes *= options.resizeHeight;
-  totalBytes *= options.bytesPerPixel;
+  totalBytes *= SkColorTypeBytesPerPixel(options.latestColorType);
   if (!totalBytes.IsValid())
     return true;
 
@@ -187,14 +222,19 @@ static void swizzleImageData(unsigned char* srcAddr,
   }
 }
 
-static sk_sp<SkImage> flipSkImageVertically(SkImage* input,
-                                            AlphaDisposition alphaOp) {
+static sk_sp<SkImage> flipSkImageVertically(
+    SkImage* input,
+    bool enforceAlphaPremultiply = false,

[dcheng, 2016/12/11 20:11:58]

Ditto about preferring enums over bools here as well: at the callsites, it's not
obvious what behavior the bool controls.

[zakerinasab1, 2016/12/12 18:35:21]

Done.

+    const ParsedOptions& options = defaultOptions()) {
   unsigned width = static_cast<unsigned>(input->width());
   unsigned height = static_cast<unsigned>(input->height());
-  SkImageInfo info = SkImageInfo::MakeN32(input->width(), input->height(),
-                                          (alphaOp == PremultiplyAlpha)
-                                              ? kPremul_SkAlphaType
-                                              : kUnpremul_SkAlphaType);
+  SkAlphaType alphaType = (enforceAlphaPremultiply || options.premultiplyAlpha)
+                              ? kPremul_SkAlphaType
+                              : kUnpremul_SkAlphaType;
+  SkImageInfo info = SkImageInfo::Make(input->width(), input->height(),
+                                       options.latestColorType, alphaType,
+                                       options.latestColorSpace);
+
   unsigned imageRowBytes = width * info.bytesPerPixel();
   RefPtr<Uint8Array> imagePixels = copySkImageData(input, info);
   if (!imagePixels)
@@ -210,9 +250,13 @@ static sk_sp<SkImage> flipSkImageVertically(SkImage* input,
   return newSkImageFromRaster(info, std::move(imagePixels), imageRowBytes);
 }
 
-static sk_sp<SkImage> premulSkImageToUnPremul(SkImage* input) {
-  SkImageInfo info = SkImageInfo::Make(input->width(), input->height(),
-                                       kN32_SkColorType, kUnpremul_SkAlphaType);
+static sk_sp<SkImage> premulSkImageToUnPremul(
+    SkImage* input,
+    const ParsedOptions& options = defaultOptions()) {
+  SkImageInfo info = SkImageInfo::Make(
+      input->width(), input->height(), options.latestColorType,
+      kUnpremul_SkAlphaType, options.latestColorSpace);
+
   RefPtr<Uint8Array> dstPixels = copySkImageData(input, info);
   if (!dstPixels)
     return nullptr;
@@ -221,9 +265,13 @@ static sk_sp<SkImage> premulSkImageToUnPremul(SkImage* input) {
       static_cast<unsigned>(input->width()) * info.bytesPerPixel());
 }
 
-static sk_sp<SkImage> unPremulSkImageToPremul(SkImage* input) {
-  SkImageInfo info = SkImageInfo::Make(input->width(), input->height(),
-                                       kN32_SkColorType, kPremul_SkAlphaType);
+static sk_sp<SkImage> unPremulSkImageToPremul(
+    SkImage* input,
+    const ParsedOptions& options = defaultOptions()) {
+  SkImageInfo info = SkImageInfo::Make(
+      input->width(), input->height(), options.latestColorType,
+      kPremul_SkAlphaType, options.latestColorSpace);
+
   RefPtr<Uint8Array> dstPixels = copySkImageData(input, info);
   if (!dstPixels)
     return nullptr;
@@ -232,15 +280,124 @@ static sk_sp<SkImage> unPremulSkImageToPremul(SkImage* input) {
       static_cast<unsigned>(input->width()) * info.bytesPerPixel());
 }
 
+// This code is borrowed from third_party/skia/src/codec/SkCodecPriv.h
+// If you need to change this, please check SkColorSpaceXform::ColorFormat
+// and SkColorType for proper coverage.
+static inline SkColorSpaceXform::ColorFormat getXformFormat(
+    SkColorType colorType) {
+  switch (colorType) {
+    case kRGBA_8888_SkColorType:
+      return SkColorSpaceXform::kRGBA_8888_ColorFormat;
+    case kBGRA_8888_SkColorType:
+      return SkColorSpaceXform::kBGRA_8888_ColorFormat;
+    case kRGBA_F16_SkColorType:
+      return SkColorSpaceXform::kRGBA_F16_ColorFormat;
+    default:
+      SkASSERT(false);

[dcheng, 2016/12/11 20:11:58]

I think we should be writing NOTREACHED in Blink code.

[zakerinasab1, 2016/12/12 18:35:21]

Done.

+      return SkColorSpaceXform::kRGBA_8888_ColorFormat;
+  }
+}
+
+static inline void updateLatestColorInformation(ParsedOptions& options) {
+  options.latestColorType = options.dstColorType;
+  options.latestColorSpace = options.dstColorSpace;
+}
+
+// TODO (zakrinasab). Rewrite this when SkImage::readPixels() respectes the
+// color space of the passed SkImageInfo (crbug.com/skia/6021) and SkImage
+// exposes SkColorSpace and SkColorType (crbug.com/skia/6022).
+
+static sk_sp<SkImage> applyColorSpaceConversion(const sk_sp<SkImage> image,

[dcheng, 2016/12/11 20:11:58]

This refs the SkImage unnecessarily, as this method does not take ownership of
|image|. Instead, pass the image by SkImage* (similar to
unPremulSkImageToPremul, etc)

(The general guideline in Chromium and Blink is that if ownership is transferred
or taken, pass by smart pointer. Otherwise, pass by raw pointer.)

[zakerinasab1, 2016/12/12 18:35:21]

I changed the function signature. PTAL.
Passing SkImage* and returning sk_sp<SkImage> results in SkAssert(SkRefCnt > 0)
errors. I suspect this might have something to do with in-place color conversion
that we do, i.e., we use the same memory buffer of the input SkImage to write
the new pixel values for the color corrected SkImage.

[dcheng, 2016/12/12 21:13:57]

Ah... another option is to pass by value (instead of const value); then you can
std::move() and be slightly more efficient.

This is fine though.

+                                                ParsedOptions& options) {
+  if (!options.dstColorSpace)
+    return image;
+  if (SkColorSpace::Equals(options.latestColorSpace.get(),
+                           options.dstColorSpace.get()))
+    return image;
+
+  // If we have the color space information of the source image, we can use
+  // SkColorSpaceXform. Otherwise, we need to draw the image on a canvas and
+  // take a snapshot.
+  if (options.latestColorSpace) {
+    SkImageInfo info = SkImageInfo::Make(
+        image->width(), image->height(), options.latestColorType,
+        image->alphaType(), options.latestColorSpace);
+    size_t size = image->width() * image->height() * info.bytesPerPixel();
+    auto srcPixels = WTF::makeUnique<uint8_t>(size);
+    if (!image->readPixels(info, &srcPixels,
+                           image->width() * info.bytesPerPixel(), 0, 0)) {
+      return image;
+    }
+
+    // For in-place color correction, bytes per pixel must be equal for source
+    // and destination color spaces.
+    std::unique_ptr<uint8_t> dstPixels = nullptr;
+    if (SkColorTypeBytesPerPixel(options.dstColorType) !=
+        SkColorTypeBytesPerPixel(options.latestColorType)) {
+      size = image->width() * image->height() *
+             SkColorTypeBytesPerPixel(options.latestColorType);
+      dstPixels = WTF::makeUnique<uint8_t>(size);
+    }
+
+    std::unique_ptr<SkColorSpaceXform> xform = SkColorSpaceXform::New(
+        options.latestColorSpace.get(), options.dstColorSpace.get());
+    xform->apply(getXformFormat(options.dstColorType),
+                 dstPixels ? &dstPixels : &srcPixels,
+                 getXformFormat(options.latestColorType), &srcPixels,
+                 image->width() * image->height(), kUnpremul_SkAlphaType);
+
+    SkImageInfo dstInfo =
+        SkImageInfo::Make(image->width(), image->height(), options.dstColorType,
+                          image->alphaType(), options.dstColorSpace);
+    sk_sp<SkData> data(SkData::MakeWithoutCopy(&dstPixels, size));
+    sk_sp<SkImage> coloredImage = SkImage::MakeRasterData(
+        dstInfo, data, image->width() * dstInfo.bytesPerPixel());
+    if (coloredImage) {
+      updateLatestColorInformation(options);
+      return coloredImage;
+    }
+    return image;
+  }
+
+  // Skia does not support drawing to unpremul surfaces/canvases.
+  sk_sp<SkImage> unPremulImage = nullptr;
+  if (image->alphaType() == kUnpremul_SkAlphaType)
+    unPremulImage = unPremulSkImageToPremul(image.get(), options);
+
+  // If the color space of the source SkImage is null, the following code
+  // does not do any color conversion even thought the new SkImage will be
+  // tagged by the new color space. If this is the case, the following code
+  // will tag a wrong color space to the SkImage. However, this cannot be
+  // addressed here and the code that creates the SkImage must tag the
+  // SkImage with proper color space.
+  SkImageInfo imageInfo = SkImageInfo::Make(
+      image->width(), image->height(), options.dstColorType,
+      unPremulImage ? unPremulImage->alphaType() : image->alphaType(),
+      options.dstColorSpace);
+  sk_sp<SkSurface> surface = SkSurface::MakeRaster(imageInfo);
+  surface->getCanvas()->drawImage(unPremulImage ? unPremulImage : image, 0, 0);
+  sk_sp<SkImage> coloredImage = surface->makeImageSnapshot();
+  updateLatestColorInformation(options);
+  return coloredImage;
+}
+
 sk_sp<SkImage> ImageBitmap::getSkImageFromDecoder(
-    std::unique_ptr<ImageDecoder> decoder) {
+    std::unique_ptr<ImageDecoder> decoder,
+    SkColorType* decodedColorType,
+    sk_sp<SkColorSpace> decodedColorSpace,
+    bool returnColorSpaceInformation) {
   if (!decoder->frameCount())
     return nullptr;
   ImageFrame* frame = decoder->frameBufferAtIndex(0);
   if (!frame || frame->getStatus() != ImageFrame::FrameComplete)
     return nullptr;
   DCHECK(!frame->bitmap().isNull() && !frame->bitmap().empty());
-  return frame->finalizePixelsAndGetImage();
+  sk_sp<SkImage> image = frame->finalizePixelsAndGetImage();
+  if (returnColorSpaceInformation) {
+    *decodedColorType = frame->bitmap().colorType();
+    decodedColorSpace = sk_sp<SkColorSpace>(frame->bitmap().colorSpace());

[dcheng, 2016/12/11 20:11:58]

This parameter is passed by value, so the caller will never see this.

Since this is an optional out param, we can combine the bool and the output
param and just have the param be a sk_sp<SkColorpace>* decodedColorSpace.

Then we can DCHECK((decodedColorType && decodedColorSpace) || (!decodedCOlorType
&& decodedColorSpace)); to make sure the arguments always agree.

[zakerinasab1, 2016/12/12 18:35:21]

The parameter is fixed. The flag is needed as the input SkColorSpace might be
null and we still need to fill it with ptoper color space information.

+  }
+  return image;
 }
 
 bool ImageBitmap::isResizeOptionValid(const ImageBitmapOptions& options,
@@ -272,9 +429,9 @@ bool ImageBitmap::isSourceSizeValid(int sourceWidth,
 // premuliplied format For example, if the image is already in unpremultiplied
 // format and we want the created ImageBitmap in the same format, then we don't
 // need to use the ImageDecoder to decode the image.
-static PassRefPtr<StaticBitmapImage> cropImage(
+static PassRefPtr<StaticBitmapImage> cropImageAndApplyColorSpaceConversion(
     Image* image,
-    const ParsedOptions& parsedOptions,
+    ParsedOptions& parsedOptions,
     AlphaDisposition imageFormat,
     ColorBehavior colorBehavior) {
   ASSERT(image);
@@ -314,18 +471,22 @@ static PassRefPtr<StaticBitmapImage> cropImage(
         colorBehavior));
     if (!decoder)
       return nullptr;
-    skiaImage = ImageBitmap::getSkImageFromDecoder(std::move(decoder));
+    skiaImage = ImageBitmap::getSkImageFromDecoder(
+        std::move(decoder), &parsedOptions.latestColorType,
+        parsedOptions.latestColorSpace, true);
     if (!skiaImage)
       return nullptr;
   }
 
   if (parsedOptions.cropRect == srcRect && !parsedOptions.shouldScaleInput) {
     sk_sp<SkImage> croppedSkImage = skiaImage->makeSubset(srcRect);
-    if (parsedOptions.flipY)
-      return StaticBitmapImage::create(flipSkImageVertically(
-          croppedSkImage.get(), parsedOptions.premultiplyAlpha
-                                    ? PremultiplyAlpha
-                                    : DontPremultiplyAlpha));
+    if (parsedOptions.dstColorSpace) {
+      croppedSkImage = applyColorSpaceConversion(croppedSkImage, parsedOptions);
+    }
+    if (parsedOptions.flipY) {
+      return StaticBitmapImage::create(
+          flipSkImageVertically(croppedSkImage.get(), false, parsedOptions));
+    }
     // Special case: The first parameter image is unpremul but we need to turn
     // it into premul.
     if (parsedOptions.premultiplyAlpha && imageFormat == DontPremultiplyAlpha)
@@ -367,6 +528,8 @@ static PassRefPtr<StaticBitmapImage> cropImage(
     surface->getCanvas()->drawImage(skiaImage, dstLeft, dstTop);
   }
   skiaImage = surface->makeImageSnapshot();
+  if (parsedOptions.dstColorSpace)
+    skiaImage = applyColorSpaceConversion(skiaImage, parsedOptions);
 
   if (parsedOptions.premultiplyAlpha) {
     if (imageFormat == DontPremultiplyAlpha)
@@ -388,12 +551,14 @@ ImageBitmap::ImageBitmap(HTMLImageElement* image,
     return;
 
   if (options.colorSpaceConversion() == "none") {
-    m_image = cropImage(input.get(), parsedOptions, PremultiplyAlpha,
-                        ColorBehavior::ignore());
+    m_image = cropImageAndApplyColorSpaceConversion(
+        input.get(), parsedOptions, PremultiplyAlpha, ColorBehavior::ignore());
   } else {
-    m_image = cropImage(input.get(), parsedOptions, PremultiplyAlpha,
-                        ColorBehavior::transformToGlobalTarget());
+    m_image = cropImageAndApplyColorSpaceConversion(
+        input.get(), parsedOptions, PremultiplyAlpha,
+        ColorBehavior::transformToGlobalTarget());
   }
+
   if (!m_image)
     return;
   // In the case where the source image is lazy-decoded, m_image may not be in
@@ -401,8 +566,10 @@ ImageBitmap::ImageBitmap(HTMLImageElement* image,
   sk_sp<SkImage> skImage = m_image->imageForCurrentFrame();
   SkPixmap pixmap;
   if (!skImage->isTextureBacked() && !skImage->peekPixels(&pixmap)) {
-    sk_sp<SkSurface> surface =
-        SkSurface::MakeRasterN32Premul(skImage->width(), skImage->height());
+    SkImageInfo imageInfo = SkImageInfo::Make(
+        skImage->width(), skImage->height(), parsedOptions.dstColorType,
+        kPremul_SkAlphaType, parsedOptions.dstColorSpace);
+    sk_sp<SkSurface> surface = SkSurface::MakeRaster(imageInfo);
     surface->getCanvas()->drawImage(skImage, 0, 0);
     m_image = StaticBitmapImage::create(surface->makeImageSnapshot());
   }
@@ -483,8 +650,9 @@ ImageBitmap::ImageBitmap(HTMLCanvasElement* canvas,
     parsedOptions.premultiplyAlpha = true;
     isPremultiplyAlphaReverted = true;
   }
-  m_image = cropImage(input.get(), parsedOptions, PremultiplyAlpha,
-                      ColorBehavior::transformToGlobalTarget());
+  m_image = cropImageAndApplyColorSpaceConversion(
+      input.get(), parsedOptions, PremultiplyAlpha,
+      ColorBehavior::transformToGlobalTarget());
   if (!m_image)
     return;
   if (isPremultiplyAlphaReverted) {
@@ -667,7 +835,7 @@ ImageBitmap::ImageBitmap(ImageData* data,
   sk_sp<SkImage> skImage =
       buffer->newSkImageSnapshot(PreferNoAcceleration, SnapshotReasonUnknown);
   if (parsedOptions.flipY)
-    skImage = flipSkImageVertically(skImage.get(), PremultiplyAlpha);
+    skImage = flipSkImageVertically(skImage.get(), true);
   if (!skImage)
     return;
   if (parsedOptions.shouldScaleInput) {
@@ -695,7 +863,7 @@ ImageBitmap::ImageBitmap(ImageBitmap* bitmap,
   if (dstBufferSizeHasOverflow(parsedOptions))
     return;
 
-  m_image = cropImage(
+  m_image = cropImageAndApplyColorSpaceConversion(
       input.get(), parsedOptions,
       bitmap->isPremultiplied() ? PremultiplyAlpha : DontPremultiplyAlpha,
       ColorBehavior::transformToGlobalTarget());
@@ -714,8 +882,9 @@ ImageBitmap::ImageBitmap(PassRefPtr<StaticBitmapImage> image,
   if (dstBufferSizeHasOverflow(parsedOptions))
     return;
 
-  m_image = cropImage(input.get(), parsedOptions, PremultiplyAlpha,
-                      ColorBehavior::transformToGlobalTarget());
+  m_image = cropImageAndApplyColorSpaceConversion(
+      input.get(), parsedOptions, PremultiplyAlpha,
+      ColorBehavior::transformToGlobalTarget());
   if (!m_image)
     return;