Fix ImageBitmap constructor from ImageData to consider the color space tags

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

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/frame/ImageBitmap.h"
 
 #include "core/html/Float32ImageData.h"
 #include "core/html/HTMLCanvasElement.h"
 #include "core/html/HTMLVideoElement.h"
 #include "core/html/ImageData.h"
@@ skipping 52 matching lines @@
     DCHECK(options.imageOrientation() == imageBitmapOptionNone);
   }
   if (options.premultiplyAlpha() == imageBitmapOptionNone) {
     parsedOptions.premultiplyAlpha = false;
   } else {
     parsedOptions.premultiplyAlpha = true;
     DCHECK(options.premultiplyAlpha() == "default" ||
            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");
       if (RuntimeEnabledFeatures::colorCorrectRenderingDefaultModeEnabled()) {
         parsedOptions.dstColorSpace = ColorBehavior::globalTargetColorSpace();
         parsedOptions.dstColorType = SkColorType::kN32_SkColorType;
       }
     } else {
       if (options.colorSpaceConversion() == "default" ||
           options.colorSpaceConversion() == "srgb") {
@@ skipping 221 matching lines @@
 }
 
 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 void applyColorSpaceConversion(sk_sp<SkImage>& image,
                                       ParsedOptions& options) {
   if (!options.dstColorSpace)
     return;
   if (SkColorSpace::Equals(options.latestColorSpace.get(),
                            options.dstColorSpace.get()))
     return;
 
   // 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();
-    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;
       return;
     }
     return;
   }
 
   // Skia does not support drawing to unpremul surfaces/canvases.
   sk_sp<SkImage> unPremulImage = nullptr;
@@ skipping 181 matching lines @@
 ImageBitmap::ImageBitmap(HTMLImageElement* image,
                          Optional<IntRect> cropRect,
                          Document* document,
                          const ImageBitmapOptions& options) {
   RefPtr<Image> input = image->cachedImage()->getImage();
   ParsedOptions parsedOptions =
       parseOptions(options, cropRect, image->bitmapSourceSize());
   if (dstBufferSizeHasOverflow(parsedOptions))
     return;
 
-  if (options.colorSpaceConversion() == "none") {
+  if (options.colorSpaceConversion() == imageBitmapOptionNone) {
     m_image = cropImageAndApplyColorSpaceConversion(
         input.get(), parsedOptions, PremultiplyAlpha, ColorBehavior::ignore());
   } else {
     m_image = cropImageAndApplyColorSpaceConversion(
         input.get(), parsedOptions, PremultiplyAlpha,
         ColorBehavior::transformToGlobalTarget());
   }
 
   if (!m_image)
     return;
@@ skipping 158 matching lines @@
   m_image = StaticBitmapImage::create(SkImage::MakeRasterCopy(pixmap));
   if (!m_image)
     return;
   m_image->setPremultiplied(isImageBitmapPremultiplied);
   m_image->setOriginClean(isImageBitmapOriginClean);
 }
 
 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) {
   // TODO(xidachen): implement the resize option
   IntRect dataSrcRect = IntRect(IntPoint(), data->size());
   ParsedOptions parsedOptions =
       parseOptions(options, cropRect, data->bitmapSourceSize());
   if (dstBufferSizeHasOverflow(parsedOptions))
     return;
   IntRect srcRect = cropRect ? intersection(parsedOptions.cropRect, dataSrcRect)
                              : dataSrcRect;
 
   // treat non-premultiplyAlpha as a special case
   if (!parsedOptions.premultiplyAlpha) {
     unsigned char* srcAddr = data->data()->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 =
         bytesPerPixel * parsedOptions.cropRect.width();
     sk_sp<SkImage> skImage;
     if (parsedOptions.cropRect == IntRect(IntPoint(), data->size())) {
       swizzleImageData(srcAddr, data->size().height(), srcPixelBytesPerRow,
                        parsedOptions.flipY);
       skImage =
           SkImage::MakeRasterCopy(SkPixmap(info, srcAddr, dstPixelBytesPerRow));
@@ skipping 56 matching lines @@
                   srcAddr[srcStartCopyPosition + j];
             }
           }
         }
       }
       skImage = newSkImageFromRaster(info, std::move(copiedDataBuffer),
                                      dstPixelBytesPerRow);
     }
     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;
 
   if (srcRect.isEmpty()) {
     m_image = StaticBitmapImage::create(buffer->newSkImageSnapshot(
         PreferNoAcceleration, SnapshotReasonUnknown));
     return;
   }
 
   IntPoint dstPoint = IntPoint(std::min(0, -parsedOptions.cropRect.x()),
                                std::min(0, -parsedOptions.cropRect.y()));
   if (parsedOptions.cropRect.x() < 0)
     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;
     paint.setFilterQuality(parsedOptions.resizeQuality);
     SkRect dstDrawRect =
         SkRect::MakeWH(parsedOptions.resizeWidth, parsedOptions.resizeHeight);
     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) {}
 
 ImageBitmap::ImageBitmap(ImageBitmap* bitmap,
                          Optional<IntRect> cropRect,
                          const ImageBitmapOptions& options) {
   RefPtr<Image> input = bitmap->bitmapImage();
   if (!input)
     return;
@@ skipping 187 matching lines @@
 void ImageBitmap::adjustDrawRects(FloatRect* srcRect,
                                   FloatRect* dstRect) const {}
 
 FloatSize ImageBitmap::elementSize(const FloatSize&) const {
   return FloatSize(width(), height());
 }
 
 DEFINE_TRACE(ImageBitmap) {}
 
 }  // namespace blink