color: Convert BitmapImage to sRGB ahead of time

color: Convert BitmapImage to sRGB ahead of time

We are always sending these images to sRGB canvases, so do the
conversion ahead of time.

This fixes a bug wherein, when a WebGLRenderingContext reads a
BitmapImage it wants the image to be in sRGB (if not-none was
requested). We will eventually need to decide how a color-canvas-using
WebGLRenderingContext should handle non-sRGB images.

BUG=730983
Review-Url: https://codereview.chromium.org/2924373002
Cr-Commit-Position: refs/heads/master@{#478564}
Committed: https://chromium.googlesource.com/chromium/src/+/64db61998368ffd368f477a57b749f9a5a0dc9aa

[ccameron, 2017-06-08 21:24:23]

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

[ccameron, 2017-06-08 21:24:23]

ptal -- this is the patch we discussed

I may have to rebaseline a few tests if some are affected by this (e.g, anything
that was converting at display list canvas time).

[ccameron, 2017-06-08 22:09:04]

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

[commit-bot: I haz the power, 2017-06-08 22:09:41]

Dry run: CQ is trying da patch.

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

[Justin Novosad, 2017-06-09 14:08:34]

https://codereview.chromium.org/2924373002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2924373002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:49: bool convert_to_srgb =
false;
This new member is confusing IMHO because it potentially conflicts with the
color space stored in color_params, and it may not be clear which of the two is
the ground truth.  I think the cleaner solution is to tweak the code that sets
the color space in color_params.  That might be the only code you need to touch
to fix this issue.

https://codereview.chromium.org/2924373002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:490: sk_sp<SkSurface>
surface = SkSurface::MakeRasterN32Premul(
I think it would be better to just specify a color space for this SkSurface.
Then we would not need the ColorSpaceXformCanvas below.

https://codereview.chromium.org/2924373002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:574: kPremul_SkAlphaType,
dst_color_space);
We should just tweak the dst_color_space used here instead of setting up a
ColorSpaceXformCanvas below.

https://codereview.chromium.org/2924373002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:936:
data->GetSkColorSpace()));
data->GetColorSpace() should return return srgb if that is the desired color
space.  Then the ColorSpaceXformCanvas would not be needed.

[ccameron, 2017-06-09 17:01:33]

Updated to be a bit more clear about behavior here.

I'm not using the SkSurface color API because (in
non-color-canvas-extensions-mode), we don't necessarily have this API to use,
and because the API behaves subtly differently.

https://codereview.chromium.org/2924373002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2924373002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:49: bool convert_to_srgb =
false;
On 2017/06/09 14:08:34, Justin Novosad wrote:
> This new member is confusing IMHO because it potentially conflicts with the
> color space stored in color_params, and it may not be clear which of the two
is
> the ground truth.  I think the cleaner solution is to tweak the code that sets
> the color space in color_params.  That might be the only code you need to
touch
> to fix this issue.

I removed this and replaced it with "color_params.UsesOutputSpaceBlending &&
ColorCorrectRenderingEnabled".

https://codereview.chromium.org/2924373002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:490: sk_sp<SkSurface>
surface = SkSurface::MakeRasterN32Premul(
On 2017/06/09 14:08:34, Justin Novosad wrote:
> I think it would be better to just specify a color space for this SkSurface.
> Then we would not need the ColorSpaceXformCanvas below.

Doing that will opt us in to linear blending, which has a couple of
side-effects. One is that it will not match previous behavior. The other, more
serious effect is that that path doesn't have as wide support -- it depends on
the GL sRGB extensions (which aren't always there).

https://codereview.chromium.org/2924373002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:574: kPremul_SkAlphaType,
dst_color_space);
On 2017/06/09 14:08:34, Justin Novosad wrote:
> We should just tweak the dst_color_space used here instead of setting up a
> ColorSpaceXformCanvas below.

Updated this to still use the xform canvas, but to also use
parsed_options.color_params.GetSkColorSpace, since there's no reason to
hard-code sRGB.

https://codereview.chromium.org/2924373002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:936:
data->GetSkColorSpace()));
On 2017/06/09 14:08:34, Justin Novosad wrote:
> data->GetColorSpace() should return return srgb if that is the desired color
> space.  Then the ColorSpaceXformCanvas would not be needed.

Same here.

[Justin Novosad, 2017-06-09 19:59:42]

lgtm

[ccameron, 2017-06-09 23:47:10]

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

[commit-bot: I haz the power, 2017-06-09 23:47:34]

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-06-10 01:42:54]

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

[commit-bot: I haz the power, 2017-06-10 01:42:56]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ccameron, 2017-06-12 06:50:11]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-06-12 06:50:12]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org
Link to the patchset: https://codereview.chromium.org/2924373002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2017-06-12 06:50:14]

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-06-12 08:27:07]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1497250211918370,
"parent_rev": "91aff5625b214afb9005b0dac0d42a218359e51a", "commit_rev":
"64db61998368ffd368f477a57b749f9a5a0dc9aa"}

[commit-bot: I haz the power, 2017-06-12 08:27:16]

Description was changed from

==========
color: Convert BitmapImage to sRGB ahead of time

We are always sending these images to sRGB canvases, so do the
conversion ahead of time.

This fixes a bug wherein, when a WebGLRenderingContext reads a
BitmapImage it wants the image to be in sRGB (if not-none was
requested). We will eventually need to decide how a color-canvas-using
WebGLRenderingContext should handle non-sRGB images.

BUG=730983
==========

to

==========
color: Convert BitmapImage to sRGB ahead of time

We are always sending these images to sRGB canvases, so do the
conversion ahead of time.

This fixes a bug wherein, when a WebGLRenderingContext reads a
BitmapImage it wants the image to be in sRGB (if not-none was
requested). We will eventually need to decide how a color-canvas-using
WebGLRenderingContext should handle non-sRGB images.

BUG=730983

Review-Url: https://codereview.chromium.org/2924373002
Cr-Commit-Position: refs/heads/master@{#478564}
Committed:
https://chromium.googlesource.com/chromium/src/+/64db61998368ffd368f477a57b74...
==========

[commit-bot: I haz the power, 2017-06-12 08:27:17]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/64db61998368ffd368f477a57b74...