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..c236761f152501d35614380e39c7e5812835aa67 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 = SkColorType::kN32_SkColorType;

[msarett1, 2016/11/30 21:32:39]

FWIW, I don't think the SkColorType:: is necessary.

[zakerinasab, 2016/12/01 19:49:46]

Removed.

+  SkColorType latestColorType = SkColorType::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,34 @@ 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.
+      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 +143,7 @@ ParsedOptions parseOptions(const ImageBitmapOptions& options,
 bool dstBufferSizeHasOverflow(ParsedOptions options) {
   CheckedNumeric<unsigned> totalBytes = options.cropRect.width();
   totalBytes *= options.cropRect.height();
-  totalBytes *= options.bytesPerPixel;
+  totalBytes *= SkColorTypeBytesPerPixel(options.latestColorType);
   if (!totalBytes.IsValid())
     return true;
 
@@ -118,7 +151,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 +220,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,
+    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 +248,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 +263,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 +278,126 @@ 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;
+    case kIndex_8_SkColorType:

[msarett1, 2016/11/30 21:32:39]

I'm guessing that this case isn't necessary for now - don't think Chrome uses
kIndex8?

[zakerinasab, 2016/12/01 19:49:45]

Removed.

+#ifdef SK_PMCOLOR_IS_RGBA
+      return SkColorSpaceXform::kRGBA_8888_ColorFormat;
+#else
+      return SkColorSpaceXform::kBGRA_8888_ColorFormat;
+#endif
+    default:
+      SkASSERT(false);
+      return SkColorSpaceXform::kRGBA_8888_ColorFormat;
+  }
+}
+
+static inline void updateLatestColorInformation(ParsedOptions& options) {
+  options.latestColorType = options.dstColorType;
+  options.latestColorSpace = options.dstColorSpace;
+}
+
+static sk_sp<SkImage> applyColorSpaceConversion(const sk_sp<SkImage> image,

[msarett1, 2016/11/30 21:32:39]

Unless I'm missing something, I think that this function could just be a couple
lines if not for a Skia bug...

readPixels(SkImageInfo ...) should respect whatever info is passed in.  Ex: if
the info has a color space, we should transform to that color space.  This
currently doesn't work.

FWIW, if the the info has a nullptr color space or the src image has a nullptr
color space, we should not do a color space transformation (I at least think
this probably works correctly now).

So I think there should be no need to transform after calling readPixels()... 
and I'm not sure I completely understand the case where we need to draw the
image to a canvas.

I'm fine with this as a workaround.  But I know there is a bit of ongoing work
towards fixing readPixels()... and I'll go ahead and help get this working
properly.

[zakerinasab, 2016/12/01 19:49:46]

Proper TODO added.

+                                                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 = 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 = 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 supports color conversion for unpremultiplied images.

[msarett1, 2016/11/30 21:32:39]

Not sure what this comment means...

I would say that: "Skia does not support drawing to unpremul surfaces/canvases."

But I do think the we do (or should) support drawing unpremultiplied images.

[zakerinasab, 2016/12/01 19:49:45]

Done.

+  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());
+  }
+  return image;
 }
 
 bool ImageBitmap::isResizeOptionValid(const ImageBitmapOptions& options,
@@ -274,7 +431,7 @@ bool ImageBitmap::isSourceSizeValid(int sourceWidth,
 // need to use the ImageDecoder to decode the image.
 static PassRefPtr<StaticBitmapImage> cropImage(
     Image* image,
-    const ParsedOptions& parsedOptions,
+    ParsedOptions& parsedOptions,
     AlphaDisposition imageFormat = PremultiplyAlpha,
     ImageDecoder::ColorSpaceOption colorSpaceOp =
         ImageDecoder::ColorSpaceApplied) {
@@ -315,18 +472,22 @@ static PassRefPtr<StaticBitmapImage> cropImage(
         colorSpaceOp));
     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)
@@ -368,6 +529,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,13 +551,10 @@ ImageBitmap::ImageBitmap(HTMLImageElement* image,
   if (dstBufferSizeHasOverflow(parsedOptions))
     return;
 
-  if (options.colorSpaceConversion() == "none") {
-    m_image = cropImage(input.get(), parsedOptions, PremultiplyAlpha,
-                        ImageDecoder::ColorSpaceIgnored);
-  } else {
-    m_image = cropImage(input.get(), parsedOptions, PremultiplyAlpha,
-                        ImageDecoder::ColorSpaceApplied);
-  }
+  m_image =
+      cropImage(input.get(), parsedOptions, PremultiplyAlpha,
+                parsedOptions.dstColorSpace ? ImageDecoder::ColorSpaceApplied
+                                            : ImageDecoder::ColorSpaceIgnored);
   if (!m_image)
     return;
   // In the case where the source image is lazy-decoded, m_image may not be in
@@ -667,7 +827,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) {