Swizzler changes Index8 and 565

src/codec/SkSwizzler.cpp

Index: src/codec/SkSwizzler.cpp
diff --git a/src/codec/SkSwizzler.cpp b/src/codec/SkSwizzler.cpp
index 0486cf27f13b693b7435ac1d182a58e1d3eddc6a..f1ff3ba9511a8d9c1c23a1ff0b414f9abf3980c2 100644
--- a/src/codec/SkSwizzler.cpp
+++ b/src/codec/SkSwizzler.cpp
@@ -9,6 +9,7 @@
 #include "SkColorPriv.h"
 #include "SkSwizzler.h"
 #include "SkTemplates.h"
+#include "SkUtils.h"
 
 SkSwizzler::ResultAlpha SkSwizzler::GetResult(uint8_t zeroAlpha,
                                               uint8_t maxAlpha) {
@@ -20,11 +21,11 @@ SkSwizzler::ResultAlpha SkSwizzler::GetResult(uint8_t zeroAlpha,
 
 // kIndex1, kIndex2, kIndex4
 
-static SkSwizzler::ResultAlpha swizzle_small_index_to_n32(
+static SkSwizzler::ResultAlpha swizzle_small_index_to_index(
         void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int width,
         int bitsPerPixel, int y, const SkPMColor ctable[]) {
 
-    SkPMColor* SK_RESTRICT dst = (SkPMColor*) dstRow;
+    uint8_t* SK_RESTRICT dst = (uint8_t*) dstRow;
     INIT_RESULT_ALPHA;
     const uint32_t pixelsPerByte = 8 / bitsPerPixel;
     const size_t rowBytes = compute_row_bytes_ppb(width, pixelsPerByte);
@@ -34,9 +35,8 @@ static SkSwizzler::ResultAlpha swizzle_small_index_to_n32(
         uint8_t pixelData = src[byte];
         for (uint32_t p = 0; p < pixelsPerByte && x < width; p++) {
             uint8_t index = (pixelData >> (8 - bitsPerPixel)) & mask;
-            SkPMColor c = ctable[index];
-            UPDATE_RESULT_ALPHA(c >> SK_A32_SHIFT);
-            dst[x] = c;
+            UPDATE_RESULT_ALPHA(ctable[index] >> SK_A32_SHIFT);
+            dst[x] = index;
             pixelData <<= bitsPerPixel;
             x++;
         }
@@ -44,11 +44,12 @@ static SkSwizzler::ResultAlpha swizzle_small_index_to_n32(
     return COMPUTE_RESULT_ALPHA;
 }
 
-static SkSwizzler::ResultAlpha swizzle_small_index_to_565(
+static SkSwizzler::ResultAlpha swizzle_small_index_to_n32(
         void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int width,
         int bitsPerPixel, int y, const SkPMColor ctable[]) {
 
-    uint16_t* SK_RESTRICT dst = (uint16_t*) dstRow;
+    SkPMColor* SK_RESTRICT dst = (SkPMColor*) dstRow;
+    INIT_RESULT_ALPHA;
     const uint32_t pixelsPerByte = 8 / bitsPerPixel;
     const size_t rowBytes = compute_row_bytes_ppb(width, pixelsPerByte);
     const uint8_t mask = (1 << bitsPerPixel) - 1;
@@ -57,13 +58,30 @@ static SkSwizzler::ResultAlpha swizzle_small_index_to_565(
         uint8_t pixelData = src[byte];
         for (uint32_t p = 0; p < pixelsPerByte && x < width; p++) {
             uint8_t index = (pixelData >> (8 - bitsPerPixel)) & mask;
-            uint16_t c = SkPixel32ToPixel16(ctable[index]);
+            SkPMColor c = ctable[index];
+            UPDATE_RESULT_ALPHA(c >> SK_A32_SHIFT);
             dst[x] = c;
             pixelData <<= bitsPerPixel;
             x++;
         }
     }
-    return SkSwizzler::kOpaque_ResultAlpha;
+    return COMPUTE_RESULT_ALPHA;
+}
+
+static SkSwizzler::ResultAlpha swizzle_index_to_index(
+        void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int width,
+        int bytesPerPixel, int y, const SkPMColor ctable[]) {
+
+    uint8_t* SK_RESTRICT dst = (uint8_t*) dstRow;
+    memcpy(dst, src, width);
+    // TODO (msarett): Should we skip the loop here and guess that the row is opaque/not opaque?
+    //                 SkScaledBitmap sampler just guesses that it is opaque.  This is dangerous
+    //                 and probably wrong since gif and bmp (rarely) may have alpha.
+    INIT_RESULT_ALPHA;
+    for (int x = 0; x < width; x++) {
+        UPDATE_RESULT_ALPHA(ctable[src[x]] >> SK_A32_SHIFT);
+    }
+    return COMPUTE_RESULT_ALPHA;
 }
 
 // kIndex
@@ -75,10 +93,9 @@ static SkSwizzler::ResultAlpha swizzle_index_to_n32(
     SkPMColor* SK_RESTRICT dst = (SkPMColor*)dstRow;
     INIT_RESULT_ALPHA;
     for (int x = 0; x < width; x++) {
-        SkPMColor c = ctable[*src];
+        SkPMColor c = ctable[src[x]];
         UPDATE_RESULT_ALPHA(c >> SK_A32_SHIFT);
         dst[x] = c;
-        src++;
     }
     return COMPUTE_RESULT_ALPHA;
 }
@@ -90,29 +107,15 @@ static SkSwizzler::ResultAlpha swizzle_index_to_n32_skipZ(
     SkPMColor* SK_RESTRICT dst = (SkPMColor*)dstRow;
     INIT_RESULT_ALPHA;
     for (int x = 0; x < width; x++) {
-        SkPMColor c = ctable[*src];
+        SkPMColor c = ctable[src[x]];
         UPDATE_RESULT_ALPHA(c >> SK_A32_SHIFT);
         if (c != 0) {
             dst[x] = c;
         }
-        src++;
     }
     return COMPUTE_RESULT_ALPHA;
 }
 
-static SkSwizzler::ResultAlpha swizzle_index_to_565(
-        void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int width,
-        int bytesPerPixel, int y, const SkPMColor ctable[]) {
-
-    uint16_t* SK_RESTRICT dst = (uint16_t*)dstRow;
-    for (int x = 0; x < width; x++) {
-        uint16_t c = SkPixel32ToPixel16(ctable[*src]);
-        dst[x] = c;
-        src++;
-    }
-    return SkSwizzler::kOpaque_ResultAlpha;
-}
-
 #undef A32_MASK_IN_PLACE
 
 static SkSwizzler::ResultAlpha swizzle_bgrx_to_n32(
@@ -127,18 +130,6 @@ static SkSwizzler::ResultAlpha swizzle_bgrx_to_n32(
     return SkSwizzler::kOpaque_ResultAlpha;
 }
 
-static SkSwizzler::ResultAlpha swizzle_bgrx_to_565(
-        void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int width,
-        int bytesPerPixel, int y, const SkPMColor ctable[]) {
-
-    uint16_t* SK_RESTRICT dst = (uint16_t*)dstRow;
-    for (int x = 0; x < width; x++) {
-        dst[x] = SkPack888ToRGB16(src[2], src[1], src[0]);
-        src += bytesPerPixel;
-    }
-    return SkSwizzler::kOpaque_ResultAlpha;
-}
-
 // kBGRA
 
 static SkSwizzler::ResultAlpha swizzle_bgra_to_n32_unpremul(
@@ -282,8 +273,8 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
                 case kN32_SkColorType:
                     proc = &swizzle_small_index_to_n32;
                     break;
-                case kRGB_565_SkColorType:
-                    proc = &swizzle_small_index_to_565;
+                case kIndex_8_SkColorType:
+                    proc = &swizzle_small_index_to_index;
                     break;
                 default:
                     break;
@@ -301,8 +292,8 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
                         break;
                     }
                     break;
-                case kRGB_565_SkColorType:
-                    proc = &swizzle_index_to_565;
+                case kIndex_8_SkColorType:
+                    proc = &swizzle_index_to_index;
                     break;
                 default:
                     break;
@@ -314,9 +305,6 @@ SkSwizzler* SkSwizzler::CreateSwizzler(SkSwizzler::SrcConfig sc,
                 case kN32_SkColorType:
                     proc = &swizzle_bgrx_to_n32;
                     break;
-                case kRGB_565_SkColorType:
-                    proc = &swizzle_bgrx_to_565;
-                    break;
                 default:
                     break;
             }
@@ -433,3 +421,36 @@ SkSwizzler::ResultAlpha SkSwizzler::next(const uint8_t* SK_RESTRICT src,
     return fRowProc(row, src, fDstInfo.width(), fDeltaSrc, fCurrY,
             fColorTable);
 }
+
+void SkSwizzler::Fill(void* dst, const SkImageInfo& dstInfo, size_t dstRowBytes, uint32_t y,
+        uint32_t input, SkPMColor* colorTable) {
+    SkASSERT(dst != NULL);
+
+    // Get dst start row
+    void* dstRow = SkTAddOffset<void*>(dst, y * dstRowBytes);
+
+    // Calculate remaining bytes.  This is tricky since the final row may not be padded.
+    size_t totalBytes = dstInfo.getSafeSize(dstRowBytes);

[scroggo, 2015/04/06 14:55:11]

const

[msarett, 2015/04/06 18:10:56]

Done.

+    size_t bytes = totalBytes - y * dstRowBytes;

[scroggo, 2015/04/06 14:55:11]

const.

Also, Maybe this is better described as remainingBytes?

[msarett, 2015/04/06 18:10:55]

Agreed

+
+    // Use the proper memset routine to fill the remaining bytes
+    switch(dstInfo.colorType()) {
+        case kN32_SkColorType:
+            // Assume input is an index if we have a color table
+            if (NULL != colorTable) {

[msarett, 2015/04/03 18:01:32]

My original version passed a bool parameter indicating whether the input was an
index into a color table or a color.  Instead, if the color table is non-NULL,
we assume it's an index.  I can't decide if this is confusing or clever.

FWIW: This function looks a lot cleaner if I implement it as non-static (doesn't
need a ton of parameters).  This version would be useful in one place in
SkGifCodec and one place in SkJpegCodec.

[scroggo, 2015/04/06 14:55:11]

It is a little bit confusing, although I think that's better than having
redundant parameters. A comment in the header will help with any confusion
though.
What is the advantage to doing it this way?

[msarett, 2015/04/06 18:10:55]

Added the comment.

I don't think there isn't a great advantage to a non-static version, and given
that it wouldn't be usable in all cases, it probably isn't worthwhile to
consider.

+                sk_memset32((uint32_t*) dstRow, colorTable[(uint8_t) input],

[scroggo, 2015/04/06 14:55:11]

Is this cast just in case? I think it might be interesting to assert that
(uint8_t) input == input.

[msarett, 2015/04/06 18:10:55]

Done.

+                        bytes / sizeof(SkPMColor));

[scroggo, 2015/04/06 14:55:11]

This seems potentially dangerous. In general, I would expect rowBytes to be
divisible by 4, in which case this is safe. That said, we allow the caller to
specify, so they could supply some weird number (maybe they have their
reasons?). If the padding on the end was not a multiple of 4, we would end up
not setting our colors properly.

That said, this is likely faster than doing a loop over each row. Maybe do a
check that this is safe based on the rowBytes, and if not, have a backup option
which handles each row, with a comment that (and maybe even an SkCodecPrintf)
that this is a weird rowBytes and an unlikely case.

[msarett, 2015/04/06 18:10:55]

Done

+            // Otherwise, assume the input is a color
+            } else {
+                sk_memset32((uint32_t*) dstRow, input, bytes / sizeof(SkPMColor));
+            }
+            break;
+        // On an index destination color type, always assume the input is an index
+        case kIndex_8_SkColorType:
+            memset(dstRow, (uint8_t) input, bytes);
+            break;
+        default:
+            SkCodecPrintf("Warning: Unsupported dst color type for fill().  Doing nothing.\n");

[scroggo, 2015/04/06 14:55:12]

Maybe SkASSERT(false);

[msarett, 2015/04/06 18:10:55]

Done.

+            break;
+    }
+}