color: Towards ColorTransform optimizations and code generation

color: Towards ColorTransform optimizations and code generation

Make ColorTransform store a list of individual steps, using the
separate type ColorTransform::Step. Move the optimization of
steps into a separate ColorTransformInternal::Simplify method,
called after constructing the full chain.

For readability, change ColorTransformInternal::Simplify to do
double-dispatch by adding a "get subclass of type" method instead
of using visitor pattern.

Use a ScopedQcmsProfile to manage the lifetime of QCMS transforms, and
add the ability to Join QCMS-based transform steps.

Separate frequently-used SkColorSpace functions into a new file
skia_color_space_util.cc/h.

A follow-on patch will allow merging arbitrary SkColorSpaceTransferFns
and add shader code generation.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2696603003
Cr-Commit-Position: refs/heads/master@{#450647}
Committed: https://chromium.googlesource.com/chromium/src/+/e2fc74b70d6ea11c08c6df1ef2b1372eccc5385f

[ccameron, 2017-02-13 19:56:57]

Description was changed from

==========
color: Towards ColorTransform optimizations

Separate frequently-used SkColorSpace functions into a new file
skia_color_space_util.cc/h.

Add the implementation for ColorSpace::CreateCustom, and use this in
ICCProfile.

Make ColorTransform hold an explicit list of steps. Use queries of the
number of steps to verify ColorTransforms have been appropriately
optimized.

Use a ScopedQcmsProfile to manage the lifetime of QCMS transforms, and
add the ability to Join QCMS-based transform steps.

BUG=667966
==========

to

==========
color: Towards ColorTransform optimizations and code generation

Make ColorTransform store a list of individual steps, using the
separate type ColorTransform::Step. Move the optimization of
steps into a separate ColorTransform::Simplify method called after
constructing the full chain.

Use a ScopedQcmsProfile to manage the lifetime of QCMS transforms, and
add the ability to Join QCMS-based transform steps.

Separate frequently-used SkColorSpace functions into a new file
skia_color_space_util.cc/h.

A follow-on patch will allow merging arbitrary SkColorSpaceTransferFns
and add shader code generation.

BUG=667966
==========

[ccameron, 2017-02-13 19:56:57]

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

[ccameron, 2017-02-13 19:59:58]

ptal

I'm cleaning up a follow-on patch will allow merging arbitrary
SkColorSpaceTransferFns and add shader code generation. I wanted to do that
change separately from this one so that the diffs would be clearer (and also to
separate structural and functional changes). This patch should be a no-op
(except for allowing QCMS chaining).

[hubbe, 2017-02-13 22:03:18]

Could you please explain why you need the Step class?
Replacing a virtual interface with a pimpl seems like a step in the wrong
direction to me, but I suspect that I'm just missing the point. Why do we need
to do this?

(I didn't finish the review, as I felt a need to understand things better
first...)

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

https://codereview.chromium.org/2696603003/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:8: #include <cmath>
verbose :)

https://codereview.chromium.org/2696603003/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:148: float c = -0.02025f;  // FromLinear(BT1361_ECG,
-l)
Not a fan of refactoring and adding slight modifications to functionality in the
same CL.

https://codereview.chromium.org/2696603003/diff/40001/ui/gfx/color_transform.h
File ui/gfx/color_transform.h (right):

https://codereview.chromium.org/2696603003/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.h:30: void transform(TriStim* colors, size_t num);
I'm not a big fan of this, because it makes it impossible to extend the
ColorTransform class.

[ccameron, 2017-02-13 23:18:42]

I don't feel that this was so much changing the structure of ColorTransform and
ColorTransformInternal, as just making the structure that was already there more
explicit.

In the code as it is before this change
- A ColorTransform is a sequence of sub-steps which can be merged or deleted.
- A ColorTransformInternal is one of those sub-steps.

The fact that a ColorTransformInternal was also a ColorTransform was confusing
to me, and that was why I made it more explicit what the relationship was.

There's no loss of generality here -- anything we wanted to do in a subclass of
ColorTransform can be done in a sub-class of ColorTransform::Step.

This also really helps with testing -- we know that we're writing something to
optimize steps away, so it's nice to have the function to verify how many steps
the transform has.

I also like that optimization step is a completely separate step (we just call
"Simplify" or we don't).

Moreover, for shader generation, it's likely that we'll have a different
behavior for code generation at the higher level (ColorTransform will emit a
function to call) compared with the lower level (ColorTransform::Step will just
emit part of the body of that function).

So, I'd really like to be able to proceed with this.

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

https://codereview.chromium.org/2696603003/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:8: #include <cmath>
On 2017/02/13 22:03:18, hubbe wrote:
> verbose :)

Agree :/

...but I got a whole bunch of presubmit errors errors for using the C math
functions recently (IIRC in cc/), so I've started mass-switching things.

https://codereview.chromium.org/2696603003/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:148: float c = -0.02025f;  // FromLinear(BT1361_ECG,
-l)
On 2017/02/13 22:03:18, hubbe wrote:
> Not a fan of refactoring and adding slight modifications to functionality in
the
> same CL.

This should be functionally identical -- it just rolls the constant in, so that
we don't have the dependency between functions and so that we don't re-compute
the same value repeatedly.

I've removed this for now.

[chromium-reviews, 2017-02-13 23:46:55]

On Mon, Feb 13, 2017 at 3:18 PM, <ccameron@chromium.org> wrote:

> I don't feel that this was so much changing the structure of
> ColorTransform and
> ColorTransformInternal, as just making the structure that was already
> there more
> explicit.
>
> In the code as it is before this change
> - A ColorTransform is a sequence of sub-steps which can be merged or
> deleted.
>

I see a colortransform as a function which can map colors.
The steps are an entirely internal detail.


> - A ColorTransformInternal is one of those sub-steps.
>
> The fact that a ColorTransformInternal was also a ColorTransform was
> confusing
> to me, and that was why I made it more explicit what the relationship was.
>
>
To me it seems like you changed a color transform from "any function" to "a
list of pre-defined steps".


> There's no loss of generality here -- anything we wanted to do in a
> subclass of
> ColorTransform can be done in a sub-class of ColorTransform::Step.
>
>
Except it was previously possible to override and implement  your own
transforms.


> This also really helps with testing -- we know that we're writing
> something to
> optimize steps away, so it's nice to have the function to verify how many
> steps
> the transform has.
>
>
Agreed, although there are other ways to do that, not sure if they are
better or worse though.


> I also like that optimization step is a completely separate step (we just
> call
> "Simplify" or we don't).
>
>
Not sure why, it does exactly the same thing.


> Moreover, for shader generation, it's likely that we'll have a different
> behavior for code generation at the higher level (ColorTransform will emit
> a
> function to call) compared with the lower level (ColorTransform::Step will
> just
> emit part of the body of that function).
>
> I was envisioning just adding a function like:

virtual bool AppendGLSL(string *glsl) { return false; }

to ColorTransform. I think it's easy enough for the caller to wrap that
in a function, no?


> So, I'd really like to be able to proceed with this.
>
>
How about putting the steps inside ColorTransformInternal instead?
We can put ColorTransformInternal into it's own header file and expose it
for testing purposes.


> https://codereview.chromium.org/2696603003/diff/40001/ui/
> gfx/color_transform.cc
> File ui/gfx/color_transform.cc (right):
>
> https://codereview.chromium.org/2696603003/diff/40001/ui/
> gfx/color_transform.cc#newcode8
> ui/gfx/color_transform.cc:8: #include <cmath>
> On 2017/02/13 22:03:18, hubbe wrote:
> > verbose :)
>
> Agree :/
>
> ...but I got a whole bunch of presubmit errors errors for using the C
> math functions recently (IIRC in cc/), so I've started mass-switching
> things.
>
> https://codereview.chromium.org/2696603003/diff/40001/ui/
> gfx/color_transform.cc#newcode148
> ui/gfx/color_transform.cc:148: float c = -0.02025f; //
> FromLinear(BT1361_ECG, -l)
> On 2017/02/13 22:03:18, hubbe wrote:
> > Not a fan of refactoring and adding slight modifications to
> functionality in the
> > same CL.
>
> This should be functionally identical -- it just rolls the constant in,
> so that we don't have the dependency between functions and so that we
> don't re-compute the same value repeatedly.
>
> I've removed this for now.
>
> https://codereview.chromium.org/2696603003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ccameron, 2017-02-14 09:51:10]

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

[ccameron, 2017-02-14 09:53:32]

On 2017/02/13 23:46:55, chromium-reviews wrote:

> How about putting the steps inside ColorTransformInternal instead?
> We can put ColorTransformInternal into it's own header file and expose it
> for testing purposes.

Sure -- I've updated the patch to use a ColorTransformInternal class. For the
purposes of keeping the diffs parse-able, I've kept it embedded inside the .cc
file, but we can move it around in subsequent patches.

[ccameron, 2017-02-14 09:56:16]

Description was changed from

==========
color: Towards ColorTransform optimizations and code generation

Make ColorTransform store a list of individual steps, using the
separate type ColorTransform::Step. Move the optimization of
steps into a separate ColorTransform::Simplify method called after
constructing the full chain.

Use a ScopedQcmsProfile to manage the lifetime of QCMS transforms, and
add the ability to Join QCMS-based transform steps.

Separate frequently-used SkColorSpace functions into a new file
skia_color_space_util.cc/h.

A follow-on patch will allow merging arbitrary SkColorSpaceTransferFns
and add shader code generation.

BUG=667966
==========

to

==========
color: Towards ColorTransform optimizations and code generation

Make ColorTransform store a list of individual steps, using the
separate type ColorTransform::Step. Move the optimization of
steps into a separate ColorTransform::Simplify method called after
constructing the full chain.

Use a ScopedQcmsProfile to manage the lifetime of QCMS transforms, and
add the ability to Join QCMS-based transform steps.

Separate frequently-used SkColorSpace functions into a new file
skia_color_space_util.cc/h.

A follow-on patch will allow merging arbitrary SkColorSpaceTransferFns
and add shader code generation.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[enne (OOO), 2017-02-14 17:36:31]

cc rubber stamp lgtm

[hubbe, 2017-02-14 19:04:51]

Not sure why I want ColorTransform to be an interface so badly, but I like this
CL a lot better now. :)

Finished the review, and I have a few suggestions.

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

https://codereview.chromium.org/2696603003/diff/80001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:233: virtual ColorTransformFromLinear* GetFromLinear()
{ return nullptr; }
When I first saw this, I thought:
Why'd you go and destroy my beautful visitor pattern?
However, after reading it a few times, it's pretty obvious that the new code is
more straightforward than the old one. (Especially to anybody who doesn't know
what "visitor pattern" or "double dispatch" means.)
Would have been nice to have this in the CL description along with the
motivation for it though.

https://codereview.chromium.org/2696603003/diff/80001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:267: static void Append(ColorSpace from,
How about making this non-static and have it append to steps_ ?
Then there will be no need to take a steplist in the constructor I think.

https://codereview.chromium.org/2696603003/diff/80001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:272: static void Simplify(ColorTransformStepList*
steps);
Make non-static and operate on steps_ ?

https://codereview.chromium.org/2696603003/diff/80001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:535: void ColorTransformInternal::Append(ColorSpace
from,
Append is kind of a strange name for this.
Maybe AppendColorSpaceToColorSpaceTransform() ?

https://codereview.chromium.org/2696603003/diff/80001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:683: std::unique_ptr<ColorTransform>
ColorTransform::NewColorTransform(
Probably move all the code in this function into
ColorTransformInternal::ColorTransformInternal()

[ccameron, 2017-02-15 00:10:46]

Description was changed from

==========
color: Towards ColorTransform optimizations and code generation

Make ColorTransform store a list of individual steps, using the
separate type ColorTransform::Step. Move the optimization of
steps into a separate ColorTransform::Simplify method called after
constructing the full chain.

Use a ScopedQcmsProfile to manage the lifetime of QCMS transforms, and
add the ability to Join QCMS-based transform steps.

Separate frequently-used SkColorSpace functions into a new file
skia_color_space_util.cc/h.

A follow-on patch will allow merging arbitrary SkColorSpaceTransferFns
and add shader code generation.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
color: Towards ColorTransform optimizations and code generation

Make ColorTransform store a list of individual steps, using the
separate type ColorTransform::Step. Move the optimization of
steps into a separate ColorTransformInternal::Simplify method,
called after constructing the full chain.

For readability, change ColorTransformInternal::Simplify to do
double-dispatch by adding a "get subclass of type" method instead
of using visitor pattern.

Use a ScopedQcmsProfile to manage the lifetime of QCMS transforms, and
add the ability to Join QCMS-based transform steps.

Separate frequently-used SkColorSpace functions into a new file
skia_color_space_util.cc/h.

A follow-on patch will allow merging arbitrary SkColorSpaceTransferFns
and add shader code generation.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ccameron, 2017-02-15 00:11:31]

Thanks -- updated

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

https://codereview.chromium.org/2696603003/diff/80001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:233: virtual ColorTransformFromLinear* GetFromLinear()
{ return nullptr; }
On 2017/02/14 19:04:51, hubbe wrote:
> When I first saw this, I thought:
> Why'd you go and destroy my beautful visitor pattern?
> However, after reading it a few times, it's pretty obvious that the new code
is
> more straightforward than the old one. (Especially to anybody who doesn't know
> what "visitor pattern" or "double dispatch" means.)
> Would have been nice to have this in the CL description along with the
> motivation for it though.

Sure -- updated the CL description (after skimming Wikipedia's visitor pattern
and double dispatch articles).

https://codereview.chromium.org/2696603003/diff/80001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:267: static void Append(ColorSpace from,
On 2017/02/14 19:04:51, hubbe wrote:
> How about making this non-static and have it append to steps_ ?
> Then there will be no need to take a steplist in the constructor I think.

That makes sense -- done.

https://codereview.chromium.org/2696603003/diff/80001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:272: static void Simplify(ColorTransformStepList*
steps);
On 2017/02/14 19:04:51, hubbe wrote:
> Make non-static and operate on steps_ ?

(same deal).

https://codereview.chromium.org/2696603003/diff/80001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:535: void ColorTransformInternal::Append(ColorSpace
from,
On 2017/02/14 19:04:51, hubbe wrote:
> Append is kind of a strange name for this.
> Maybe AppendColorSpaceToColorSpaceTransform() ?

Done.

https://codereview.chromium.org/2696603003/diff/80001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:683: std::unique_ptr<ColorTransform>
ColorTransform::NewColorTransform(
On 2017/02/14 19:04:51, hubbe wrote:
> Probably move all the code in this function into
> ColorTransformInternal::ColorTransformInternal()

Good point -- done.

[hubbe, 2017-02-15 04:25:41]

lgtm

[ccameron, 2017-02-15 04:34:22]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-02-15 04:34:22]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2696603003/#ps100001
(title: "Incorporate review feedback")

[commit-bot: I haz the power, 2017-02-15 04:35:07]

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-02-15 04:46:11]

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

[commit-bot: I haz the power, 2017-02-15 04:46:13]

Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[ccameron, 2017-02-15 05:02:49]

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

[commit-bot: I haz the power, 2017-02-15 05:04: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, 2017-02-15 05:44:38]

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

[commit-bot: I haz the power, 2017-02-15 05:44:39]

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

[ccameron, 2017-02-15 07:52:38]

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

[commit-bot: I haz the power, 2017-02-15 07:52:56]

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

[ccameron, 2017-02-15 08:37:06]

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

[commit-bot: I haz the power, 2017-02-15 08:37:18]

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

[ccameron, 2017-02-15 09:34:55]

The CQ bit was unchecked by ccameron@chromium.org

[ccameron, 2017-02-15 09:35:00]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-02-15 09:35:00]

The patchset sent to the CQ was uploaded after l-g-t-m from hubbe@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2696603003/#ps160001
(title: "Keep fixing the windows build")

[commit-bot: I haz the power, 2017-02-15 09:35:16]

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-02-15 09:59:31]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487151300016890,
"parent_rev": "bd2665062187808938d8f91bf9288737380ceddd", "commit_rev":
"e2fc74b70d6ea11c08c6df1ef2b1372eccc5385f"}

[commit-bot: I haz the power, 2017-02-15 10:00:10]

Description was changed from

==========
color: Towards ColorTransform optimizations and code generation

Make ColorTransform store a list of individual steps, using the
separate type ColorTransform::Step. Move the optimization of
steps into a separate ColorTransformInternal::Simplify method,
called after constructing the full chain.

For readability, change ColorTransformInternal::Simplify to do
double-dispatch by adding a "get subclass of type" method instead
of using visitor pattern.

Use a ScopedQcmsProfile to manage the lifetime of QCMS transforms, and
add the ability to Join QCMS-based transform steps.

Separate frequently-used SkColorSpace functions into a new file
skia_color_space_util.cc/h.

A follow-on patch will allow merging arbitrary SkColorSpaceTransferFns
and add shader code generation.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
color: Towards ColorTransform optimizations and code generation

Make ColorTransform store a list of individual steps, using the
separate type ColorTransform::Step. Move the optimization of
steps into a separate ColorTransformInternal::Simplify method,
called after constructing the full chain.

For readability, change ColorTransformInternal::Simplify to do
double-dispatch by adding a "get subclass of type" method instead
of using visitor pattern.

Use a ScopedQcmsProfile to manage the lifetime of QCMS transforms, and
add the ability to Join QCMS-based transform steps.

Separate frequently-used SkColorSpace functions into a new file
skia_color_space_util.cc/h.

A follow-on patch will allow merging arbitrary SkColorSpaceTransferFns
and add shader code generation.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2696603003
Cr-Commit-Position: refs/heads/master@{#450647}
Committed:
https://chromium.googlesource.com/chromium/src/+/e2fc74b70d6ea11c08c6df1ef2b1...
==========

[commit-bot: I haz the power, 2017-02-15 10:00:11]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/e2fc74b70d6ea11c08c6df1ef2b1...