Fix ImageBitmap constructor from ImageData to consider the color space tags

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 d5f6e8af1790a049ad772ed2c5e34db56f804efc..f382d5cf1bb8666512484a1774065baf8e2ab332 100644
--- a/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
+++ b/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
@@ -70,7 +70,7 @@ ParsedOptions parseOptions(const ImageBitmapOptions& options,
            options.premultiplyAlpha() == "premultiply");
   }
 
-  if (options.colorSpaceConversion() != "none") {
+  if (options.colorSpaceConversion() != imageBitmapOptionNone) {

[Justin Novosad, 2017/01/17 20:00:09]

Please file a crbug to get rid of all the string literals in this file, in favor
of using constants that are defined at the top.

[zakerinasab1, 2017/01/18 18:34:21]

Done. crbug.com/682321

     if (!RuntimeEnabledFeatures::experimentalCanvasFeaturesEnabled() ||
         !RuntimeEnabledFeatures::colorCorrectRenderingEnabled()) {
       DCHECK_EQ(options.colorSpaceConversion(), "default");
@@ -312,7 +312,6 @@ static inline void updateLatestColorInformation(ParsedOptions& options) {
 // 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 void applyColorSpaceConversion(sk_sp<SkImage>& image,
                                       ParsedOptions& options) {
   if (!options.dstColorSpace)
@@ -329,35 +328,29 @@ static void applyColorSpaceConversion(sk_sp<SkImage>& image,
         image->width(), image->height(), options.latestColorType,
         image->alphaType(), options.latestColorSpace);
     size_t size = image->width() * image->height() * info.bytesPerPixel();
-    std::unique_ptr<uint8_t[]> srcPixels(new uint8_t[size]());
-    if (!image->readPixels(info, &srcPixels,
+    sk_sp<SkData> srcData = SkData::MakeUninitialized(size);

[Justin Novosad, 2017/01/17 20:00:09]

When out of memory, does this crash or return a null pointer?  If it can return
a null pointer, you have to check it.

[zakerinasab1, 2017/01/18 18:34:21]

When out of memory, it calls SK_ABORT("sk_throw") down the road. I added size
check just to be safe.

+    if (!image->readPixels(info, srcData->writable_data(),
                            image->width() * info.bytesPerPixel(), 0, 0)) {
       return;
     }
-
-    // For in-place color correction, bytes per pixel must be equal for source
-    // and destination color spaces.
-    std::unique_ptr<uint8_t[]> dstPixels = nullptr;
+    // Proceed with in-place color correction, if possible.
+    sk_sp<SkData> dstData = srcData;
     if (SkColorTypeBytesPerPixel(options.dstColorType) !=
         SkColorTypeBytesPerPixel(options.latestColorType)) {
       size = image->width() * image->height() *
-             SkColorTypeBytesPerPixel(options.latestColorType);
-      dstPixels = std::unique_ptr<uint8_t[]>(new uint8_t[size]());
+             SkColorTypeBytesPerPixel(options.dstColorType);
+      dstData = SkData::MakeUninitialized(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));
+    std::unique_ptr<SkColorSpaceXform> xform = SkColorSpaceXform::New(
+        options.latestColorSpace.get(), options.dstColorSpace.get());
+    xform->apply(getXformFormat(options.dstColorType), dstData->writable_data(),
+                 getXformFormat(options.latestColorType), srcData->data(),
+                 image->width() * image->height(), kUnpremul_SkAlphaType);
     sk_sp<SkImage> coloredImage = SkImage::MakeRasterData(
-        dstInfo, data, image->width() * dstInfo.bytesPerPixel());
+        dstInfo, dstData, image->width() * dstInfo.bytesPerPixel());

[Justin Novosad, 2017/01/17 20:00:09]

There is a potential security vulnerability here.  If the above call to
SkColorSpaceXform::apply fails, the this line will be generating an image from
uninitialized RAM, which may leak secrets.  You should check the return value of
apply() to know whether it succeeded.

[zakerinasab1, 2017/01/18 18:34:21]

Done.

     if (coloredImage) {
       updateLatestColorInformation(options);
       image = coloredImage;
@@ -559,7 +552,7 @@ ImageBitmap::ImageBitmap(HTMLImageElement* image,
   if (dstBufferSizeHasOverflow(parsedOptions))
     return;
 
-  if (options.colorSpaceConversion() == "none") {
+  if (options.colorSpaceConversion() == imageBitmapOptionNone) {
     m_image = cropImageAndApplyColorSpaceConversion(
         input.get(), parsedOptions, PremultiplyAlpha, ColorBehavior::ignore());
   } else {
@@ -738,28 +731,48 @@ ImageBitmap::ImageBitmap(const void* pixelData,
 static sk_sp<SkImage> scaleSkImage(sk_sp<SkImage> skImage,
                                    unsigned resizeWidth,
                                    unsigned resizeHeight,
-                                   SkFilterQuality resizeQuality) {
+                                   SkFilterQuality resizeQuality,
+                                   sk_sp<SkColorSpace> colorSpace = nullptr) {
+  SkColorType colorType = kN32_SkColorType;
+  if (colorSpace &&
+      SkColorSpace::Equals(
+          colorSpace.get(),
+          SkColorSpace::MakeNamed(SkColorSpace::kSRGBLinear_Named).get()))
+    colorType = kRGBA_F16_SkColorType;

[Justin Novosad, 2017/01/17 20:00:09]

This approach is fragile.  You should add a colorType argument to this function.
 The destination ColorType should be determined at the same place where we
determine the colorSpace (i.e. where we parse the options.)

[zakerinasab1, 2017/01/18 18:34:21]

Done.

   SkImageInfo resizedInfo = SkImageInfo::Make(
-      resizeWidth, resizeHeight, kN32_SkColorType, kUnpremul_SkAlphaType);
+      resizeWidth, resizeHeight, colorType, kUnpremul_SkAlphaType, colorSpace);
   RefPtr<ArrayBuffer> dstBuffer = ArrayBuffer::createOrNull(
       resizeWidth * resizeHeight, resizedInfo.bytesPerPixel());
   if (!dstBuffer)
     return nullptr;
-  RefPtr<Uint8Array> resizedPixels =
-      Uint8Array::create(dstBuffer, 0, dstBuffer->byteLength());
+
+  if (colorType == kN32_SkColorType) {
+    RefPtr<Uint8Array> resizedPixels =
+        Uint8Array::create(dstBuffer, 0, dstBuffer->byteLength());
+    SkPixmap pixmap(
+        resizedInfo, resizedPixels->data(),
+        static_cast<unsigned>(resizeWidth) * resizedInfo.bytesPerPixel());
+    skImage->scalePixels(pixmap, resizeQuality);
+    return SkImage::MakeFromRaster(pixmap,
+                                   [](const void*, void* pixels) {
+                                     static_cast<Uint8Array*>(pixels)->deref();
+                                   },
+                                   resizedPixels.release().leakRef());
+  }
+
+  RefPtr<Float32Array> resizedPixels =
+      Float32Array::create(dstBuffer, 0, dstBuffer->byteLength());
   SkPixmap pixmap(
       resizedInfo, resizedPixels->data(),
       static_cast<unsigned>(resizeWidth) * resizedInfo.bytesPerPixel());
   skImage->scalePixels(pixmap, resizeQuality);
   return SkImage::MakeFromRaster(pixmap,
                                  [](const void*, void* pixels) {
-                                   static_cast<Uint8Array*>(pixels)->deref();
+                                   static_cast<Float32Array*>(pixels)->deref();
                                  },
                                  resizedPixels.release().leakRef());
 }
 
-// TODO(zakerinasab): Fix this and the constructor from Float32ImageData
-// when the CL for Float32ImageData landed.
 ImageBitmap::ImageBitmap(ImageData* data,
                          Optional<IntRect> cropRect,
                          const ImageBitmapOptions& options) {
@@ -779,7 +792,7 @@ ImageBitmap::ImageBitmap(ImageData* data,
     // Using kN32 type, swizzle input if necessary.
     SkImageInfo info = SkImageInfo::Make(
         parsedOptions.cropRect.width(), parsedOptions.cropRect.height(),
-        kN32_SkColorType, kUnpremul_SkAlphaType);
+        kN32_SkColorType, kUnpremul_SkAlphaType, data->getSkColorSpace());
     unsigned bytesPerPixel = static_cast<unsigned>(info.bytesPerPixel());
     unsigned srcPixelBytesPerRow = bytesPerPixel * data->size().width();
     unsigned dstPixelBytesPerRow =
@@ -856,20 +869,26 @@ ImageBitmap::ImageBitmap(ImageData* data,
     }
     if (!skImage)
       return;
-    if (parsedOptions.shouldScaleInput)
+    if (data->getSkColorSpace()) {
+      parsedOptions.latestColorSpace = data->getSkColorSpace();
+      applyColorSpaceConversion(skImage, parsedOptions);
+    }
+    if (parsedOptions.shouldScaleInput) {
       m_image = StaticBitmapImage::create(scaleSkImage(
           skImage, parsedOptions.resizeWidth, parsedOptions.resizeHeight,
-          parsedOptions.resizeQuality));
-    else
+          parsedOptions.resizeQuality, data->getSkColorSpace()));
+    } else {
       m_image = StaticBitmapImage::create(skImage);
+    }
     if (!m_image)
       return;
     m_image->setPremultiplied(parsedOptions.premultiplyAlpha);
     return;
   }
 
-  std::unique_ptr<ImageBuffer> buffer = ImageBuffer::create(
-      parsedOptions.cropRect.size(), NonOpaque, DoNotInitializeImagePixels);
+  std::unique_ptr<ImageBuffer> buffer =
+      ImageBuffer::create(parsedOptions.cropRect.size(), NonOpaque,
+                          DoNotInitializeImagePixels, data->getSkColorSpace());
   if (!buffer)
     return;
 
@@ -885,17 +904,23 @@ ImageBitmap::ImageBitmap(ImageData* data,
     dstPoint.setX(-parsedOptions.cropRect.x());
   if (parsedOptions.cropRect.y() < 0)
     dstPoint.setY(-parsedOptions.cropRect.y());
+
   buffer->putByteArray(Unmultiplied, data->data()->data(), data->size(),
                        srcRect, dstPoint);
   sk_sp<SkImage> skImage =
       buffer->newSkImageSnapshot(PreferNoAcceleration, SnapshotReasonUnknown);
+  SkImageInfo imageInfo = SkImageInfo::Make(1, 1, SkColorType::kN32_SkColorType,

[Justin Novosad, 2017/01/17 20:00:09]

This variable appear to be unused.

[zakerinasab1, 2017/01/18 18:34:21]

Ah, that was for debug. Removed.

+                                            SkAlphaType::kUnpremul_SkAlphaType,
+                                            data->getSkColorSpace());
+
   if (parsedOptions.flipY)
     skImage = flipSkImageVertically(skImage.get(), EnforceAlphaPremultiply);
   if (!skImage)
     return;
   if (parsedOptions.shouldScaleInput) {
-    sk_sp<SkSurface> surface = SkSurface::MakeRasterN32Premul(
-        parsedOptions.resizeWidth, parsedOptions.resizeHeight);
+    sk_sp<SkSurface> surface = SkSurface::MakeRaster(SkImageInfo::MakeN32Premul(

[Justin Novosad, 2017/01/17 20:00:09]

I don'think it is Okay to assume N32 here.

[zakerinasab1, 2017/01/18 18:34:21]

I think it is safe because this constructor always uses ImageData (not
Float32ImageData), so there is no need to consider non-N32 color types.

+        parsedOptions.resizeWidth, parsedOptions.resizeHeight,
+        data->getSkColorSpace()));
     if (!surface)
       return;
     SkPaint paint;
@@ -905,9 +930,14 @@ ImageBitmap::ImageBitmap(ImageData* data,
     surface->getCanvas()->drawImageRect(skImage, dstDrawRect, &paint);
     skImage = surface->makeImageSnapshot();
   }
+  if (data->getSkColorSpace()) {
+    parsedOptions.latestColorSpace = data->getSkColorSpace();
+    applyColorSpaceConversion(skImage, parsedOptions);
+  }
   m_image = StaticBitmapImage::create(std::move(skImage));
 }
 
+// TODO(zakerinasab): Fix the constructor from Float32ImageData.
 ImageBitmap::ImageBitmap(Float32ImageData* data,
                          Optional<IntRect> cropRect,
                          const ImageBitmapOptions& options) {}