Fix BaseRenderingContext2D create/put/get-ImageData() for color managed canvas

third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp

Index: third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
diff --git a/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp b/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
index 9653160eccf6da95850252b47b4b17683b287e83..e9f188fa5d5a22508c33e02580be0dfb2849da65 100644
--- a/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
+++ b/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
@@ -36,8 +36,11 @@
 namespace blink {
 
 BaseRenderingContext2D::BaseRenderingContext2D()
-    : clip_antialiasing_(kNotAntiAliased) {
+    : clip_antialiasing_(kNotAntiAliased), is_color_managed_(false) {
   state_stack_.push_back(CanvasRenderingContext2DState::Create());
+  is_color_managed_ =

[fserb, 2017/04/11 03:21:44]

a RenderingContext is color managed even if it has kLegacyCanvasColorSpace? Or
are you using this just as a feature flag? Maybe name it accordingly
"color_management_enabled_"?

[zakerinasab, 2017/04/11 19:52:59]

Done.

+      RuntimeEnabledFeatures::experimentalCanvasFeaturesEnabled() &&
+      RuntimeEnabledFeatures::colorCorrectRenderingEnabled();
 }
 
 BaseRenderingContext2D::~BaseRenderingContext2D() {}
@@ -1506,10 +1509,38 @@ bool BaseRenderingContext2D::ComputeDirtyRect(
   return true;
 }
 
+bool BaseRenderingContext2D::ColorSettingsAsImageDataColorSettings(
+    ImageDataColorSettings& color_settings) const {

[fserb, 2017/04/11 03:21:44]

maybe ImageDataColorSettings* ?

[zakerinasab, 2017/04/11 19:52:59]

Rewritten.

+  if (!is_color_managed_)
+    return false;
+  color_settings.setColorSpace(ColorSpaceAsString());
+  switch (PixelFormat()) {
+    case kRGBA8CanvasPixelFormat:
+      color_settings.setStorageFormat(kUint8ClampedArrayStorageFormatName);
+      break;
+    case kF16CanvasPixelFormat:
+      color_settings.setStorageFormat(kFloat32ArrayStorageFormatName);
+      break;
+    case kRGB10A2CanvasPixelFormat:
+    case kRGBA12CanvasPixelFormat:
+    default:
+      NOTREACHED();
+      return false;
+  }
+  return true;
+}
+
 ImageData* BaseRenderingContext2D::createImageData(
     ImageData* image_data,
     ExceptionState& exception_state) const {
-  ImageData* result = ImageData::Create(image_data->size());
+  ImageData* result = nullptr;
+  if (is_color_managed_) {
+    ImageDataColorSettings color_settings;
+    if (ColorSettingsAsImageDataColorSettings(color_settings))
+      result = ImageData::Create(image_data->size(), &color_settings);
+  } else {
+    result = ImageData::Create(image_data->size());
+  }
   if (!result)
     exception_state.ThrowRangeError("Out of memory at ImageData creation");
   return result;
@@ -1527,8 +1558,15 @@ ImageData* BaseRenderingContext2D::createImageData(
   }
 
   IntSize size(abs(sw), abs(sh));
+  ImageData* result = nullptr;
+  if (is_color_managed_) {
+    ImageDataColorSettings color_settings;
+    if (ColorSettingsAsImageDataColorSettings(color_settings))
+      result = ImageData::Create(size, &color_settings);
+  } else {
+    result = ImageData::Create(size);
+  }
 
-  ImageData* result = ImageData::Create(size);
   if (!result)
     exception_state.ThrowRangeError("Out of memory at ImageData creation");
   return result;
@@ -1594,10 +1632,21 @@ ImageData* BaseRenderingContext2D::getImageData(
     timer.emplace(scoped_us_counter_cpu);
   }
 
+  ImageDataColorSettings color_settings;
+  bool valid_color_settings = false;
+  if (is_color_managed_) {
+    valid_color_settings =

[fserb, 2017/04/11 03:21:44]

just use is_color_managed_?

[zakerinasab, 2017/04/11 19:52:59]

Done.

+        ColorSettingsAsImageDataColorSettings(color_settings);
+  }
   IntRect image_data_rect(sx, sy, sw, sh);
+  DVLOG(1) << sx << ", " << sy << ", " << sw << ", " << sh;

[fserb, 2017/04/11 03:21:44]

^^^^

[zakerinasab, 2017/04/11 19:52:59]

Removed, but it's funny that I don't remember putting this here.

   ImageBuffer* buffer = GetImageBuffer();
   if (!buffer || isContextLost()) {
-    ImageData* result = ImageData::Create(image_data_rect.size());
+    ImageData* result = nullptr;
+    if (valid_color_settings)
+      result = ImageData::Create(image_data_rect.size(), &color_settings);
+    else
+      result = ImageData::Create(image_data_rect.size());
     if (!result)
       exception_state.ThrowRangeError("Out of memory at ImageData creation");
     return result;
@@ -1611,7 +1660,39 @@ ImageData* BaseRenderingContext2D::getImageData(
 
   NeedsFinalizeFrame();
 
-  DOMArrayBuffer* array_buffer = DOMArrayBuffer::Create(contents);
+  DOMArrayBuffer* array_buffer = nullptr;
+  DOMArrayBufferView* array_buffer_view = nullptr;
+  DOMFloat32Array* data_array = nullptr;
+
+  if (is_color_managed_) {
+    ImageDataStorageFormat storage_format =
+        ImageData::GetImageDataStorageFormat(color_settings.storageFormat());
+    switch (storage_format) {

[fserb, 2017/04/11 03:21:44]

this code is looking weird.
I'm Assuming is_color_managed_ is just an experimental flag.

If that is so, put the final return on the else.
move the data_array, array_buffer_view, array_buffer inside the if.
I'm almost inclined to use a series of ifs here instead of a switch, given that
they don't share anything and just return.
The way it is now, there's a big mental work reading the code to keep track of
stuff that doesn't matter.

[zakerinasab, 2017/04/11 19:52:59]

Simplified.

+      case kUint8ClampedArrayStorageFormat:
+        array_buffer = DOMArrayBuffer::Create(contents);
+        return ImageData::Create(
+            image_data_rect.size(),
+            DOMUint8ClampedArray::Create(array_buffer, 0,
+                                         array_buffer->ByteLength()));
+        break;
+      case kUint16ArrayStorageFormat:
+        NOTREACHED();
+        break;
+      case kFloat32ArrayStorageFormat:
+        array_buffer_view = ImageData::
+            ConvertPixelsFromCanvasPixelFormatToImageDataStorageFormat(
+                contents, PixelFormat(), storage_format);
+        data_array = const_cast<DOMFloat32Array*>(
+            static_cast<const DOMFloat32Array*>(array_buffer_view));
+        return ImageData::Create(image_data_rect.size(), array_buffer_view,
+                                 &color_settings);
+      default:
+        NOTREACHED();
+    }
+    return nullptr;
+  }
+
+  array_buffer = DOMArrayBuffer::Create(contents);
   return ImageData::Create(image_data_rect.size(),
                            DOMUint8ClampedArray::Create(
                                array_buffer, 0, array_buffer->ByteLength()));
@@ -1636,14 +1717,30 @@ void BaseRenderingContext2D::putImageData(ImageData* data,
   if (!WTF::CheckMul(dirty_width, dirty_height).IsValid<int>()) {
     return;
   }
-
   usage_counters_.num_put_image_data_calls++;
   usage_counters_.area_put_image_data_calls += dirty_width * dirty_height;
-  if (data->data()->BufferBase()->IsNeutered()) {
+
+  bool data_is_neutered = false;
+  if (is_color_managed_) {

[fserb, 2017/04/11 03:21:44]

again, if this is an experimental flag thingy, I'd really just leave an else if
the original code, and copy the exception inside.

[zakerinasab, 2017/04/11 19:52:59]

You mean if(original_check) {something} else {check color managed case}? This
would not work as the original check will crash.

+    data_is_neutered =
+        (data->dataUnion().isUint8ClampedArray() &&
+         data->dataUnion()
+             .getAsUint8ClampedArray()
+             ->BufferBase()
+             ->IsNeutered()) ||
+        (data->dataUnion().isUint16Array() &&
+         data->dataUnion().getAsUint16Array()->BufferBase()->IsNeutered()) ||
+        (data->dataUnion().isFloat32Array() &&
+         data->dataUnion().getAsFloat32Array()->BufferBase()->IsNeutered());
+  } else {
+    data_is_neutered = data->data()->BufferBase()->IsNeutered();
+  }
+  if (data_is_neutered) {
     exception_state.ThrowDOMException(kInvalidStateError,
                                       "The source data has been neutered.");
     return;
   }
+
   ImageBuffer* buffer = GetImageBuffer();
   if (!buffer)
     return;
@@ -1693,10 +1790,21 @@ void BaseRenderingContext2D::putImageData(ImageData* data,
   CheckOverdraw(dest_rect, 0, CanvasRenderingContext2DState::kNoImage,
                 kUntransformedUnclippedFill);
 
-  buffer->PutByteArray(kUnmultiplied, data->data()->Data(),
-                       IntSize(data->width(), data->height()), source_rect,
-                       IntPoint(dest_offset));
-
+  if (is_color_managed_) {
+    unsigned data_length = data->width() * data->height();

[fserb, 2017/04/11 03:21:44]

name the type?

[zakerinasab, 2017/04/11 19:52:59]

?

+    if (PixelFormat() == kF16CanvasPixelFormat)
+      data_length *= 2;
+    std::unique_ptr<uint8_t[]> converted_pixels(new uint8_t[4 * data_length]);
+    data->ImageDataInCanvasColorSettings(ColorSpace(), PixelFormat(),
+                                         converted_pixels);
+    buffer->PutByteArray(kUnmultiplied, converted_pixels.get(),
+                         IntSize(data->width(), data->height()), source_rect,
+                         IntPoint(dest_offset));
+  } else {
+    buffer->PutByteArray(kUnmultiplied, data->data()->Data(),
+                         IntSize(data->width(), data->height()), source_rect,
+                         IntPoint(dest_offset));
+  }
   DidDraw(dest_rect);
 }