Color: Separate ICCProfile and ColorSpace structures

Color: Separate ICCProfile and ColorSpace structures

Remove the extra effort being made to avoid replication of ICC profile
data across object instances. With the two separate structures, we will
always know when we have the "heavy" version.

Add plumbing to pass ColorSpace as part of cc::TransferableResource.

Note that the functionality to convert between ICCProfile and
ColorSpace is not present in this patch.

BUG=622133
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel

Committed: https://crrev.com/efdab1600b787e4e1f82fb05ae6a397d90a0d7b5
Cr-Commit-Position: refs/heads/master@{#407622}

[ccameron, 2016-07-19 21:50:10]

Description was changed from

==========
Color: Separate ICCProfile and ColorSpace structures

Add plumbing to pass ColorSpace as part of cc::TransferableResource.

Remove the extra effort being made to avoid replication of ICC profile
data across object instances. With the two separate structures, we will
always know when we have the "heavy" version.

BUG=622133
==========

to

==========
Color: Separate ICCProfile and ColorSpace structures

Add plumbing to pass ColorSpace as part of cc::TransferableResource.

Remove the extra effort being made to avoid replication of ICC profile
data across object instances. With the two separate structures, we will
always know when we have the "heavy" version.

BUG=622133
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-07-19 21:50:10]

Description was changed from

==========
Color: Separate ICCProfile and ColorSpace structures

Add plumbing to pass ColorSpace as part of cc::TransferableResource.

Remove the extra effort being made to avoid replication of ICC profile
data across object instances. With the two separate structures, we will
always know when we have the "heavy" version.

BUG=622133
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

==========
Color: Separate ICCProfile and ColorSpace structures

Add plumbing to pass ColorSpace as part of cc::TransferableResource.

Remove the extra effort being made to avoid replication of ICC profile
data across object instances. With the two separate structures, we will
always know when we have the "heavy" version.

BUG=622133
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[ccameron, 2016-07-19 21:52:41]

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

[commit-bot: I haz the power, 2016-07-19 21:53:38]

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-07-19 22:08:19]

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

[commit-bot: I haz the power, 2016-07-19 22:08:20]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ccameron, 2016-07-19 22:46:26]

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

[commit-bot: I haz the power, 2016-07-19 22:46:58]

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-07-19 23:29:53]

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

[commit-bot: I haz the power, 2016-07-19 23:29:54]

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

[ccameron, 2016-07-20 02:11:05]

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

[commit-bot: I haz the power, 2016-07-20 02:11:32]

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-07-20 04:18:48]

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

[commit-bot: I haz the power, 2016-07-20 04:18:49]

Dry run: This issue passed the CQ dry run.

[ccameron, 2016-07-20 05:41:04]

Description was changed from

==========
Color: Separate ICCProfile and ColorSpace structures

Add plumbing to pass ColorSpace as part of cc::TransferableResource.

Remove the extra effort being made to avoid replication of ICC profile
data across object instances. With the two separate structures, we will
always know when we have the "heavy" version.

BUG=622133
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Color: Separate ICCProfile and ColorSpace structures

Remove the extra effort being made to avoid replication of ICC profile
data across object instances. With the two separate structures, we will
always know when we have the "heavy" version.

Add plumbing to pass ColorSpace as part of cc::TransferableResource.

BUG=622133
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[ccameron, 2016-07-20 05:42:38]

Description was changed from

==========
Color: Separate ICCProfile and ColorSpace structures

Remove the extra effort being made to avoid replication of ICC profile
data across object instances. With the two separate structures, we will
always know when we have the "heavy" version.

Add plumbing to pass ColorSpace as part of cc::TransferableResource.

BUG=622133
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Color: Separate ICCProfile and ColorSpace structures

Remove the extra effort being made to avoid replication of ICC profile
data across object instances. With the two separate structures, we will
always know when we have the "heavy" version.

Add plumbing to pass ColorSpace as part of cc::TransferableResource.

Note that the functionality to convert between ICCProfile and
ColorSpace is not present in this patch. 

BUG=622133
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[ccameron, 2016-07-20 05:42:38]

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

[ccameron, 2016-07-20 05:49:07]

ptal.

A follow-on patch will add a gfx::ColorSpace to cc::TextureMailbox -- this can
be populated in the video code (e.g, [1]), and then it will be query-able in the
GLRenderer when drawing the quad.

[1]
https://cs.chromium.org/chromium/src/cc/resources/video_resource_updater.cc?r...

[ccameron, 2016-07-21 19:38:39]

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

[commit-bot: I haz the power, 2016-07-21 19:39:19]

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-07-21 19:50:56]

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

[commit-bot: I haz the power, 2016-07-21 19:50:57]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)

[hubbe, 2016-07-21 22:08:54]

LGTM + some comments

https://codereview.chromium.org/2161293002/diff/80001/ui/gfx/icc_profile.cc
File ui/gfx/icc_profile.cc (right):

https://codereview.chromium.org/2161293002/diff/80001/ui/gfx/icc_profile.cc#n...
ui/gfx/icc_profile.cc:43: // TODO(ccameron): Support constructing ICC profiles
from arbitrary ColorSpace
Why add this function if it's not actually implemented yet?

https://codereview.chromium.org/2161293002/diff/80001/ui/gfx/icc_profile.h
File ui/gfx/icc_profile.h (right):

https://codereview.chromium.org/2161293002/diff/80001/ui/gfx/icc_profile.h#ne...
ui/gfx/icc_profile.h:62: uint64_t id_ = 0;
id_ sees to be completely unused right now.
Either remove it or set it, use it and document it?

[ccameron, 2016-07-21 22:08:55]

ccameron@chromium.org changed reviewers:
+ dcheng@chromium.org, enne@chromium.org

[ccameron, 2016-07-21 22:08:56]

Adding enne@ for cc/ stuff, and dcheng@ for IPC

[enne (OOO), 2016-07-21 22:22:53]

https://codereview.chromium.org/2161293002/diff/80001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2161293002/diff/80001/ui/gfx/color_space.h#ne...
ui/gfx/color_space.h:25: // Used to represet a color space for the purpose of
color conversion.
typo

[enne (OOO), 2016-07-21 22:22:55]

lgtm

[ccameron, 2016-07-22 00:59:17]

Thanks!

Adding avi@ for content/OWNER

https://codereview.chromium.org/2161293002/diff/80001/ui/gfx/icc_profile.cc
File ui/gfx/icc_profile.cc (right):

https://codereview.chromium.org/2161293002/diff/80001/ui/gfx/icc_profile.cc#n...
ui/gfx/icc_profile.cc:43: // TODO(ccameron): Support constructing ICC profiles
from arbitrary ColorSpace
On 2016/07/21 22:08:54, hubbe wrote:
> Why add this function if it's not actually implemented yet?

Good point. I fleshed out this part of the functionality, and got rid of the xyz
matrix, cause I'm not actually populating it, and you will likely end up
representing the primaries differently.

https://codereview.chromium.org/2161293002/diff/80001/ui/gfx/icc_profile.h
File ui/gfx/icc_profile.h (right):

https://codereview.chromium.org/2161293002/diff/80001/ui/gfx/icc_profile.h#ne...
ui/gfx/icc_profile.h:62: uint64_t id_ = 0;
On 2016/07/21 22:08:54, hubbe wrote:
> id_ sees to be completely unused right now.
> Either remove it or set it, use it and document it?

This is now used in the fleshed-out version.

[ccameron, 2016-07-22 01:13:19]

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

[commit-bot: I haz the power, 2016-07-22 01:14:01]

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-07-22 02:30:11]

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

[commit-bot: I haz the power, 2016-07-22 02:30:12]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-07-22 05:00:27]

https://codereview.chromium.org/2161293002/diff/100001/ui/gfx/icc_profile.cc
File ui/gfx/icc_profile.cc (right):

https://codereview.chromium.org/2161293002/diff/100001/ui/gfx/icc_profile.cc#...
ui/gfx/icc_profile.cc:22: static base::LazyInstance<std::list<ICCProfile>>
g_mru_cache;
FWIW, we do have some MRU cache abstractions in //base/containers/mru_cache.h.
Does this intentionally avoid using them?

https://codereview.chromium.org/2161293002/diff/100001/ui/gfx/icc_profile.cc#...
ui/gfx/icc_profile.cc:89: // objects.
There are plans to start sending this from less-privileged to more-privileged
processes right? I'm just curious what the eventual plan is, since a renderer
can just make up an arbitrary icc profile ID.

[ccameron, 2016-07-22 21:02:41]

Thanks!

Updated to use the MRU cache, and also merged in some constants from
https://codereview.chromium.org/2169913003/.

https://codereview.chromium.org/2161293002/diff/100001/ui/gfx/icc_profile.cc
File ui/gfx/icc_profile.cc (right):

https://codereview.chromium.org/2161293002/diff/100001/ui/gfx/icc_profile.cc#...
ui/gfx/icc_profile.cc:22: static base::LazyInstance<std::list<ICCProfile>>
g_mru_cache;
On 2016/07/22 05:00:27, dcheng wrote:
> FWIW, we do have some MRU cache abstractions in //base/containers/mru_cache.h.
> Does this intentionally avoid using them?

Oh, that's nice! Switched to using them.

https://codereview.chromium.org/2161293002/diff/100001/ui/gfx/icc_profile.cc#...
ui/gfx/icc_profile.cc:89: // objects.
On 2016/07/22 05:00:27, dcheng wrote:
> There are plans to start sending this from less-privileged to more-privileged
> processes right? I'm just curious what the eventual plan is, since a renderer
> can just make up an arbitrary icc profile ID.

Full plan:

We'll only be sending gfx::ICCProfile more-privileged->less-privileged. Parsing
these is complicated and I wouldn't vouch for a parser of this being secure.

We will be sending gfx::ColorSpace from less-privileged->more-privileged. We'll
be expanding gfx::ColorSpace to include a number of enums and a matrix. The
browser process will use this to adjust the color of content coming from that
renderer (and only that renderer). Making up an arbitrary ID for the
gfx::ColorSpace of a cc::TransferableResource will result in that resource
having different/wrong color. The globally cached ICC profiles are not
privileged information.

WRT generating ICC profiles, we will not be generating new ICC profile data from
gfx::ColorSpaces coming from the renderer. When we add ICC profile generation
code, it will have limited functionality, and be robust to any inputs (so that
even if we were to generate an ICC profile from a renderer's color space, that
would still be secure).

[ccameron, 2016-07-22 21:06:28]

Err, actually updated now.

[dcheng, 2016-07-25 13:42:26]

LGTM with nits

https://codereview.chromium.org/2161293002/diff/120001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2161293002/diff/120001/ui/gfx/color_space.h#n...
ui/gfx/color_space.h:23: // Used to represet a color space for the purpose of
color conversion.
Perhaps also note that this is designed to be safe to send over IPC in either
direction unlike ICCProfile, which has a (binary?) data component.

(ICCProfile could probably note that it's only safe to send from browser to
renderer or something, I'm not sure what wording we'd want to use for this in
//ui)

https://codereview.chromium.org/2161293002/diff/120001/ui/gfx/icc_profile.cc
File ui/gfx/icc_profile.cc (right):

https://codereview.chromium.org/2161293002/diff/120001/ui/gfx/icc_profile.cc#...
ui/gfx/icc_profile.cc:29: uint64_t next_unused_id = 5;
Dumb question: how do the initial hardcoded entries get into the cache? Or do
they have a completely separate path? Or is this part of the TODO noted in
FromColorspace?

https://codereview.chromium.org/2161293002/diff/120001/ui/gfx/icc_profile.h
File ui/gfx/icc_profile.h (right):

https://codereview.chromium.org/2161293002/diff/120001/ui/gfx/icc_profile.h#n...
ui/gfx/icc_profile.h:24: ICCProfile(ICCProfile&& other);
I would probably default the move assignment operator too, just for symmetry.

[ccameron, 2016-07-25 21:30:06]

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

[commit-bot: I haz the power, 2016-07-25 21:30:42]

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

[ccameron, 2016-07-25 21:31:53]

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

[ccameron, 2016-07-25 21:31:54]

Thanks!

Adding esprehn@ to stamp content/renderer/render_view_impl.cc

https://codereview.chromium.org/2161293002/diff/120001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2161293002/diff/120001/ui/gfx/color_space.h#n...
ui/gfx/color_space.h:23: // Used to represet a color space for the purpose of
color conversion.
On 2016/07/25 13:42:26, dcheng wrote:
> Perhaps also note that this is designed to be safe to send over IPC in either
> direction unlike ICCProfile, which has a (binary?) data component.
> 
> (ICCProfile could probably note that it's only safe to send from browser to
> renderer or something, I'm not sure what wording we'd want to use for this in
> //ui)

Put in both notes (in "higher-privilege" to "lower-privilege" language).

https://codereview.chromium.org/2161293002/diff/120001/ui/gfx/icc_profile.cc
File ui/gfx/icc_profile.cc (right):

https://codereview.chromium.org/2161293002/diff/120001/ui/gfx/icc_profile.cc#...
ui/gfx/icc_profile.cc:29: uint64_t next_unused_id = 5;
On 2016/07/25 13:42:26, dcheng wrote:
> Dumb question: how do the initial hardcoded entries get into the cache? Or do
> they have a completely separate path? Or is this part of the TODO noted in
> FromColorspace?

They're part of the TODO -- we don't compute them yet.

https://codereview.chromium.org/2161293002/diff/120001/ui/gfx/icc_profile.h
File ui/gfx/icc_profile.h (right):

https://codereview.chromium.org/2161293002/diff/120001/ui/gfx/icc_profile.h#n...
ui/gfx/icc_profile.h:24: ICCProfile(ICCProfile&& other);
On 2016/07/25 13:42:26, dcheng wrote:
> I would probably default the move assignment operator too, just for symmetry.

Done.

[commit-bot: I haz the power, 2016-07-25 22:41:57]

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

[commit-bot: I haz the power, 2016-07-25 22:41:58]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-07-25 22:50:17]

lgtm

[ccameron, 2016-07-25 22:53:27]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-07-25 22:53:28]

The patchset sent to the CQ was uploaded after l-g-t-m from hubbe@chromium.org,
enne@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2161293002/#ps140001
(title: "incorporate review feedback")

[commit-bot: I haz the power, 2016-07-25 22:54:15]

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-07-25 23:00:23]

Description was changed from

==========
Color: Separate ICCProfile and ColorSpace structures

Remove the extra effort being made to avoid replication of ICC profile
data across object instances. With the two separate structures, we will
always know when we have the "heavy" version.

Add plumbing to pass ColorSpace as part of cc::TransferableResource.

Note that the functionality to convert between ICCProfile and
ColorSpace is not present in this patch. 

BUG=622133
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Color: Separate ICCProfile and ColorSpace structures

Remove the extra effort being made to avoid replication of ICC profile
data across object instances. With the two separate structures, we will
always know when we have the "heavy" version.

Add plumbing to pass ColorSpace as part of cc::TransferableResource.

Note that the functionality to convert between ICCProfile and
ColorSpace is not present in this patch. 

BUG=622133
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-07-25 23:00:28]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-25 23:02:31]

Description was changed from

==========
Color: Separate ICCProfile and ColorSpace structures

Remove the extra effort being made to avoid replication of ICC profile
data across object instances. With the two separate structures, we will
always know when we have the "heavy" version.

Add plumbing to pass ColorSpace as part of cc::TransferableResource.

Note that the functionality to convert between ICCProfile and
ColorSpace is not present in this patch. 

BUG=622133
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Color: Separate ICCProfile and ColorSpace structures

Remove the extra effort being made to avoid replication of ICC profile
data across object instances. With the two separate structures, we will
always know when we have the "heavy" version.

Add plumbing to pass ColorSpace as part of cc::TransferableResource.

Note that the functionality to convert between ICCProfile and
ColorSpace is not present in this patch.

BUG=622133
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel

Committed: https://crrev.com/efdab1600b787e4e1f82fb05ae6a397d90a0d7b5
Cr-Commit-Position: refs/heads/master@{#407622}
==========

[commit-bot: I haz the power, 2016-07-25 23:02:33]

Patchset 8 (id:??) landed as
https://crrev.com/efdab1600b787e4e1f82fb05ae6a397d90a0d7b5
Cr-Commit-Position: refs/heads/master@{#407622}