Add capability for SkColorXform to output half floats

Add capability for SkColorXform to output half floats

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2147763002
CQ_INCLUDE_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot;master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot

Committed: https://skia.googlesource.com/skia/+/9ce3a543c92a73e6daca420defc042886b3f2019

[msarett, 2016-07-12 22:15:52]

Description was changed from

==========
Add capability for SkColorXform to output half floats

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

to

==========
Add capability for SkColorXform to output half floats

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2147763002
CQ_INCLUDE_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[msarett, 2016-07-13 16:14:13]

Patchset #1 (id:1) has been deleted

[msarett, 2016-07-13 16:32:19]

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

[msarett, 2016-07-13 16:32:20]

Still a lot of TODOs but I think this is a step in the right direction...

[reed1, 2016-07-13 17:25:12]

https://codereview.chromium.org/2147763002/diff/40001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2147763002/diff/40001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.h:32: virtual void apply(SkPMColor* dst, const
uint32_t* src, uint32_t len) const = 0;
1. Multiple virtuals w/ the same name is a little tricky -- overriders must
override both if they want to override one (or get a warning).

2. Wonder if a couple of typedefs in this class might help w/ documentation...

typedef uint32_t RGBA32;
typedef uint64_t RGBAF16;

3. skia uses int for counts, and size_t for byte-lengths. uint32_t is funny as
the last parameter type.

[msarett, 2016-07-13 19:26:03]

https://codereview.chromium.org/2147763002/diff/40001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2147763002/diff/40001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.h:32: virtual void apply(SkPMColor* dst, const
uint32_t* src, uint32_t len) const = 0;
On 2016/07/13 17:25:12, reed1 wrote:
> 1. Multiple virtuals w/ the same name is a little tricky -- overriders must
> override both if they want to override one (or get a warning).
>

Renaming these.  FWIW, it's looking to me like (eventually) we won't actually
need to subclass SkColorXform.  So these might not be virtual forever.

> 2. Wonder if a couple of typedefs in this class might help w/ documentation...
> 
> typedef uint32_t RGBA32;
> typedef uint64_t RGBAF16;
> 

SGTM, I think this helps.

> 3. skia uses int for counts, and size_t for byte-lengths. uint32_t is funny as
> the last parameter type.

Done.  Need to break this habit.

[reed1, 2016-07-13 20:32:33]

api lgtm

[mtklein, 2016-07-15 14:31:17]

Two thoughts:
  1) should be a fine time to rebase on the _01 -> _finite change
  2) let's see what happens if we build these transforms as an SkRasterPipeline?
 It's my new toy, designed to let us factor this code more succinctly without
hurting perf too much.

[msarett, 2016-07-15 14:42:58]

Description was changed from

==========
Add capability for SkColorXform to output half floats

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2147763002
CQ_INCLUDE_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Add capability for SkColorXform to output half floats

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2147763002
CQ_INCLUDE_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot;master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[msarett, 2016-07-15 14:45:02]

On 2016/07/15 14:31:17, mtklein wrote:
> Two thoughts:
>   1) should be a fine time to rebase on the _01 -> _finite change

Agreed!  Done.  Thanks for writing this!

>   2) let's see what happens if we build these transforms as an
SkRasterPipeline?
>  It's my new toy, designed to let us factor this code more succinctly without
> hurting perf too much.

Do you mind if I leave this for a later CL?  I'd like to consider this type
of change on its own.

[msarett, 2016-07-15 16:12:28]

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

[commit-bot: I haz the power, 2016-07-15 16:12:37]

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 16:12:38]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     client.skia
   For more details, see http://crbug.com/617627.

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

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

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

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[mtklein, 2016-07-15 16:45:41]

https://codereview.chromium.org/2147763002/diff/100001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXform.cpp (right):

https://codereview.chromium.org/2147763002/diff/100001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:404: // Swap R and B if necessary to make sure
that we output SkPMColor order.
Kill?

https://codereview.chromium.org/2147763002/diff/100001/src/opts/SkColorXform_...
File src/opts/SkColorXform_opts.h (right):

https://codereview.chromium.org/2147763002/diff/100001/src/opts/SkColorXform_...
src/opts/SkColorXform_opts.h:94: auto rgba = (Sk4f_round(dstReds)        )
Might feel better to write this sort of stuff using the _SHIFT or _INDEX macros
like this:

auto rgba = (Sk4f_round(dstReds  ) << SK_R32_SHIFT)
          | (Sk4f_round(dstGreens) << SK_G32_SHIFT)
          | ...

https://codereview.chromium.org/2147763002/diff/100001/src/opts/SkColorXform_...
src/opts/SkColorXform_opts.h:144: // Should we write Intel/Arm specific code? 
Should we add a transpose
I think,

Very likely,
No (Or Maybe, but let's make it not your problem),
Yes,
Maybe,
No.

[msarett, 2016-07-15 17:18:24]

https://codereview.chromium.org/2147763002/diff/100001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXform.cpp (right):

https://codereview.chromium.org/2147763002/diff/100001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:404: // Swap R and B if necessary to make sure
that we output SkPMColor order.
On 2016/07/15 16:45:41, mtklein wrote:
> Kill?

Oops yes!

https://codereview.chromium.org/2147763002/diff/100001/src/opts/SkColorXform_...
File src/opts/SkColorXform_opts.h (right):

https://codereview.chromium.org/2147763002/diff/100001/src/opts/SkColorXform_...
src/opts/SkColorXform_opts.h:94: auto rgba = (Sk4f_round(dstReds)        )
On 2016/07/15 16:45:41, mtklein wrote:
> Might feel better to write this sort of stuff using the _SHIFT or _INDEX
macros
> like this:
> 
> auto rgba = (Sk4f_round(dstReds  ) << SK_R32_SHIFT)
>           | (Sk4f_round(dstGreens) << SK_G32_SHIFT)
>           | ...
> 

Yes way better!

https://codereview.chromium.org/2147763002/diff/100001/src/opts/SkColorXform_...
src/opts/SkColorXform_opts.h:144: // Should we write Intel/Arm specific code? 
Should we add a transpose
On 2016/07/15 16:45:41, mtklein wrote:
> I think,
> 
> Very likely,
> No (Or Maybe, but let's make it not your problem),
> Yes,
> Maybe,
> No.

Haha thanks.

https://codereview.chromium.org/2147763002/diff/100001/src/opts/SkColorXform_...
src/opts/SkColorXform_opts.h:175: #ifdef SK_PMCOLOR_IS_RGBA
This is nonsense.  Never was going to work...

https://codereview.chromium.org/2147763002/diff/140001/src/opts/SkColorXform_...
File src/opts/SkColorXform_opts.h (right):

https://codereview.chromium.org/2147763002/diff/140001/src/opts/SkColorXform_...
src/opts/SkColorXform_opts.h:174: #ifdef SK_PMCOLOR_IS_BGRA
This is the last remaining bit of ugliness (and it's also slow).  But at least
it's right this time...

[msarett, 2016-07-15 17:19:37]

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

[commit-bot: I haz the power, 2016-07-15 17:19:40]

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 17:19:42]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     client.skia
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-15 17:40:45]

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

[commit-bot: I haz the power, 2016-07-15 17:40:47]

Dry run: This issue passed the CQ dry run.

[mtklein, 2016-07-15 20:05:39]

lgtm, with one last suggestion

https://codereview.chromium.org/2147763002/diff/140001/src/opts/SkColorXform_...
File src/opts/SkColorXform_opts.h (right):

https://codereview.chromium.org/2147763002/diff/140001/src/opts/SkColorXform_...
src/opts/SkColorXform_opts.h:174: #ifdef SK_PMCOLOR_IS_BGRA
On 2016/07/15 17:18:24, msarett wrote:
> This is the last remaining bit of ugliness (and it's also slow).  But at least
> it's right this time...

How about an unconditional call to SkSwizzle_RGBA_to_PMColor() ?

[msarett, 2016-07-15 20:10:41]

https://codereview.chromium.org/2147763002/diff/140001/src/opts/SkColorXform_...
File src/opts/SkColorXform_opts.h (right):

https://codereview.chromium.org/2147763002/diff/140001/src/opts/SkColorXform_...
src/opts/SkColorXform_opts.h:174: #ifdef SK_PMCOLOR_IS_BGRA
On 2016/07/15 20:05:39, mtklein wrote:
> On 2016/07/15 17:18:24, msarett wrote:
> > This is the last remaining bit of ugliness (and it's also slow).  But at
least
> > it's right this time...
> 
> How about an unconditional call to SkSwizzle_RGBA_to_PMColor() ?

Yup, sgtm

[msarett, 2016-07-15 20:10:58]

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

[commit-bot: I haz the power, 2016-07-15 20:11:05]

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 20:11:07]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     client.skia
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-15 20:29:37]

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

[commit-bot: I haz the power, 2016-07-15 20:29:38]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-07-15 20:53:09]

The CQ bit was checked by msarett@google.com

[msarett, 2016-07-15 20:53:09]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com,
mtklein@google.com
Link to the patchset: https://codereview.chromium.org/2147763002/#ps160001
(title: "Call swizzle fn")

[commit-bot: I haz the power, 2016-07-15 20:53:21]

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 20:53:23]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     client.skia
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-15 20:54:41]

Description was changed from

==========
Add capability for SkColorXform to output half floats

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2147763002
CQ_INCLUDE_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot;master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Add capability for SkColorXform to output half floats

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2147763002
CQ_INCLUDE_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot;master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot

Committed:
https://skia.googlesource.com/skia/+/9ce3a543c92a73e6daca420defc042886b3f2019
==========

[commit-bot: I haz the power, 2016-07-15 20:54:42]

Committed patchset #8 (id:160001) as
https://skia.googlesource.com/skia/+/9ce3a543c92a73e6daca420defc042886b3f2019

[commit-bot: I haz the power, 2016-07-15 20:54:44]

CQ bit was unchecked.