SkCodec: add wbmp class

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
new file mode 100644
index 0000000000000000000000000000000000000000..877ab94d277000eaf29a48bbab710a9424732ea3
--- /dev/null
+++ b/src/codec/SkCodec_wbmp.cpp
@@ -0,0 +1,130 @@
+/*
+ * 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 "SkStream.h"
+#include "SkCodec_wbmp.h"
+
+static bool read_byte(SkStream* stream, uint8_t* data) {
+    return stream->read(data, 1) == 1;

[msarett, 2015/03/26 12:37:35]

I was going to suggest using get_byte, get_short, and get_int from
SkCodecPriv.h, but it looks like you are reading a variable length quantity. 
Because of that, this probably makes sense.

[hal.canary, 2015/03/26 17:10:53]

Acknowledged.

+}
+
+// http://en.wikipedia.org/wiki/Variable-length_quantity
+static bool read_mbf(SkStream* stream, unsigned* value) {
+    unsigned n = 0;
+    uint8_t data;
+    do {
+        if (!read_byte(stream, &data)) {
+            return false;
+        }
+        n = (n << 7) | (data & 0x7F);
+    } while (data & 0x80);
+    *value = n;
+    return true;
+}
+
+static bool read_header(SkStream* stream, SkISize* size) {
+    unsigned width, height;
+    uint8_t data;
+    if (!read_byte(stream, &data) || data != 0) {
+        return false;
+    }
+    if (!read_byte(stream, &data) || (data & 0x9F)) {  // skip fixed header
+        return false;
+    }
+    if (!read_mbf(stream, &width) || width > 0xFFFF || !width) {
+        return false;
+    }
+    if (!read_mbf(stream, &height) || width > 0xFFFF || !height) {
+        return false;
+    }
+    if (size) {
+        *size = SkISize::Make(SkToS32(width), SkToS32(height));
+    }
+    return true;
+}
+
+static void expand_bits_to_bytes(uint8_t dst[], const uint8_t src[], int bits) {

[scroggo, 2015/03/26 14:37:57]

Matt, doesn't some of your code do something like this? Think it's possible to
share? I don't remember where yours is to compare though...

[msarett, 2015/03/26 15:03:56]

Yes thanks for the reminder.  The swizzler supports this type of input format
(it is called kIndex1).  However we only support conversions to from kIndex1 to
kN32 with a color table of SkPMColor (there is also 565, but I intend to delete
that).

It's probably worth considering using the swizzler and adding functionality to
support conversions to kGray etc.  I'm not 100% that it's definitely the way to
go, but it's worth thinking about.

My concerns would be:
Does using the swizzler impact the performance on wbmp decodes?
Would having these swizzles be useful to other decoders?  Bmp is the other file
format that may use small indices, but I'm not sure we would ever want to
convert this to greyscale etc.

[hal.canary, 2015/03/26 17:10:53]

I put a TODO in here.

+    int bytes = bits >> 3;
+
+    for (int i = 0; i < bytes; i++) {
+        unsigned mask = *src++;
+        dst[0] = (mask >> 7) & 1;
+        dst[1] = (mask >> 6) & 1;
+        dst[2] = (mask >> 5) & 1;
+        dst[3] = (mask >> 4) & 1;
+        dst[4] = (mask >> 3) & 1;
+        dst[5] = (mask >> 2) & 1;
+        dst[6] = (mask >> 1) & 1;
+        dst[7] = (mask >> 0) & 1;
+        dst += 8;
+    }
+
+    bits &= 7;
+    if (bits > 0) {
+        unsigned mask = *src;
+        do {
+            *dst++ = (mask >> 7) & 1;
+            mask <<= 1;
+        } while (--bits != 0);
+    }
+}
+
+namespace {
+class WbmpCodec : public SkCodec {

[msarett, 2015/03/26 12:37:35]

nit: We have been declaring the class in the header file and just using the .cpp
file for implementations.  It's probably just a matter of preference, but it
might be good to be consistent.  It might be worth getting Leon's opinion on
this.

[scroggo, 2015/03/26 14:37:57]

+1 for consistency.

[hal.canary, 2015/03/26 17:10:53]

Done.

+public:
+    WbmpCodec(const SkImageInfo& info, SkStream* stream)
+        : SkCodec(info, stream) {}
+    ~WbmpCodec() {}

[msarett, 2015/03/26 12:37:35]

nit: Probably just a matter of preference, but we have been omitting the blank
destructor.

[hal.canary, 2015/03/26 17:10:53]

Done.

+    SkEncodedFormat onGetEncodedFormat() const SK_OVERRIDE {
+        return kWBMP_SkEncodedFormat;
+    }
+    SkImageGenerator::Result onGetPixels(
+    const SkImageInfo&, void*, size_t, const Options&, SkPMColor[], int*);

[msarett, 2015/03/26 12:37:35]

nit: Should these arguments be indented twice and lined up?

[scroggo, 2015/03/26 14:37:57]

Yes.

[hal.canary, 2015/03/26 17:10:53]

Done.

+};
+}  // namespace
+
+SkImageGenerator::Result WbmpCodec::onGetPixels(const SkImageInfo& info,
+                                                void* pixels,
+                                                size_t rowBytes,
+                                                const Options&,
+                                                SkPMColor ctable[],
+                                                int* ctableCount) {
+    if (info.dimensions() != this->getInfo().dimensions()) {
+        return SkImageGenerator::kInvalidScale;
+    } else if (info != this->getInfo()) {
+        return SkImageGenerator::kInvalidConversion;

[msarett, 2015/03/26 12:37:35]

We should also allow conversions to kN32_SkColorType.  This has been our default
even for images that are more suitable for kIndex8 because we can draw to kN32
and I think it's what android wants.  It shouldn't take too much code to support
this, just use SkSwizzler.

It's good to have kIndex8 supported as well because we are planning to add that
for other image types as well.

[scroggo, 2015/03/26 14:37:57]

That's correct. Android actually will copy an Index8 bitmap to N32 before
uploading to a texture.
Possibly... I wonder if Android will learn the value of Index8 (saving RAM) if
we stop using it.

[hal.canary, 2015/03/26 17:10:53]

Done.

+    }
+    ctable[0] = SK_ColorBLACK;
+    ctable[1] = SK_ColorWHITE;
+    *ctableCount = 2;
+    SkISize size = info.dimensions();
+    uint8_t* dst = static_cast<uint8_t*>(pixels);
+    size_t srcRowBytes = SkAlign8(size.width()) >> 3;
+    SkAutoTMalloc<uint8_t> src(srcRowBytes);
+    for (int y = 0; y < size.height(); ++y) {
+        if (this->stream()->read(src.get(), srcRowBytes) != srcRowBytes) {
+            return SkImageGenerator::kIncompleteInput;
+        }
+        expand_bits_to_bytes(dst, src.get(), size.width());

[msarett, 2015/03/26 12:37:35]

If you create a swizzler outside of the loop, after expanding bits to bytes, you
should be able to just call swizzler->next(src.get()) to produce each row of the
output in kN32_SkColorType.

+        dst += rowBytes;
+    }
+    return SkImageGenerator::kSuccess;
+}
+
+bool SkWbmpCodec::IsWbmp(SkStream* stream) { return read_header(stream, NULL); }
+
+SkCodec* SkWbmpCodec::NewFromStream(SkStream* stream) {
+    SkISize size;
+    if (!read_header(stream, &size)) {
+        return NULL;
+    }
+    SkImageInfo info =
+            SkImageInfo::Make(size.width(), size.height(), kIndex_8_SkColorType,
+                              kOpaque_SkAlphaType);
+    return SkNEW_ARGS(WbmpCodec, (info, stream));
+}