Scanline decoding for wbmp

src/codec/SkCodec_wbmp.cpp

Index: src/codec/SkCodec_wbmp.cpp
diff --git a/src/codec/SkCodec_wbmp.cpp b/src/codec/SkCodec_wbmp.cpp
index 35ac808219c625ad1bacf045d6414fb4b8e40b76..657866228bd59abfef6d126b4fae111417fce6d9 100644
--- a/src/codec/SkCodec_wbmp.cpp
+++ b/src/codec/SkCodec_wbmp.cpp
@@ -7,9 +7,15 @@
 
 #include "SkCodec.h"
 #include "SkColorPriv.h"
+#include "SkColorTable.h"
 #include "SkStream.h"
 #include "SkCodec_wbmp.h"
 
+// Each bit represents a pixel, so rowBytes can be calculated as width / 8

[scroggo, 2015/07/28 14:03:34]

Almost - the code handles SkAlign8, but the comment does not acknowledge it:

If the width is 7, rowBytes is 1, but 7 / 8 (in integer math) is 0.

[msarett, 2015/07/28 22:22:07]

Agreed that the comment should acknowledge SkAlign8.

I also think you are pointing out a bug that I have introduced.  SkAlign8 should
only apply to width, rounding up to the nearest 8 before dividing by 8.

+static size_t get_src_row_bytes(int width) {
+    return SkAlign8(width >> 3);
+}
+
 // http://en.wikipedia.org/wiki/Variable-length_quantity
 static bool read_mbf(SkStream* stream, uint64_t* value) {
     uint64_t n = 0;
@@ -38,7 +44,7 @@ static bool read_header(SkStream* stream, SkISize* size) {
     if (!read_mbf(stream, &width) || width > 0xFFFF || !width) {
         return false;
     }
-    if (!read_mbf(stream, &height) || width > 0xFFFF || !height) {
+    if (!read_mbf(stream, &height) || height > 0xFFFF || !height) {
         return false;
     }
     if (size) {
@@ -47,49 +53,20 @@ static bool read_header(SkStream* stream, SkISize* size) {
     return true;
 }
 
-#define BLACK SkPackARGB32NoCheck(0xFF, 0, 0, 0)
-#define WHITE SkPackARGB32NoCheck(0xFF, 0xFF, 0xFF, 0xFF)
-
-#define GRAYSCALE_BLACK 0
-#define GRAYSCALE_WHITE 0xFF
-
-#define RGB565_BLACK 0
-#define RGB565_WHITE 0xFFFF
+SkSwizzler* SkWbmpCodec::initializeSwizzler(const SkImageInfo& info,
+        const SkPMColor* ctable, const Options& opts) {
+    // TODO (msarett): Reenable support for 565 if it is desired
+    //                 skbug.com/3683
 
-static SkPMColor bit_to_pmcolor(U8CPU bit) { return bit ? WHITE : BLACK; }
-
-static uint8_t bit_to_bit(U8CPU bit) { return bit; }
-
-static uint8_t bit_to_grayscale(U8CPU bit) {
-    return bit ? GRAYSCALE_WHITE : GRAYSCALE_BLACK;
-}
-
-static uint16_t bit_to_rgb565(U8CPU bit) {
-    return bit ? RGB565_WHITE : RGB565_BLACK;
-}
-
-typedef void (*ExpandProc)(uint8_t*, const uint8_t*, int);
-
-// TODO(halcanary): Add this functionality (grayscale and indexed output) to
-//                  SkSwizzler and use it here.
-template <typename T, T (*TRANSFORM)(U8CPU)>
-static void expand_bits_to_T(uint8_t* dstptr, const uint8_t* src, int bits) {
-    T* dst = reinterpret_cast<T*>(dstptr);
-    int bytes = bits >> 3;
-    for (int i = 0; i < bytes; i++) {
-        U8CPU mask = *src++;
-        for (int j = 0; j < 8; j++) {
-            dst[j] = TRANSFORM((mask >> (7 - j)) & 1);
-        }
-        dst += 8;
-    }
-    bits &= 7;
-    if (bits > 0) {
-        U8CPU mask = *src;
-        do {
-            *dst++ = TRANSFORM((mask >> 7) & 1);
-            mask <<= 1;
-        } while (--bits != 0);
+    // Create the swizzler based on the desired color type
+    switch (info.colorType()) {
+        case kIndex_8_SkColorType:
+        case kN32_SkColorType:
+        case kGray_8_SkColorType:
+            return SkSwizzler::CreateSwizzler(
+                    SkSwizzler::kIndex1, ctable, info, opts.fZeroInitialized);
+        default:
+            return NULL;
     }
 }
 
@@ -101,7 +78,7 @@ SkEncodedFormat SkWbmpCodec::onGetEncodedFormat() const {
 }
 
 SkCodec::Result SkWbmpCodec::onGetPixels(const SkImageInfo& info,
-                                         void* pixels,
+                                         void* dst,
                                          size_t rowBytes,
                                          const Options& options,
                                          SkPMColor ctable[],
@@ -119,38 +96,41 @@ SkCodec::Result SkWbmpCodec::onGetPixels(const SkImageInfo& info,
     if (info.dimensions() != this->getInfo().dimensions()) {
         return kInvalidScale;
     }
-    ExpandProc proc = NULL;
-    switch (info.colorType()) {
-        case kGray_8_SkColorType:
-            proc = expand_bits_to_T<uint8_t, bit_to_grayscale>;
-            break;
-        case kN32_SkColorType:
-            proc = expand_bits_to_T<SkPMColor, bit_to_pmcolor>;
-            break;
-        case kIndex_8_SkColorType:
-            ctable[0] = BLACK;
-            ctable[1] = WHITE;
-            *ctableCount = 2;
-            proc = expand_bits_to_T<uint8_t, bit_to_bit>;
-            break;
-        case kRGB_565_SkColorType:
-            proc = expand_bits_to_T<uint16_t, bit_to_rgb565>;
-            break;
-        default:
-            return kInvalidConversion;
+
+    // Prepare a color table
+    SkPMColor alternateCtable[2];

[scroggo, 2015/07/28 14:03:34]

I believe in Skia we would typically name this something like "colorStorage".
This name is fine, although it seems a little odd to have a capital "C" followed
by lowercase "table" (although I suppose it fits with treating "ctable" as one
word with no camelCase).

[msarett, 2015/07/28 22:22:07]

I agree, I prefer colorStorage :).  Having said that, we don't need this
anymore.

+    SkPMColor* colorPtr;
+    if (kIndex_8_SkColorType == info.colorType()) {
+        ctable[0] = SK_ColorBLACK;

[scroggo, 2015/07/28 14:03:34]

How do you feel about the following:

if (kIndex_8_SkColorType == info.colorType()) {
  colorPtr = ctable;
} else {
  colorPtr = alternateCtable;
}

colorPtr[0] = SK_ColorBLACK;
colorPtr[1] = SK_ColorWHITE;

[msarett, 2015/07/28 22:22:06]

Agreed, however, the new version only uses the ctable in kIndex_8 mode.

+        ctable[1] = SK_ColorWHITE;
+        colorPtr = ctable;
+        *ctableCount = 2;
+    } else {
+        alternateCtable[0] = SK_ColorBLACK;
+        alternateCtable[1] = SK_ColorWHITE;
+        colorPtr = alternateCtable;
+    }
+
+    // Initialize the swizzler
+    SkAutoTDelete<SkSwizzler> swizzler(
+            initializeSwizzler(info, colorPtr, options));
+    if (NULL == swizzler.get()) {
+        return kInvalidConversion;
     }
+
+    // Perform the decode
     SkISize size = info.dimensions();
-    uint8_t* dst = static_cast<uint8_t*>(pixels);
-    size_t srcRowBytes = SkAlign8(size.width()) >> 3;
+    size_t srcRowBytes = get_src_row_bytes(this->getInfo().width());
     SkAutoTMalloc<uint8_t> src(srcRowBytes);
+    void* dstRow = dst;
     for (int y = 0; y < size.height(); ++y) {
-        if (this->stream()->read(src.get(), srcRowBytes) != srcRowBytes) {
-            return kIncompleteInput;
+        if (stream()->read(src.get(), srcRowBytes) != srcRowBytes) {
+            return SkCodec::kIncompleteInput;
         }
-        proc(dst, src.get(), size.width());
-        dst += rowBytes;
+        swizzler->swizzle(dstRow, src.get());
+        dstRow = SkTAddOffset<void>(dstRow, rowBytes);
     }
-    return kSuccess;
+    return SkCodec::kSuccess;

[scroggo, 2015/07/28 14:03:34]

I do not feel strongly about whether to add SkCodec:: here, but it is not
needed, is it?

[msarett, 2015/07/28 22:22:06]

Yep it's not needed.  Removing it.

 }
 
 bool SkWbmpCodec::IsWbmp(SkStream* stream) {
@@ -168,3 +148,84 @@ SkCodec* SkWbmpCodec::NewFromStream(SkStream* stream) {
                               kOpaque_SkAlphaType);
     return SkNEW_ARGS(SkWbmpCodec, (info, streamDeleter.detach()));
 }
+
+class SkWbmpScanlineDecoder : public SkScanlineDecoder {
+public:
+    SkWbmpScanlineDecoder(const SkImageInfo& dstInfo, SkWbmpCodec* codec,

[scroggo, 2015/07/28 14:03:34]

/**
 * Takes ownership of all its pointer parameters.
 */

[msarett, 2015/07/28 22:22:07]

Done.

+            SkColorTable* colorTable, SkSwizzler* swizzler)
+        : INHERITED(dstInfo)
+        , fCodec(codec)
+        , fColorTable(colorTable)
+        , fSwizzler(swizzler)
+        , fSrcRowBytes(get_src_row_bytes(codec->getInfo().width()))
+        , fSrcBuffer(fSrcRowBytes)
+    {}
+
+    SkCodec::Result onGetScanlines(void* dst, int count, size_t dstRowBytes) override {
+        void* dstRow = dst;
+        for (int y = 0; y < count; ++y) {
+            if (fCodec->stream()->read(fSrcBuffer.get(), fSrcRowBytes) != fSrcRowBytes) {
+                return SkCodec::kIncompleteInput;
+            }
+            this->fSwizzler->swizzle(dstRow, fSrcBuffer.get());

[scroggo, 2015/07/28 14:03:34]

We use this->[methodName] so it is easy to distinguish between calling a member
function and calling some other function. We label our member variables
[fVariable] (for "Field") to distinguish them from other variables (i.e. local -
globals get their own markers e.g. gGlobal).

You do not need this-> here.

[msarett, 2015/07/28 22:22:06]

Gotcha.

+            dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
+        }
+        return SkCodec::kSuccess;
+    }
+
+private:
+    SkAutoTDelete<SkWbmpCodec>   fCodec;
+    SkAutoTDelete<SkColorTable>  fColorTable;
+    SkAutoTDelete<SkSwizzler>    fSwizzler;
+    const size_t                 fSrcRowBytes;
+    SkAutoTMalloc<uint8_t>       fSrcBuffer;
+
+    typedef SkScanlineDecoder INHERITED;
+};
+
+
+SkScanlineDecoder* SkWbmpCodec::onGetScanlineDecoder(const SkImageInfo& dstInfo,
+        const Options& options, SkPMColor ctable[], int* ctableCount) {
+    if (options.fSubset) {
+        // Subsets are not supported.
+        return NULL;
+    }
+    if (dstInfo.dimensions() != this->getInfo().dimensions()) {
+        return NULL;
+    }
+    // Create a new SkWbmpCodec, to be owned by the scanline decoder.
+    SkStream* stream = this->stream()->duplicate();
+    if (!stream) {
+        return NULL;
+    }
+    SkAutoTDelete<SkWbmpCodec> codec(static_cast<SkWbmpCodec*>(
+            SkWbmpCodec::NewFromStream(stream)));
+    if (!codec) {
+        return NULL;
+    }
+
+    // Prepare a color table

[scroggo, 2015/07/28 14:03:34]

Can you move this into a helper function? (I think we already have a static
function which chooses between local storage and passed in pointers. The code to
share here goes further, choosing its colors, but maybe it can call that
function as well.)

[msarett, 2015/07/28 22:22:07]

Yes done!

+    SkPMColor alternateCtable[2];
+    SkPMColor* colorPtr;
+    if (kIndex_8_SkColorType == dstInfo.colorType()) {
+        ctable[0] = SK_ColorBLACK;
+        ctable[1] = SK_ColorWHITE;
+        colorPtr = ctable;
+        *ctableCount = 2;
+    } else {
+        alternateCtable[0] = SK_ColorBLACK;
+        alternateCtable[1] = SK_ColorWHITE;
+        colorPtr = alternateCtable;
+    }
+    SkAutoTDelete<SkColorTable> colorTable(SkNEW_ARGS(SkColorTable, (colorPtr, 2)));

[scroggo, 2015/07/28 14:03:34]

SkColorTable is refcounted, so it should be in an SkAutoTUnref, here and in the
class.

[msarett, 2015/07/28 22:22:07]

Acknowledged.  I'll try not to forget that next time :).

+
+    // Initialize the swizzler
+    SkAutoTDelete<SkSwizzler> swizzler(
+            initializeSwizzler(dstInfo, colorTable->readColors(), options));
+    if (NULL == swizzler.get()) {
+        return NULL;
+    }
+
+    return SkNEW_ARGS(SkWbmpScanlineDecoder, (dstInfo, codec.detach(),
+            colorTable.detach(), swizzler.detach()));
+}