Use NEON optimizations for RGB -> RGB(FF) or BGR(FF) in SkSwizzler

Use NEON optimizations for RGB -> RGB(FF) or BGR(FF) in SkSwizzler

Swizzle Bench Runtime Nexus 6P
xxx_xxxa        0.32x
xxx_swaprb_xxxa 0.31x

Swizzle Bench Runtime Nexus 9
xxx_xxxa        1.11x
xxx_swaprb_xxxa 1.14x
(This is a slow down.)

Swizzle Bench Runtime Nexus 5
xxx_xxxa        0.12x
xxx_swaprb      0.12x

RGB PNG Decode Runtime
Nexus 6P        0.94x
Nexus 9         0.98x

I don't know how to explain the fact that the Swizzle Bench was
slower on Nexus 9, but the decode times got faster.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1618003002
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot

Committed: https://skia.googlesource.com/skia/+/f1b8b6ae34e5a1f4b29e423401da39f88f0c117a

[msarett, 2016-01-21 21:47:27]

Description was changed from

==========
Use NEON optimizations for RGB -> RGB(FF) or BGR(FF) in SkSwizzler

Swizzle Bench Runtime Nexus 6P
xxx_xxxa        0.32x
xxx_swaprb_xxxa 0.31x

Swizzle Bench Runtime Nexus 9
xxx_xxxa        1.11x
xxx_swaprb_xxxa 1.14x
(This is a slow down.)

Swizzle Bench Runtime Nexus 5
xxx_xxxa        0.12x
xxx_swaprb      0.12x

RGB PNG Decode Runtime
Nexus 6P        0.94x
Nexus 9         0.98x

I don't know how to explain the fact that the Swizzle Bench was
slower on Nexus 9, but the decode times got faster.

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

to

==========
Use NEON optimizations for RGB -> RGB(FF) or BGR(FF) in SkSwizzler

Swizzle Bench Runtime Nexus 6P
xxx_xxxa        0.32x
xxx_swaprb_xxxa 0.31x

Swizzle Bench Runtime Nexus 9
xxx_xxxa        1.11x
xxx_swaprb_xxxa 1.14x
(This is a slow down.)

Swizzle Bench Runtime Nexus 5
xxx_xxxa        0.12x
xxx_swaprb      0.12x

RGB PNG Decode Runtime
Nexus 6P        0.94x
Nexus 9         0.98x

I don't know how to explain the fact that the Swizzle Bench was
slower on Nexus 9, but the decode times got faster.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[msarett, 2016-01-21 22:09:06]

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

[msarett, 2016-01-21 22:09:07]

https://codereview.chromium.org/1618003002/diff/1/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (left):

https://codereview.chromium.org/1618003002/diff/1/src/codec/SkSwizzler.cpp#ol...
src/codec/SkSwizzler.cpp:16: // This function must not be called if we are
sampling.  If we are not
I could be convinced to add these comments back in, if we feel they are useful.

https://codereview.chromium.org/1618003002/diff/1/src/opts/SkOpts_ssse3.cpp
File src/opts/SkOpts_ssse3.cpp (right):

https://codereview.chromium.org/1618003002/diff/1/src/opts/SkOpts_ssse3.cpp#n...
src/opts/SkOpts_ssse3.cpp:24: xxx_xxxa = sk_ssse3::xxx_xxxa;
If I don't do this, the compiler complains that xxx_xxxa_portable is unused.

https://codereview.chromium.org/1618003002/diff/1/src/opts/SkSwizzler_opts.h
File src/opts/SkSwizzler_opts.h (right):

https://codereview.chromium.org/1618003002/diff/1/src/opts/SkSwizzler_opts.h#...
src/opts/SkSwizzler_opts.h:65: const uint8_t* src8 = (uint8_t*) src;
It's a little strange that we pass in src as a pointer uint32_t.

[msarett, 2016-01-21 22:29:24]

Side Note to Self:

Decode comparisons measure the effect of this CL, but not the total effect of
optimizations.  We used to use libpng for RGB->RGB(FF), but stopped doing this
as of:
https://codereview.chromium.org/1582083005/

[scroggo, 2016-01-21 23:19:40]

https://codereview.chromium.org/1618003002/diff/1/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (left):

https://codereview.chromium.org/1618003002/diff/1/src/codec/SkSwizzler.cpp#ol...
src/codec/SkSwizzler.cpp:16: // This function must not be called if we are
sampling.  If we are not
On 2016/01/21 22:09:07, msarett wrote:
> I could be convinced to add these comments back in, if we feel they are
useful.

I see no reason to take them out.

[msarett, 2016-01-21 23:29:48]

https://codereview.chromium.org/1618003002/diff/1/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (left):

https://codereview.chromium.org/1618003002/diff/1/src/codec/SkSwizzler.cpp#ol...
src/codec/SkSwizzler.cpp:16: // This function must not be called if we are
sampling.  If we are not
On 2016/01/21 23:19:39, scroggo wrote:
> On 2016/01/21 22:09:07, msarett wrote:
> > I could be convinced to add these comments back in, if we feel they are
> useful.
> 
> I see no reason to take them out.

Added back in.

[mtklein, 2016-01-22 14:37:33]

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opts.h
File src/opts/SkSwizzler_opts.h (right):

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:15: // These variable names in these functions just
pretend the input is BGRA.
 (or BGR)

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:16: // They work fine with both RGBA and BGRA.
 (or both BGR and RGB).

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:18: static void premul_xxxa_portable(uint32_t dst[],
const uint32_t src[], int count) {
Do you think we made things unnecessarily complicated by naming these xxxa_,
etc?

Am I thinking right that when working with PNGs, the use cases we want are
really these?

    RGBA_to_rgbA()
    RGBA_to_bgrA()
    RGB_to_rgbA()
    RGB_to_bgrA()

    RGBA_to_BGRA()
    RGB_to_RGBA()
    RGB_to_BGRA()

where all-caps are unpremul, mixed-caps are premul?

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:63: static void xxx_xxxa_portable(uint32_t dst[],
const uint32_t src[], int count) {
src is three-byte right, not some sort of RGBx?  Seems really weird to pass that
as a uint32_t, particularly when we immediately cast it to a uint8_t*.

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:70: dst[i32] = (uint32_t)   b <<  0
Let's keep our order consistent with the rest of the file?

uint8_t r = src8[2],
        g = src8[1],
        b = src8[0];
src8 += 3;
dst[i] = (uint32_t)0xFF << 24
       | (uint32_t)r    << 16
       | (uint32_t)g    <<  8
       | (uint32_t)b    <<  0;

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:220: src8 += 48;
might write this as += 16*3?

I find it really pleasant when code is obviously correct at a sleepy glance,
rather than correct after remembering your times tables. :P

[mtklein, 2016-01-22 14:44:04]

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opts.h
File src/opts/SkSwizzler_opts.h (right):

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:18: static void premul_xxxa_portable(uint32_t dst[],
const uint32_t src[], int count) {
On 2016/01/22 14:37:33, mtklein wrote:
> Do you think we made things unnecessarily complicated by naming these xxxa_,
> etc?
> 
> Am I thinking right that when working with PNGs, the use cases we want are
> really these?
> 
>     RGBA_to_rgbA()
>     RGBA_to_bgrA()
>     RGB_to_rgbA()
>     RGB_to_bgrA()
> 
>     RGBA_to_BGRA()
>     RGB_to_RGBA()
>     RGB_to_BGRA()
> 
> where all-caps are unpremul, mixed-caps are premul?

Gonna write up a CL to demonstrate what I mean.

[msarett, 2016-01-22 15:00:36]

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opts.h
File src/opts/SkSwizzler_opts.h (right):

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:15: // These variable names in these functions just
pretend the input is BGRA.
On 2016/01/22 14:37:32, mtklein wrote:
>  (or BGR)

Done.

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:16: // They work fine with both RGBA and BGRA.
On 2016/01/22 14:37:32, mtklein wrote:
>  (or both BGR and RGB).

Done.

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:18: static void premul_xxxa_portable(uint32_t dst[],
const uint32_t src[], int count) {
On 2016/01/22 14:37:33, mtklein wrote:
> Do you think we made things unnecessarily complicated by naming these xxxa_,
> etc?
> 
> Am I thinking right that when working with PNGs, the use cases we want are
> really these?
> 
>     RGBA_to_rgbA()

Yes.

>     RGBA_to_bgrA()

Yes.

>     RGB_to_rgbA()
>     RGB_to_bgrA()

Yes, but the output of these is not really premul/unpremul, it's opaque.

> 
>     RGBA_to_BGRA()

Yes.

>     RGB_to_RGBA()
>     RGB_to_BGRA()
> 

These are identical to RGB_to_bgrA() and RGB_to_rgbA.

> where all-caps are unpremul, mixed-caps are premul?

So far yes, minus my nits.  I don't want to limit it to that yet though.  Even
if I fail to come up with something for kIndex8, I think we can add functions
for kGray8.  And possibly for sampling.

I'm not opposed to renaming these functions, though I'm fine with the names as
is.  I'd prefer to rename everything in a separate follow-up though if that's
the approach we want to take.

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:63: static void xxx_xxxa_portable(uint32_t dst[],
const uint32_t src[], int count) {
On 2016/01/22 14:37:32, mtklein wrote:
> src is three-byte right, not some sort of RGBx?  Seems really weird to pass
that
> as a uint32_t, particularly when we immediately cast it to a uint8_t*.

Agreed.  I made a similar comment in Patch Set 1.  What do you think about
making both dst and src either void* or uint8_t*?

FWIW, SkSwizzler makes dst a void* and src a uint8_t*.  I would argue for making
them both void* (though this means more casts).  The types are apparent from the
name of the function (not from any type info that we pass in).

This has the feeling of something that should be done in a separate CL. 
Probably before this one along with any function renaming?  I'm changing my mind
here and suggesting that we clean things up first :).

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:70: dst[i32] = (uint32_t)   b <<  0
On 2016/01/22 14:37:32, mtklein wrote:
> Let's keep our order consistent with the rest of the file?
> 
> uint8_t r = src8[2],
>         g = src8[1],
>         b = src8[0];
> src8 += 3;
> dst[i] = (uint32_t)0xFF << 24
>        | (uint32_t)r    << 16
>        | (uint32_t)g    <<  8
>        | (uint32_t)b    <<  0;

Done.

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:220: src8 += 48;
On 2016/01/22 14:37:33, mtklein wrote:
> might write this as += 16*3?
> 
> I find it really pleasant when code is obviously correct at a sleepy glance,
> rather than correct after remembering your times tables. :P

Done.

[mtklein, 2016-01-22 15:23:57]

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opts.h
File src/opts/SkSwizzler_opts.h (right):

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:18: static void premul_xxxa_portable(uint32_t dst[],
const uint32_t src[], int count) {
> >     RGB_to_rgbA()
> >     RGB_to_bgrA()
> Yes, but the output of these is not really premul/unpremul, it's opaque.
Oh, right, how about RGB_to_RGB1 / RGB_to_BGR1?
 
 
> >     RGB_to_RGBA()
> >     RGB_to_BGRA()
> These are identical to RGB_to_bgrA() and RGB_to_rgbA.
Oops, duh, yes.

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:63: static void xxx_xxxa_portable(uint32_t dst[],
const uint32_t src[], int count) {
> I'm changing my mind here and suggesting that we clean things up first :).

Good.  Was going to suggest that. :)

While they're all producing 8888 output, let's keep dst uint32_t*, but change
that if/when we add something that wants to create a different sort of output.

[msarett, 2016-01-22 16:36:42]

Patchset #8 (id:140001) has been deleted

[msarett, 2016-01-22 17:26:38]

Patchset #8 (id:160001) has been deleted

[msarett, 2016-01-22 17:27:23]

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opts.h
File src/opts/SkSwizzler_opts.h (right):

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:18: static void premul_xxxa_portable(uint32_t dst[],
const uint32_t src[], int count) {
On 2016/01/22 15:23:57, mtklein wrote:
> > >     RGB_to_rgbA()
> > >     RGB_to_bgrA()
> > Yes, but the output of these is not really premul/unpremul, it's opaque.
> Oh, right, how about RGB_to_RGB1 / RGB_to_BGR1?
>  
>  
> > >     RGB_to_RGBA()
> > >     RGB_to_BGRA()
> > These are identical to RGB_to_bgrA() and RGB_to_rgbA.
> Oops, duh, yes.
> 

Done.

https://codereview.chromium.org/1618003002/diff/80001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:63: static void xxx_xxxa_portable(uint32_t dst[],
const uint32_t src[], int count) {
On 2016/01/22 15:23:57, mtklein wrote:
> > I'm changing my mind here and suggesting that we clean things up first :).
> 
> Good.  Was going to suggest that. :)
> 
> While they're all producing 8888 output, let's keep dst uint32_t*, but change
> that if/when we add something that wants to create a different sort of output.

Done.

[mtklein, 2016-01-22 17:35:15]

lgtm

https://codereview.chromium.org/1618003002/diff/180001/src/opts/SkSwizzler_op...
File src/opts/SkSwizzler_opts.h (right):

https://codereview.chromium.org/1618003002/diff/180001/src/opts/SkSwizzler_op...
src/opts/SkSwizzler_opts.h:66: uint8_t b = src[2],
probably clearer to go back to r = src[0], g = src[1], b = src[2] order for src
reads now that we've named everything RGB?  

(sorry to keep yanking you around)

https://codereview.chromium.org/1618003002/diff/180001/src/opts/SkSwizzler_op...
src/opts/SkSwizzler_opts.h:127: uint8x8x4_t rgba = vld4_u8((const uint8_t*)
src);
Ah, thanks for catching the rest of this.

[msarett, 2016-01-22 17:38:18]

https://codereview.chromium.org/1618003002/diff/180001/src/opts/SkSwizzler_op...
File src/opts/SkSwizzler_opts.h (right):

https://codereview.chromium.org/1618003002/diff/180001/src/opts/SkSwizzler_op...
src/opts/SkSwizzler_opts.h:66: uint8_t b = src[2],
On 2016/01/22 17:35:15, mtklein wrote:
> probably clearer to go back to r = src[0], g = src[1], b = src[2] order for
src
> reads now that we've named everything RGB?  
> 
> (sorry to keep yanking you around)

Done.

[msarett, 2016-01-22 17:38:26]

The CQ bit was checked by msarett@google.com

[msarett, 2016-01-22 17:38: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/1618003002/#ps200001
(title: "Fix")

[commit-bot: I haz the power, 2016-01-22 17:38:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1618003002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1618003002/200001

[commit-bot: I haz the power, 2016-01-22 17:54:23]

Description was changed from

==========
Use NEON optimizations for RGB -> RGB(FF) or BGR(FF) in SkSwizzler

Swizzle Bench Runtime Nexus 6P
xxx_xxxa        0.32x
xxx_swaprb_xxxa 0.31x

Swizzle Bench Runtime Nexus 9
xxx_xxxa        1.11x
xxx_swaprb_xxxa 1.14x
(This is a slow down.)

Swizzle Bench Runtime Nexus 5
xxx_xxxa        0.12x
xxx_swaprb      0.12x

RGB PNG Decode Runtime
Nexus 6P        0.94x
Nexus 9         0.98x

I don't know how to explain the fact that the Swizzle Bench was
slower on Nexus 9, but the decode times got faster.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Use NEON optimizations for RGB -> RGB(FF) or BGR(FF) in SkSwizzler

Swizzle Bench Runtime Nexus 6P
xxx_xxxa        0.32x
xxx_swaprb_xxxa 0.31x

Swizzle Bench Runtime Nexus 9
xxx_xxxa        1.11x
xxx_swaprb_xxxa 1.14x
(This is a slow down.)

Swizzle Bench Runtime Nexus 5
xxx_xxxa        0.12x
xxx_swaprb      0.12x

RGB PNG Decode Runtime
Nexus 6P        0.94x
Nexus 9         0.98x

I don't know how to explain the fact that the Swizzle Bench was
slower on Nexus 9, but the decode times got faster.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot

Committed:
https://skia.googlesource.com/skia/+/f1b8b6ae34e5a1f4b29e423401da39f88f0c117a
==========

[commit-bot: I haz the power, 2016-01-22 17:54:24]

Committed patchset #9 (id:200001) as
https://skia.googlesource.com/skia/+/f1b8b6ae34e5a1f4b29e423401da39f88f0c117a