Introduce GrColorSpaceXform, for gamut conversion on textures

Introduce GrColorSpaceXform, for gamut conversion on textures

GrTextureAccess optionally includes an instance, computed from the src
and dst color spaces. In all common cases (no color space for either src
or dst, or same color space for both), no object is allocated.

This change is orthogonal to my attempts to get color space attached to
render targets - regardless of how we choose to do that, this will give
us the source color space at all points where we are connecting src to
dst.

There are many dangling injection points where I've been inserting
nullptr, but I have a record of all of them. Additionally, there are now
three places (the most common simple paths for bitmap/image rendering)
where things are plumbed enough that I expect to have access to the dst
color space (all marked with XFORMTODO).

In addition to getting the dst color space, I need to inject shader code
and uniform uploading for appendTextureLookup and friends.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2154753003

Committed: https://skia.googlesource.com/skia/+/54f30c13fc0a5d89797fc9be5f0fb1050d96b6f4

[Brian Osman, 2016-07-15 14:51:07]

Description was changed from

==========
Introduce GrColorSpaceXform, for gamut conversion on textures

GrTextureAccess optionally includes an instance, computed from the src
and dst color spaces. In all common cases (no color space for either src
or dst, or same color space for both), no object is allocated.

This change is orthogonal to my attempts to get color space attached to
render targets - regardless of how we choose to do that, this will give
us the source color space at all points where we are connecting src to
dst.

There are many dangling injection points where I've been inserting
nullptr, but I have a record of all of them. Additionally, there are now
three places (the most common simple paths for bitmap/image rendering)
where things are plumbed enough that I expect to have access to the dst
color space (all marked with XFORMTODO).

In addition to getting the dst color space, I need to inject shader code
and uniform uploading for appendTextureLookup and friends.

BUG=skia:
==========

to

==========
Introduce GrColorSpaceXform, for gamut conversion on textures

GrTextureAccess optionally includes an instance, computed from the src
and dst color spaces. In all common cases (no color space for either src
or dst, or same color space for both), no object is allocated.

This change is orthogonal to my attempts to get color space attached to
render targets - regardless of how we choose to do that, this will give
us the source color space at all points where we are connecting src to
dst.

There are many dangling injection points where I've been inserting
nullptr, but I have a record of all of them. Additionally, there are now
three places (the most common simple paths for bitmap/image rendering)
where things are plumbed enough that I expect to have access to the dst
color space (all marked with XFORMTODO).

In addition to getting the dst color space, I need to inject shader code
and uniform uploading for appendTextureLookup and friends.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2154753003
==========

[Brian Osman, 2016-07-15 14:55:45]

The most interesting parts of this are:

- GrColorSpaceXform.h/cpp
- GrTextureAccess.h
- GrTextureParamsAdjuster.cpp
- SkBitmapProcShader.cpp

Pretty much everything else is just connecting the dots.

[Brian Osman, 2016-07-15 14:56:26]

brianosman@google.com changed reviewers:
+ bsalomon@google.com, reed@google.com, robertphillips@google.com

[Brian Osman, 2016-07-15 14:56:27]

The most interesting parts of this are:

- GrColorSpaceXform.h/cpp
- GrTextureAccess.h
- GrTextureParamsAdjuster.cpp
- SkBitmapProcShader.cpp

Pretty much everything else is just connecting the dots.

[Brian Osman, 2016-07-15 15:02:08]

The CQ bit was checked by brianosman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 15:02:18]

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-15 15:04:06]

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

[commit-bot: I haz the power, 2016-07-15 15:04:07]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on
master.client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[bsalomon, 2016-07-15 15:09:25]

https://codereview.chromium.org/2154753003/diff/1/include/gpu/GrTextureAccess.h
File include/gpu/GrTextureAccess.h (right):

https://codereview.chromium.org/2154753003/diff/1/include/gpu/GrTextureAccess...
include/gpu/GrTextureAccess.h:22: class GrTextureAccess : public SkNoncopyable {
What do you think about having a constructor for this that doesn't take a xform
(and uses identity)? Too easy to forget to do the right thing when writing a new
effect?

https://codereview.chromium.org/2154753003/diff/1/src/gpu/effects/GrBicubicEf...
File src/gpu/effects/GrBicubicEffect.cpp (right):

https://codereview.chromium.org/2154753003/diff/1/src/gpu/effects/GrBicubicEf...
src/gpu/effects/GrBicubicEffect.cpp:141: : INHERITED(texture,
std::move(colorSpaceXform), matrix,
Should this effect forward on the xform to the access or should it filter and
then apply the xform itself?

[Brian Osman, 2016-07-15 15:29:02]

https://codereview.chromium.org/2154753003/diff/1/include/gpu/GrTextureAccess.h
File include/gpu/GrTextureAccess.h (right):

https://codereview.chromium.org/2154753003/diff/1/include/gpu/GrTextureAccess...
include/gpu/GrTextureAccess.h:22: class GrTextureAccess : public SkNoncopyable {
On 2016/07/15 15:09:24, bsalomon wrote:
> What do you think about having a constructor for this that doesn't take a
xform
> (and uses identity)? Too easy to forget to do the right thing when writing a
new
> effect?

That was my thought process - I'd sure like to have these default to nullptr so
you don't need to always supply it - but even for this process of catching all
uses of the call-tree, I really needed to make it mandatory. I'm not strongly
opposed - it trades the ability to forget for much cleaner code in all the
places that we know it's never an issue (like any code path that's drawing
masks).

https://codereview.chromium.org/2154753003/diff/1/src/gpu/effects/GrBicubicEf...
File src/gpu/effects/GrBicubicEffect.cpp (right):

https://codereview.chromium.org/2154753003/diff/1/src/gpu/effects/GrBicubicEf...
src/gpu/effects/GrBicubicEffect.cpp:141: : INHERITED(texture,
std::move(colorSpaceXform), matrix,
On 2016/07/15 15:09:24, bsalomon wrote:
> Should this effect forward on the xform to the access or should it filter and
> then apply the xform itself?

Ah, that's a good point. The math should come out the same, and converting after
filtering will be a big perf win. I'll look into fixing that.

[Brian Osman, 2016-07-15 15:33:56]

The CQ bit was checked by brianosman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 15:34:03]

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-15 15:51:54]

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

[commit-bot: I haz the power, 2016-07-15 15:51:55]

Dry run: This issue passed the CQ dry run.

[Brian Osman, 2016-07-15 19:44:30]

GrBicubicEffect now works. I'll probably end up factoring some of the
shader-injection code into helper functions, but this gives an idea of the
amount of work needed to inject the gamut transformation.

Tested locally by forcing the src and dst profiles to sRGB/Adobe, and the
results appear correct (ie matches Photoshop on my monitor with an AdobeRGB
profile).

[bsalomon, 2016-07-18 13:56:36]

https://codereview.chromium.org/2154753003/diff/60001/src/gpu/GrColorSpaceXfo...
File src/gpu/GrColorSpaceXform.cpp (right):

https://codereview.chromium.org/2154753003/diff/60001/src/gpu/GrColorSpaceXfo...
src/gpu/GrColorSpaceXform.cpp:16: return
Is this something we should compute and store on the SkColorSpace once rather
than repeatedly when it is used?

https://codereview.chromium.org/2154753003/diff/60001/src/gpu/effects/GrBicub...
File src/gpu/effects/GrBicubicEffect.cpp (right):

https://codereview.chromium.org/2154753003/diff/60001/src/gpu/effects/GrBicub...
src/gpu/effects/GrBicubicEffect.cpp:119: if (xformMatrix) {
Do we need a TODO about handling translations in the matrix correctly?

[Brian Osman, 2016-07-18 14:07:32]

brianosman@google.com changed reviewers:
+ msarett@google.com

[Brian Osman, 2016-07-18 14:07:34]

Replying to Brian's last comments, and adding Matt re: caching inverse XYZ on
SkColorSpace (perhaps on first use?)

https://codereview.chromium.org/2154753003/diff/60001/src/gpu/GrColorSpaceXfo...
File src/gpu/GrColorSpaceXform.cpp (right):

https://codereview.chromium.org/2154753003/diff/60001/src/gpu/GrColorSpaceXfo...
src/gpu/GrColorSpaceXform.cpp:16: return
On 2016/07/18 13:56:36, bsalomon wrote:
> Is this something we should compute and store on the SkColorSpace once rather
> than repeatedly when it is used?

Unfortunately, this is a property of the src + dst combination, rather than any
one color space. (We should consider caching the inverse of the XYZ matrix,
though).

https://codereview.chromium.org/2154753003/diff/60001/src/gpu/effects/GrBicub...
File src/gpu/effects/GrBicubicEffect.cpp (right):

https://codereview.chromium.org/2154753003/diff/60001/src/gpu/effects/GrBicub...
src/gpu/effects/GrBicubicEffect.cpp:119: if (xformMatrix) {
On 2016/07/18 13:56:36, bsalomon wrote:
> Do we need a TODO about handling translations in the matrix correctly?

I thought about it more - I think that even with a translation, we can safely
filter in the original color space (we're just computing a weighted sum), and
then transform the filtered value. In fact, I think that everything is still
linear, so the results will remain the same?

[bsalomon, 2016-07-18 15:16:15]

lgtm

[Brian Osman, 2016-07-18 15:24:11]

Thanks for the lgtm. Just wanted to point out my latest revision before I land -
I've added a stack helper object for use in emitCode, and un-plumbed
GrTextureAccess. This code could land right now, with the following next steps:

1) Scrape the xform in other texture-based effects.
2) Add another (const char*) argument to the appendTextureLookup functions that
is the color matrix name, if any. That, plus the helper will make the amount of
code added to all of the texture-based effects pretty minimal.
3) Wire up the destination color space, once I get SkImageInfo on GrDrawContext.
4) Optimizations (caching xform, etc...)

[msarett, 2016-07-18 15:39:40]

https://codereview.chromium.org/2154753003/diff/60001/src/gpu/GrColorSpaceXfo...
File src/gpu/GrColorSpaceXform.cpp (right):

https://codereview.chromium.org/2154753003/diff/60001/src/gpu/GrColorSpaceXfo...
src/gpu/GrColorSpaceXform.cpp:16: return
On 2016/07/18 14:07:33, Brian Osman wrote:
> On 2016/07/18 13:56:36, bsalomon wrote:
> > Is this something we should compute and store on the SkColorSpace once
rather
> > than repeatedly when it is used?
> 
> Unfortunately, this is a property of the src + dst combination, rather than
any
> one color space. (We should consider caching the inverse of the XYZ matrix,
> though).

Yes I think we should cache the inverse matrix.

[Brian Osman, 2016-07-18 17:31:15]

The CQ bit was checked by brianosman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 17:31:27]

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-18 17:51:40]

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

[commit-bot: I haz the power, 2016-07-18 17:51:41]

Dry run: This issue passed the CQ dry run.

[Brian Osman, 2016-07-18 17:52:47]

The CQ bit was checked by brianosman@google.com

[Brian Osman, 2016-07-18 17:52:48]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2154753003/#ps80001
(title: "More progress & refactoring")

[commit-bot: I haz the power, 2016-07-18 17:52:51]

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-18 17:53:55]

Description was changed from

==========
Introduce GrColorSpaceXform, for gamut conversion on textures

GrTextureAccess optionally includes an instance, computed from the src
and dst color spaces. In all common cases (no color space for either src
or dst, or same color space for both), no object is allocated.

This change is orthogonal to my attempts to get color space attached to
render targets - regardless of how we choose to do that, this will give
us the source color space at all points where we are connecting src to
dst.

There are many dangling injection points where I've been inserting
nullptr, but I have a record of all of them. Additionally, there are now
three places (the most common simple paths for bitmap/image rendering)
where things are plumbed enough that I expect to have access to the dst
color space (all marked with XFORMTODO).

In addition to getting the dst color space, I need to inject shader code
and uniform uploading for appendTextureLookup and friends.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2154753003
==========

to

==========
Introduce GrColorSpaceXform, for gamut conversion on textures

GrTextureAccess optionally includes an instance, computed from the src
and dst color spaces. In all common cases (no color space for either src
or dst, or same color space for both), no object is allocated.

This change is orthogonal to my attempts to get color space attached to
render targets - regardless of how we choose to do that, this will give
us the source color space at all points where we are connecting src to
dst.

There are many dangling injection points where I've been inserting
nullptr, but I have a record of all of them. Additionally, there are now
three places (the most common simple paths for bitmap/image rendering)
where things are plumbed enough that I expect to have access to the dst
color space (all marked with XFORMTODO).

In addition to getting the dst color space, I need to inject shader code
and uniform uploading for appendTextureLookup and friends.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2154753003

Committed:
https://skia.googlesource.com/skia/+/54f30c13fc0a5d89797fc9be5f0fb1050d96b6f4
==========

[commit-bot: I haz the power, 2016-07-18 17:53:56]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/54f30c13fc0a5d89797fc9be5f0fb1050d96b6f4