Add SkColorSpaceXform to the public API

Add SkColorSpaceXform to the public API

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

Committed: https://skia.googlesource.com/skia/+/31d097e865f266c8398f45114e4c75c0dfdef058

[msarett, 2016-10-04 13:38:02]

Description was changed from

==========
Add SkColorSpaceXform to the public API

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

to

==========
Add SkColorSpaceXform to the public API

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

[msarett, 2016-10-04 13:54:34]

Patchset #2 (id:20001) has been deleted

[msarett, 2016-10-04 13:55:09]

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

[msarett, 2016-10-04 13:55:09]

Think it would be good to start talking about this.

[reed1, 2016-10-04 14:54:43]

https://codereview.chromium.org/2390263002/diff/40001/include/core/SkColorSpa...
File include/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2390263002/diff/40001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:25: static std::unique_ptr<SkColorSpaceXform>
New(SkColorSpace* srcSpace, SkColorSpace* dstSpace);
Do you copy or ref or neither these two pointers (for the life of the returned
xform)?

https://codereview.chromium.org/2390263002/diff/40001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:51: */
consider making apply non-virtual, and just dispatch to a (new) virtual under
the hood. Makes it clearer that this is intended to be a closed set of
subclasses.

https://codereview.chromium.org/2390263002/diff/40001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:52: virtual void apply(void* dst, const
uint32_t* src, int len, ColorFormat dstColorFormat,
why is src uint32* and dst void* if both have a format descriptor?

https://codereview.chromium.org/2390263002/diff/40001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:52: virtual void apply(void* dst, const
uint32_t* src, int len, ColorFormat dstColorFormat,
nit: more common is use "count" when counting logical objects in skia

https://codereview.chromium.org/2390263002/diff/40001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:53: ColorFormat srcColorFormat, SkAlphaType
alphaType) const = 0;
Its tricky to see how the params group... what about:

apply(DstFormat, dst, SrcFormat, src, count, alpha) ?

[msarett, 2016-10-04 23:21:58]

https://codereview.chromium.org/2390263002/diff/40001/include/core/SkColorSpa...
File include/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2390263002/diff/40001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:25: static std::unique_ptr<SkColorSpaceXform>
New(SkColorSpace* srcSpace, SkColorSpace* dstSpace);
On 2016/10/04 14:54:43, reed1 wrote:
> Do you copy or ref or neither these two pointers (for the life of the returned
> xform)?

Neither.  I actually used to make this take sk_sps just in case...  I think this
is how we want it, of course now I'm worried I might change my mind.

https://codereview.chromium.org/2390263002/diff/40001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:51: */
On 2016/10/04 14:54:43, reed1 wrote:
> consider making apply non-virtual, and just dispatch to a (new) virtual under
> the hood. Makes it clearer that this is intended to be a closed set of
> subclasses.

Done.

https://codereview.chromium.org/2390263002/diff/40001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:52: virtual void apply(void* dst, const
uint32_t* src, int len, ColorFormat dstColorFormat,
On 2016/10/04 14:54:43, reed1 wrote:
> why is src uint32* and dst void* if both have a format descriptor?

I think it is cleaner to make both void*.  I'm changing this.

FWIW, (as of right now) only RBGA888 and BGRA8888 are supported as inputs, so
uint32_t* is somewhat accurate.

https://codereview.chromium.org/2390263002/diff/40001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:52: virtual void apply(void* dst, const
uint32_t* src, int len, ColorFormat dstColorFormat,
On 2016/10/04 14:54:43, reed1 wrote:
> nit: more common is use "count" when counting logical objects in skia

Done.

https://codereview.chromium.org/2390263002/diff/40001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:53: ColorFormat srcColorFormat, SkAlphaType
alphaType) const = 0;
On 2016/10/04 14:54:43, reed1 wrote:
> Its tricky to see how the params group... what about:
> 
> apply(DstFormat, dst, SrcFormat, src, count, alpha) ?

Done.

[reed1, 2016-10-05 16:14:19]

https://codereview.chromium.org/2390263002/diff/60001/include/core/SkColorSpa...
File include/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2390263002/diff/60001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:38: *  is linear.  This function will assert on
invalid or unsupported uses.
assert but not return failure or crash? What are the conditions (today) that
would make you assert?

https://codereview.chromium.org/2390263002/diff/60001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:61: virtual void onApply(ColorFormat dstFormat,
void* dst, ColorFormat srcFormat, const void* src,
This is fine, but not what I was suggesting earlier. That idea was to introduce
the notion of a virtual in a private subclass, and have no virtuals at this
level (except the destructor).

[msarett, 2016-10-06 14:17:51]

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

[msarett, 2016-10-06 14:17:52]

+mtklein, brianosman

https://codereview.chromium.org/2390263002/diff/60001/include/core/SkColorSpa...
File include/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2390263002/diff/60001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:38: *  is linear.  This function will assert on
invalid or unsupported uses.
On 2016/10/05 16:14:19, reed1 wrote:
> assert but not return failure or crash? What are the conditions (today) that
> would make you assert?

We assert on unsupported src formats (F16, F32) and on invalid dst combinations
(F16-nonlinear, F32-nonlinear).

After discussing in person, I'm leaning towards making this have a bool return
value.  Adding mtklein in case he has any thoughts.  I believe he was in favor
of the current model.

https://codereview.chromium.org/2390263002/diff/60001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:61: virtual void onApply(ColorFormat dstFormat,
void* dst, ColorFormat srcFormat, const void* src,
On 2016/10/05 16:14:19, reed1 wrote:
> This is fine, but not what I was suggesting earlier. That idea was to
introduce
> the notion of a virtual in a private subclass, and have no virtuals at this
> level (except the destructor).

Oh ok, I'll explore this...

[mtklein_C, 2016-10-06 14:26:19]

mtklein@chromium.org changed reviewers:
+ mtklein@chromium.org

[mtklein_C, 2016-10-06 14:26:20]

https://codereview.chromium.org/2390263002/diff/60001/include/core/SkColorSpa...
File include/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2390263002/diff/60001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:38: *  is linear.  This function will assert on
invalid or unsupported uses.
On 2016/10/06 at 14:17:52, msarett wrote:
> On 2016/10/05 16:14:19, reed1 wrote:
> > assert but not return failure or crash? What are the conditions (today) that
> > would make you assert?
> 
> We assert on unsupported src formats (F16, F32) and on invalid dst
combinations (F16-nonlinear, F32-nonlinear).
> 
> After discussing in person, I'm leaning towards making this have a bool return
value.  Adding mtklein in case he has any thoughts.  I believe he was in favor
of the current model.

ok by me

[msarett, 2016-10-10 17:52:29]

https://codereview.chromium.org/2390263002/diff/60001/include/core/SkColorSpa...
File include/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2390263002/diff/60001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:38: *  is linear.  This function will assert on
invalid or unsupported uses.
On 2016/10/06 14:26:20, mtklein_C wrote:
> On 2016/10/06 at 14:17:52, msarett wrote:
> > On 2016/10/05 16:14:19, reed1 wrote:
> > > assert but not return failure or crash? What are the conditions (today)
that
> > > would make you assert?
> > 
> > We assert on unsupported src formats (F16, F32) and on invalid dst
> combinations (F16-nonlinear, F32-nonlinear).
> > 
> > After discussing in person, I'm leaning towards making this have a bool
return
> value.  Adding mtklein in case he has any thoughts.  I believe he was in favor
> of the current model.
> 
> ok by me

This now returns a bool.

https://codereview.chromium.org/2390263002/diff/60001/include/core/SkColorSpa...
include/core/SkColorSpaceXform.h:61: virtual void onApply(ColorFormat dstFormat,
void* dst, ColorFormat srcFormat, const void* src,
On 2016/10/06 14:17:52, msarett wrote:
> On 2016/10/05 16:14:19, reed1 wrote:
> > This is fine, but not what I was suggesting earlier. That idea was to
> introduce
> > the notion of a virtual in a private subclass, and have no virtuals at this
> > level (except the destructor).
> 
> Oh ok, I'll explore this...

I think this makes sense as is.  The casting trick doesn't really work given
that SkColorSpaceXform_Base is templated.

[msarett, 2016-10-10 18:13:36]

Patchset #5 (id:100001) has been deleted

[msarett, 2016-10-10 18:14:01]

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

[commit-bot: I haz the power, 2016-10-10 18:14:12]

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

[msarett, 2016-10-10 18:20:53]

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

[msarett, 2016-10-10 18:20:53]

This now returns a bool.

+scroggo for codec changes

[commit-bot: I haz the power, 2016-10-10 18:32:58]

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

[commit-bot: I haz the power, 2016-10-10 18:32:59]

Dry run: This issue passed the CQ dry run.

[scroggo, 2016-10-10 19:07:40]

codec changes lgtm

[msarett, 2016-10-10 22:55:52]

https://codereview.chromium.org/2390263002/diff/120001/include/core/SkColorSp...
File include/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2390263002/diff/120001/include/core/SkColorSp...
include/core/SkColorSpaceXform.h:25: static std::unique_ptr<SkColorSpaceXform>
New(SkColorSpace* srcSpace, SkColorSpace* dstSpace);
Should these be "const SkColorSpaceXform*"?

[reed1, 2016-10-11 16:35:51]

https://codereview.chromium.org/2390263002/diff/120001/include/core/SkColorSp...
File include/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2390263002/diff/120001/include/core/SkColorSp...
include/core/SkColorSpaceXform.h:25: static std::unique_ptr<SkColorSpaceXform>
New(SkColorSpace* srcSpace, SkColorSpace* dstSpace);
On 2016/10/10 22:55:52, msarett wrote:
> Should these be "const SkColorSpaceXform*"?

no, I think that complicates mattesr for a class that is inherently const,
unless we ALWAYS pass const colorspacexform EVERYWHERE (which we don't). We had
this same mush for a while w/ shaders etc., and (mostly) have unified on not
using const inside the template owner (sk_sp or unique_ptr) if the type is
always const.

https://codereview.chromium.org/2390263002/diff/120001/include/core/SkColorSp...
include/core/SkColorSpaceXform.h:27: enum ColorFormat : uint8_t {
Why is it important to declare the actual type of the enum?

[msarett, 2016-10-11 16:46:22]

https://codereview.chromium.org/2390263002/diff/120001/include/core/SkColorSp...
File include/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2390263002/diff/120001/include/core/SkColorSp...
include/core/SkColorSpaceXform.h:25: static std::unique_ptr<SkColorSpaceXform>
New(SkColorSpace* srcSpace, SkColorSpace* dstSpace);
On 2016/10/11 16:35:50, reed1 wrote:
> On 2016/10/10 22:55:52, msarett wrote:
> > Should these be "const SkColorSpaceXform*"?
> 
> no, I think that complicates mattesr for a class that is inherently const,
> unless we ALWAYS pass const colorspacexform EVERYWHERE (which we don't). We
had
> this same mush for a while w/ shaders etc., and (mostly) have unified on not
> using const inside the template owner (sk_sp or unique_ptr) if the type is
> always const.

sgtm

https://codereview.chromium.org/2390263002/diff/120001/include/core/SkColorSp...
include/core/SkColorSpaceXform.h:27: enum ColorFormat : uint8_t {
On 2016/10/11 16:35:50, reed1 wrote:
> Why is it important to declare the actual type of the enum? 

It's not, removed.

[reed1, 2016-10-11 17:03:33]

lgtm

[msarett, 2016-10-11 17:52:01]

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

[commit-bot: I haz the power, 2016-10-11 17:52:11]

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-10-11 18:39:43]

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

[commit-bot: I haz the power, 2016-10-11 18:39:44]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-10-11 18:42:10]

The CQ bit was checked by msarett@google.com

[msarett, 2016-10-11 18:42:10]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/2390263002/#ps140001
(title: "Remove type on enum")

[commit-bot: I haz the power, 2016-10-11 18:42:14]

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-10-11 19:15:05]

Description was changed from

==========
Add SkColorSpaceXform to the public API

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

to

==========
Add SkColorSpaceXform to the public API

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

Committed:
https://skia.googlesource.com/skia/+/31d097e865f266c8398f45114e4c75c0dfdef058
==========

[commit-bot: I haz the power, 2016-10-11 19:15:06]

Committed patchset #6 (id:140001) as
https://skia.googlesource.com/skia/+/31d097e865f266c8398f45114e4c75c0dfdef058