Fix the linear-rgb canvas color space so that it renders

Fix the linear-rgb canvas color space so that it renders

This change fixes colorSpace:"linear-rgb" for 2D canvas so that
the skia rendering pipeline actually renders correctly into its
backing.  The results still do not propagate to the display,
but they are inspectable via getImageData.

The main issue was that skia only support linear transfer curves
when the color type is 16-bit float. Also, getImage data had to
work around some output format limitations in skia

BUG=634542
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/75885ebb694faeabc19dd751561a82a582703793
Cr-Commit-Position: refs/heads/master@{#426558}

[Justin Novosad, 2016-10-17 22:01:11]

Description was changed from

==========
Fix the linear-rgb canvas color space so that it renders

This change fixes colorSpace:"linear-rgb" for 2D canvas so that
the skia rendering pipeline actually renders correctly into its
backing.  The results still do not propagate to the display,
but they are inspectable via getImageData.

BUG=634542
==========

to

==========
Fix the linear-rgb canvas color space so that it renders

This change fixes colorSpace:"linear-rgb" for 2D canvas so that
the skia rendering pipeline actually renders correctly into its
backing.  The results still do not propagate to the display,
but they are inspectable via getImageData.

BUG=634542
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Justin Novosad, 2016-10-17 22:04:29]

Description was changed from

==========
Fix the linear-rgb canvas color space so that it renders

This change fixes colorSpace:"linear-rgb" for 2D canvas so that
the skia rendering pipeline actually renders correctly into its
backing.  The results still do not propagate to the display,
but they are inspectable via getImageData.

BUG=634542
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix the linear-rgb canvas color space so that it renders

This change fixes colorSpace:"linear-rgb" for 2D canvas so that
the skia rendering pipeline actually renders correctly into its
backing.  The results still do not propagate to the display,
but they are inspectable via getImageData.

The main issue was that skia only support linear transfer curves
when the color type is 16-bit float. Also, getImage data had to
work around some output format limitations in skia

BUG=634542
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Justin Novosad, 2016-10-17 22:05:04]

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

[Justin Novosad, 2016-10-17 22:05:04]

PTAL

[Justin Novosad, 2016-10-17 22:06:25]

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

[commit-bot: I haz the power, 2016-10-17 22:06:52]

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, 2016-10-18 01:26:18]

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

[commit-bot: I haz the power, 2016-10-18 01:26:19]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[zakerinasab, 2016-10-18 15:12:42]

https://codereview.chromium.org/2425113002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/2425113002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp:400: 
When creating the SkImageInfo the color space is set to sRGB. As I understand it
says readPixels to return the data in sRGB (as it is the color space of the
SkBitmap attached to the SkCanvas on which the snapshot is drawn - 
SkImage_Base::onReadPixels). If this is correct, why the colors that you read in
sRGB is the same as you set in linear RGB? Shouldn't be a conversion here?

[xidachen, 2016-10-18 15:40:37]

lgtm with nits.

https://codereview.chromium.org/2425113002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
(right):

https://codereview.chromium.org/2425113002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:89:
SkColorType colorType() const;
Should we make it more consistent? As we call skColorSpace(), maybe we should
call this one skColorType(), WDYT?

https://codereview.chromium.org/2425113002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.h (right):

https://codereview.chromium.org/2425113002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.h:146:
SkColorType colorType() const { return m_colorType; }
I think we should be consistent here. Either call is colorSpace() and
colorType(), or skColorSpace() and skColorType(), to match the CRC class.

[Justin Novosad, 2016-10-18 17:02:41]

https://codereview.chromium.org/2425113002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
(right):

https://codereview.chromium.org/2425113002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:89:
SkColorType colorType() const;
On 2016/10/18 15:40:37, xidachen wrote:
> Should we make it more consistent? As we call skColorSpace(), maybe we should
> call this one skColorType(), WDYT?

In general we should not use sk prefixes in blink.
The only reason the method is called skColorSpace in CanvasRenderingContext is
to avoid a name conflict. Basically, that class has two representations: the
ColorSpace enum (which maps to the IDL dictionary) and the SkColorSpace. C++
does not allow overloads that differ only by return type.

https://codereview.chromium.org/2425113002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/2425113002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp:400: 
On 2016/10/18 15:12:42, zakerinasab wrote:
> When creating the SkImageInfo the color space is set to sRGB. As I understand
it
> says readPixels to return the data in sRGB (as it is the color space of the
> SkBitmap attached to the SkCanvas on which the snapshot is drawn - 
> SkImage_Base::onReadPixels). If this is correct, why the colors that you read
in
> sRGB is the same as you set in linear RGB? Shouldn't be a conversion here?

Right now, sRGB is the assumed color spece of ImageData and of literal color
values.  So the values are converted from sRGB to linear when drawn to the
canvas, and converted back to sRGB when read. This will likely change in the
future when we extend the APIs too allow ImageData and color values to be
specified in alternate color spaces.

[zakerinasab, 2016-10-18 18:54:23]

On 2016/10/18 17:02:41, Justin Novosad wrote:
>
https://codereview.chromium.org/2425113002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
> (right):
> 
>
https://codereview.chromium.org/2425113002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:89:
> SkColorType colorType() const;
> On 2016/10/18 15:40:37, xidachen wrote:
> > Should we make it more consistent? As we call skColorSpace(), maybe we
should
> > call this one skColorType(), WDYT?
> 
> In general we should not use sk prefixes in blink.
> The only reason the method is called skColorSpace in CanvasRenderingContext is
> to avoid a name conflict. Basically, that class has two representations: the
> ColorSpace enum (which maps to the IDL dictionary) and the SkColorSpace. C++
> does not allow overloads that differ only by return type.
> 
>
https://codereview.chromium.org/2425113002/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp (right):
> 
>
https://codereview.chromium.org/2425113002/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp:400: 
> On 2016/10/18 15:12:42, zakerinasab wrote:
> > When creating the SkImageInfo the color space is set to sRGB. As I
understand
> it
> > says readPixels to return the data in sRGB (as it is the color space of the
> > SkBitmap attached to the SkCanvas on which the snapshot is drawn - 
> > SkImage_Base::onReadPixels). If this is correct, why the colors that you
read
> in
> > sRGB is the same as you set in linear RGB? Shouldn't be a conversion here?
> 
> Right now, sRGB is the assumed color spece of ImageData and of literal color
> values.  So the values are converted from sRGB to linear when drawn to the
> canvas, and converted back to sRGB when read. This will likely change in the
> future when we extend the APIs too allow ImageData and color values to be
> specified in alternate color spaces.

lgtm

[Justin Novosad, 2016-10-20 17:34:41]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2016-10-20 17:35:18]

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, 2016-10-20 19:18:41]

Description was changed from

==========
Fix the linear-rgb canvas color space so that it renders

This change fixes colorSpace:"linear-rgb" for 2D canvas so that
the skia rendering pipeline actually renders correctly into its
backing.  The results still do not propagate to the display,
but they are inspectable via getImageData.

The main issue was that skia only support linear transfer curves
when the color type is 16-bit float. Also, getImage data had to
work around some output format limitations in skia

BUG=634542
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix the linear-rgb canvas color space so that it renders

This change fixes colorSpace:"linear-rgb" for 2D canvas so that
the skia rendering pipeline actually renders correctly into its
backing.  The results still do not propagate to the display,
but they are inspectable via getImageData.

The main issue was that skia only support linear transfer curves
when the color type is 16-bit float. Also, getImage data had to
work around some output format limitations in skia

BUG=634542
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-10-20 19:18:43]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-21 13:21:08]

Description was changed from

==========
Fix the linear-rgb canvas color space so that it renders

This change fixes colorSpace:"linear-rgb" for 2D canvas so that
the skia rendering pipeline actually renders correctly into its
backing.  The results still do not propagate to the display,
but they are inspectable via getImageData.

The main issue was that skia only support linear transfer curves
when the color type is 16-bit float. Also, getImage data had to
work around some output format limitations in skia

BUG=634542
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix the linear-rgb canvas color space so that it renders

This change fixes colorSpace:"linear-rgb" for 2D canvas so that
the skia rendering pipeline actually renders correctly into its
backing.  The results still do not propagate to the display,
but they are inspectable via getImageData.

The main issue was that skia only support linear transfer curves
when the color type is 16-bit float. Also, getImage data had to
work around some output format limitations in skia

BUG=634542
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/75885ebb694faeabc19dd751561a82a582703793
Cr-Commit-Position: refs/heads/master@{#426558}
==========

[commit-bot: I haz the power, 2016-10-21 13:21:10]

Patchset 1 (id:??) landed as
https://crrev.com/75885ebb694faeabc19dd751561a82a582703793
Cr-Commit-Position: refs/heads/master@{#426558}