Use SkOpts routines in SkSwizzler

src/codec/SkSwizzler.cpp

Index: src/codec/SkSwizzler.cpp
diff --git a/src/codec/SkSwizzler.cpp b/src/codec/SkSwizzler.cpp
index d783380294ab81f18a5c67e2d66bf8e5a33db2d4..42f09591a4f81a4065e057a101fc4de16b408ebf 100644
--- a/src/codec/SkSwizzler.cpp
+++ b/src/codec/SkSwizzler.cpp
@@ -8,6 +8,7 @@
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
 #include "SkSwizzler.h"
+#include "SkSwizzlerOpts.h"
 #include "SkTemplates.h"
 
 SkSwizzler::ResultAlpha SkSwizzler::GetResult(uint8_t zeroAlpha,
@@ -428,7 +429,6 @@ static SkSwizzler::ResultAlpha swizzle_rgbx_to_565(
     return SkSwizzler::kOpaque_ResultAlpha;
 }
 
-
 // kRGBA
 static SkSwizzler::ResultAlpha swizzle_rgba_to_n32_premul(
         void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int dstWidth,
@@ -603,22 +603,23 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
             && nullptr == ctable) {
         return nullptr;
     }
-    RowProc proc = nullptr;
+    RowProc optProc = nullptr;
+    RowProc scalarProc = nullptr;
     SkCodec::ZeroInitialized zeroInit = options.fZeroInitialized;
     switch (sc) {
         case kBit:
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
-                    proc = &swizzle_bit_to_n32;
+                    optProc = &swizzle_bit_to_n32;
                     break;
                 case kIndex_8_SkColorType:
-                    proc = &swizzle_bit_to_index;
+                    optProc = &swizzle_bit_to_index;
                     break;
                 case kRGB_565_SkColorType:
-                    proc = &swizzle_bit_to_565;
+                    optProc = &swizzle_bit_to_565;
                     break;
                 case kGray_8_SkColorType:
-                    proc = &swizzle_bit_to_grayscale;
+                    optProc = &swizzle_bit_to_grayscale;
                     break;
                 default:
                     break;
@@ -629,13 +630,13 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
         case kIndex4:
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
-                    proc = &swizzle_small_index_to_n32;
+                    optProc = &swizzle_small_index_to_n32;
                     break;
                 case kRGB_565_SkColorType:
-                    proc = &swizzle_small_index_to_565;
+                    optProc = &swizzle_small_index_to_565;
                     break;
                 case kIndex_8_SkColorType:
-                    proc = &swizzle_small_index_to_index;
+                    optProc = &swizzle_small_index_to_index;
                     break;
                 default:
                     break;
@@ -646,18 +647,18 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
                 case kN32_SkColorType:
                     // We assume the color premultiplied ctable (or not) as desired.
                     if (SkCodec::kYes_ZeroInitialized == zeroInit) {
-                        proc = &swizzle_index_to_n32_skipZ;
+                        optProc = &swizzle_index_to_n32_skipZ;
                         break;
                     } else {
-                        proc = &swizzle_index_to_n32;
+                        optProc = &swizzle_index_to_n32;
                         break;
                     }
                     break;
                 case kRGB_565_SkColorType:
-                    proc = &swizzle_index_to_565;
+                    optProc = &swizzle_index_to_565;
                     break;
                 case kIndex_8_SkColorType:
-                    proc = &swizzle_index_to_index;
+                    optProc = &swizzle_index_to_index;
                     break;
                 default:
                     break;
@@ -666,13 +667,13 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
         case kGray:
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
-                    proc = &swizzle_gray_to_n32;
+                    optProc = &swizzle_gray_to_n32;
                     break;
                 case kGray_8_SkColorType:
-                    proc = &swizzle_gray_to_gray;
+                    optProc = &swizzle_gray_to_gray;
                     break;
                 case kRGB_565_SkColorType:
-                    proc = &swizzle_gray_to_565;
+                    optProc = &swizzle_gray_to_565;
                     break;
                 default:
                     break;
@@ -682,10 +683,10 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
         case kBGRX:
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
-                    proc = &swizzle_bgrx_to_n32;
+                    optProc = &swizzle_bgrx_to_n32;
                     break;
                 case kRGB_565_SkColorType:
-                    proc = &swizzle_bgrx_to_565;
+                    optProc = &swizzle_bgrx_to_565;
                     break;
                 default:
                     break;
@@ -696,10 +697,10 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
                 case kN32_SkColorType:
                     switch (dstInfo.alphaType()) {
                         case kUnpremul_SkAlphaType:
-                            proc = &swizzle_bgra_to_n32_unpremul;
+                            optProc = &swizzle_bgra_to_n32_unpremul;
                             break;
                         case kPremul_SkAlphaType:
-                            proc = &swizzle_bgra_to_n32_premul;
+                            optProc = &swizzle_bgra_to_n32_premul;
                             break;
                         default:
                             break;
@@ -710,13 +711,12 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
             }
             break;
         case kRGBX:
-            // TODO: Support other swizzles.
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
-                    proc = &swizzle_rgbx_to_n32;
+                    optProc = &swizzle_rgbx_to_n32;
                     break;
                 case kRGB_565_SkColorType:
-                    proc = &swizzle_rgbx_to_565;
+                    optProc = &swizzle_rgbx_to_565;
                 default:
                     break;
             }
@@ -726,12 +726,13 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
                 case kN32_SkColorType:
                     if (dstInfo.alphaType() == kUnpremul_SkAlphaType) {
                         // Respect zeroInit?
-                        proc = &swizzle_rgba_to_n32_unpremul;
+                        optProc = &swizzle_rgba_to_n32_unpremul;
                     } else {
+                        optProc = &opt_swizzle_rgba_to_n32_premul;
                         if (SkCodec::kYes_ZeroInitialized == zeroInit) {
-                            proc = &swizzle_rgba_to_n32_premul_skipZ;
+                            scalarProc = &swizzle_rgba_to_n32_premul_skipZ;

[msarett, 2016/01/08 13:56:00]

We'll want to benchmark against the regular version and the "skip zeros"
version.  This is a good time to find out the benefit of skipping zeros.

[mtklein, 2016/01/08 15:20:19]

There's more to this feature than just speed, right?  Even if we can go as fast,
seems like the feature is really to avoid physically touching those bytes so
that those pages stay backed by a shared zero page instead of their own physical
RAM?  (Note I am personally still skeptical that optimizing for ~1000 pixel
transparent blocks in Skia is better than, say, cropping the images...)

Skipping zeros is something we could do well in SSE2+ (especially SSE4.1+ or
AVX2), but I suspect it's the sort of thing that will completely hose NEON
performance (no movemask/ptest, and it's quite slow to communicate from NEON
back to ARM).  Still, if we must do it, it seems worth trying.

[scroggo, 2016/01/08 16:02:56]

Yes.
+1. But I think we'll need to update lots of client code in order to crop the
images instead. Not saying that it's not worth doing, but we'll need to change
the framework (and probably get buy-in, because they'll need to continue doing
it the new way) *and* make apps update to the new way. I think the right way to
switch is to still support this strange optimization but show the all-around
benefits of cropping the images, so people will opt-in. If we were to drop the
feature, suddenly Android devices may have a lot less free memory.

[mtklein, 2016/01/08 16:23:01]

And we're certain there really are runs of >=1024 transparent pixels right?  If
we touch any byte on a (4K) page, we might as well touch them all as far as
memory savings go.  1024 transparent pixels is the smallest value that makes
this worth doing, and that's when we're perfectly aligned to a page, which may
be optimistic.

[scroggo, 2016/01/08 16:50:51]

Yes. (I can dig up the bug, but when the change first landed it saved many MB of
RAM, IIRC.) For all around simplicity, I did not make any checks to see how many
consecutive transparent pixels there are. I'm not opposed to making the check if
we can do it efficiently.

[msarett, 2016/01/11 20:33:24]

I think Mike's idea to skip leading zeros will be sufficient.
https://bugs.chromium.org/p/skia/issues/detail?id=4787

                         } else {

[mtklein, 2016/01/08 15:20:19]

If I'm right above about this being more than just a speed thing, can't we only
use the opt_ method inside the else here?

[msarett, 2016/01/11 20:33:24]

Yes we could.  I don't think that's a good option though because Android decodes
are almost always zeroInit (and our first goal is to optimize Android).

-                            proc = &swizzle_rgba_to_n32_premul;
+                            scalarProc = &swizzle_rgba_to_n32_premul;
                         }
                     }
                     break;
@@ -742,7 +743,7 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
         case kRGB:
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
-                    proc = &swizzle_rgbx_to_n32;
+                    optProc = &swizzle_rgbx_to_n32;
                     break;
                 default:
                     break;
@@ -751,7 +752,7 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
         case kRGB_565:
             switch (dstInfo.colorType()) {
                 case kRGB_565_SkColorType:
-                    proc = &sample565;
+                    optProc = &sample565;
                     break;
                 default:
                     break;
@@ -760,10 +761,10 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
         case kCMYK:
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
-                    proc = &swizzle_cmyk_to_n32;
+                    optProc = &swizzle_cmyk_to_n32;
                     break;
                 case kRGB_565_SkColorType:
-                    proc = &swizzle_cmyk_to_565;
+                    optProc = &swizzle_cmyk_to_565;
                     break;
                 default:
                     break;
@@ -772,9 +773,12 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
         default:
             break;
     }
-    if (nullptr == proc) {
+    if (nullptr == optProc) {

[scroggo, 2016/01/08 16:02:57]

So are we guaranteed that there's an optProc, but there might also be a
scalarProc?

[mtklein, 2016/01/08 16:23:01]

(That's what I find confusing.  In reality, we're guaranteed there's a
general-case proc, and the optimized procs are sparse.  This code makes it feel
the other way around.)

[msarett, 2016/01/11 20:33:24]

Done.

         return nullptr;
     }
+    if (nullptr == scalarProc) {
+        scalarProc = optProc;
+    }
 
     // Store bpp in bytes if it is an even multiple, otherwise use bits
     int srcBPP = SkIsAlign8(BitsPerPixel(sc)) ? BytesPerPixel(sc) : BitsPerPixel(sc);
@@ -796,12 +800,14 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
         srcWidth = frame->width();
     }
 
-    return new SkSwizzler(proc, ctable, srcOffset, srcWidth, dstOffset, dstWidth, srcBPP, dstBPP);
+    return new SkSwizzler(optProc, scalarProc, ctable, srcOffset, srcWidth, dstOffset, dstWidth,
+            srcBPP, dstBPP);
 }
 
-SkSwizzler::SkSwizzler(RowProc proc, const SkPMColor* ctable, int srcOffset, int srcWidth,
-        int dstOffset, int dstWidth, int srcBPP, int dstBPP)
-    : fRowProc(proc)
+SkSwizzler::SkSwizzler(RowProc optProc, RowProc scalarProc, const SkPMColor* ctable, int srcOffset,
+        int srcWidth, int dstOffset, int dstWidth, int srcBPP, int dstBPP)
+    : fRowProc(optProc)
+    , fScalarRowProc(scalarProc)
     , fColorTable(ctable)
     , fSrcOffset(srcOffset)
     , fDstOffset(dstOffset)
@@ -825,6 +831,9 @@ int SkSwizzler::onSetSampleX(int sampleX) {
     fSwizzleWidth = get_scaled_dimension(fSrcWidth, sampleX);
     fAllocatedWidth = get_scaled_dimension(fDstWidth, sampleX);
 
+    // The optimized swizzler routines do not (yet) support sampling.
+    fRowProc = fScalarRowProc;

[mtklein, 2016/01/08 15:20:19]

This preference logic might be simpler to follow generally if we tweak it a
little.  Today it seems like the logic is "use optProc unless we can't, then
fall back to scalarProc".  What if we write it as "use optProc if we have one,
defaulting to scalarProc"?  I.e. make having optProc the special case, rather
than not having optProc?

That'd make this line something like

   fOptProc = nullptr;

and I think it might be less intrusive for the rest of SkSwizzler.

[msarett, 2016/01/11 20:33:24]

Done.

+
     return fAllocatedWidth;
 }