Clean up unused or overly complex modes 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 "SkOpts.h"
 #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++) {
-        dst[x] = src[1] << 8 | src[0];
+        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++) {
+        dst32[x] = *((const uint32_t*) src);
+        src += deltaSrc;
+    }
+}
 
 // TODO (msarett): Investigate SIMD optimizations for swizzle routines.

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

Not related to this patch set, but time to remove this line?

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

Done.

 
 // kBit
 // These routines exclusively choose between white and black
 
 #define GRAYSCALE_BLACK 0
 #define GRAYSCALE_WHITE 0xFF
 
 
 // same as swizzle_bit_to_index and swizzle_bit_to_n32 except for value assigned to dst[x]
 static void swizzle_bit_to_grayscale(
@@ skipping 148 matching lines @@
         int bitOffset = bitIndex + deltaSrc;
         bitIndex = bitOffset % 8;
         currByte = *(src += bitOffset / 8);
         index = (currByte >> (8 - bpp - bitIndex)) & mask;
         dst[x] = ctable[index];
     }
 }
 
 // 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[]) {
 
     src += offset;
     SkPMColor* SK_RESTRICT dst = (SkPMColor*)dstRow;
     for (int x = 0; x < dstWidth; x++) {
         SkPMColor c = ctable[*src];
         dst[x] = c;
         src += deltaSrc;
@@ skipping 33 matching lines @@
         int bpp, int deltaSrc, int offset, const SkPMColor ctable[]) {
 
     src += offset;
     SkPMColor* SK_RESTRICT dst = (SkPMColor*)dstRow;
     for (int x = 0; x < dstWidth; x++) {
         dst[x] = SkPackARGB32NoCheck(0xFF, *src, *src, *src);
         src += deltaSrc;
     }
 }
 
-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[]) {
 
     src += offset;
     uint16_t* SK_RESTRICT dst = (uint16_t*)dstRow;
     for (int x = 0; x < dstWidth; x++) {
         dst[x] = SkPack888ToRGB16(src[0], src[0], src[0]);
         src += deltaSrc;
     }
 }
 
 // kBGRX
 
 static void swizzle_bgrx_to_n32(
         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;
     for (int x = 0; x < dstWidth; x++) {
         dst[x] = SkPackARGB32NoCheck(0xFF, src[2], src[1], src[0]);
         src += deltaSrc;
     }
 }
 
-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[]) {
 
     src += offset;
     uint16_t* SK_RESTRICT dst = (uint16_t*)dstRow;
     for (int x = 0; x < dstWidth; x++) {
         dst[x] = SkPack888ToRGB16(src[2], src[1], src[0]);
         src += deltaSrc;
     }
 }
 
 // kBGRA
 
 static void swizzle_bgra_to_n32_unpremul(
         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;
     for (int x = 0; x < dstWidth; x++) {
         uint8_t alpha = src[3];
         dst[x] = SkPackARGB32NoCheck(alpha, src[2], src[1], src[0]);
         src += deltaSrc;
     }
 }
 
+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[]) {
 
     src += offset;
     SkPMColor* SK_RESTRICT dst = (SkPMColor*)dstRow;
     for (int x = 0; x < dstWidth; x++) {
         uint8_t alpha = src[3];
         dst[x] = SkPremultiplyARGBInline(alpha, src[2], src[1], src[0]);
         src += deltaSrc;
     }
 }
 
 static void fast_swizzle_bgra_to_n32_premul(
         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
+// 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[]) {
 
     src += offset;
     SkPMColor* SK_RESTRICT dst = (SkPMColor*)dstRow;
     for (int x = 0; x < dstWidth; x++) {
         dst[x] = SkPackARGB32(0xFF, src[0], src[1], src[2]);
         src += deltaSrc;
     }
 }
 
-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[]) {
 
     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;
     }
 }
@@ skipping 34 matching lines @@
 
     src += offset;
     uint32_t* SK_RESTRICT dst = reinterpret_cast<uint32_t*>(dstRow);
     for (int x = 0; x < dstWidth; x++) {
         unsigned alpha = src[3];
         dst[x] = SkPackARGB32NoCheck(alpha, src[0], src[1], src[2]);
         src += deltaSrc;
     }
 }
 
+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.
 //
 // We will implement a crude conversion from CMYK -> RGB using formulas
 // from easyrgb.com.
 //
 // CMYK -> CMY
 // C = C * (1 - K) + K
 // M = M * (1 - K) + K
@@ skipping 143 matching lines @@
                         break;
                     } else {
                         proc = &swizzle_index_to_n32;
                         break;
                     }
                     break;
                 case kRGB_565_SkColorType:
                     proc = &swizzle_index_to_565;
                     break;
                 case kIndex_8_SkColorType:
-                    proc = &swizzle_index_to_index;
+                    proc = &sample1;
+                    fastProc = ©
                     break;
                 default:
                     break;
             }
             break;
         case kGray:
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
                     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;
                     break;
                 default:
                     break;
             }
             break;
         case kBGR:
         case kBGRX:
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
                     proc = &swizzle_bgrx_to_n32;
                     break;
                 case kRGB_565_SkColorType:
                     proc = &swizzle_bgrx_to_565;
                     break;
                 default:
                     break;
             }
             break;
         case kBGRA:
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
                     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) {
                             proc = &SkipLeading8888ZerosThen<swizzle_bgra_to_n32_premul>;
                             fastProc = &SkipLeading8888ZerosThen<fast_swizzle_bgra_to_n32_premul>;
                         } else {
                             proc = &swizzle_bgra_to_n32_premul;
                             fastProc = &fast_swizzle_bgra_to_n32_premul;
                         }
                     }
                     break;
                 default:
                     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;
             }
             break;
         case kRGBA:
             switch (dstInfo.colorType()) {
                 case kN32_SkColorType:
                     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) {
                             proc = &SkipLeading8888ZerosThen<swizzle_rgba_to_n32_premul>;
                             fastProc = &SkipLeading8888ZerosThen<fast_swizzle_rgba_to_n32_premul>;
                         } else {
                             proc = &swizzle_rgba_to_n32_premul;
                             fastProc = &fast_swizzle_rgba_to_n32_premul;
                         }
                     }
                     break;
                 default:
                     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:
                     proc = &swizzle_cmyk_to_n32;
                     break;
                 case kRGB_565_SkColorType:
                     proc = &swizzle_cmyk_to_565;
                     break;
                 default:
                     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;
     }
 
     // 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();
@@ skipping 36 matching lines @@
 int SkSwizzler::onSetSampleX(int sampleX) {
     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.
     fFastProc = nullptr;

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

This predates this CL, but I could imagine the following:

- setSampleX ( > 1)
  - we null out fFastProc
- decode
- setSampleX (1)
  - ideally we should go back to using fFastProc, but it's gone

Maybe I'm imagining things, since we'll recreate the swizzler anyway? But
perhaps we shouldn't?

Also, I realized that if someone called setSampleX(1) ever we'll unnecessarily
null out the fFastProc. Should we special case 1? I'm guessing no one
(currently) calls it that way, but we could make them (if we ever decide to
reuse the swizzler).

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

Right on all counts.  Can I fix a follow-up CL?

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

sgtm

 
     return fAllocatedWidth;
 }
 
 void SkSwizzler::swizzle(void* dst, const uint8_t* SK_RESTRICT src) {
     SkASSERT(nullptr != dst && nullptr != src);
     RowProc proc = fFastProc ? fFastProc : fProc;

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

We could fix the (future?) problem I mentioned above and remove this if
statement by adding a third pointer:

class SkSwizzler {
  RowProc fFastProc, fSlowProc, fActualProc;
};

::onSetSampleX(x) {
  if (1 == x) {
    fActualProc = fFastProc;
    ...
  } else {
    fActualProc = fSlowProc;
  }
}

None of this necessarily relates to this CL, but would that be worth doing? Or
am I prematurely optimizing?

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

+1 on this idea.  That will be a nice way to handle things as we add fastProcs
for sampling as well.  Can I do this in a follow-up?

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

sgtm

     proc(SkTAddOffset<void>(dst, fDstOffsetBytes), src, fSwizzleWidth, fSrcBPP,
             fSampleX * fSrcBPP, fSrcOffsetUnits, fColorTable);
 }