Scanline decoding for wbmp

src/codec/SkCodec_wbmp.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 "SkCodec.h"
+#include "SkCodecPriv.h"
 #include "SkColorPriv.h"
+#include "SkColorTable.h"
 #include "SkStream.h"
 #include "SkCodec_wbmp.h"
 
+// Each bit represents a pixel, so width is actually a number of bits.
+// A row will always be stored in bytes, so we round width up to the
+// nearest multiple of 8 to get the number of bits actually in the row.
+// We then divide by 8 to convert to bytes.
+static inline size_t get_src_row_bytes(int width) {
+    return SkAlign8(width) >> 3;
+}
+
+static inline void setup_color_table(SkColorType colorType,
+        SkPMColor* colorPtr, int* colorCount) {
+    if (kIndex_8_SkColorType == colorType) {
+        colorPtr[0] = SK_ColorBLACK;
+        colorPtr[1] = SK_ColorWHITE;
+        *colorCount = 2;
+    }
+}
+
 // http://en.wikipedia.org/wiki/Variable-length_quantity
 static bool read_mbf(SkStream* stream, uint64_t* value) {
     uint64_t n = 0;
     uint8_t data;
     const uint64_t kLimit = 0xFE00000000000000;
     SkASSERT(kLimit == ~((~static_cast<uint64_t>(0)) >> 7));
     do {
         if (n & kLimit) { // Will overflow on shift by 7.
             return false;
         }
         if (stream->read(&data, 1) != 1) {
             return false;
         }
         n = (n << 7) | (data & 0x7F);
     } while (data & 0x80);
     *value = n;
     return true;
 }
 
 static bool read_header(SkStream* stream, SkISize* size) {
     uint64_t width, height;
     uint16_t data;
     if (stream->read(&data, 2) != 2 || data != 0) {
         return false;
     }
     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) {
         *size = SkISize::Make(SkToS32(width), SkToS32(height));
     }
     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
-
-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;
+bool SkWbmpCodec::handleRewind() {
+    SkCodec::RewindState rewindState = this->rewindIfNeeded();
+    if (rewindState == kCouldNotRewind_RewindState) {
+        return false;
+    } else if (rewindState == kRewound_RewindState) {
+        (void)read_header(this->stream(), NULL);
+    }
+    return true;
 }
 
-static uint16_t bit_to_rgb565(U8CPU bit) {
-    return bit ? RGB565_WHITE : RGB565_BLACK;
-}
+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
 
-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::kBit, ctable, info, opts.fZeroInitialized);
+        default:
+            return NULL;
     }
 }
 
 SkWbmpCodec::SkWbmpCodec(const SkImageInfo& info, SkStream* stream)
-    : INHERITED(info, stream) {}
+    : INHERITED(info, stream)
+    , fSwizzler(NULL)
+{}
 
 SkEncodedFormat SkWbmpCodec::onGetEncodedFormat() const {
     return kWBMP_SkEncodedFormat;
 }
 
 SkCodec::Result SkWbmpCodec::onGetPixels(const SkImageInfo& info,
-                                         void* pixels,
+                                         void* dst,
                                          size_t rowBytes,
                                          const Options& options,
                                          SkPMColor ctable[],
                                          int* ctableCount) {
-    SkCodec::RewindState rewindState = this->rewindIfNeeded();
-    if (rewindState == kCouldNotRewind_RewindState) {
+    if (!this->handleRewind()) {

[emmaleer, 2015/08/05 15:06:55]

I think it's best to call handleRewind after the other checks.
So then if we will fail anyways we haven't spent time rewinding 
(I'm assuming rewinding takes time). 
The other checks are constant time, so I think it would be faster to check them
first.

[scroggo, 2015/08/05 15:23:16]

I'm not so sure about that assumption. For an SkMemoryStream, for example, it
just means updating an offset. Various buffered streams ought to work the same
way. For a file stream, I am guessing it would be the same.
We might consider what errors are more likely to happen first. I do not know the
answer to that question though.

[msarett, 2015/08/05 15:29:42]

I think you're right to start thinking about in what order we should perform the
parameter/state checks.  I want to keep rewind first for consistency with the
other codecs, but this is worth revisiting.

         return kCouldNotRewind;
-    } else if (rewindState == kRewound_RewindState) {
-        (void)read_header(this->stream(), NULL);
     }
     if (options.fSubset) {
         // Subsets are not supported.
         return kUnimplemented;
     }
     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 if necessary
+    setup_color_table(info.colorType(), ctable, ctableCount);
+
+
+    // Initialize the swizzler
+    fSwizzler.reset(initializeSwizzler(info, ctable, options));

[scroggo, 2015/08/05 14:36:20]

Ah, now I understand. The swizzler *is* used by the codec, but it does not need
to last beyond the call to onGetPixels. (We *could* keep it around, and in the
next call check to see if we can reuse the same one, although that seems like
some extra complicated code, without an obvious benefit.)

The old code is probably better, where you put the swizzler in an auto deleter
on the stack, and have the SD have its own swizzler. Sorry for the bad advice
:-/

also, style nit: this->initializeSwizzler.

[msarett, 2015/08/05 15:29:42]

No worries.  I'll take 99% spot on feedback :).

+    if (NULL == fSwizzler.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;

[emmaleer, 2015/08/05 15:06:55]

Do you need SkCodec:: here? For the other return values above there isn't
SkCodec:: before them.
Did you change this for a specific reason? 

[msarett, 2015/08/05 15:29:42]

Nope it's not needed, nice catch!

         }
-        proc(dst, src.get(), size.width());
-        dst += rowBytes;
+        fSwizzler->swizzle(dstRow, src.get());
+        dstRow = SkTAddOffset<void>(dstRow, rowBytes);
     }
     return kSuccess;
 }
 
 bool SkWbmpCodec::IsWbmp(SkStream* stream) {
     return read_header(stream, NULL);
 }
 
 SkCodec* SkWbmpCodec::NewFromStream(SkStream* stream) {
     SkAutoTDelete<SkStream> streamDeleter(stream);
     SkISize size;
     if (!read_header(stream, &size)) {
         return NULL;
     }
     SkImageInfo info =
             SkImageInfo::Make(size.width(), size.height(), kGray_8_SkColorType,
                               kOpaque_SkAlphaType);
     return SkNEW_ARGS(SkWbmpCodec, (info, streamDeleter.detach()));
 }
+
+class SkWbmpScanlineDecoder : public SkScanlineDecoder {
+public:
+    /*
+     * Takes ownership of all pointer paramters.
+     */
+    SkWbmpScanlineDecoder(const SkImageInfo& dstInfo, SkWbmpCodec* codec)

[scroggo, 2015/08/05 14:36:20]

It is weird that you call this dstInfo here, but srcInfo when you pass it to
this function. I think it's actually the srcInfo.

[msarett, 2015/08/05 15:29:42]

Yes I'll fix.

+        : INHERITED(dstInfo)
+        , fCodec(codec)
+        , fColorTable(NULL)
+        , fSrcRowBytes(get_src_row_bytes(codec->getInfo().width()))

[scroggo, 2015/08/05 14:36:20]

FWIW, this->getInfo().width() is the same. (Maybe it's not ideal that internal
codec and the SD have the same info value...)

[msarett, 2015/08/05 15:29:42]

Yeah that is strange...I think it makes sense to use "this" and mull over the
fact that we store two infos.

+        , 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) {

[scroggo, 2015/08/05 14:36:20]

Maybe this should be a function on SkWbmpCodec?

Result readRow(void* dst) {
  if (this->stream()->read(dst, fSrcRowBytes) != fSrcRowBytes) {
    return kIncompleteInput;
  }
  return kSuccess;
}

[msarett, 2015/08/05 15:29:42]

Sounds good.

I renamed the parameter to src and added rowBytes as a param.  It's awkward to
call the pointer we pass in src, since we are writing to it, but I think it
confuses me more to call it dst (since we have been using dst as the name of the
output image memory.

+                return SkCodec::kIncompleteInput;
+            }
+            fCodec->fSwizzler->swizzle(dstRow, fSrcBuffer.get());
+            dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
+        }
+        return SkCodec::kSuccess;
+    }
+
+    SkCodec::Result onStart(const SkImageInfo& dstInfo,
+            const SkCodec::Options& options, SkPMColor inputColorTable[],
+            int* inputColorCount) {
+        if (!fCodec->handleRewind()) {
+            return SkCodec::kCouldNotRewind;
+        }
+        if (options.fSubset) {
+            // Subsets are not supported.
+            return SkCodec::kUnimplemented;
+        }
+        if (dstInfo.dimensions() != this->getInfo().dimensions()) {
+            return SkCodec::kInvalidScale;
+        }
+
+        // Fill in the color table
+        setup_color_table(dstInfo.colorType(), inputColorTable, inputColorCount);
+
+        // Copy the color table to a pointer that can be owned by the scanline decoder
+        if (kIndex_8_SkColorType == dstInfo.colorType()) {
+            fColorTable.reset(SkNEW_ARGS(SkColorTable, (inputColorTable, 2)));
+        }
+
+        // Initialize the swizzler
+        fCodec->fSwizzler.reset(fCodec->initializeSwizzler(dstInfo,
+                get_color_ptr(fColorTable.get()), options));
+        if (NULL == fCodec->fSwizzler.get()) {
+            return SkCodec::kInvalidInput;
+        }
+
+        return SkCodec::kSuccess;
+    }
+
+private:
+    SkAutoTDelete<SkWbmpCodec>   fCodec;
+    SkAutoTUnref<SkColorTable>   fColorTable;
+    const size_t                 fSrcRowBytes;
+    SkAutoTMalloc<uint8_t>       fSrcBuffer;
+
+    typedef SkScanlineDecoder INHERITED;
+};
+
+SkScanlineDecoder* SkWbmpCodec::NewSDFromStream(SkStream* stream) {
+    SkAutoTDelete<SkWbmpCodec> codec(static_cast<SkWbmpCodec*>(
+            SkWbmpCodec::NewFromStream(stream)));
+    if (!codec) {
+        return NULL;
+    }
+
+    const SkImageInfo& srcInfo = codec->getInfo();

[scroggo, 2015/08/05 14:36:20]

This seems weird - you pass getInfo to the constructor, even though you could
just get it from the codec, which also is passed to the constructor.

[msarett, 2015/08/05 15:29:42]

Yes it is unnecessary, will fix.

+    // Return the new scanline decoder
+    return SkNEW_ARGS(SkWbmpScanlineDecoder, (srcInfo, codec.detach()));
+}