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

src/opts/SkSwizzler_opts.h

 /*
  * Copyright 2016 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #ifndef SkSwizzler_opts_DEFINED
 #define SkSwizzler_opts_DEFINED
 
 #include "SkColorPriv.h"
 
 namespace SK_OPTS_NS {
 
 // These variable names in these functions just pretend the input is BGRA.

[mtklein, 2016/01/22 14:37:32]

 (or BGR)

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

Done.

 // They work fine with both RGBA and BGRA.

[mtklein, 2016/01/22 14:37:32]

 (or both BGR and RGB).

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

Done.

 
 static void premul_xxxa_portable(uint32_t dst[], const uint32_t src[], int count) {

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

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?

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

Gonna write up a CL to demonstrate what I mean.

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

Yes.
Yes.
Yes, but the output of these is not really premul/unpremul, it's opaque.
Yes.
These are identical to RGB_to_bgrA() and RGB_to_rgbA.
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.

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

Oh, right, how about RGB_to_RGB1 / RGB_to_BGR1?
 
 
Oops, duh, yes.

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

Done.

     for (int i = 0; i < count; i++) {
         uint8_t a = src[i] >> 24,
                 r = src[i] >> 16,
                 g = src[i] >>  8,
                 b = src[i] >>  0;
         r = (r*a+127)/255;
         g = (g*a+127)/255;
         b = (b*a+127)/255;
         dst[i] = (uint32_t)a << 24
                | (uint32_t)r << 16
@@ skipping 24 matching lines @@
                 r = src[i] >> 16,
                 g = src[i] >>  8,
                 b = src[i] >>  0;
         dst[i] = (uint32_t)a << 24
                | (uint32_t)b << 16
                | (uint32_t)g <<  8
                | (uint32_t)r <<  0;
     }
 }
 
+static void xxx_xxxa_portable(uint32_t dst[], const uint32_t src[], int count) {

[mtklein, 2016/01/22 14:37:32]

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

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

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

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

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 17:27:23]

Done.

+    int i8 = 0;
+    const uint8_t* src8 = (const uint8_t*) src;
+    for (int i32 = 0; i32 < count; i32++) {
+        uint8_t b = src8[i8++],
+                g = src8[i8++],
+                r = src8[i8++];
+        dst[i32] = (uint32_t)   b <<  0

[mtklein, 2016/01/22 14:37:32]

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;

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

Done.

+                 | (uint32_t)   g <<  8
+                 | (uint32_t)   r << 16
+                 | (uint32_t)0xFF << 24;
+    }
+}
+
+static void xxx_swaprb_xxxa_portable(uint32_t dst[], const uint32_t src[], int count) {
+    int i8 = 0;
+    const uint8_t* src8 = (const uint8_t*) src;
+    for (int i32 = 0; i32 < count; i32++) {
+        uint8_t b = src8[i8++],
+                g = src8[i8++],
+                r = src8[i8++];
+        dst[i32] = (uint32_t)   r <<  0
+                 | (uint32_t)   g <<  8
+                 | (uint32_t)   b << 16
+                 | (uint32_t)0xFF << 24;
+    }
+}
+
 #if defined(SK_ARM_HAS_NEON)
 
 // Rounded divide by 255, (x + 127) / 255
 static uint8x8_t div255_round(uint16x8_t x) {
     // result = (x + 127) / 255
     // result = (x + 127) / 256 + error1
     //
     // error1 = (x + 127) / (255 * 256)
     // error1 = (x + 127) / (256 * 256) + error2
     //
@@ skipping 88 matching lines @@
         // Store 8 pixels.
         vst4_u8((uint8_t*) dst, bgra);
         src += 8;
         dst += 8;
         count -= 8;
     }
 
     swaprb_xxxa_portable(dst, src, count);
 }
 
+template <bool kSwapRB>
+static void xxx_xxxa_should_swaprb(uint32_t dst[], const uint32_t src[], int count) {
+    const uint8_t* src8 = (const uint8_t*) src;
+    while (count >= 16) {
+        // Load 16 pixels.
+        uint8x16x3_t bgr = vld3q_u8(src8);
+
+        // Insert an opaque alpha channel and swap if needed.
+        uint8x16x4_t bgra;
+        if (kSwapRB) {
+            bgra.val[0] = bgr.val[2];
+            bgra.val[2] = bgr.val[0];
+        } else {
+            bgra.val[0] = bgr.val[0];
+            bgra.val[2] = bgr.val[2];
+        }
+        bgra.val[1] = bgr.val[1];
+        bgra.val[3] = vdupq_n_u8(0xFF);
+
+        // Store 16 pixels.
+        vst4q_u8((uint8_t*) dst, bgra);
+        src8 += 48;

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

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

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

Done.

+        dst += 16;
+        count -= 16;
+    }
+
+    if (count >= 8) {
+        // Load 8 pixels.
+        uint8x8x3_t bgr = vld3_u8(src8);
+
+        // Insert an opaque alpha channel and swap if needed.
+        uint8x8x4_t bgra;
+        if (kSwapRB) {
+            bgra.val[0] = bgr.val[2];
+            bgra.val[2] = bgr.val[0];
+        } else {
+            bgra.val[0] = bgr.val[0];
+            bgra.val[2] = bgr.val[2];
+        }
+        bgra.val[1] = bgr.val[1];
+        bgra.val[3] = vdup_n_u8(0xFF);
+
+        // Store 8 pixels.
+        vst4_u8((uint8_t*) dst, bgra);
+        src8 += 24;
+        dst += 8;
+        count -= 8;
+    }
+
+    // Call portable code to finish up the tail of [0,8) pixels.
+    auto proc = kSwapRB ? xxx_swaprb_xxxa_portable : xxx_xxxa_portable;
+    proc(dst, (const uint32_t*) src8, count);
+}
+
+static void xxx_xxxa(uint32_t dst[], const uint32_t src[], int count) {
+    xxx_xxxa_should_swaprb<false>(dst, src, count);
+}
+
+static void xxx_swaprb_xxxa(uint32_t dst[], const uint32_t src[], int count) {
+    xxx_xxxa_should_swaprb<true>(dst, src, count);
+}
+
 #elif SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSSE3
 
 template <bool kSwapRB>
 static void premul_xxxa_should_swaprb(uint32_t dst[], const uint32_t src[], int count) {
 
     auto premul8 = [](__m128i* lo, __m128i* hi) {
         const __m128i zeros = _mm_setzero_si128();
         const __m128i _128 = _mm_set1_epi16(128);
         const __m128i _257 = _mm_set1_epi16(257);
         __m128i planar;
@@ skipping 76 matching lines @@
         _mm_storeu_si128((__m128i*) dst, rgba);
 
         src += 4;
         dst += 4;
         count -= 4;
     }
 
     swaprb_xxxa_portable(dst, src, count);
 }
 
+static void xxx_xxxa(uint32_t dst[], const uint32_t src[], int count) {
+    xxx_xxxa_portable(dst, src, count);
+}
+
+static void xxx_swaprb_xxxa(uint32_t dst[], const uint32_t src[], int count) {
+    xxx_swaprb_xxxa_portable(dst, src, count);
+}
+
 #else
 
 static void premul_xxxa(uint32_t dst[], const uint32_t src[], int count) {
     premul_xxxa_portable(dst, src, count);
 }
 
 static void premul_swaprb_xxxa(uint32_t dst[], const uint32_t src[], int count) {
     premul_swaprb_xxxa_portable(dst, src, count);
 }
 
 static void swaprb_xxxa(uint32_t dst[], const uint32_t src[], int count) {
     swaprb_xxxa_portable(dst, src, count);
 }
 
+static void xxx_xxxa(uint32_t dst[], const uint32_t src[], int count) {
+    xxx_xxxa_portable(dst, src, count);
+}
+
+static void xxx_swaprb_xxxa(uint32_t dst[], const uint32_t src[], int count) {
+    xxx_swaprb_xxxa_portable(dst, src, count);
+}
+
 #endif
 
 }
 
 #endif // SkSwizzler_opts_DEFINED