Refactor of SkColorSpaceXformOpts

Refactor of SkColorSpaceXformOpts

(1) Performance is better or stays the same.

(2) Code is split into functions (RasterPipeline-ish
    design).  IMO, it's not really more or less readable.
    But I think it's now much easier add capabilities,
    apply optimizations, or do more refactors.  Or to
    actually use RasterPipeline.  I help back from trying
    any of these to try to keep this CL sane.

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

Committed: https://skia.googlesource.com/skia/+/15ee3deee8aca2bf6e658449f25ee34a8153e6ee

[msarett, 2016-08-01 19:11:46]

Description was changed from

==========
Draft

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

to

==========
Draft

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

[msarett, 2016-08-01 19:51:38]

Description was changed from

==========
Draft

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

to

==========
Refactor of SkColorSpaceXformOpts

(1) Performance is better or stays the same.

(2) Code is split into functions (RasterPipeline-ish
    design).  IMO, it's not really more or less readable.
    But I think it's now much easier add capabilities and
    apply optimizations.  Or to actually use
    RasterPipeline.  I help back from trying either of
    these to try to keep this CL sane.

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

[msarett, 2016-08-01 19:52:31]

Description was changed from

==========
Refactor of SkColorSpaceXformOpts

(1) Performance is better or stays the same.

(2) Code is split into functions (RasterPipeline-ish
    design).  IMO, it's not really more or less readable.
    But I think it's now much easier add capabilities and
    apply optimizations.  Or to actually use
    RasterPipeline.  I help back from trying either of
    these to try to keep this CL sane.

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

to

==========
Refactor of SkColorSpaceXformOpts

(1) Performance is better or stays the same.

(2) Code is split into functions (RasterPipeline-ish
    design).  IMO, it's not really more or less readable.
    But I think it's now much easier add capabilities,
    apply optimizations, or do more refactors.  Or to
    actually use RasterPipeline.  I help back from trying
    any of these to try to keep this CL sane.

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

[msarett, 2016-08-01 19:53:57]

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

[msarett, 2016-08-01 19:53:58]

I can merge this with https://codereview.chromium.org/2184543003/ if it seems
logical.

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXform.cpp (left):

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.cpp:81: static void
build_table_linear_from_gamma(float* outTable, float exponent) {
Moved this code block somewhere else.

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXformOpts.h (right):

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:8: #ifndef SkColorSpaceXformOpts_DEFINED
Any way we can compile this file with SSE4 (when it's available)?

[mtklein, 2016-08-02 13:45:50]

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXformOpts.h (right):

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:17: static SK_ALWAYS_INLINE void
load_matrix(const float matrix[16], Sk4f& rXgXbX, Sk4f& rYgYbY,
Are you finding the SK_ALWAYS_INLINE necessary over, say, inline?

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:25: static SK_ALWAYS_INLINE void
load_generic(const uint32_t* src, Sk4f& r, Sk4f& g, Sk4f& b, Sk4f& a,
_generic doesn't seem to add much.

load_from_tables?

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:40: ia = SkNx_cast<int32_t>(Sk4u::Load(src) >>
24);
This bit where we thread ia and dia through all the functions is a little
awkward.  Any chance that if we just Sk4u::Load(src) >> 24 again in the store
it'll get constant subexpression eliminated?  The _1 variants already take the
src pointer.

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:55: const Sk4f& rZgZbZ, Sk4f& dr, Sk4f& dg,
Sk4f& db, Sk4f& da,
These would be a lot easier to follow if grouped coherently, e.g.

(const Sk4f& r, const Sk4f& g, const Sk4f& b, const Sk4f& a, 
 const Sk4i& ia,
 const Sk4f& rXgXbX, const Sk4f& rYgYbY, const Sk4f& rZgZbZ,
 Sk4f& dr, Sk4f& dg, Sk4f& db, Sk4f& da,
 Sk4f& dia)

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:70: static SK_ALWAYS_INLINE void
xlate_gamut(const Sk4f& rTgTbT, Sk4f& dr, Sk4f& dg, Sk4f& db) {
I think we're running up against the limits of readability in naming here... I
be we can afford the bytes to substitute 'x' with 'trans'.

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:123: *dst32 = (*src & 0xFF000000) | (tmp &
0x00FFFFFF);
tmp = ( ... ) | ( ... )
if (kSwapRB) {
   tmp = SkSwizzle_RB(tmp)
}
*dst32 = tmp;

?

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:251: void (*store)(void*, Sk4f&, Sk4f&, Sk4f&,
Sk4f&, Sk4i&, const uint8_t* const[3], bool);
An alternative here is

decltype(store_srgb  )* store;
decltype(store_srgb_1)* store_1;

or

decltype(&store_srgb  ) store;
decltype(&store_srgb_1) store_1;

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:280: if (len >= 4) {
// Naively this'd be a loop of load-transform-store, but we found it faster to
move the N+1th load ahead of the Nth store.  We don't bother doing this for N<4.

[msarett, 2016-08-02 16:49:02]

Patchset #3 (id:40001) has been deleted

[msarett, 2016-08-02 16:50:17]

FYI, this can't land until the png CL lands:
https://codereview.chromium.org/2184543003/

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXformOpts.h (right):

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:17: static SK_ALWAYS_INLINE void
load_matrix(const float matrix[16], Sk4f& rXgXbX, Sk4f& rYgYbY,
On 2016/08/02 13:45:49, mtklein wrote:
> Are you finding the SK_ALWAYS_INLINE necessary over, say, inline?

No, inline is fine.  Fixed.

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:25: static SK_ALWAYS_INLINE void
load_generic(const uint32_t* src, Sk4f& r, Sk4f& g, Sk4f& b, Sk4f& a,
On 2016/08/02 13:45:49, mtklein wrote:
> _generic doesn't seem to add much.
> 
> load_from_tables?

SGTM

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:40: ia = SkNx_cast<int32_t>(Sk4u::Load(src) >>
24);
On 2016/08/02 13:45:49, mtklein wrote:
> This bit where we thread ia and dia through all the functions is a little
> awkward.  Any chance that if we just Sk4u::Load(src) >> 24 again in the store
> it'll get constant subexpression eliminated?  The _1 variants already take the
> src pointer.

I've dropped dia.  It is confusing and annoying.

This change has actually made me look closer and realize that the compiler is
not doing much of anything for us.  Not only does it not do CSE on the Load, it
is actually doing the convert to float whether we will use it or not.

I'm seeing a nice speed boost by refactoring into two load functions.  (FWIW, I
think this is where I was confused, when I thought that we could speed up by
writing a special "load_srgb").

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:55: const Sk4f& rZgZbZ, Sk4f& dr, Sk4f& dg,
Sk4f& db, Sk4f& da,
On 2016/08/02 13:45:49, mtklein wrote:
> These would be a lot easier to follow if grouped coherently, e.g.
> 
> (const Sk4f& r, const Sk4f& g, const Sk4f& b, const Sk4f& a, 
>  const Sk4i& ia,
>  const Sk4f& rXgXbX, const Sk4f& rYgYbY, const Sk4f& rZgZbZ,
>  Sk4f& dr, Sk4f& dg, Sk4f& db, Sk4f& da,
>  Sk4f& dia)

SGTM.  Done.

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:70: static SK_ALWAYS_INLINE void
xlate_gamut(const Sk4f& rTgTbT, Sk4f& dr, Sk4f& dg, Sk4f& db) {
On 2016/08/02 13:45:49, mtklein wrote:
> I think we're running up against the limits of readability in naming here... I
> be we can afford the bytes to substitute 'x' with 'trans'.

Haha ok.  Going with transform and translate, it'll annoy me if they're not
consistent.

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:123: *dst32 = (*src & 0xFF000000) | (tmp &
0x00FFFFFF);
On 2016/08/02 13:45:49, mtklein wrote:
> tmp = ( ... ) | ( ... )
> if (kSwapRB) {
>    tmp = SkSwizzle_RB(tmp)
> }
> *dst32 = tmp;
> 
> ?

Done.

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:251: void (*store)(void*, Sk4f&, Sk4f&, Sk4f&,
Sk4f&, Sk4i&, const uint8_t* const[3], bool);
On 2016/08/02 13:45:49, mtklein wrote:
> An alternative here is
> 
> decltype(store_srgb  )* store;
> decltype(store_srgb_1)* store_1;
> 
> or
> 
> decltype(&store_srgb  ) store;
> decltype(&store_srgb_1) store_1;

Cool!  Done.

https://codereview.chromium.org/2194303002/diff/20001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:280: if (len >= 4) {
On 2016/08/02 13:45:49, mtklein wrote:
> // Naively this'd be a loop of load-transform-store, but we found it faster to
> move the N+1th load ahead of the Nth store.  We don't bother doing this for
N<4.

Done.

[mtklein, 2016-08-02 17:07:01]

lgtm

https://codereview.chromium.org/2194303002/diff/60001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXformOpts.h (right):

https://codereview.chromium.org/2194303002/diff/60001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:139: | (da                 << kAShift);
Sk4i::Load(src) & 0xff000000

No need to shift back and forth any more.

https://codereview.chromium.org/2194303002/diff/60001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:188: Sk4i da =
SkNx_cast<int32_t>(Sk4u::Load(src) >> 24);
ditto

https://codereview.chromium.org/2194303002/diff/60001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformOpts.h:249: Sk4i da =
SkNx_cast<int32_t>(Sk4u::Load(src) >> 24);
ditto

[msarett, 2016-08-02 17:31:54]

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

[commit-bot: I haz the power, 2016-08-02 17:32: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-08-02 17:42:13]

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

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

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

[msarett, 2016-08-02 17:46:22]

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

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

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 18:29:03]

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

[commit-bot: I haz the power, 2016-08-02 18:29:04]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-08-02 18:29:26]

The CQ bit was checked by msarett@google.com

[msarett, 2016-08-02 18:29:26]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/2194303002/#ps120001
(title: "Fix")

[commit-bot: I haz the power, 2016-08-02 18:29:34]

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 18:30:33]

Description was changed from

==========
Refactor of SkColorSpaceXformOpts

(1) Performance is better or stays the same.

(2) Code is split into functions (RasterPipeline-ish
    design).  IMO, it's not really more or less readable.
    But I think it's now much easier add capabilities,
    apply optimizations, or do more refactors.  Or to
    actually use RasterPipeline.  I help back from trying
    any of these to try to keep this CL sane.

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

to

==========
Refactor of SkColorSpaceXformOpts

(1) Performance is better or stays the same.

(2) Code is split into functions (RasterPipeline-ish
    design).  IMO, it's not really more or less readable.
    But I think it's now much easier add capabilities,
    apply optimizations, or do more refactors.  Or to
    actually use RasterPipeline.  I help back from trying
    any of these to try to keep this CL sane.

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

Committed:
https://skia.googlesource.com/skia/+/15ee3deee8aca2bf6e658449f25ee34a8153e6ee
==========

[commit-bot: I haz the power, 2016-08-02 18:30:34]

Committed patchset #6 (id:120001) as
https://skia.googlesource.com/skia/+/15ee3deee8aca2bf6e658449f25ee34a8153e6ee