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

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

* createImageData() and getImageData() must return an ImageData for which the color space and storage format matches those of canvas.
If canvas pixel format is "float16", the storage format of the returned ImageData must be "float32". Values must be properly converted from float16 to float32.

* putImageData must do the color conversion from the color space and storage format of given ImageData to the color space and matching pixel format of canvas prior to putting data into the canvas.

BUG=704155, 709777
Review-Url: https://codereview.chromium.org/2797213002
Cr-Commit-Position: refs/heads/master@{#467635}
Committed: https://chromium.googlesource.com/chromium/src/+/03d8a3084d1e21b685437607961f5ddccdf032b1

[zakerinasab, 2017-04-05 16:44:08]

Description was changed from

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

* createImageData() and getImageData() must return an ImageData for which the
color space and storage format matches those of canvas.
If canvas pixel format is "float16", the storage format of the returned
ImageData must be "float32". Values must be properly converted from float16 to
float32.

* putImageData must do the color conversion from the color space and storage
format of given ImageData to the color space and matching pixel format of canvas
prior to putting data into the canvas.

BUG=704155
==========

to

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

* createImageData() and getImageData() must return an ImageData for which the
color space and storage format matches those of canvas.
If canvas pixel format is "float16", the storage format of the returned
ImageData must be "float32". Values must be properly converted from float16 to
float32.

* putImageData must do the color conversion from the color space and storage
format of given ImageData to the color space and matching pixel format of canvas
prior to putting data into the canvas.

BUG=704155
==========

[zakerinasab, 2017-04-05 16:44:09]

zakerinasab@chromium.org changed reviewers:
+ junov@chromium.org

[zakerinasab, 2017-04-06 18:10:31]

A layout test is provided to test user side call site for all the possible
combinations of calls. putImageData test scenarios pass if the browser does not
crash. To test the pixel values after putImageData->getImageData call, a unit
test will be added to CanvasRenderingContext2DTest.

[Justin Novosad, 2017-04-06 19:01:04]

https://codereview.chromium.org/2797213002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2797213002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1698:
RuntimeEnabledFeatures::colorCorrectRenderingEnabled();
This check is copied in many placed, perhaps we could just check once in the
constructor and store the result in a bool. m_isColorManaged

https://codereview.chromium.org/2797213002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h
(right):

https://codereview.chromium.org/2797213002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:379: virtual
CanvasRenderingContext* renderingContext() const { return nullptr; }
This should either be a non-const method, or it should return a const.If this is
problematic, you can also do both a const and a non-const version of this
method.

[fserb, 2017-04-07 20:03:32]

fserb@chromium.org changed reviewers:
+ fserb@chromium.org

[fserb, 2017-04-07 20:03:33]

https://codereview.chromium.org/2797213002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h
(right):

https://codereview.chromium.org/2797213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:379: virtual
const CanvasRenderingContext* renderingContext() const {
talking to junov@ offline, I don't think we should do this.
The current inheritance situation is a bit messy, and we are thinking of ways to
solve this, but I think adding a renderingContext() here is going to make it
messier.

Could you please add the virtual methods you need (colorSpaceAsString() and
pixelFormat() with reasonable defaults) instead and implement them on the
*Context2D classes?

That way, we keep BaseRenderingContext2D independent of CanvasRenderingContext
and then, once we solve the bigger inheritance issue (i.e., having a proper base
class for Canvas and OffscreenCanvas), we can move it all there.

[zakerinasab, 2017-04-10 14:28:32]

Patchset #6 (id:100001) has been deleted

[zakerinasab, 2017-04-10 21:09:10]

On 2017/04/07 20:03:33, fserb wrote:
>
https://codereview.chromium.org/2797213002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h
> (right):
> 
>
https://codereview.chromium.org/2797213002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:379:
virtual
> const CanvasRenderingContext* renderingContext() const {
> talking to junov@ offline, I don't think we should do this.
> The current inheritance situation is a bit messy, and we are thinking of ways
to
> solve this, but I think adding a renderingContext() here is going to make it
> messier.
> 
> Could you please add the virtual methods you need (colorSpaceAsString() and
> pixelFormat() with reasonable defaults) instead and implement them on the
> *Context2D classes?
> 
> That way, we keep BaseRenderingContext2D independent of CanvasRenderingContext
> and then, once we solve the bigger inheritance issue (i.e., having a proper
base
> class for Canvas and OffscreenCanvas), we can move it all there.

Done.

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

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:41:
is_color_managed_ =
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_"?

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1513:
ImageDataColorSettings& color_settings) const {
maybe ImageDataColorSettings* ?

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1638:
valid_color_settings =
just use is_color_managed_?

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1642:
DVLOG(1) << sx << ", " << sy << ", " << sw << ", " << sh;
^^^^

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1670:
switch (storage_format) {
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.

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1724: if
(is_color_managed_) {
again, if this is an experimental flag thingy, I'd really just leave an else if
the original code, and copy the exception inside.

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1794:
unsigned data_length = data->width() * data->height();
name the type?

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h
(right):

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:11: #include
"core/html/canvas/CanvasRenderingContext.h"
no need for this anymore.

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:387: bool
ColorSettingsAsImageDataColorSettings(ImageDataColorSettings&) const;
can we name this with a verb? Do you mean something like
"(Update|Set)ImageDataColorSetting"?
Also, the semantic of this return is very weird. And you are just doing the
is_color_managed_ check twice when you call it. 
If the semantics is "has the settings changed", then the function needs to be
updated. Maybe return void here?

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:300:
return ImageData::createImageData(width, height, color_settings,
I'm a bit confused why don't you have to use BaseRenderingContext2D here?
It feels to me that all those functions should disappear and just have a
BaseRenderingContext2D implementation of all of them.

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:315:
return CanvasRenderingContext::ColorSpace();
fyi: Yeah, I'm fixing this on the next refactor. :)

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h
(right):

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h:203:
ImageData* createImageData(ImageData*, ExceptionState&) const;
do we need those on CanvasRenderingContext2D at all?

[zakerinasab, 2017-04-11 19:53:00]

New CL uploaded. I addressed Fernando's comments (except one which I didn't get
the question), added the unit test (still has minor bugs to fix) and hopefully
corrected the interface listings. If there was no more comments, I'll try to
land after fixing the issue with the unit test.

https://codereview.chromium.org/2797213002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2797213002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1698:
RuntimeEnabledFeatures::colorCorrectRenderingEnabled();
On 2017/04/06 19:01:04, Justin Novosad wrote:
> This check is copied in many placed, perhaps we could just check once in the
> constructor and store the result in a bool. m_isColorManaged

Done.

https://codereview.chromium.org/2797213002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h
(right):

https://codereview.chromium.org/2797213002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:379: virtual
CanvasRenderingContext* renderingContext() const { return nullptr; }
On 2017/04/06 19:01:04, Justin Novosad wrote:
> This should either be a non-const method, or it should return a const.If this
is
> problematic, you can also do both a const and a non-const version of this
> method.

Done.

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:41:
is_color_managed_ =
On 2017/04/11 03:21:44, fserb wrote:
> 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_"?

Done.

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1513:
ImageDataColorSettings& color_settings) const {
On 2017/04/11 03:21:44, fserb wrote:
> maybe ImageDataColorSettings* ?

Rewritten.

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1638:
valid_color_settings =
On 2017/04/11 03:21:44, fserb wrote:
> just use is_color_managed_?

Done.

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1642:
DVLOG(1) << sx << ", " << sy << ", " << sw << ", " << sh;
On 2017/04/11 03:21:44, fserb wrote:
> ^^^^

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

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1670:
switch (storage_format) {
On 2017/04/11 03:21:44, fserb wrote:
> 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.

Simplified.

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1724: if
(is_color_managed_) {
On 2017/04/11 03:21:44, fserb wrote:
> again, if this is an experimental flag thingy, I'd really just leave an else
if
> the original code, and copy the exception inside.

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

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1794:
unsigned data_length = data->width() * data->height();
On 2017/04/11 03:21:44, fserb wrote:
> name the type?

?

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h
(right):

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:11: #include
"core/html/canvas/CanvasRenderingContext.h"
On 2017/04/11 03:21:44, fserb wrote:
> no need for this anymore.

Done.

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:387: bool
ColorSettingsAsImageDataColorSettings(ImageDataColorSettings&) const;
On 2017/04/11 03:21:44, fserb wrote:
> can we name this with a verb? Do you mean something like
> "(Update|Set)ImageDataColorSetting"?
> Also, the semantic of this return is very weird. And you are just doing the
> is_color_managed_ check twice when you call it. 
> If the semantics is "has the settings changed", then the function needs to be
> updated. Maybe return void here?

This method converts the canvas color settings (if valid) to an image data color
settings. is_color_managed_ must be called on the call site, but it's checked
again to be safe. I'll remove it. On another thought, we can assume that if
something is wrong with canvas color settings, we can continue with the default
color space settings for image data. I'll simplify the method to reflect this
behavior.

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:300:
return ImageData::createImageData(width, height, color_settings,
On 2017/04/11 03:21:44, fserb wrote:
> I'm a bit confused why don't you have to use BaseRenderingContext2D here?
> It feels to me that all those functions should disappear and just have a
> BaseRenderingContext2D implementation of all of them.

Done.

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:315:
return CanvasRenderingContext::ColorSpace();
On 2017/04/11 03:21:44, fserb wrote:
> fyi: Yeah, I'm fixing this on the next refactor. :)

:)

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h
(right):

https://codereview.chromium.org/2797213002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h:203:
ImageData* createImageData(ImageData*, ExceptionState&) const;
On 2017/04/11 03:21:44, fserb wrote:
> do we need those on CanvasRenderingContext2D at all?

All moved to BaseRenderingContext2D.

[Justin Novosad, 2017-04-11 21:28:05]

https://codereview.chromium.org/2797213002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/ImageData.h (right):

https://codereview.chromium.org/2797213002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageData.h:92: static ImageData*
createImageData(unsigned width,
With these function no longer in the IDL, they should start with capital 'C'.

https://codereview.chromium.org/2797213002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2797213002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1706:
(data->dataUnion().isUint8ClampedArray() &&
This block of code is not fun to read. Let's hide the dataUnion logic in a
helper method: ImageData::BufferBase().  That way, the code seen here can just
be:
if (data->BufferBase()->isNeutered()) { ...

https://codereview.chromium.org/2797213002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.idl
(right):

https://codereview.chromium.org/2797213002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.idl:134:
[RuntimeEnabled=ExperimentalCanvasFeatures, RaisesException] ImageData
createImageData(unsigned long sw, unsigned long sh, ImageDataColorSettings
imageDataColorSettings);
imageDataColorSettings should be optional.  I am okay with doing that in another
CL, but please file a crbug for it.

https://codereview.chromium.org/2797213002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.idl:135:
[RuntimeEnabled=ExperimentalCanvasFeatures, RaisesException] ImageData
createImageData(ImageDataArray data, unsigned long sw, unsigned long sh,
ImageDataColorSettings imageDataColorSettings);
Same here.

[zakerinasab, 2017-04-15 07:04:43]

Comments addressed. Unit tests are done. Will try to land after another quick
review from fserb@ as junov@ is OoO.

https://codereview.chromium.org/2797213002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/ImageData.h (right):

https://codereview.chromium.org/2797213002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageData.h:92: static ImageData*
createImageData(unsigned width,
On 2017/04/11 21:28:05, junov - OoO - back April 24 wrote:
> With these function no longer in the IDL, they should start with capital 'C'.

Done.

https://codereview.chromium.org/2797213002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2797213002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1706:
(data->dataUnion().isUint8ClampedArray() &&
On 2017/04/11 21:28:05, junov - OoO - back April 24 wrote:
> This block of code is not fun to read. Let's hide the dataUnion logic in a
> helper method: ImageData::BufferBase().  That way, the code seen here can just
> be:
> if (data->BufferBase()->isNeutered()) { ...

Done.

https://codereview.chromium.org/2797213002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.idl
(right):

https://codereview.chromium.org/2797213002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.idl:134:
[RuntimeEnabled=ExperimentalCanvasFeatures, RaisesException] ImageData
createImageData(unsigned long sw, unsigned long sh, ImageDataColorSettings
imageDataColorSettings);
On 2017/04/11 21:28:05, junov - OoO - back April 24 wrote:
> imageDataColorSettings should be optional.  I am okay with doing that in
another
> CL, but please file a crbug for it.

This cannot be done now, as the call without ImageDataColorSettings parameter
already exist and cannot go behind the flag. I filed a bug: crbug.com/711886.

https://codereview.chromium.org/2797213002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.idl:135:
[RuntimeEnabled=ExperimentalCanvasFeatures, RaisesException] ImageData
createImageData(ImageDataArray data, unsigned long sw, unsigned long sh,
ImageDataColorSettings imageDataColorSettings);
On 2017/04/11 21:28:05, junov - OoO - back April 24 wrote:
> Same here.

Addressed in the same CL.

[zakerinasab, 2017-04-16 04:49:20]

Description was changed from

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

* createImageData() and getImageData() must return an ImageData for which the
color space and storage format matches those of canvas.
If canvas pixel format is "float16", the storage format of the returned
ImageData must be "float32". Values must be properly converted from float16 to
float32.

* putImageData must do the color conversion from the color space and storage
format of given ImageData to the color space and matching pixel format of canvas
prior to putting data into the canvas.

BUG=704155
==========

to

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

* createImageData() and getImageData() must return an ImageData for which the
color space and storage format matches those of canvas.
If canvas pixel format is "float16", the storage format of the returned
ImageData must be "float32". Values must be properly converted from float16 to
float32.

* putImageData must do the color conversion from the color space and storage
format of given ImageData to the color space and matching pixel format of canvas
prior to putting data into the canvas.

BUG=704155,709777
==========

[zakerinasab, 2017-04-17 21:36:22]

Description was changed from

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

* createImageData() and getImageData() must return an ImageData for which the
color space and storage format matches those of canvas.
If canvas pixel format is "float16", the storage format of the returned
ImageData must be "float32". Values must be properly converted from float16 to
float32.

* putImageData must do the color conversion from the color space and storage
format of given ImageData to the color space and matching pixel format of canvas
prior to putting data into the canvas.

BUG=704155,709777
==========

to

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

* createImageData() and getImageData() must return an ImageData for which the
color space and storage format matches those of canvas.
If canvas pixel format is "float16", the storage format of the returned
ImageData must be "float32". Values must be properly converted from float16 to
float32.

* putImageData must do the color conversion from the color space and storage
format of given ImageData to the color space and matching pixel format of canvas
prior to putting data into the canvas.

BUG=704155,709777,712270
==========

[zakerinasab, 2017-04-17 21:52:57]

The CQ bit was checked by zakerinasab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 21:53:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[zakerinasab, 2017-04-17 21:54:57]

Description was changed from

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

* createImageData() and getImageData() must return an ImageData for which the
color space and storage format matches those of canvas.
If canvas pixel format is "float16", the storage format of the returned
ImageData must be "float32". Values must be properly converted from float16 to
float32.

* putImageData must do the color conversion from the color space and storage
format of given ImageData to the color space and matching pixel format of canvas
prior to putting data into the canvas.

BUG=704155,709777,712270
==========

to

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

* createImageData() and getImageData() must return an ImageData for which the
color space and storage format matches those of canvas.
If canvas pixel format is "float16", the storage format of the returned
ImageData must be "float32". Values must be properly converted from float16 to
float32.

* putImageData must do the color conversion from the color space and storage
format of given ImageData to the color space and matching pixel format of canvas
prior to putting data into the canvas.

BUG=704155,709777
==========

[zakerinasab, 2017-04-17 21:58:31]

Patchset #16 (id:320001) has been deleted

[zakerinasab, 2017-04-17 21:58:44]

Patchset #15 (id:300001) has been deleted

[zakerinasab, 2017-04-17 21:58:54]

Patchset #14 (id:280001) has been deleted

[zakerinasab, 2017-04-17 21:59:06]

Patchset #12 (id:240001) has been deleted

[zakerinasab, 2017-04-17 21:59:28]

The CQ bit was checked by zakerinasab@chromium.org

[commit-bot: I haz the power, 2017-04-17 21:59:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 21:59:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 21:59:56]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[zakerinasab, 2017-04-17 22:01:25]

The CQ bit was unchecked by zakerinasab@chromium.org

[zakerinasab, 2017-04-17 22:02:43]

The CQ bit was checked by zakerinasab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 22:03:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 23:38:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 23:38:36]

Dry run: This issue passed the CQ dry run.

[fserb, 2017-04-19 17:32:52]

lgtm

https://codereview.chromium.org/2797213002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
(left):

https://codereview.chromium.org/2797213002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1699:
buffer->PutByteArray(kUnmultiplied, data->data()->Data(),
lol "data->data()->Data()" :))

[zakerinasab, 2017-04-24 14:03:42]

The CQ bit was checked by zakerinasab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 14:04:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 16:23:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-24 16:23:23]

Dry run: This issue passed the CQ dry run.

[Justin Novosad, 2017-04-26 18:37:40]

lgtm

[zakerinasab, 2017-04-26 18:48:12]

The CQ bit was checked by zakerinasab@chromium.org

[zakerinasab, 2017-04-26 18:48:13]

The patchset sent to the CQ was uploaded after l-g-t-m from fserb@chromium.org,
junov@chromium.org
Link to the patchset: https://codereview.chromium.org/2797213002/#ps380001
(title: "Rebaseline")

[commit-bot: I haz the power, 2017-04-26 18:49:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 20:35:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 20:35:21]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zakerinasab, 2017-04-26 20:36:18]

The CQ bit was checked by zakerinasab@chromium.org

[commit-bot: I haz the power, 2017-04-26 20:37:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 22:42:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 22:42:27]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zakerinasab, 2017-04-27 01:26:00]

The CQ bit was checked by zakerinasab@chromium.org

[commit-bot: I haz the power, 2017-04-27 01:26:56]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 03:20:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 03:20:23]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zakerinasab, 2017-04-27 10:31:20]

The CQ bit was checked by zakerinasab@chromium.org

[commit-bot: I haz the power, 2017-04-27 10:31:43]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 11:10:49]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1493289080189320,
"parent_rev": "93bc5d7c02d275d27c030ab15a8c68efc8efdd99", "commit_rev":
"03d8a3084d1e21b685437607961f5ddccdf032b1"}

[commit-bot: I haz the power, 2017-04-27 11:11:07]

Description was changed from

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

* createImageData() and getImageData() must return an ImageData for which the
color space and storage format matches those of canvas.
If canvas pixel format is "float16", the storage format of the returned
ImageData must be "float32". Values must be properly converted from float16 to
float32.

* putImageData must do the color conversion from the color space and storage
format of given ImageData to the color space and matching pixel format of canvas
prior to putting data into the canvas.

BUG=704155,709777
==========

to

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

* createImageData() and getImageData() must return an ImageData for which the
color space and storage format matches those of canvas.
If canvas pixel format is "float16", the storage format of the returned
ImageData must be "float32". Values must be properly converted from float16 to
float32.

* putImageData must do the color conversion from the color space and storage
format of given ImageData to the color space and matching pixel format of canvas
prior to putting data into the canvas.

BUG=704155,709777

Review-Url: https://codereview.chromium.org/2797213002
Cr-Commit-Position: refs/heads/master@{#467635}
Committed:
https://chromium.googlesource.com/chromium/src/+/03d8a3084d1e21b685437607961f...
==========

[commit-bot: I haz the power, 2017-04-27 11:11:08]

Committed patchset #15 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/03d8a3084d1e21b685437607961f...

[RE66, 2017-04-27 11:43:37]

On 2017/04/27 11:11:08, commit-bot: I haz the power wrote:
> Committed patchset #15 (id:380001) as
>
https://chromium.googlesource.com/chromium/src/+/03d8a3084d1e21b685437607961f...

I do not understand why a conversion from float16 to float32 
Is done.
It take twice the memory with no bettecoloring.
Many many machine atr using float16. For this machines
You double memory of image
Total memory huge

[zakerinasab, 2017-04-27 12:17:47]

On 2017/04/27 11:43:37, RE66 wrote:
> On 2017/04/27 11:11:08, commit-bot: I haz the power wrote:
> > Committed patchset #15 (id:380001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/03d8a3084d1e21b685437607961f...
> 
> I do not understand why a conversion from float16 to float32 
> Is done.
> It take twice the memory with no bettecoloring.
> Many many machine atr using float16. For this machines
> You double memory of image
> Total memory huge

Half-float arrays are not supported by JavaScript (see here:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Typed_arrays).
Therefore, to expose float data in JS we have to use Float32Array (or
Float64Array).