Add some actual data to gfx::ColorSpace

Add some actual data to gfx::ColorSpace

To be followed up with gfx::ColorTransform which will actually do something with the data.

BUG=622133
Committed: https://crrev.com/09cc75df80f24329aca8576a76618d801dd13586
Cr-Commit-Position: refs/heads/master@{#408209}

[hubbe, 2016-07-26 23:05:27]

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

[commit-bot: I haz the power, 2016-07-26 23:06:02]

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-27 00:37:31]

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

[commit-bot: I haz the power, 2016-07-27 00:37:32]

Dry run: This issue passed the CQ dry run.

[hubbe, 2016-07-27 05:57:00]

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

[ccameron, 2016-07-27 17:10:30]

lgtm with the style nits.

Dumb question: Why do we specify both the matrix and the primaries? Doesn't the
matrix convey all of the relevant info about the primaries?

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

https://codereview.chromium.org/2183953002/diff/1/ui/gfx/color_space.h#newcode28
ui/gfx/color_space.h:28: enum PrimaryID : int16_t {
nit: Please change this to

enum class Primary {
  RESERVED0
};

This way we can just say gfx::ColorSpace::Primary::BT709

https://codereview.chromium.org/2183953002/diff/1/ui/gfx/color_space.h#newcode51
ui/gfx/color_space.h:51: enum TransferID : uint16_t {
(same thing, "enum class TransferFunction", and no "TRC_" prefix).

https://codereview.chromium.org/2183953002/diff/1/ui/gfx/color_space.h#newcode81
ui/gfx/color_space.h:81: // The first 0-255 values should match the H264
specification.
(& again)

https://codereview.chromium.org/2183953002/diff/1/ui/gfx/color_space.h#newcod...
ui/gfx/color_space.h:105: bool isValid() const;
Remove isValid cause it has no callers now (with the comments in IPCs).

We may need it in the future, but we'll have to be precise about its meaning
there.

https://codereview.chromium.org/2183953002/diff/1/ui/gfx/color_space.h#newcod...
ui/gfx/color_space.h:121: // TODO(hubbe): Range might be different depending on
bit depth, if so,
I'd go for putting this in an enum from the get-go.

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

https://codereview.chromium.org/2183953002/diff/1/ui/gfx/icc_profile.h#newcode69
ui/gfx/icc_profile.h:69: 
Good riddance.

https://codereview.chromium.org/2183953002/diff/1/ui/gfx/ipc/color/gfx_param_...
File ui/gfx/ipc/color/gfx_param_traits.cc (right):

https://codereview.chromium.org/2183953002/diff/1/ui/gfx/ipc/color/gfx_param_...
ui/gfx/ipc/color/gfx_param_traits.cc:19: return;
Just remove the "if is valid part" -- unconditionally read icc_profile_id_

I put the conditional there to elide reading the 4x3 matrix, back when it was
there. We can re-add it if it matters.

[hubbe, 2016-07-27 17:44:04]

PTAL

> Dumb question: Why do we specify both the matrix and the primaries? Doesn't
the matrix convey all of the relevant info about the primaries?

The transfer function goes between the matrix and the primary matrix. (Except in
the case of BT2020_CL)

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

https://codereview.chromium.org/2183953002/diff/1/ui/gfx/color_space.h#newcode28
ui/gfx/color_space.h:28: enum PrimaryID : int16_t {
On 2016/07/27 17:10:30, ccameron wrote:
> nit: Please change this to
> 
> enum class Primary {
>   RESERVED0
> };
> 
> This way we can just say gfx::ColorSpace::Primary::BT709

Done.
Although I'm still calling it PrimaryID, as I feel that is more descriptive.

https://codereview.chromium.org/2183953002/diff/1/ui/gfx/color_space.h#newcode51
ui/gfx/color_space.h:51: enum TransferID : uint16_t {
On 2016/07/27 17:10:30, ccameron wrote:
> (same thing, "enum class TransferFunction", and no "TRC_" prefix).

Done, but still calling it TransferID

https://codereview.chromium.org/2183953002/diff/1/ui/gfx/color_space.h#newcode81
ui/gfx/color_space.h:81: // The first 0-255 values should match the H264
specification.
On 2016/07/27 17:10:29, ccameron wrote:
> (& again)

Done.

https://codereview.chromium.org/2183953002/diff/1/ui/gfx/color_space.h#newcod...
ui/gfx/color_space.h:105: bool isValid() const;
On 2016/07/27 17:10:30, ccameron wrote:
> Remove isValid cause it has no callers now (with the comments in IPCs).
> 
> We may need it in the future, but we'll have to be precise about its meaning
> there.

Done.

https://codereview.chromium.org/2183953002/diff/1/ui/gfx/color_space.h#newcod...
ui/gfx/color_space.h:121: // TODO(hubbe): Range might be different depending on
bit depth, if so,
On 2016/07/27 17:10:30, ccameron wrote:
> I'd go for putting this in an enum from the get-go.

Done.

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

https://codereview.chromium.org/2183953002/diff/1/ui/gfx/icc_profile.h#newcode69
ui/gfx/icc_profile.h:69: 
On 2016/07/27 17:10:30, ccameron wrote:
> Good riddance.

Acknowledged.

https://codereview.chromium.org/2183953002/diff/1/ui/gfx/ipc/color/gfx_param_...
File ui/gfx/ipc/color/gfx_param_traits.cc (right):

https://codereview.chromium.org/2183953002/diff/1/ui/gfx/ipc/color/gfx_param_...
ui/gfx/ipc/color/gfx_param_traits.cc:19: return;
On 2016/07/27 17:10:30, ccameron wrote:
> Just remove the "if is valid part" -- unconditionally read icc_profile_id_
> 
> I put the conditional there to elide reading the 4x3 matrix, back when it was
> there. We can re-add it if it matters.

The intention is to only read/encode the matrix if primaries is
PrimaryID::CUSTOM

[hubbe, 2016-07-27 17:44:57]

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

[hubbe, 2016-07-27 17:44:57]

Adding tsepez for IPC review.

[Tom Sepez, 2016-07-27 17:52:26]

https://codereview.chromium.org/2183953002/diff/20001/ui/gfx/color_space.cc
File ui/gfx/color_space.cc (right):

https://codereview.chromium.org/2183953002/diff/20001/ui/gfx/color_space.cc#n...
ui/gfx/color_space.cc:35: color_space.primaries_ = PrimaryID::BT709;
nit: Why not just a one-liner like
  return ColorSpace(PrimaryID::BT709, ransferID::IEC61966_2_1,  MatrixID::RGB,
RangeID::FULL);

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

https://codereview.chromium.org/2183953002/diff/20001/ui/gfx/color_space.h#ne...
ui/gfx/color_space.h:48: LAST = XYZ_D50
This is kind of strange, in that if you were to try and pass CUSTOM into the
IPC, it would CHECK() given the IPC_ENUM_TRAITS_MAX_VALUE() later on.

[hubbe, 2016-07-27 18:05:53]

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

[commit-bot: I haz the power, 2016-07-27 18:06:29]

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

[hubbe, 2016-07-27 18:08:29]

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

[hubbe, 2016-07-27 18:08:45]

https://codereview.chromium.org/2183953002/diff/20001/ui/gfx/color_space.cc
File ui/gfx/color_space.cc (right):

https://codereview.chromium.org/2183953002/diff/20001/ui/gfx/color_space.cc#n...
ui/gfx/color_space.cc:35: color_space.primaries_ = PrimaryID::BT709;
On 2016/07/27 17:52:26, Tom Sepez wrote:
> nit: Why not just a one-liner like
>   return ColorSpace(PrimaryID::BT709, ransferID::IEC61966_2_1,  MatrixID::RGB,
> RangeID::FULL);

Done.

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

https://codereview.chromium.org/2183953002/diff/20001/ui/gfx/color_space.h#ne...
ui/gfx/color_space.h:48: LAST = XYZ_D50
On 2016/07/27 17:52:26, Tom Sepez wrote:
> This is kind of strange, in that if you were to try and pass CUSTOM into the
> IPC, it would CHECK() given the IPC_ENUM_TRAITS_MAX_VALUE() later on.  

OOps, done, fixed here and below.

[commit-bot: I haz the power, 2016-07-27 18:08:55]

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

[Tom Sepez, 2016-07-27 18:26:30]

lgtm

[commit-bot: I haz the power, 2016-07-27 19:04:58]

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

[commit-bot: I haz the power, 2016-07-27 19:04:59]

Dry run: This issue passed the CQ dry run.

[hubbe, 2016-07-27 19:10:09]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2016-07-27 19:10:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/2183953002/#ps60001
(title: "fix LAST")

[commit-bot: I haz the power, 2016-07-27 19:10:27]

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-27 19:14:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-27 19:16:26]

Description was changed from

==========
Add some actual data to gfx::ColorSpace

To be followed up with gfx::ColorTransform which will actually do something with
the data.

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

to

==========
Add some actual data to gfx::ColorSpace

To be followed up with gfx::ColorTransform which will actually do something with
the data.

BUG=622133

Committed: https://crrev.com/09cc75df80f24329aca8576a76618d801dd13586
Cr-Commit-Position: refs/heads/master@{#408209}
==========

[commit-bot: I haz the power, 2016-07-27 19:16:27]

Patchset 4 (id:??) landed as
https://crrev.com/09cc75df80f24329aca8576a76618d801dd13586
Cr-Commit-Position: refs/heads/master@{#408209}