Use SkOpts routines in SkSwizzler

src/codec/SkSwizzler.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
 #include "SkSwizzler.h"
+#include "SkSwizzlerOpts.h"
 #include "SkTemplates.h"
 
 SkSwizzler::ResultAlpha SkSwizzler::GetResult(uint8_t zeroAlpha,
                                               uint8_t maxAlpha) {
     // In the transparent case, this returns 0x0000
     // In the opaque case, this returns 0xFFFF
     // If the row is neither transparent nor opaque, returns something else
     return (((uint16_t) maxAlpha) << 8) | zeroAlpha;
 }
 
@@ skipping 400 matching lines @@
     // FIXME: Support dithering?
     src += offset;
     uint16_t* SK_RESTRICT dst = (uint16_t*)dstRow;
     for (int x = 0; x < dstWidth; x++) {
         dst[x] = SkPack888ToRGB16(src[0], src[1], src[2]);
         src += deltaSrc;
     }
     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,
         int bpp, int deltaSrc, int offset, const SkPMColor ctable[]) {
 
     src += offset;
     SkPMColor* SK_RESTRICT dst = (SkPMColor*)dstRow;
     INIT_RESULT_ALPHA;
     for (int x = 0; x < dstWidth; x++) {
         unsigned alpha = src[3];
@@ skipping 154 matching lines @@
                                        const SkImageInfo& dstInfo,
                                        const SkCodec::Options& options,
                                        const SkIRect* frame) {
     if (dstInfo.colorType() == kUnknown_SkColorType || kUnknown == sc) {
         return nullptr;
     }
     if ((kIndex == sc || kIndex4 == sc || kIndex2 == sc || kIndex1 == 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;
             }
             break;
         case kIndex1:
         case kIndex2:
         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;
             }
             break;
         case kIndex:
             switch (dstInfo.colorType()) {
                 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;
             }
             break;
         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;
             }
             break;
         case kBGR:
         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;
             }
             break;
         case kBGRA:
             switch (dstInfo.colorType()) {
                 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;
                     }
                     break;
                 default:
                     break;
             }
             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;
             }
             break;
         case kRGBA:
             switch (dstInfo.colorType()) {
                 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;
                 default:
                     break;
             }
             break;
         case kRGB:
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
-                    proc = &swizzle_rgbx_to_n32;
+                    optProc = &swizzle_rgbx_to_n32;
                     break;
                 default:
                     break;
             }
             break;
         case kRGB_565:
             switch (dstInfo.colorType()) {
                 case kRGB_565_SkColorType:
-                    proc = &sample565;
+                    optProc = &sample565;
                     break;
                 default:
                     break;
             }
             break;
         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;
             }
             break;
         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);
     int dstBPP = SkColorTypeBytesPerPixel(dstInfo.colorType());
     
     int srcOffset = 0;
     int srcWidth = dstInfo.width();
     int dstOffset = 0;
     int dstWidth = srcWidth;
     if (options.fSubset) {
         // We do not currently support subset decodes for image types that may have
         // frames (gif).
         SkASSERT(!frame);
         srcOffset = options.fSubset->left();
         srcWidth = options.fSubset->width();
         dstWidth = srcWidth;
     } else if (frame) {
         dstOffset = frame->left();
         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)
     , fSrcOffsetUnits(srcOffset * srcBPP)
     , fDstOffsetBytes(dstOffset * dstBPP)
     , fSrcWidth(srcWidth)
     , fDstWidth(dstWidth)
     , fSwizzleWidth(srcWidth)
     , fAllocatedWidth(dstWidth)
     , fSampleX(1)
     , fSrcBPP(srcBPP)
     , fDstBPP(dstBPP)
 {}
 
 int SkSwizzler::onSetSampleX(int sampleX) {

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

I don't suppose x-sampling can move to the constructor?

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

It used to be in the constructor, but we moved it out in crrev.com/1372973002.
This allowed us to factor out sampling from the codec implementations.

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

I think it makes sense to keep things this way for now.

If we find another client of Codec that also wants sampling, maybe it would be
worth considering making sampling a part of Codec?

     SkASSERT(sampleX > 0); // Surely there is an upper limit? Should there be
                            // way to report failure?
     fSampleX = sampleX;
     fSrcOffsetUnits = (get_start_coord(sampleX) + fSrcOffset) * fSrcBPP;
     fDstOffsetBytes = (fDstOffset / sampleX) * fDstBPP;
     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;
 }
 
 SkSwizzler::ResultAlpha SkSwizzler::swizzle(void* dst, const uint8_t* SK_RESTRICT src) {
     SkASSERT(nullptr != dst && nullptr != src);
     return fRowProc(SkTAddOffset<void>(dst, fDstOffsetBytes), src, fSwizzleWidth, fSrcBPP,
             fSampleX * fSrcBPP, fSrcOffsetUnits, fColorTable);
 }