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 c1cb64a471998ea3f1508f3306a180c6237de77a..b08f3993498e72317c307bd6b765d83931bfeea4 100644
--- a/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
+++ b/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
@@ -232,6 +232,64 @@ static sk_sp<SkImage> unPremulSkImageToPremul(SkImage* input) {
       static_cast<unsigned>(input->width()) * info.bytesPerPixel());
 }
 
+static sk_sp<SkImage> applyColorSpaceConversion(
+    sk_sp<SkImage> image,
+    bool premultiplyAlpha,
+    const ImageBitmapOptions* options) {
+  // Valid values for colorSpaceConversion are "default", "srgb" and
+  // "linear-rgb" here.
+  if (options->colorSpaceConversion() != "default" &&
+      options->colorSpaceConversion() != "srgb" &&
+      options->colorSpaceConversion() != "linear-rgb") {
+    NOTREACHED()
+        << "Invalid ImageBitmap creation attribute colorSpaceConversion: "
+        << options->colorSpaceConversion();
+    return image;

[xidachen, 2016/11/22 20:34:37]

This part should be moved to the function parseOptions().

+  }
+
+  bool colorCorrectRendering =
+      RuntimeEnabledFeatures::experimentalCanvasFeaturesEnabled() &&
+      RuntimeEnabledFeatures::colorCorrectRenderingEnabled();
+
+  // If color correct rendering is not set, only "default" is accepted here.
+  if (options->colorSpaceConversion() != "default" && !colorCorrectRendering) {
+    NOTREACHED()
+        << "Invalid ImageBitmap creation attribute colorSpaceConversion: "
+        << options->colorSpaceConversion();
+    return image;

[xidachen, 2016/11/22 20:34:37]

This part should be moved to parseOptions() as well.

[zakerinasab, 2016/11/24 18:18:32]

Done.

+  }
+
+  SkImageInfo imageInfo;
+  // If color space conversion is default and color correct rendering is
+  // not set, color correct the image to match display profile.
+  // For now we assume 8 bit per channel for display. This must be fixed
+  // to properly support color space conversion on HDR displays when
+  // they are supported in SkColorType.
+  if (options->colorSpaceConversion() == "default" && !colorCorrectRendering) {
+    imageInfo = SkImageInfo::Make(image->width(), image->height(),
+                                  SkColorType::kN32_SkColorType,
+                                  SkAlphaType::kPremul_SkAlphaType,
+                                  ImageDecoder::globalTargetColorSpace());
+
+    // If color space conversion is default and color correct rendering is
+    // set, color correct the image to sRGB.
+  } else if (options->colorSpaceConversion() == "default" ||
+             options->colorSpaceConversion() == "srgb") {
+    imageInfo = SkImageInfo::MakeS32(image->width(), image->height(),
+                                     SkAlphaType::kPremul_SkAlphaType);
+
+  } else if (options->colorSpaceConversion() == "linear-rgb") {
+    imageInfo = SkImageInfo::Make(
+        image->width(), image->height(), SkColorType::kRGBA_F16_SkColorType,
+        SkAlphaType::kPremul_SkAlphaType,
+        SkColorSpace::MakeNamed(SkColorSpace::kSRGBLinear_Named));
+  }
+
+  sk_sp<SkSurface> surface = SkSurface::MakeRaster(imageInfo);
+  surface->getCanvas()->drawImage(image, 0, 0);
+  return surface->makeImageSnapshot();
+}
+
 sk_sp<SkImage> ImageBitmap::getSkImageFromDecoder(
     std::unique_ptr<ImageDecoder> decoder) {
   if (!decoder->frameCount())
@@ -277,7 +335,8 @@ static PassRefPtr<StaticBitmapImage> cropImage(
     const ParsedOptions& parsedOptions,
     AlphaDisposition imageFormat = PremultiplyAlpha,
     ImageDecoder::ColorSpaceOption colorSpaceOp =
-        ImageDecoder::ColorSpaceApplied) {
+        ImageDecoder::ColorSpaceApplied,
+    const ImageBitmapOptions* imageBitmapOptions = nullptr) {
   ASSERT(image);
   IntRect imgRect(IntPoint(), IntSize(image->width(), image->height()));
   const IntRect srcRect = intersection(imgRect, parsedOptions.cropRect);
@@ -322,6 +381,10 @@ static PassRefPtr<StaticBitmapImage> cropImage(
 
   if (parsedOptions.cropRect == srcRect && !parsedOptions.shouldScaleInput) {
     sk_sp<SkImage> croppedSkImage = skiaImage->makeSubset(srcRect);
+    if (colorSpaceOp == ImageDecoder::ColorSpaceApplied) {
+      croppedSkImage = applyColorSpaceConversion(
+          croppedSkImage, parsedOptions.premultiplyAlpha, imageBitmapOptions);

[xidachen, 2016/11/22 20:34:37]

Pass |parsedOptions| instead of the last two arguments.

[zakerinasab, 2016/11/24 18:18:32]

Done.

+    }
     if (parsedOptions.flipY)
       return StaticBitmapImage::create(flipSkImageVertically(
           croppedSkImage.get(), parsedOptions.premultiplyAlpha
@@ -337,6 +400,9 @@ static PassRefPtr<StaticBitmapImage> cropImage(
     return StaticBitmapImage::create(std::move(croppedSkImage));
   }
 
+  // Currently ImageDecoder assumes at most 8 bits per channel for the decoded
+  // image. When this is fixed, we should change this code to create the
+  // surface according to the color depth of the decoded image.
   sk_sp<SkSurface> surface = SkSurface::MakeRasterN32Premul(
       parsedOptions.resizeWidth, parsedOptions.resizeHeight);
   if (!surface)
@@ -369,6 +435,10 @@ static PassRefPtr<StaticBitmapImage> cropImage(
   }
   skiaImage = surface->makeImageSnapshot();
 
+  if (colorSpaceOp == ImageDecoder::ColorSpaceApplied) {
+    skiaImage = applyColorSpaceConversion(
+        skiaImage, parsedOptions.premultiplyAlpha, imageBitmapOptions);

[xidachen, 2016/11/22 20:34:38]

Pass |parsedOptions| instead of the last two arguments.

[zakerinasab, 2016/11/24 18:18:32]

Done.

+  }
   if (parsedOptions.premultiplyAlpha) {
     if (imageFormat == DontPremultiplyAlpha)
       return StaticBitmapImage::create(
@@ -393,7 +463,7 @@ ImageBitmap::ImageBitmap(HTMLImageElement* image,
                         ImageDecoder::ColorSpaceIgnored);
   } else {
     m_image = cropImage(input.get(), parsedOptions, PremultiplyAlpha,
-                        ImageDecoder::ColorSpaceApplied);
+                        ImageDecoder::ColorSpaceApplied, &options);

[xidachen, 2016/11/22 20:34:37]

This is not the best way, and it is caused by my original implementation, sigh.

Please change the implementation to add a new item in the struct ParsedOptions,
maybe called "dstColorspace". It could be an enum, to represent the destination
color space.

Then in the function parseOptions(), you parse the input options, and assign the
right value to dstColorspace.

By doing the above, you don't need this additional |options| argument, you can
also get rid of the second last argument here which is the
|ImageDecoder::ColorSpaceApplied| because that should be determined by the
dstColorspace. Does that make sense?

[zakerinasab, 2016/11/24 18:18:32]

For now we need to keep ImageDecoder::ColorSpaceOption parameter as other parts
of the code expect the default value to be true. We will remove this after we
added the color conversion support to all the ImageBitmap constructors.

   }
   if (!m_image)
     return;