Clean up unused or overly complex modes in SkSwizzler

src/codec/SkSwizzler.cpp

Index: src/codec/SkSwizzler.cpp
diff --git a/src/codec/SkSwizzler.cpp b/src/codec/SkSwizzler.cpp
index e560d6f3cd999a13698d4607392817018f974a37..7a909073c7bd2ddd92e85d45c67aaaf0c99021d6 100644
--- a/src/codec/SkSwizzler.cpp
+++ b/src/codec/SkSwizzler.cpp
@@ -11,14 +11,41 @@
 #include "SkSwizzler.h"
 #include "SkTemplates.h"
 
-// samples the row. Does not do anything else but sampling
-static void sample565(void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src,
-        int width, int bpp, int deltaSrc, int offset, const SkPMColor ctable[]){
+static void copy(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);
 
+    memcpy(dst, src + offset, width * bpp);
+}
+
+static void sample1(void* dst, const uint8_t* src, int width, int bpp, int deltaSrc, int offset,
+        const SkPMColor ctable[]) {
     src += offset;
-    uint16_t* SK_RESTRICT dst = (uint16_t*) dstRow;
+    uint8_t* dst8 = (uint8_t*) dst;
+    for (int x = 0; x < width; x++) {
+        dst8[x] = *src;
+        src += deltaSrc;
+    }
+}
+
+static void sample2(void* dst, const uint8_t* src, int width, int bpp, int deltaSrc, int offset,
+        const SkPMColor ctable[]) {
+    src += offset;
+    uint16_t* dst16 = (uint16_t*) dst;
+    for (int x = 0; x < width; x++) {
+        dst16[x] = *((const uint16_t*) src);
+        src += deltaSrc;
+    }
+}
+
+static void sample4(void* dst, const uint8_t* src, int width, int bpp, int deltaSrc, int offset,
+        const SkPMColor ctable[]) {
+    src += offset;
+    uint32_t* dst32 = (uint32_t*) dst;
     for (int x = 0; x < width; x++) {
-        dst[x] = src[1] << 8 | src[0];
+        dst32[x] = *((const uint32_t*) src);
         src += deltaSrc;
     }
 }
@@ -192,22 +219,6 @@ static void swizzle_small_index_to_n32(
 
 // kIndex
 
-static void swizzle_index_to_index(
-        void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int dstWidth,
-        int bpp, int deltaSrc, int offset, const SkPMColor ctable[]) {
-
-    src += offset;
-    uint8_t* SK_RESTRICT dst = (uint8_t*) dstRow;
-    if (1 == deltaSrc) {
-        memcpy(dst, src, dstWidth);
-    } else {
-        for (int x = 0; x < dstWidth; x++) {
-            dst[x] = *src;
-            src += deltaSrc;
-        }
-    }
-}
-
 static void swizzle_index_to_n32(
         void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int dstWidth,
         int bpp, int deltaSrc, int offset, const SkPMColor ctable[]) {
@@ -261,22 +272,6 @@ static void swizzle_gray_to_n32(
     }
 }
 
-static void swizzle_gray_to_gray(
-        void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int dstWidth,
-        int bpp, int deltaSrc, int offset, const SkPMColor ctable[]) {
-
-    src += offset;
-    uint8_t* SK_RESTRICT dst = (uint8_t*) dstRow;
-    if (1 == deltaSrc) {
-        memcpy(dstRow, src, dstWidth);
-    } else {
-        for (int x = 0; x < dstWidth; x++) {
-            dst[x] = src[0];
-            src += deltaSrc;
-        }
-    }
-}
-
 static void swizzle_gray_to_565(
         void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int dstWidth,
         int bytesPerPixel, int deltaSrc, int offset, const SkPMColor ctable[]) {
@@ -303,26 +298,6 @@ static void swizzle_bgrx_to_n32(
     }
 }
 
-static void fast_swizzle_bgrx_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);
-
-    // 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);
-#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[]) {
@@ -350,6 +325,22 @@ static void swizzle_bgra_to_n32_unpremul(
     }
 }
 
+static void fast_swizzle_bgra_to_n32_unpremul(

[scroggo, 2016/01/14 21:28:18]

I thought we were going to use the same methods for n32 and non-native 32.
Should this name take that into account?

[msarett, 2016/01/14 22:25:20]

We can and maybe should consider sharing more code between n32 and non-native
32, but we don't actually do that.  Where we used to share code was byte
swapping on Unpremul and Opaque.  I've discussed why we no longer do this below.

We should treat Unpremul BGRA and BGR(FF) identically:

However, only JPEG uses BGR(FF).  And we don't really need to know BGR(FF) in
the JPEG case, since libjpeg-turbo will have already put the bytes in right
order for us.

BMP uses BGRX, but in this case the X really means anything, so it needs to be
handled separately (in order to fill the alpha channel with FF).

So this function is no longer shared because BGR(FF) is gone.

--------------------------------------------------------------------

We should treat Unpremul RGBA and RGB(FF) identically:

JPEG is one user of RGB(FF), but as we said above, it is always a noop.

PNG *was* another user of RGB(FF), but treating it as RGB will allow us to be
more efficient.

There are now no users of RGB(FF) or RGB(X), so it is gone.

RGB remains and is used by PNG (as of this CL) and will be used by RAW.

+        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);
+
+    // 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);
+#endif
+}
+
 static void swizzle_bgra_to_n32_premul(
         void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int dstWidth,
         int bpp, int deltaSrc, int offset, const SkPMColor ctable[]) {
@@ -378,9 +369,9 @@ static void fast_swizzle_bgra_to_n32_premul(
 #endif
 }
 
-// kRGBX
+// kRGB
 
-static void swizzle_rgbx_to_n32(
+static void swizzle_rgb_to_n32(
         void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int dstWidth,
         int bpp, int deltaSrc, int offset, const SkPMColor ctable[]) {
 
@@ -392,27 +383,9 @@ static void swizzle_rgbx_to_n32(
     }
 }
 
-static void fast_swizzle_rgbx_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);
 
-    // 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(
+static void swizzle_rgb_to_565(
        void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int dstWidth,
        int bytesPerPixel, int deltaSrc, int offset, const SkPMColor ctable[]) {
 
@@ -467,6 +440,22 @@ static void swizzle_rgba_to_n32_unpremul(
     }
 }
 
+static void fast_swizzle_rgba_to_n32_unpremul(

[scroggo, 2016/01/14 21:28:18]

This name is confusing. It looks like it actually handles RGB(0xFF) to N32?

[msarett, 2016/01/14 22:25:20]

Let me know if the name is still confusing after our conversation.

This could handle RGB(FF), but none of the Codecs produce RGB(FF) anymore.

This also handles RGBA (unpremul) because the logic is exactly the same.

[scroggo, 2016/01/19 18:06:35]

No, the confusion was mine :)

+        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);
+
+    // 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
+}
+
 // kCMYK
 //
 // CMYK is stored as four bytes per pixel.
@@ -630,7 +619,8 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
                     proc = &swizzle_index_to_565;
                     break;
                 case kIndex_8_SkColorType:
-                    proc = &swizzle_index_to_index;
+                    proc = &sample1;
+                    fastProc = ©
                     break;
                 default:
                     break;
@@ -642,7 +632,8 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
                     proc = &swizzle_gray_to_n32;
                     break;
                 case kGray_8_SkColorType:
-                    proc = &swizzle_gray_to_gray;
+                    proc = &sample1;
+                    fastProc = ©
                     break;
                 case kRGB_565_SkColorType:
                     proc = &swizzle_gray_to_565;
@@ -670,10 +661,10 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
                     if (dstInfo.alphaType() == kUnpremul_SkAlphaType) {
                         if (SkCodec::kYes_ZeroInitialized == zeroInit) {
                             proc = &SkipLeading8888ZerosThen<swizzle_bgra_to_n32_unpremul>;
-                            fastProc = &SkipLeading8888ZerosThen<fast_swizzle_bgrx_to_32>;
+                            fastProc = &SkipLeading8888ZerosThen<fast_swizzle_bgra_to_n32_unpremul>;
                         } else {
                             proc = &swizzle_bgra_to_n32_unpremul;
-                            fastProc = &fast_swizzle_bgrx_to_32;
+                            fastProc = &fast_swizzle_bgra_to_n32_unpremul;
                         }
                     } else {
                         if (SkCodec::kYes_ZeroInitialized == zeroInit) {
@@ -689,14 +680,13 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
                     break;
             }
             break;
-        case kRGBX:
+        case kRGB:
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
-                    proc = &swizzle_rgbx_to_n32;
-                    fastProc = &fast_swizzle_rgbx_to_32;
+                    proc = &swizzle_rgb_to_n32;
                     break;
                 case kRGB_565_SkColorType:
-                    proc = &swizzle_rgbx_to_565;
+                    proc = &swizzle_rgb_to_565;
                 default:
                     break;
             }
@@ -707,10 +697,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_32>;
+                            fastProc = &SkipLeading8888ZerosThen<fast_swizzle_rgba_to_n32_unpremul>;
                         } else {
                             proc = &swizzle_rgba_to_n32_unpremul;
-                            fastProc = &fast_swizzle_rgbx_to_32;
+                            fastProc = &fast_swizzle_rgba_to_n32_unpremul;
                         }
                     } else {
                         if (SkCodec::kYes_ZeroInitialized == zeroInit) {
@@ -726,24 +716,6 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
                     break;
             }
             break;
-        case kRGB:
-            switch (dstInfo.colorType()) {
-                case kN32_SkColorType:
-                    proc = &swizzle_rgbx_to_n32;
-                    break;
-                default:
-                    break;
-            }
-            break;
-        case kRGB_565:
-            switch (dstInfo.colorType()) {
-                case kRGB_565_SkColorType:
-                    proc = &sample565;
-                    break;
-                default:
-                    break;
-            }
-            break;
         case kCMYK:
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
@@ -756,6 +728,18 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
                     break;
             }
             break;
+        case kNOOP1:
+            proc = &sample1;
+            fastProc = &copy;

[scroggo, 2016/01/14 21:28:18]

This looks like the right thing to do, except I thought you said you were
planning to add opts for sampling? I was thinking those would be fastProcs as
well. Which means that sometimes we'll be able to use fastProcs for sampling,
but othertimes not (I'm guessing there won't be fastProcs for these samples).

[msarett, 2016/01/14 22:25:20]

We will never be able to know that we are sampling (or what the sampleSize is)
in this function, so we will not be able to set the fastProc to a sampling proc
here.

I envision that we will be able to adjust fastProc to a new fastProc (that
samples) in setSampleSize.

Regardless, I don't think this breaks our current pattern except that the names
are confusing.  Maybe it should be:

fast_copy(), copy_8(), copy_16(), copy_32().  All the copy_X() functions do the
same thing as fast_copy(), they just do it one pixel at a time which allows them
to support sampling.

+            break;
+        case kNOOP2:
+            proc = sample2;
+            fastProc = ©
+            break;
+        case kNOOP4:
+            proc = &sample4;
+            fastProc = ©
+            break;
         default:
             break;
     }