ColorTransform, transforms colors from one color space to another

ColorTransform, transforms colors from one color space to another

Support for ICC transformations will be added in a separate CL.

BUG=622133
Committed: https://crrev.com/415f9680cbce251e8c18a2d0fe3b2ff4ba9e7377
Cr-Commit-Position: refs/heads/master@{#409579}

[hubbe, 2016-08-02 00:29:44]

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

[hubbe, 2016-08-02 00:30:05]

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

[commit-bot: I haz the power, 2016-08-02 00:30:10]

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-08-02 00:45:34]

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

[commit-bot: I haz the power, 2016-08-02 00:45:35]

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_...)

[ccameron, 2016-08-02 02:47:39]

lg, couple of suggestions

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc
File ui/gfx/color_transform.cc (right):

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc#n...
ui/gfx/color_transform.cc:29: typedef std::pair<float, float> XY;
use gfx::PointF instead of std::pair<float,float>

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc#n...
ui/gfx/color_transform.cc:31: std::vector<XY> GetPrimaries(ColorSpace::PrimaryID
id) {
Don't return a std::vector here -- pass the return arguments by pointer as

void GetPrimaries(ColorSpace::PrimaryID id, ColorTransform::TriStim* red,
                  ColorTransform::TriStim* green, ColorTransform::TriStim* blue,
                  ColorTransform::TriStim* white) {
  XY red_xy, green_xy, blue_xy, white_xy;

  switch (id) {
    default:
    // If we don't know, assume BT709

    case ColorSpace::PrimaryID::BT709:
      // Red
      red_xy.set_x(0.640);
      red_xy.set_y(0.330);
      // Green

... this might as well compute the trisim values, too ... that way only this
function need think of 2d values ...


  *red = XYToTriStim(red_xy);
  *green = XYToTriStim(green_xy);
  *blue = XYToTriStim(blue_xy);
  *white = XYToTriStim(white_xy);
}

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc#n...
ui/gfx/color_transform.cc:191: ColorTransform::TriStim xy2xyz(const XY& xy) {
naming nit: rename to XYToTriStim

Function names in ui/gfx should begin with a capital.

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc#n...
ui/gfx/color_transform.cc:196: GFX_EXPORT Transform
GetPrimaryMatrix(ColorSpace::PrimaryID id) {
This can be made simpler by using the existing matrix transforms (particularly
scale matrices): The following should be equivalent (unit tests still pass):

GFX_EXPORT Transform GetPrimaryMatrix(ColorSpace::PrimaryID id) {
  const Transform bradford(
     0.8951,  0.2664, -0.1614, 0.000f,
    -0.7502,  1.7135,  0.0367, 0.000f,
     0.0389, -0.0685,  1.0296, 0.000f,
     0.0000,  0.0000,  0.0000, 1.0000);
  const Transform bradford_inv(
     0.9870,  -0.1471, 0.1600, 0.0000,
     0.4323,   0.5184, 0.0493, 0.0000,
    -0.0085,   0.0400, 0.9685, 0.0000,
     0.0000,   0.0000, 0.0000, 1.0000);
  const ColorTransform::TriStim D50(0.9642, 1.0000, 0.8249);

  ColorTransform::TriStim r_xyz, g_xyz, b_xyz, w_xyz;
  GetPrimaries(id, &r_xyz, &g_xyz, &b_xyz, &w_xyz);

  ColorTransform::TriStim w_XYZ = w_xyz;
  w_XYZ.Scale(1.0f / w_XYZ.y());
  ColorTransform::TriStim source_response = Map(bradford, w_XYZ);

  // Chromatic adaptation.
  ColorTransform::TriStim dest_response = Map(bradford, D50);
  Transform adapter;
  adapter.Scale3d(dest_response.x() / source_response.x(),
                  dest_response.y() / source_response.y(),
                  dest_response.z() / source_response.z());

  Transform result(
      r_xyz.x(), g_xyz.x(), b_xyz.x(), 0,
      r_xyz.y(), g_xyz.y(), b_xyz.y(), 0,
      r_xyz.z(), g_xyz.z(), b_xyz.z(), 0,
      0, 0, 0, 1);
  ColorTransform::TriStim conv = Map(Invert(result), w_XYZ);
  result.Scale3d(conv.x(), conv.y(), conv.z());
  result.ConcatTransform(bradford_inv);
  result.ConcatTransform(adapter);
  result.ConcatTransform(bradford);
  return result;
}

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc#n...
ui/gfx/color_transform.cc:236: GFX_EXPORT float
fromLinear(ColorSpace::TransferID id, float v) {
Nit: capitalization: FromLinear.

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc#n...
ui/gfx/color_transform.cc:341: GFX_EXPORT float toLinear(ColorSpace::TransferID
id, float v) {
nit: ToLinear

[hubbe, 2016-08-02 08:09:05]

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

[commit-bot: I haz the power, 2016-08-02 08:09:22]

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

[hubbe, 2016-08-02 08:10:57]

PTAL

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc
File ui/gfx/color_transform.cc (right):

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc#n...
ui/gfx/color_transform.cc:29: typedef std::pair<float, float> XY;
On 2016/08/02 02:47:38, ccameron wrote:
> use gfx::PointF instead of std::pair<float,float>

Gone.

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc#n...
ui/gfx/color_transform.cc:31: std::vector<XY> GetPrimaries(ColorSpace::PrimaryID
id) {
On 2016/08/02 02:47:38, ccameron wrote:
> Don't return a std::vector here -- pass the return arguments by pointer as
> 
> void GetPrimaries(ColorSpace::PrimaryID id, ColorTransform::TriStim* red,
>                   ColorTransform::TriStim* green, ColorTransform::TriStim*
blue,
>                   ColorTransform::TriStim* white) {
>   XY red_xy, green_xy, blue_xy, white_xy;
> 
>   switch (id) {
>     default:
>     // If we don't know, assume BT709
> 
>     case ColorSpace::PrimaryID::BT709:
>       // Red
>       red_xy.set_x(0.640);
>       red_xy.set_y(0.330);
>       // Green
> 
> ... this might as well compute the trisim values, too ... that way only this
> function need think of 2d values ...
> 
> 
>   *red = XYToTriStim(red_xy);
>   *green = XYToTriStim(green_xy);
>   *blue = XYToTriStim(blue_xy);
>   *white = XYToTriStim(white_xy);
> }

Calling the arguments red/green/blue is somewhat misleading, so I'm using
TriStim primaries[4] instead.

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc#n...
ui/gfx/color_transform.cc:191: ColorTransform::TriStim xy2xyz(const XY& xy) {
On 2016/08/02 02:47:39, ccameron wrote:
> naming nit: rename to XYToTriStim
> 
> Function names in ui/gfx should begin with a capital.

That doesn't specify what color space the tristim is in though.

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc#n...
ui/gfx/color_transform.cc:196: GFX_EXPORT Transform
GetPrimaryMatrix(ColorSpace::PrimaryID id) {
On 2016/08/02 02:47:38, ccameron wrote:
> This can be made simpler by using the existing matrix transforms (particularly
> scale matrices): The following should be equivalent (unit tests still pass):
> 
> GFX_EXPORT Transform GetPrimaryMatrix(ColorSpace::PrimaryID id) {
>   const Transform bradford(
>      0.8951,  0.2664, -0.1614, 0.000f,
>     -0.7502,  1.7135,  0.0367, 0.000f,
>      0.0389, -0.0685,  1.0296, 0.000f,
>      0.0000,  0.0000,  0.0000, 1.0000);
>   const Transform bradford_inv(
>      0.9870,  -0.1471, 0.1600, 0.0000,
>      0.4323,   0.5184, 0.0493, 0.0000,
>     -0.0085,   0.0400, 0.9685, 0.0000,
>      0.0000,   0.0000, 0.0000, 1.0000);
>   const ColorTransform::TriStim D50(0.9642, 1.0000, 0.8249);
> 
>   ColorTransform::TriStim r_xyz, g_xyz, b_xyz, w_xyz;
>   GetPrimaries(id, &r_xyz, &g_xyz, &b_xyz, &w_xyz);
> 
>   ColorTransform::TriStim w_XYZ = w_xyz;
>   w_XYZ.Scale(1.0f / w_XYZ.y());
>   ColorTransform::TriStim source_response = Map(bradford, w_XYZ);
> 
>   // Chromatic adaptation.
>   ColorTransform::TriStim dest_response = Map(bradford, D50);
>   Transform adapter;
>   adapter.Scale3d(dest_response.x() / source_response.x(),
>                   dest_response.y() / source_response.y(),
>                   dest_response.z() / source_response.z());
> 
>   Transform result(
>       r_xyz.x(), g_xyz.x(), b_xyz.x(), 0,
>       r_xyz.y(), g_xyz.y(), b_xyz.y(), 0,
>       r_xyz.z(), g_xyz.z(), b_xyz.z(), 0,
>       0, 0, 0, 1);
>   ColorTransform::TriStim conv = Map(Invert(result), w_XYZ);
>   result.Scale3d(conv.x(), conv.y(), conv.z());
>   result.ConcatTransform(bradford_inv);
>   result.ConcatTransform(adapter);
>   result.ConcatTransform(bradford);
>   return result;
> }

I like Scale3d, but I think the order in your function is a bit odd.
ConcatTransform seems less clear than matrix multiplication to me.

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc#n...
ui/gfx/color_transform.cc:236: GFX_EXPORT float
fromLinear(ColorSpace::TransferID id, float v) {
On 2016/08/02 02:47:39, ccameron wrote:
> Nit: capitalization: FromLinear.

Done.

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc#n...
ui/gfx/color_transform.cc:341: GFX_EXPORT float toLinear(ColorSpace::TransferID
id, float v) {
On 2016/08/02 02:47:38, ccameron wrote:
> nit: ToLinear

Done.

[commit-bot: I haz the power, 2016-08-02 08:24:41]

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

[commit-bot: I haz the power, 2016-08-02 08:24:42]

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_...)

[hubbe, 2016-08-02 17:10:40]

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

[commit-bot: I haz the power, 2016-08-02 17:11:16]

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

[ccameron, 2016-08-02 17:12:57]

lgtm

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc
File ui/gfx/color_transform.cc (right):

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc#n...
ui/gfx/color_transform.cc:31: std::vector<XY> GetPrimaries(ColorSpace::PrimaryID
id) {
On 2016/08/02 08:10:57, hubbe wrote:
> On 2016/08/02 02:47:38, ccameron wrote:
> > Don't return a std::vector here -- pass the return arguments by pointer as
> > 
> > void GetPrimaries(ColorSpace::PrimaryID id, ColorTransform::TriStim* red,
> >                   ColorTransform::TriStim* green, ColorTransform::TriStim*
> blue,
> >                   ColorTransform::TriStim* white) {
> >   XY red_xy, green_xy, blue_xy, white_xy;
> > 
> >   switch (id) {
> >     default:
> >     // If we don't know, assume BT709
> > 
> >     case ColorSpace::PrimaryID::BT709:
> >       // Red
> >       red_xy.set_x(0.640);
> >       red_xy.set_y(0.330);
> >       // Green
> > 
> > ... this might as well compute the trisim values, too ... that way only this
> > function need think of 2d values ...
> > 
> > 
> >   *red = XYToTriStim(red_xy);
> >   *green = XYToTriStim(green_xy);
> >   *blue = XYToTriStim(blue_xy);
> >   *white = XYToTriStim(white_xy);
> > }
> 
> Calling the arguments red/green/blue is somewhat misleading, so I'm using
> TriStim primaries[4] instead.

Yeah, I was wondering about that ... in particular because of ...

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc#n...
ui/gfx/color_transform.cc:196: GFX_EXPORT Transform
GetPrimaryMatrix(ColorSpace::PrimaryID id) {
On 2016/08/02 08:10:57, hubbe wrote:
> On 2016/08/02 02:47:38, ccameron wrote:
> > This can be made simpler by using the existing matrix transforms
(particularly
> > scale matrices): The following should be equivalent (unit tests still pass):
> > 
> > GFX_EXPORT Transform GetPrimaryMatrix(ColorSpace::PrimaryID id) {
> >   const Transform bradford(
> >      0.8951,  0.2664, -0.1614, 0.000f,
> >     -0.7502,  1.7135,  0.0367, 0.000f,
> >      0.0389, -0.0685,  1.0296, 0.000f,
> >      0.0000,  0.0000,  0.0000, 1.0000);
> >   const Transform bradford_inv(
> >      0.9870,  -0.1471, 0.1600, 0.0000,
> >      0.4323,   0.5184, 0.0493, 0.0000,
> >     -0.0085,   0.0400, 0.9685, 0.0000,
> >      0.0000,   0.0000, 0.0000, 1.0000);
> >   const ColorTransform::TriStim D50(0.9642, 1.0000, 0.8249);
> > 
> >   ColorTransform::TriStim r_xyz, g_xyz, b_xyz, w_xyz;
> >   GetPrimaries(id, &r_xyz, &g_xyz, &b_xyz, &w_xyz);
> > 
> >   ColorTransform::TriStim w_XYZ = w_xyz;
> >   w_XYZ.Scale(1.0f / w_XYZ.y());
> >   ColorTransform::TriStim source_response = Map(bradford, w_XYZ);
> > 
> >   // Chromatic adaptation.
> >   ColorTransform::TriStim dest_response = Map(bradford, D50);
> >   Transform adapter;
> >   adapter.Scale3d(dest_response.x() / source_response.x(),
> >                   dest_response.y() / source_response.y(),
> >                   dest_response.z() / source_response.z());
> > 
> >   Transform result(
> >       r_xyz.x(), g_xyz.x(), b_xyz.x(), 0,
> >       r_xyz.y(), g_xyz.y(), b_xyz.y(), 0,
> >       r_xyz.z(), g_xyz.z(), b_xyz.z(), 0,
> >       0, 0, 0, 1);
> >   ColorTransform::TriStim conv = Map(Invert(result), w_XYZ);
> >   result.Scale3d(conv.x(), conv.y(), conv.z());
> >   result.ConcatTransform(bradford_inv);
> >   result.ConcatTransform(adapter);
> >   result.ConcatTransform(bradford);
> >   return result;
> > }
> 
> I like Scale3d, but I think the order in your function is a bit odd.
> ConcatTransform seems less clear than matrix multiplication to me.

Makes sense.

https://codereview.chromium.org/2203663002/diff/1/ui/gfx/color_transform.cc#n...
ui/gfx/color_transform.cc:201: ColorTransform::TriStim Wxyz =
xy2xyz(primaries[3]);
Y was wondering why we didn't call them just "primaries0...3", here. Huh.

[commit-bot: I haz the power, 2016-08-02 17:55:28]

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

[commit-bot: I haz the power, 2016-08-02 17:55:29]

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

[hubbe, 2016-08-02 19:31:40]

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

[commit-bot: I haz the power, 2016-08-02 19:32:51]

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-08-02 20:11:22]

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

[commit-bot: I haz the power, 2016-08-02 20:11:23]

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

[hubbe, 2016-08-02 22:22:33]

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

[commit-bot: I haz the power, 2016-08-02 22:23:15]

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-08-02 23:06:41]

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

[commit-bot: I haz the power, 2016-08-02 23:06:42]

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

[hubbe, 2016-08-02 23:27:56]

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

[commit-bot: I haz the power, 2016-08-02 23:28:35]

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-08-03 00:36:16]

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

[commit-bot: I haz the power, 2016-08-03 00:36:16]

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

[hubbe, 2016-08-03 05:03:52]

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

[commit-bot: I haz the power, 2016-08-03 05:04:10]

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-08-03 06:31:50]

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

[commit-bot: I haz the power, 2016-08-03 06:31:51]

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

[hubbe, 2016-08-03 17:53:44]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2016-08-03 17:53:45]

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/2203663002/#ps100001
(title: "use floats")

[commit-bot: I haz the power, 2016-08-03 17:54:25]

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-08-03 18:44:13]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-03 18:46:12]

Description was changed from

==========
ColorTransform, transforms colors from one color space to another

Support for ICC transformations will be added in a separate CL.

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

to

==========
ColorTransform, transforms colors from one color space to another

Support for ICC transformations will be added in a separate CL.

BUG=622133

Committed: https://crrev.com/415f9680cbce251e8c18a2d0fe3b2ff4ba9e7377
Cr-Commit-Position: refs/heads/master@{#409579}
==========

[commit-bot: I haz the power, 2016-08-03 18:46:13]

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