Add ColorBehavior to blink::Image draw methods

Add ColorBehavior to Image draw methods

Add a ColorBehavior argument to
- blink::Image::imageForCurrentFrame
- blink::Image::draw
- blink::Image::applyShader

Plumb these arguments through to all sub-classes. For sub-classes where
there is a trivial implementation, add it (e.g, blink::BitmapImage).
For all other sub-classes and callsites that need behavior changes, add
TODOs referencing the appropriate bugs.

This patch should have no behavior change -- it just formally
propagates arguments that were previously implied or pulled out of
global variables.

BUG=667420
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/7d997bde5eb18172951ccc321744929aea560d36
Cr-Commit-Position: refs/heads/master@{#437666}

[ccameron, 2016-12-07 23:47:41]

Description was changed from

==========
Add tons o arguments

BUG=
==========

to

==========
Add tons o arguments

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ccameron, 2016-12-07 23:47:44]

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

[commit-bot: I haz the power, 2016-12-07 23:48:07]

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-12-07 23:51:09]

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

[commit-bot: I haz the power, 2016-12-07 23:51:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[ccameron, 2016-12-08 01:11:12]

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

[commit-bot: I haz the power, 2016-12-08 01:12:06]

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-12-08 02:26:14]

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

[commit-bot: I haz the power, 2016-12-08 02:26:14]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[ccameron, 2016-12-08 03:07:19]

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

[commit-bot: I haz the power, 2016-12-08 03:07:44]

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

[ccameron, 2016-12-08 03:10:06]

Description was changed from

==========
Add tons o arguments

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add ColorBehavior to Image draw methods

Add a ColorBehavior argument to
- blink::Image::imageForCurrentFrame
- blink::Image::draw
- blink::Image::applyShader

Plumb these arguments through to all sub-classes. For sub-classes where there is
a trivial implementation, add it (e.g, blink::BitmapImage). For all other
sub-classes
and callsites that need behavior changes, add TODOs referencing the appropriate
bugs.

BUG=667420
==========

[commit-bot: I haz the power, 2016-12-08 03:10:58]

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

[commit-bot: I haz the power, 2016-12-08 03:10:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ccameron, 2016-12-08 03:11:11]

Description was changed from

==========
Add ColorBehavior to Image draw methods

Add a ColorBehavior argument to
- blink::Image::imageForCurrentFrame
- blink::Image::draw
- blink::Image::applyShader

Plumb these arguments through to all sub-classes. For sub-classes where there is
a trivial implementation, add it (e.g, blink::BitmapImage). For all other
sub-classes
and callsites that need behavior changes, add TODOs referencing the appropriate
bugs.

BUG=667420
==========

to

==========
Add ColorBehavior to Image draw methods

Add a ColorBehavior argument to
- blink::Image::imageForCurrentFrame
- blink::Image::draw
- blink::Image::applyShader

Plumb these arguments through to all sub-classes. For sub-classes where there is
a trivial implementation, add it (e.g, blink::BitmapImage). For all other
sub-classes
and callsites that need behavior changes, add TODOs referencing the appropriate
bugs.

This patch should have no behavior change -- it just formally propagates
arguments
that were previously pulled out of global variables.

BUG=667420
==========

[ccameron, 2016-12-08 06:59:49]

Description was changed from

==========
Add ColorBehavior to Image draw methods

Add a ColorBehavior argument to
- blink::Image::imageForCurrentFrame
- blink::Image::draw
- blink::Image::applyShader

Plumb these arguments through to all sub-classes. For sub-classes where there is
a trivial implementation, add it (e.g, blink::BitmapImage). For all other
sub-classes
and callsites that need behavior changes, add TODOs referencing the appropriate
bugs.

This patch should have no behavior change -- it just formally propagates
arguments
that were previously pulled out of global variables.

BUG=667420
==========

to

==========
Add ColorBehavior to Image draw methods

Add a ColorBehavior argument to
- blink::Image::imageForCurrentFrame
- blink::Image::draw
- blink::Image::applyShader

Plumb these arguments through to all sub-classes. For sub-classes where
there is a trivial implementation, add it (e.g, blink::BitmapImage).
For all other sub-classes and callsites that need behavior changes, add
TODOs referencing the appropriate bugs.

This patch should have no behavior change -- it just formallypropagates
arguments that were previously implied or pulled out of
global variables.

BUG=667420
==========

[ccameron, 2016-12-08 06:59:49]

ccameron@chromium.org changed reviewers:
+ chrishtr@chromium.org, junov@chromium.org, pdr@chromium.org

[ccameron, 2016-12-08 07:08:28]

Description was changed from

==========
Add ColorBehavior to Image draw methods

Add a ColorBehavior argument to
- blink::Image::imageForCurrentFrame
- blink::Image::draw
- blink::Image::applyShader

Plumb these arguments through to all sub-classes. For sub-classes where
there is a trivial implementation, add it (e.g, blink::BitmapImage).
For all other sub-classes and callsites that need behavior changes, add
TODOs referencing the appropriate bugs.

This patch should have no behavior change -- it just formallypropagates
arguments that were previously implied or pulled out of
global variables.

BUG=667420
==========

to

==========
Add ColorBehavior to Image draw methods

Add a ColorBehavior argument to
- blink::Image::imageForCurrentFrame
- blink::Image::draw
- blink::Image::applyShader

Plumb these arguments through to all sub-classes. For sub-classes where
there is a trivial implementation, add it (e.g, blink::BitmapImage).
For all other sub-classes and callsites that need behavior changes, add
TODOs referencing the appropriate bugs.

This patch should have no behavior change -- it just formally
propagates arguments that were previously implied or pulled out of
global variables.

BUG=667420
==========

[ccameron, 2016-12-08 07:08:28]

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

[ccameron, 2016-12-08 07:11:40]

PTAL

This plumbs through the ColorBehavior, which we'll have to start respecting. The
implicit behavior of all of these functions previously was
ColorBehavior::transformToGlobalTarget

The project will be done when we no longer have any calls to
ColorBehavior::transformToGlobalTarget, and all of the TODOs that I've added are
done.

I've added chrishtr+junov+pdr to LMK if they object to this general scheme.

Added esprehn as OWNER for everything

(and, the trybots aren't happy cause I've chained this on top of a handful of
other patches).

[ccameron, 2016-12-08 21:19:35]

Description was changed from

==========
Add ColorBehavior to Image draw methods

Add a ColorBehavior argument to
- blink::Image::imageForCurrentFrame
- blink::Image::draw
- blink::Image::applyShader

Plumb these arguments through to all sub-classes. For sub-classes where
there is a trivial implementation, add it (e.g, blink::BitmapImage).
For all other sub-classes and callsites that need behavior changes, add
TODOs referencing the appropriate bugs.

This patch should have no behavior change -- it just formally
propagates arguments that were previously implied or pulled out of
global variables.

BUG=667420
==========

to

==========
Add ColorBehavior to Image draw methods

Add a ColorBehavior argument to
- blink::Image::imageForCurrentFrame
- blink::Image::draw
- blink::Image::applyShader

Plumb these arguments through to all sub-classes. For sub-classes where
there is a trivial implementation, add it (e.g, blink::BitmapImage).
For all other sub-classes and callsites that need behavior changes, add
TODOs referencing the appropriate bugs.

This patch should have no behavior change -- it just formally
propagates arguments that were previously implied or pulled out of
global variables.

BUG=667420
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ccameron, 2016-12-08 22:19:13]

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

[chrishtr, 2016-12-09 01:35:34]

lgtm

[esprehn, 2016-12-09 01:39:32]

lgtm

[Justin Novosad, 2016-12-09 02:17:36]

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

https://codereview.chromium.org/2559013002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1081: //
TODO(ccameron): Canvas should draw in sRGB by default.
Actually, we want to use whatever the canvas's colorspace is. You can assign
this to me.

https://codereview.chromium.org/2559013002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1085:
ColorBehavior::transformToGlobalTarget());
I am concerned about this approach, I understand that BitmapImage will end-up
caching the transformed copy of the source image to avoid re-tranforming it over
and over. Still, it means holding on to long-lived extra copy of the decoded
image in many cases.  Also, it means that the cached copy will thrash if the
image resource is requested in more that one colorspace at each animation frame.
Also, If the source image is another kind of Blink::Image, then what, do we want
to add caches in all subclasses? If we do that we may endup further multiplying
the redundant copies of images.

Transforming the images is not necessary.  The rasterization code only needs the
SkImages to be correctly tagged with colorspaces, and then skia will infer the
required transform and apply it on the fly at blit time. MY understanding is
that the required overhead for the on-the-fly transforms is not that high,
especially on the GPU.

Bottom line: There is an important compute vs. memory tradeoff here that needs
to be considered with caution.

[pdr., 2016-12-09 02:45:53]

Plumbing LGTM. I had not thought of Junov's point about animated gifs on two
monitors but it seems like a good one. Does m_cachedFrame need to be
transformed?

[ccameron, 2016-12-09 03:30:57]

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

https://codereview.chromium.org/2559013002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1085:
ColorBehavior::transformToGlobalTarget());
On 2016/12/09 02:17:35, Justin Novosad wrote:
> I am concerned about this approach, I understand that BitmapImage will end-up
> caching the transformed copy of the source image to avoid re-tranforming it
over
> and over. Still, it means holding on to long-lived extra copy of the decoded
> image in many cases.  Also, it means that the cached copy will thrash if the
> image resource is requested in more that one colorspace at each animation
frame.

It is true, as you mention, that, if two different monitors are repeatedly
requesting the same image, we will thrash the decoder. This is a conscious
choice.

> Also, If the source image is another kind of Blink::Image, then what, do we
want
> to add caches in all subclasses? If we do that we may endup further
multiplying
> the redundant copies of images.

For other sub-classes, it may be feasible to do on-the-fly conversion, instead
of caching. Adding this interface doesn't force either behavior here.

> Transforming the images is not necessary.  The rasterization code only needs
the
> SkImages to be correctly tagged with colorspaces, and then skia will infer the
> required transform and apply it on the fly at blit time. MY understanding is
> that the required overhead for the on-the-fly transforms is not that high,
> especially on the GPU.

This is not true. Skia will only transform images if you opt in to linear
blending, which is not permitted by the web. This is the reason that we're going
with this approach of pre-transforming all inputs, instead of using Skia's color
space support.

On-the-fly transform is efficient and precise with GPU rasterization (and
especially desirable for high-bit-depth rendering), but it's not clear if it is
efficient for CPU rasterization.

> Bottom line: There is an important compute vs. memory tradeoff here that needs
> to be considered with caution.

Either way that we make this trade-off, we will still need to use the interface
that I've added here. How we decide to do conversion (on the fly v not) will
only affect how each blink::Image sub-class behaves.

The default implementation that I've given (which thrashes the decoder in the
worst case) is the least-invasive initial behavior to add.

Also of note is that this is still all behind a flag (well, behind TODOs, which
will eventually become behind a flag), so we can perform measurements and
determine the best course action from that.

[ccameron, 2016-12-09 18:45:32]

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

[commit-bot: I haz the power, 2016-12-09 18:46:17]

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

[Justin Novosad, 2016-12-09 19:43:33]

On 2016/12/09 03:30:57, ccameron wrote:
> 
> It is true, as you mention, that, if two different monitors are repeatedly
> requesting the same image, we will thrash the decoder. This is a conscious
> choice.

Or if an image resource is simultaneously visible through an <img> and a
<canvas>
I think this is going to take a fair amount of tuning to get right, possibly a
more complex cache.  That is beyond the scope of this CL, so lgtm.


> Skia will only transform images if you opt in to linear
> blending, which is not permitted by the web. This is the reason that we're
going
> with this approach of pre-transforming all inputs, instead of using Skia's
color
> space support.

Excellent point!

[commit-bot: I haz the power, 2016-12-09 20:49:23]

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

[commit-bot: I haz the power, 2016-12-09 20:49:24]

Dry run: This issue passed the CQ dry run.

[ccameron, 2016-12-09 21:33:44]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-12-09 21:33:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, esprehn@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2559013002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-12-09 21:34:50]

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-12-09 22:06:06]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481319224216940,
"parent_rev": "901f7878d9183bfe444f77f6445b8c90e6053e76", "commit_rev":
"62b9c0da94d887bc7c672a00c35f3163d0a05bb2"}

[commit-bot: I haz the power, 2016-12-09 22:06:33]

Description was changed from

==========
Add ColorBehavior to Image draw methods

Add a ColorBehavior argument to
- blink::Image::imageForCurrentFrame
- blink::Image::draw
- blink::Image::applyShader

Plumb these arguments through to all sub-classes. For sub-classes where
there is a trivial implementation, add it (e.g, blink::BitmapImage).
For all other sub-classes and callsites that need behavior changes, add
TODOs referencing the appropriate bugs.

This patch should have no behavior change -- it just formally
propagates arguments that were previously implied or pulled out of
global variables.

BUG=667420
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add ColorBehavior to Image draw methods

Add a ColorBehavior argument to
- blink::Image::imageForCurrentFrame
- blink::Image::draw
- blink::Image::applyShader

Plumb these arguments through to all sub-classes. For sub-classes where
there is a trivial implementation, add it (e.g, blink::BitmapImage).
For all other sub-classes and callsites that need behavior changes, add
TODOs referencing the appropriate bugs.

This patch should have no behavior change -- it just formally
propagates arguments that were previously implied or pulled out of
global variables.

BUG=667420
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2559013002
==========

[commit-bot: I haz the power, 2016-12-09 22:06:35]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-12 14:56:47]

Description was changed from

==========
Add ColorBehavior to Image draw methods

Add a ColorBehavior argument to
- blink::Image::imageForCurrentFrame
- blink::Image::draw
- blink::Image::applyShader

Plumb these arguments through to all sub-classes. For sub-classes where
there is a trivial implementation, add it (e.g, blink::BitmapImage).
For all other sub-classes and callsites that need behavior changes, add
TODOs referencing the appropriate bugs.

This patch should have no behavior change -- it just formally
propagates arguments that were previously implied or pulled out of
global variables.

BUG=667420
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2559013002
==========

to

==========
Add ColorBehavior to Image draw methods

Add a ColorBehavior argument to
- blink::Image::imageForCurrentFrame
- blink::Image::draw
- blink::Image::applyShader

Plumb these arguments through to all sub-classes. For sub-classes where
there is a trivial implementation, add it (e.g, blink::BitmapImage).
For all other sub-classes and callsites that need behavior changes, add
TODOs referencing the appropriate bugs.

This patch should have no behavior change -- it just formally
propagates arguments that were previously implied or pulled out of
global variables.

BUG=667420
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/7d997bde5eb18172951ccc321744929aea560d36
Cr-Commit-Position: refs/heads/master@{#437666}
==========

[commit-bot: I haz the power, 2016-12-12 14:56:50]

Patchset 6 (id:??) landed as
https://crrev.com/7d997bde5eb18172951ccc321744929aea560d36
Cr-Commit-Position: refs/heads/master@{#437666}