Add NEON swap opts and use opts in SkSwizzler

src/codec/SkSwizzler.cpp

Index: src/codec/SkSwizzler.cpp
diff --git a/src/codec/SkSwizzler.cpp b/src/codec/SkSwizzler.cpp
index 74d6c7f87fce8fea08e61ab33099a9d72e759a54..2103a3b07526dd6ab0100f92bc808f6aef9228d8 100644
--- a/src/codec/SkSwizzler.cpp
+++ b/src/codec/SkSwizzler.cpp
@@ -303,6 +303,26 @@ static void swizzle_bgrx_to_n32(
     }
 }
 
+static void fast_swizzle_bgrx_to_n32(
+        void* dst, const uint8_t* src, int width, int bpp, int deltaSrc, int offset,
+        const SkPMColor ctable[]) {
+
+    // This function must not be called if we are sampling.  If we are not
+    // sampling, deltaSrc should equal bpp.
+    SkASSERT(deltaSrc == bpp);
+
+    // The default swizzle supports BGR->N32 and BGRX->N32.  This only
+    // supports BGRX->N32.
+    SkASSERT(4 == bpp);
+
+    // These swizzles trust that the alpha value is already 0xFF.
+#ifdef SK_PMCOLOR_IS_RGBA
+    SkOpts::swaprb_xxxa((uint32_t*) dst, (const uint32_t*) (src + offset), width);
+#else
+    memcpy(dst, src + offset, width * bpp);

[msarett, 2016/01/14 17:08:25]

This could be a no-op if we decoded directly into dst.  I think this makes an
argument to try decoding directly into dst.

I could see us making dst == src on decodes where the SrcConfig and DstColorType
have the same number of bytes per pixel.

One potential complication is subset decodes (where offset is non-zero).

[scroggo, 2016/01/14 17:55:06]

+1

+#endif
+}
+
 static void swizzle_bgrx_to_565(
         void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int dstWidth,
         int bpp, int deltaSrc, int offset, const SkPMColor ctable[]) {
@@ -343,7 +363,23 @@ static void swizzle_bgra_to_n32_premul(
     }
 }
 
+static void fast_swizzle_bgra_to_n32_premul(

[scroggo, 2016/01/14 17:55:06]

Should we have a fast_swizzle to unpremul, as well?

Also, we've talked about decoding to the non-native 32 bit version. Should we
add those along with these changes or hold off?

[msarett, 2016/01/14 18:23:19]

Sorry I missed this.

This is implemented in this CL also.  It uses fast_swizzle_bgrx_to_n32 since the
code is identical.  I originally had them separated, but then I realized that
the code was the same.

[scroggo, 2016/01/14 19:08:06]

Ah, then should these functions be named something other than "n32"? _to_32?

[msarett, 2016/01/14 19:45:55]

Changed to _to_32.

+        void* dst, const uint8_t* src, int width, int bpp, int deltaSrc, int offset,
+        const SkPMColor ctable[]) {
+
+    // This function must not be called if we are sampling.  If we are not
+    // sampling, deltaSrc should equal bpp.
+    SkASSERT(deltaSrc == bpp);
+
+#ifdef SK_PMCOLOR_IS_RGBA
+    SkOpts::premul_swaprb_xxxa((uint32_t*) dst, (const uint32_t*) (src + offset), width);
+#else
+    SkOpts::premul_xxxa((uint32_t*) dst, (const uint32_t*) (src + offset), width);
+#endif
+}
+
 // kRGBX
+
 static void swizzle_rgbx_to_n32(
         void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int dstWidth,
         int bpp, int deltaSrc, int offset, const SkPMColor ctable[]) {
@@ -356,6 +392,26 @@ static void swizzle_rgbx_to_n32(
     }
 }
 
+static void fast_swizzle_rgbx_to_n32(
+        void* dst, const uint8_t* src, int width, int bpp, int deltaSrc, int offset,
+        const SkPMColor ctable[]) {
+
+    // This function must not be called if we are sampling.  If we are not
+    // sampling, deltaSrc should equal bpp.
+    SkASSERT(deltaSrc == bpp);
+
+    // The default swizzle supports RGB->N32 and RGBX->N32.  This only
+    // supports RGBX->N32.
+    SkASSERT(4 == bpp);
+
+    // These swizzles trust that the alpha value is already 0xFF.
+#ifdef SK_PMCOLOR_IS_RGBA
+    memcpy(dst, src + offset, width * bpp);
+#else
+    SkOpts::swaprb_xxxa((uint32_t*) dst, (const uint32_t*) (src + offset), width);
+#endif
+}
+
 static void swizzle_rgbx_to_565(
        void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int dstWidth,
        int bytesPerPixel, int deltaSrc, int offset, const SkPMColor ctable[]) {
@@ -369,6 +425,7 @@ static void swizzle_rgbx_to_565(
 }
 
 // kRGBA
+
 static void swizzle_rgba_to_n32_premul(
         void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int dstWidth,
         int bpp, int deltaSrc, int offset, const SkPMColor ctable[]) {
@@ -594,11 +651,11 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
                     break;
             }
             break;
-        case kBGR:
         case kBGRX:
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
                     proc = &swizzle_bgrx_to_n32;
+                    fastProc = &fast_swizzle_bgrx_to_n32;
                     break;
                 case kRGB_565_SkColorType:
                     proc = &swizzle_bgrx_to_565;
@@ -610,15 +667,22 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
         case kBGRA:
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
-                    switch (dstInfo.alphaType()) {
-                        case kUnpremul_SkAlphaType:
+                    if (dstInfo.alphaType() == kUnpremul_SkAlphaType) {
+                        if (SkCodec::kYes_ZeroInitialized == zeroInit) {
+                            proc = &SkipLeading8888ZerosThen<swizzle_bgra_to_n32_unpremul>;
+                            fastProc = &SkipLeading8888ZerosThen<fast_swizzle_bgrx_to_n32>;
+                        } else {
                             proc = &swizzle_bgra_to_n32_unpremul;
-                            break;
-                        case kPremul_SkAlphaType:
+                            fastProc = &fast_swizzle_bgrx_to_n32;
+                        }
+                    } else {
+                        if (SkCodec::kYes_ZeroInitialized == zeroInit) {
+                            proc = &SkipLeading8888ZerosThen<swizzle_bgra_to_n32_premul>;
+                            fastProc = &SkipLeading8888ZerosThen<fast_swizzle_bgra_to_n32_premul>;
+                        } else {
                             proc = &swizzle_bgra_to_n32_premul;
-                            break;
-                        default:
-                            break;
+                            fastProc = &fast_swizzle_bgra_to_n32_premul;
+                        }
                     }
                     break;
                 default:
@@ -629,6 +693,7 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
                     proc = &swizzle_rgbx_to_n32;
+                    fastProc = &fast_swizzle_rgbx_to_n32;
                     break;
                 case kRGB_565_SkColorType:
                     proc = &swizzle_rgbx_to_565;
@@ -642,8 +707,10 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
                     if (dstInfo.alphaType() == kUnpremul_SkAlphaType) {
                         if (SkCodec::kYes_ZeroInitialized == zeroInit) {
                             proc = &SkipLeading8888ZerosThen<swizzle_rgba_to_n32_unpremul>;
+                            fastProc = &SkipLeading8888ZerosThen<fast_swizzle_rgbx_to_n32>;
                         } else {
                             proc = &swizzle_rgba_to_n32_unpremul;
+                            fastProc = &fast_swizzle_rgbx_to_n32;
                         }
                     } else {
                         if (SkCodec::kYes_ZeroInitialized == zeroInit) {
@@ -664,6 +731,21 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
                 case kN32_SkColorType:
                     proc = &swizzle_rgbx_to_n32;
                     break;
+                case kRGB_565_SkColorType:

[msarett, 2016/01/14 17:08:25]

We have never needed this because we "fill" RGB PNGs to RGB(FF) in libpng.

We will stop doing this in libpng because it is slow and an extra pass over the
output.

[scroggo, 2016/01/14 17:55:06]

Is that a separate (future) CL?

[msarett, 2016/01/14 18:09:09]

Yes.  Maybe this doesn't belong in this CL?

[scroggo, 2016/01/14 18:14:51]

If it's only needed once we turn off filling, then I don't think it belongs.
(Unless it makes the rest of this code simpler?)

[msarett, 2016/01/14 18:27:51]

Done.

+                    proc = &swizzle_rgbx_to_565;
+                    break;
+                default:
+                    break;
+            }
+            break;
+        case kBGR:

[msarett, 2016/01/14 17:08:25]

This will diverge from BGRX because of the opt functions.

[scroggo, 2016/01/14 18:14:51]

Will there be opts for BGR? Or is that not conducive to using SIMD? Would it be
faster to make two passes: one to fill (in libpng) and then use SIMD to swizzle?
I'm guessing not?

[msarett, 2016/01/14 18:23:19]

Yes, I plan on opts for BGR and RGB (coming soon).

What you've suggested is an option, but I think we can do better than that.

+            switch (dstInfo.colorType()) {
+                case kN32_SkColorType:
+                    proc = &swizzle_bgrx_to_n32;
+                    break;
+                case kRGB_565_SkColorType:
+                    proc = &swizzle_bgrx_to_565;
+                    break;
                 default:
                     break;
             }