Bmp Image Decoding

src/codec/SkCodec_libbmp.cpp

Index: src/codec/SkCodec_libbmp.cpp
diff --git a/src/codec/SkCodec_libbmp.cpp b/src/codec/SkCodec_libbmp.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..54aec0ff37c9f1fd6467ae5968f69d863467cf93
--- /dev/null
+++ b/src/codec/SkCodec_libbmp.cpp
@@ -0,0 +1,1298 @@
+/*
+ * 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_libbmp.h"
+#include "SkColor.h"
+#include "SkEndian.h"
+#include "SkStream.h"
+
+/*
+ *
+ * Get a byte from the buffer
+ *
+ */
+uint8_t get_byte(uint8_t* buffer, uint32_t i) {
+    return buffer[i];
+}
+
+/*
+ *
+ * Get a short from the buffer
+ *
+ */
+uint16_t get_short(uint8_t* buffer, uint32_t i) {
+    uint16_t result;
+    memcpy(&result, &(buffer[i]), 2);
+#ifdef SK_CPU_BENDIAN
+    return SkEndianSwap16(result);
+#else
+    return result;
+#endif
+}
+
+/*
+ *
+ * Get an int from the buffer
+ *
+ */
+uint32_t get_int(uint8_t* buffer, uint32_t i) {
+    uint32_t result;
+    memcpy(&result, &(buffer[i]), 4);
+#ifdef SK_CPU_BENDIAN
+    return SkEndianSwap32(result);
+#else
+    return result;
+#endif
+}
+
+/*
+ *
+ * Used to convert 1-7 bit color components into 8-bit color components
+ *
+ */
+const static uint8_t n_bit_to_8_bit_lookup_table[] = {
+    // 1 bit
+    0, 255,
+    // 2 bits
+    0, 85, 170, 255,
+    // 3 bits
+    0, 36, 73, 109, 146, 182, 219, 255,
+    // 4 bits
+    0, 17, 34, 51, 68, 85, 102, 119, 136, 153, 170, 187, 204, 221, 238, 255,
+    // 5 bits
+    0, 8, 16, 25, 33, 41, 49, 58, 66, 74, 82, 90, 99, 107, 115, 123, 132, 140,
+    148, 156, 165, 173, 181, 189, 197, 206, 214, 222, 230, 239, 247, 255,
+    // 6 bits
+    0, 4, 8, 12, 16, 20, 24, 28, 32, 36, 40, 45, 49, 53, 57, 61, 65, 69, 73,
+    77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 130, 134, 138,
+    142, 146, 150, 154, 158, 162, 166, 170, 174, 178, 182, 186, 190, 194, 198,
+    202, 206, 210, 215, 219, 223, 227, 231, 235, 239, 243, 247, 251, 255,
+    // 7 bits
+    0, 2, 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 26, 28, 30, 32, 34, 36, 38,
+    40, 42, 44, 46, 48, 50, 52, 54, 56, 58, 60, 62, 64, 66, 68, 70, 72, 74, 76,
+    78, 80, 82, 84, 86, 88, 90, 92, 94, 96, 98, 100, 102, 104, 106, 108, 110,
+    112, 114, 116, 118, 120, 122, 124, 126, 129, 131, 133, 135, 137, 139, 141,
+    143, 145, 147, 149, 151, 153, 155, 157, 159, 161, 163, 165, 167, 169, 171,
+    173, 175, 177, 179, 181, 183, 185, 187, 189, 191, 193, 195, 197, 199, 201,
+    203, 205, 207, 209, 211, 213, 215, 217, 219, 221, 223, 225, 227, 229, 231,
+    233, 235, 237, 239, 241, 243, 245, 247, 249, 251, 253, 255
+};
+
+/*
+ *
+ * Convert an n bit component to an 8-bit component
+ *
+ */
+static uint8_t convert_n_to_8(uint32_t component, uint32_t n) {
+    if (0 == n) {
+        return 0;
+    } else if (8 > n) {
+        return n_bit_to_8_bit_lookup_table[(1 << n) - 2 + component];
+    } else if (8 == n) {

[scroggo, 2015/03/06 18:56:13]

I would change this to:

} else {
  SkASSERT(8 == n);
  return component;
}

(Maybe the compiler is smart enough to know the last else case will never be
reached, so it doesn't need to do the if check in release, but I also think this
is more clear.)

+        return component;
+    } else {
+        SkASSERT(false);
+        return 0;
+    }
+}
+
+/*
+ *
+ * Row procedure for masked color components with 16 bits per pixel
+ *
+ */
+static SkSwizzler::ResultAlpha swizzle_mask16_to_n32(
+        void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int width,
+        const SkBmpCodec::BitMasks masks,
+        const SkBmpCodec::BitMaskShifts shifts,
+        const SkBmpCodec::BitMaskSizes sizes) {
+
+    // Use the masks to decode to the destination
+    uint16_t* srcPtr = (uint16_t*) src;
+    SkColor* SK_RESTRICT dstPtr = (SkColor*) dstRow;
+    for (uint32_t i = 0; i < width; i++) {
+        uint16_t p = srcPtr[i];
+        uint8_t red = convert_n_to_8(
+                (p & masks.redMask) >> shifts.redShift, sizes.redSize);

[scroggo, 2015/03/06 18:56:13]

It looks like this same code is repeated a few times. Can we make it a function?
Can the three BitMask objects be unified? Or do they mix and match? i.e. can we
have a single BitMask object that contains all the information and does
something like the following:

uint8_t BitMask::getRed(uint16_t p) {
  return convert_n_to_8((p & fRedMask) >> fRedShift, fRedSize);
}

+        uint8_t green = convert_n_to_8(
+                (p & masks.greenMask) >> shifts.greenShift, sizes.greenSize);
+        uint8_t blue = convert_n_to_8(
+                (p & masks.blueMask) >> shifts.blueShift, sizes.blueSize);
+        dstPtr[i] = SkColorSetARGBInline(0xFF, red, green, blue);

[scroggo, 2015/03/06 18:56:12]

This doesn't do what we want. We want either premultiplied colors, or
unpremultiplied colors (masquerading as SkPMColor, but treating the values as
unpremultiplied), not SkColors.

In this case, we know it's opaque, so premultiplication is irrelevant. So you
can use SkPackARGB32NoCheck

+    }
+    return SkSwizzler::kOpaque_ResultAlpha;
+}
+
+/*
+ *
+ * Row procedure for masked color components with 16 bits per pixel with alpha
+ *
+ */
+static SkSwizzler::ResultAlpha swizzle_mask16_alpha_to_n32(
+        void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int width,
+        const SkBmpCodec::BitMasks masks,
+        const SkBmpCodec::BitMaskShifts shifts,
+        const SkBmpCodec::BitMaskSizes sizes) {
+
+    // Use the masks to decode to the destination
+    uint16_t* srcPtr = (uint16_t*) src;
+    SkColor* SK_RESTRICT dstPtr = (SkColor*) dstRow;
+    uint8_t zAlpha = 0;

[scroggo, 2015/03/06 18:56:13]

What does zAlpha mean? zeroAlpha?

+    uint8_t mAlpha = 0xFF;

[scroggo, 2015/03/06 18:56:13]

I think this is max alpha? Refrain from using mVariable for local variables. It
looks like a member variable (at least to someone who sometimes programs in Java
- others probably use this pattern for members, too).

+    for (uint32_t i = 0; i < width; i++) {
+        uint16_t p = srcPtr[i];
+        uint8_t red = convert_n_to_8(
+                (p & masks.redMask) >> shifts.redShift, sizes.redSize);
+        uint8_t green = convert_n_to_8(
+                (p & masks.greenMask) >> shifts.greenShift, sizes.greenSize);
+        uint8_t blue = convert_n_to_8(
+                (p & masks.blueMask) >> shifts.blueShift, sizes.blueSize);
+        uint8_t alpha = convert_n_to_8((
+                p & masks.alphaMask) >> shifts.alphaShift, sizes.alphaSize);
+        zAlpha |= alpha;
+        mAlpha &= alpha;
+        dstPtr[i] = SkColorSetARGBInline(alpha, red, green, blue);
+    }
+     return (mAlpha == 0xFF) ? SkSwizzler::kOpaque_ResultAlpha :

[scroggo, 2015/03/06 18:56:13]

Can you make a helper function for this? Something like:

ResultAlpha getResult(uint8_t zeroAlpha, uint8_t maxAlpha);

(I'd like to do some tricks to get rid of the if checks if we can, but making it
a function is a good start - then we can only change it once.)

+            ((zAlpha == 0) ? SkSwizzler::kTransparent_ResultAlpha :
+            SkSwizzler::kNeither_ResultAlpha);
+}
+
+/*
+ *
+ * Row procedure for masked color components with 24 bits per pixel
+ *
+ */
+static SkSwizzler::ResultAlpha swizzle_mask24_to_n32(
+        void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int width,
+        const SkBmpCodec::BitMasks masks,
+        const SkBmpCodec::BitMaskShifts shifts,
+        const SkBmpCodec::BitMaskSizes sizes) {
+
+    // Use the masks to decode to the destination
+    SkColor* SK_RESTRICT dstPtr = (SkColor*) dstRow;
+    for (uint32_t i = 0; i < 3*width; i += 3) {
+        uint32_t p = src[i] | (src[i + 1] << 8) | src[i + 2] << 16;
+        uint8_t red = convert_n_to_8(
+                (p & masks.redMask) >> shifts.redShift, sizes.redSize);
+        uint8_t green = convert_n_to_8(
+                (p & masks.greenMask) >> shifts.greenShift, sizes.greenSize);
+        uint8_t blue = convert_n_to_8(
+                (p & masks.blueMask) >> shifts.blueShift, sizes.blueSize);
+        dstPtr[i/3] = SkColorSetARGBInline(0xFF, red, green, blue);
+    }
+    return SkSwizzler::kOpaque_ResultAlpha;
+}
+
+/*
+ *
+ * Row procedure for masked color components with 24 bits per pixel with alpha
+ *
+ */
+static SkSwizzler::ResultAlpha swizzle_mask24_alpha_to_n32(
+        void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int width,
+        const SkBmpCodec::BitMasks masks,
+        const SkBmpCodec::BitMaskShifts shifts,
+        const SkBmpCodec::BitMaskSizes sizes) {
+
+    // Use the masks to decode to the destination
+    SkColor* SK_RESTRICT dstPtr = (SkColor*) dstRow;
+    uint8_t zAlpha = 0;
+    uint8_t mAlpha = 0xFF;
+    for (uint32_t i = 0; i < 3*width; i += 3) {
+        uint32_t p = src[i] | (src[i + 1] << 8) | src[i + 2] << 16;
+        uint8_t red = convert_n_to_8(
+                (p & masks.redMask) >> shifts.redShift, sizes.redSize);
+        uint8_t green = convert_n_to_8(
+                (p & masks.greenMask) >> shifts.greenShift, sizes.greenSize);
+        uint8_t blue = convert_n_to_8(
+                (p & masks.blueMask) >> shifts.blueShift, sizes.blueSize);
+        uint8_t alpha = convert_n_to_8((
+                p & masks.alphaMask) >> shifts.alphaShift, sizes.alphaSize);
+        zAlpha |= alpha;
+        mAlpha &= alpha;
+        dstPtr[i/3] = SkColorSetARGBInline(alpha, red, green, blue);
+    }
+     return (mAlpha == 0xFF) ? SkSwizzler::kOpaque_ResultAlpha :
+            ((zAlpha == 0) ? SkSwizzler::kTransparent_ResultAlpha :
+            SkSwizzler::kNeither_ResultAlpha);
+}
+
+/*
+ *
+ * Row procedure for masked color components with 32 bits per pixel
+ *
+ */
+static SkSwizzler::ResultAlpha swizzle_mask32_to_n32(
+        void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int width,
+        const SkBmpCodec::BitMasks masks,
+        const SkBmpCodec::BitMaskShifts shifts,
+        const SkBmpCodec::BitMaskSizes sizes) {
+
+    // Use the masks to decode to the destination
+    uint32_t* srcPtr = (uint32_t*) src;
+    SkColor* SK_RESTRICT dstPtr = (SkColor*) dstRow;
+    for (uint32_t i = 0; i < width; i++) {
+        uint32_t p = srcPtr[i];
+        uint8_t red = convert_n_to_8(
+                (p & masks.redMask) >> shifts.redShift, sizes.redSize);
+        uint8_t green = convert_n_to_8(
+                (p & masks.greenMask) >> shifts.greenShift, sizes.greenSize);
+        uint8_t blue = convert_n_to_8(
+                (p & masks.blueMask) >> shifts.blueShift, sizes.blueSize);
+        dstPtr[i] = SkColorSetARGBInline(0xFF, red, green, blue);
+    }
+    return SkSwizzler::kOpaque_ResultAlpha;
+}
+
+/*
+ *
+ * Row procedure for masked color components with 32 bits per pixel
+ *
+ */
+static SkSwizzler::ResultAlpha swizzle_mask32_alpha_to_n32(
+        void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int width,
+        const SkBmpCodec::BitMasks masks,
+        const SkBmpCodec::BitMaskShifts shifts,
+        const SkBmpCodec::BitMaskSizes sizes) {
+
+    // Use the masks to decode to the destination
+    uint32_t* srcPtr = (uint32_t*) src;
+    SkColor* SK_RESTRICT dstPtr = (SkColor*) dstRow;
+    uint8_t zAlpha = 0;
+    uint8_t mAlpha = 0xFF;
+    for (uint32_t i = 0; i < width; i++) {
+        uint32_t p = srcPtr[i];
+        uint8_t red = convert_n_to_8(
+                (p & masks.redMask) >> shifts.redShift, sizes.redSize);
+        uint8_t green = convert_n_to_8(
+                (p & masks.greenMask) >> shifts.greenShift, sizes.greenSize);
+        uint8_t blue = convert_n_to_8(
+                (p & masks.blueMask) >> shifts.blueShift, sizes.blueSize);
+        uint8_t alpha = convert_n_to_8((
+                p & masks.alphaMask) >> shifts.alphaShift, sizes.alphaSize);
+        zAlpha |= alpha;
+        mAlpha &= alpha;
+        dstPtr[i] = SkColorSetARGBInline(alpha, red, green, blue);
+    }
+     return (mAlpha == 0xFF) ? SkSwizzler::kOpaque_ResultAlpha :
+            ((zAlpha == 0) ? SkSwizzler::kTransparent_ResultAlpha :
+            SkSwizzler::kNeither_ResultAlpha);
+}
+
+/*
+ *
+ * Checks if the conversion between the input image and the requested output
+ * image has been implemented
+ *
+ */
+static bool conversion_possible(const SkImageInfo& dst,
+                                const SkImageInfo& src) {
+    // TODO: Support all conversions

[scroggo, 2015/03/06 18:56:13]

nit: we won't support *all* conversions, but perhaps to 565.

+    if (kN32_SkColorType != dst.colorType()) {
+        return false;
+    }
+    if (kIgnore_SkAlphaType == dst.alphaType()) {

[scroggo, 2015/03/06 18:56:13]

Can we also check for opaque vs non-opaque?

For PNG, I decided that if the image reports that it is opaque, we consider it
opaque, and if the caller requests premul/unpremul, we return false. To be fair,
in that case, we could have just said "Sure, you can treat that as
unpremul/premul!" - they'll pay a penalty when it's time to draw, but maybe
that's on them (the current implementation hopefully means they'll catch their
error earlier, and never suffer the penalty).

More interestingly, if the image reports that it's not opaque, we don't allow
the caller to request decoding to opaque. This is because we want them to use an
alpha type that supports the actual data. This of course has its own downside -
an image may report that it's not opaque, but then later have only opaque
pixels.

I think we should be consistent across decoders, but I'm starting to wonder if I
made the wrong decision. It was partly influenced by SkImageDecoder, where the
client requested a particular Config, and we didn't use that if it didn't make
sense. But SkCodec has a different model; it tells them the correct "Config"
(color type and alpha type) to use. If they choose a different one, maybe we
should honor that, even if they're wrong? (And maybe they know better than us -
they could know the image has no alpha, even though it reported that it did? Of
course, with such control, maybe they should have written the encoded image
better?)

This model does make testing trickier though. We don't want to compare drawing
an image with alpha as opaque to drawing it with alpha...

+        SkDebugf("Error: invalid dst alpha type.");
+        return false;
+    }
+    return true;
+}
+
+/*
+ *
+ * Defines the version and type of the second bitmap header
+ *
+ */
+enum BitmapHeaderType {
+    kInfoV1_BitmapHeaderType,
+    kInfoV2_BitmapHeaderType,
+    kInfoV3_BitmapHeaderType,
+    kInfoV4_BitmapHeaderType,
+    kInfoV5_BitmapHeaderType,
+    kOS2V1_BitmapHeaderType,
+    kOS2VX_BitmapHeaderType,
+    kUnknown_BitmapHeaderType
+};
+
+/*
+ *
+ * Possible bitmap compression types
+ *
+ */
+enum BitmapCompressionMethod {
+    kNone_BitmapCompressionMethod =          0,
+    k8BitRLE_BitmapCompressionMethod =       1,
+    k4BitRLE_BitmapCompressionMethod =       2,
+    kBitMasks_BitmapCompressionMethod =      3,
+    kJpeg_BitmapCompressionMethod =          4,
+    kPng_BitmapCompressionMethod =           5,
+    kAlphaBitMasks_BitmapCompressionMethod = 6,
+    kCMYK_BitmapCompressionMethod =          11,
+    kCMYK8BitRLE_BitmapCompressionMethod =   12,
+    kCMYK4BitRLE_BitmapCompressionMethod =   13
+};
+
+/*
+ *
+ * Checks the start of the stream to see if the image is a bitmap
+ *
+ */
+bool SkBmpCodec::IsBmp(SkStream* stream) {
+    // TODO: Support "IC", "PT", "CI", "CP", "BA"
+    // TODO: ICO files may contain a BMP and need to use this decoder
+    const char bmpSig[] = { 'B', 'M' };
+    char buffer[sizeof(bmpSig)];
+    return stream->read(buffer, sizeof(bmpSig)) == sizeof(bmpSig) &&
+        !memcmp(buffer, bmpSig, sizeof(bmpSig));
+}
+
+/*
+ *
+ * Assumes IsBmp was called and returned true
+ * Creates a bitmap decoder
+ * Reads enough of the stream to determine the image format
+ *
+ */
+SkCodec* SkBmpCodec::NewFromStream(SkStream* stream) {

[msarett, 2015/03/05 23:13:17]

This function is very long.  There is not really repetitive code, but I could
try to break it up:
processHeader()
processMasks()
processColorTable() etc.
My concern is that I would have to pass a ton of parameters to each call because
most of the processing depends on the previous step.

I plan to look into this tomorrow, but also feel free to let me know your
thoughts.

As far as the really long decodeRLE(), I think that function is best left alone.

[scroggo, 2015/03/05 23:32:35]

I'm going to look at the whole of the code more in depth tomorrow, but I did
want to stop you from digging down this hole. If you have to pass a ton of
parameters to each function, that's a good sign you shouldn't break it up in
that way. (At least to me; YMMV.)

(As a side note, if the parameters are constants (e.g. kBmpHeaderBytes etc),
those can just be made global. I'm guessing you're talking about more than
passing constants, in which case my earlier statement holds.)

+    // Header size constants
+    static const uint32_t kBmpHeaderBytes = 14;
+    static const uint32_t kBmpHeaderBytesPlusFour = kBmpHeaderBytes + 4;
+    static const uint32_t kBmpOS2V1Bytes = 12;
+    static const uint32_t kBmpOS2V2Bytes = 64;
+    static const uint32_t kBmpInfoBaseBytes = 16;
+    static const uint32_t kBmpInfoV1Bytes = 40;
+    static const uint32_t kBmpInfoV2Bytes = 52;
+    static const uint32_t kBmpInfoV3Bytes = 56;
+    static const uint32_t kBmpInfoV4Bytes = 108;
+    static const uint32_t kBmpInfoV5Bytes = 124;
+    static const uint32_t kBmpMaskBytes = 12;
+
+    // Read the first header and the size of the second header
+    SkAutoTDeleteArray<uint8_t> hBuffer(
+            SkNEW_ARRAY(uint8_t, kBmpHeaderBytesPlusFour));
+    if (stream->read(hBuffer.get(), kBmpHeaderBytesPlusFour) !=
+            kBmpHeaderBytesPlusFour) {
+        SkDebugf("Error: unable to read first bitmap header.\n");
+        return NULL;
+    }
+
+    // The total bytes in the bmp file
+    // We only need to use this value for RLE decoding, so we will only check
+    // that it is valid in the RLE case.
+    const uint32_t totalBytes = get_int(hBuffer.get(), 2);
+
+    // The offset from the start of the file where the pixel data begins
+    const uint32_t offset = get_int(hBuffer.get(), 10);
+    if (offset < kBmpHeaderBytes + kBmpOS2V1Bytes) {
+        SkDebugf("Error: invalid starting location for pixel data\n");
+        return NULL;
+    }
+
+    // The size of the second (info) header in bytes
+    // The size is the first field of the second header, so we have already
+    // read the first four infoBytes.
+    const uint32_t infoBytes = get_int(hBuffer.get(), 14);
+    if (infoBytes < kBmpOS2V1Bytes) {
+        SkDebugf("Error: invalid second header size.\n");
+        return NULL;
+    }
+    const uint32_t infoBytesRemaining = infoBytes - 4;
+    hBuffer.free();
+
+    // Read the second header
+    SkAutoTDeleteArray<uint8_t> iBuffer(
+            SkNEW_ARRAY(uint8_t, infoBytesRemaining));
+    if (stream->read(iBuffer.get(), infoBytesRemaining) != infoBytesRemaining) {
+        SkDebugf("Error: unable to read second bitmap header.\n");
+        return NULL;
+    }
+
+    // The number of bits used per pixel in the pixel data
+    uint16_t bitsPerPixel;
+
+    // The compression method for the pixel data
+    uint32_t compression = kNone_BitmapCompressionMethod;
+
+    // Number of colors in the color table, defaults to 0 or max (see below)
+    uint32_t numColors = 0;
+
+    // Bytes per color in the color table, early versions use 3, most use 4
+    uint32_t bytesPerColor;
+
+    // The image width and height
+    int width, height;
+
+    // Determine image information depending on second header format
+    BitmapHeaderType headerType;
+    if (infoBytes >= kBmpInfoBaseBytes) {
+        // Check the version of the header
+        switch (infoBytes) {
+            case kBmpInfoV1Bytes:
+                headerType = kInfoV1_BitmapHeaderType;
+                break;
+            case kBmpInfoV2Bytes:
+                headerType = kInfoV2_BitmapHeaderType;
+                break;
+            case kBmpInfoV3Bytes:
+                headerType = kInfoV3_BitmapHeaderType;
+                break;
+            case kBmpInfoV4Bytes:
+                headerType = kInfoV4_BitmapHeaderType;
+                break;
+            case kBmpInfoV5Bytes:
+                headerType = kInfoV5_BitmapHeaderType;
+                break;
+            case 16:
+            case 20:
+            case 24:
+            case 28:
+            case 32:
+            case 36:
+            case 42:
+            case 46:
+            case 48:
+            case 60:
+            case kBmpOS2V2Bytes:
+                headerType = kOS2VX_BitmapHeaderType;
+                break;
+            default:
+                // We do not signal an error here because there is the
+                // possibility of new or undocumented bmp header types.  Most
+                // of the newer versions of bmp headers are similar to and
+                // build off of the older versions, so we may still be able to
+                // decode the bmp.
+                SkDebugf("Warning: unknown bmp header format.\n");
+                headerType = kUnknown_BitmapHeaderType;
+                break;
+        }
+        // We check the size of the header before entering the if statement.
+        // We should not reach this point unless the size is large enough for
+        // these required fields.
+        SkASSERT(infoBytesRemaining >= 12);
+        width = get_int(iBuffer.get(), 0);
+        height = get_int(iBuffer.get(), 4);
+        //uint16_t planes = get_short(iBuffer, 8);
+        bitsPerPixel = get_short(iBuffer.get(), 10);
+
+        // Some versions do not have this field, so we check before
+        // overwriting the default value.
+        if (infoBytesRemaining >= 16) {
+            compression = get_int(iBuffer.get(), 12);
+        }
+
+        // Some versions do not have this field, so we check before
+        // overwriting the default value.
+        if (infoBytesRemaining >= 32) {
+            numColors = get_int(iBuffer.get(), 28);
+        }
+
+        bytesPerColor = 4;
+    } else if (infoBytes >= kBmpOS2V1Bytes) {
+        // The OS2V1 is treated separately because it has a unique format
+        headerType = kOS2V1_BitmapHeaderType;
+        width = (int) get_short(iBuffer.get(), 0);
+        height = (int) get_short(iBuffer.get(), 2);
+        bitsPerPixel = get_short(iBuffer.get(), 6);
+        bytesPerColor = 3;
+    } else {
+        // There are no valid bmp headers
+        SkDebugf("Error: second bitmap header size is invalid.\n");
+        return NULL;
+    }
+
+    // Check for valid dimensions from header
+    RowOrder rowOrder = kBottomUp_RowOrder;
+    if (height < 0) {
+        height = -height;
+        rowOrder = kTopDown_RowOrder;
+    }
+    static const uint32_t kBmpMaxDim = 1 << 16;
+    if (width < 0 || width >= kBmpMaxDim || height >= kBmpMaxDim) {
+        // TODO: Decide if we want to support really large bmps.
+        SkDebugf("Error: invalid bitmap dimensions.\n");
+        return NULL;
+    }
+
+    // Create mask struct
+    BitMasks masks;
+    memset(&masks, 0, 4*sizeof(uint32_t));
+
+    // Determine the input compression format and set bit masks if necessary
+    uint32_t maskBytes = 0;
+    BitmapInputFormat inputFormat = kUnknown_BitmapInputFormat;
+    switch (compression) {
+        case kNone_BitmapCompressionMethod:
+            inputFormat = kStandard_BitmapInputFormat;
+            break;
+        case k8BitRLE_BitmapCompressionMethod:
+            if (bitsPerPixel != 8) {
+                SkDebugf("Warning: correcting invalid bitmap format.\n");
+                bitsPerPixel = 8;
+            }
+            inputFormat = kRLE_BitmapInputFormat;
+            break;
+        case k4BitRLE_BitmapCompressionMethod:
+            if (bitsPerPixel != 4) {
+                SkDebugf("Warning: correcting invalid bitmap format.\n");
+                bitsPerPixel = 4;
+            }
+            inputFormat = kRLE_BitmapInputFormat;
+            break;
+        case kAlphaBitMasks_BitmapCompressionMethod:
+        case kBitMasks_BitmapCompressionMethod:
+            // Load the masks
+            inputFormat = kBitMask_BitmapInputFormat;
+            switch (headerType) {
+                case kInfoV1_BitmapHeaderType: {
+                    // The V1 header stores the bit masks after the header
+                    SkAutoTDeleteArray<uint8_t> mBuffer(
+                            SkNEW_ARRAY(uint8_t, kBmpMaskBytes));
+                    if (stream->read(mBuffer.get(), kBmpMaskBytes) !=
+                            kBmpMaskBytes) {
+                        SkDebugf("Error: unable to read bit masks.\n");
+                        return NULL;
+                    }
+                    maskBytes = kBmpMaskBytes;
+                    masks.redMask = get_int(mBuffer.get(), 0);
+                    masks.greenMask = get_int(mBuffer.get(), 4);
+                    masks.blueMask = get_int(mBuffer.get(), 8);
+                    break;
+                }
+                case kInfoV2_BitmapHeaderType:
+                case kInfoV3_BitmapHeaderType:
+                case kInfoV4_BitmapHeaderType:
+                case kInfoV5_BitmapHeaderType:
+                    // Header types are matched based on size.  If the header
+                    // is V2+, we are guaranteed to be able at least this size.
+                    SkASSERT(infoBytesRemaining >= 48);
+                    masks.redMask = get_int(iBuffer.get(), 36);
+                    masks.greenMask = get_int(iBuffer.get(), 40);
+                    masks.blueMask = get_int(iBuffer.get(), 44);
+                    break;
+                case kOS2VX_BitmapHeaderType:
+                    // TODO: Decide if we intend to support this.
+                    //       It is unsupported in the previous version and
+                    //       in chromium.  I have not come across a test case
+                    //       that uses this format.
+                    SkDebugf("Error: huffman format unsupported.\n");
+                    return NULL;
+                default:
+                   SkDebugf("Error: invalid bmp bit masks header.\n");
+                   return NULL;
+            }
+            break;
+        case kJpeg_BitmapCompressionMethod:
+            if (24 == bitsPerPixel) {
+                inputFormat = kRLE_BitmapInputFormat;
+                break;
+            }
+            // Fall through
+        case kPng_BitmapCompressionMethod:
+            // TODO: Decide if we intend to support this.
+            //       It is unsupported in the previous version and
+            //       in chromium.  I think it is used mostly for printers.
+            SkDebugf("Error: compression format not supported.\n");
+            return NULL;
+        case kCMYK_BitmapCompressionMethod:
+        case kCMYK8BitRLE_BitmapCompressionMethod:
+        case kCMYK4BitRLE_BitmapCompressionMethod:
+            // TODO: Same as above.
+            SkDebugf("Error: CMYK not supported for bitmap decoding.\n");
+            return NULL;
+        default:
+            SkDebugf("Error: invalid format for bitmap decoding.\n");
+            return NULL;
+    }
+
+    // Most versions of bmps should to be rendered as opaque.  Either they do

[scroggo, 2015/03/06 18:56:13]

should be*

+    // not have an alpha channel, or they expect the alpha channel to be
+    // ignored.  V4+ bmp files introduce an alpha mask and allow the creator
+    // of the image to use the alpha channels.  However, many of these images
+    // leave the alpha channel blank and expect to be rendered as opaque.  For
+    // this reason, we set the alpha type to kUnknown for V4+ bmps and figure
+    // out the alpha type during the decode.
+    SkAlphaType alphaType = kOpaque_SkAlphaType;
+    if (kInfoV4_BitmapHeaderType == headerType ||
+            kInfoV5_BitmapHeaderType == headerType) {
+        // Header types are matched based on size.  If the header is
+        // V4+, we are guaranteed to be able at least this size.

[scroggo, 2015/03/06 18:56:13]

able... to read?

+        SkASSERT(infoBytesRemaining > 52);
+        masks.alphaMask = get_int(iBuffer.get(), 48);
+        if (masks.alphaMask != 0) {
+            alphaType = kUnpremul_SkAlphaType;
+        }
+    }
+    iBuffer.free();
+
+    // Check for valid bits per pixel input
+    switch (bitsPerPixel) {
+        // In addition to more standard pixel compression formats, bmp supports
+        // the use of bit masks to determine pixel components.  The standard
+        // format for representing 16-bit colors is 555 (XRRRRRGGGGGBBBBB),
+        // which does not map well to any Skia color formats.  For this reason,
+        // we will always enable mask mode with 16 bits per pixel.
+        case 16:
+            if (kBitMask_BitmapInputFormat != inputFormat) {
+                masks.redMask = 0x7C00;
+                masks.greenMask = 0x03E0;
+                masks.blueMask = 0x001F;
+                inputFormat = kBitMask_BitmapInputFormat;
+            }
+            break;
+        case 1:
+        case 2:
+        case 4:
+        case 8:
+        case 24:
+        case 32:
+            break;
+        default:
+            SkDebugf("Error: invalid input value for bits per pixel.\n");
+            return NULL;
+    }
+
+    // Process the color table
+    uint32_t colorBytes = 0;
+    SkColor* colorTable = NULL;

[scroggo, 2015/03/06 18:56:13]

SkPMColor. Same down below.

+    if (bitsPerPixel < 16) {
+        // Verify the number of colors for the color table
+        const int maxColors = 1 << bitsPerPixel;
+        // Zero is a default for maxColors
+        // Also set numColors to maxColors when input is too large
+        if (numColors <= 0 || numColors > maxColors) {
+            numColors = maxColors;
+        }
+        colorTable = SkNEW_ARRAY(SkColor, maxColors);
+
+        // Construct the color table
+        colorBytes = numColors * bytesPerColor;
+        SkAutoTDeleteArray<uint8_t> cBuffer(SkNEW_ARRAY(uint8_t, colorBytes));
+        if (stream->read(cBuffer.get(), colorBytes) != colorBytes) {
+            SkDebugf("Error: unable to read color table.\n");
+            return NULL;
+        }
+
+        // Fill in the color table
+        uint32_t i = 0;
+        for (; i < numColors; i++) {
+            uint8_t blue = get_byte(cBuffer.get(), i*bytesPerColor);
+            uint8_t green = get_byte(cBuffer.get(), i*bytesPerColor + 1);
+            uint8_t red = get_byte(cBuffer.get(), i*bytesPerColor + 2);
+            uint8_t alpha = 0xFF;
+            if (kOpaque_SkAlphaType != alphaType) {
+                alpha = (masks.alphaMask >> 24) &
+                        get_byte(cBuffer.get(), i*bytesPerColor + 3);
+            }
+            colorTable[i] = SkColorSetARGBInline(alpha, red, green, blue);
+        }
+
+        // To avoid segmentation faults on bad pixel data, fill the end of the
+        // color table with black.  This is the same the behavior as the
+        // chromium decoder.
+        for (; i < maxColors; i++) {
+            colorTable[i] = SkColorSetARGBInline(0xFF, 0, 0, 0);
+        }
+    } else {
+        // We will not use the color table if bitsPerPixel >= 16, but if there
+        // is a color table, we may need to skip the color table bytes.
+        // We will assume that the maximum color table size is the same as when
+        // there are 8 bits per pixel (the largest color table actually used).
+        // Color tables for greater than 8 bits per pixel are somewhat
+        // undocumented.  It is indicated that they may exist to store a list
+        // of colors for optimization on devices with limited color display
+        // capacity.  While we do not know for sure, we will guess that any
+        // value of numColors greater than this maximum is invalid.
+        if (numColors <= (1 << 8)) {
+            colorBytes = numColors * bytesPerColor;
+            if (stream->skip(colorBytes) != colorBytes) {
+                SkDebugf("Error: Could not skip color table bytes.\n");
+                return NULL;
+            }
+        }
+    }
+
+    // Ensure that the stream now points to the start of the pixel array
+    uint32_t bytesRead = kBmpHeaderBytes + infoBytes + maskBytes + colorBytes;
+
+    // Check that we have not read past the pixel array offset
+    if(bytesRead > offset) {
+        // This may occur on OS 2.1 and other old versions where the color
+        // table defaults to max size, and the bmp tries to use a smaller color
+        // table.  This is invalid, and our decision is to indicate an error,
+        // rather than try to guess the intended size of the color table and
+        // rewind the stream to display the image.
+        SkDebugf("Error: pixel data offset less than header size.\n");
+        return NULL;
+    }
+
+    // Skip to the start of the pixel array
+    if (stream->skip(offset - bytesRead) != offset - bytesRead) {
+        SkDebugf("Error: unable to skip to image data.\n");
+        return NULL;
+    }
+
+    // Remaining bytes is only used for RLE
+    const int remainingBytes = totalBytes - offset;
+    if (remainingBytes <= 0 && kRLE_BitmapInputFormat == inputFormat) {
+        SkDebugf("Error: RLE requires valid input size.\n");
+        return NULL;
+    }
+
+    // Return the codec
+    // We will use ImageInfo to store width, height, and alpha type.  The Skia
+    // color types do not match with the many possible bmp input color types,
+    // so we will ignore this field and depend on other parameters for input
+    // information.
+    const SkImageInfo& imageInfo = SkImageInfo::Make(width, height,
+            kUnknown_SkColorType, alphaType);
+    return SkNEW_ARGS(SkBmpCodec, (imageInfo, stream, bitsPerPixel,
+                                   inputFormat, masks, colorTable, rowOrder,
+                                   remainingBytes));
+}
+
+/*
+ *
+ * Creates an instance of the decoder
+ * Called only by NewFromStream
+ *
+ */
+SkBmpCodec::SkBmpCodec(const SkImageInfo& info, SkStream* stream,
+                       uint16_t bitsPerPixel, BitmapInputFormat inputFormat,
+                       BitMasks masks, SkColor* colorTable,
+                       RowOrder rowOrder,
+                       const uint32_t remainingBytes)
+    : INHERITED(info, stream)
+    , fBitsPerPixel(bitsPerPixel)
+    , fInputFormat(inputFormat)
+    , fBitMasks(masks)
+    , fMaskShifts( { 0, 0, 0, 0 } )
+    , fMaskSizes( { 0, 0, 0, 0 } )
+    , fColorTable(colorTable)
+    , fRowOrder(rowOrder)
+    , fRemainingBytes(remainingBytes)
+{}
+
+/*
+ *
+ * Initiates the bitmap decode
+ *
+ */
+SkCodec::Result SkBmpCodec::onGetPixels(const SkImageInfo& dstInfo,
+                                        void* dst, size_t dstRowBytes,
+                                        SkColor*, int*) {
+    if (!this->couldRewindIfNeeded()) {
+        SkDebugf("Error: rewind should not be necessary.\n");

[scroggo, 2015/03/06 18:56:13]

This comment isn't necessary.

couldRewindIfNeeded should never return true the first time; this is just for
attempting to decode again. In that case, rewinding is absolutely necessary.

+        return kCouldNotRewind;
+    }
+    if (dstInfo.dimensions() != this->getOriginalInfo().dimensions()) {
+        SkDebugf("Error: scaling not supported.\n");
+        return kInvalidScale;
+    }
+    if (!conversion_possible(dstInfo, this->getOriginalInfo())) {
+        SkDebugf("Error: cannot convert input type to output type.\n");
+        return kInvalidConversion;
+    }
+
+    switch (fInputFormat) {
+        case kBitMask_BitmapInputFormat:
+            return decodeMask(dstInfo, dst, dstRowBytes);
+        case kRLE_BitmapInputFormat:
+            return decodeRLE(dstInfo, dst, dstRowBytes);
+        case kStandard_BitmapInputFormat:
+            return decode(dstInfo, dst, dstRowBytes);
+        default:
+            SkASSERT(false);
+            return kInvalidInput;
+    }
+}
+
+/*
+ *
+ * For a continuous bit mask (ex: 0011100), retrieves the size of the mask and
+ * the number of bits to shift the mask
+ *
+ */
+void SkBmpCodec::processMasks() {

[msarett, 2015/03/05 23:13:17]

I can make this function less repetitive (but harder to understand) if I cast
the structs to arrays and use a loop.  Do you think this would be beneficial?

[scroggo, 2015/03/06 18:56:13]

Hard to say... My initial thought was to let timing/profiling tell us where we
need to optimize, but this code appears to do the same thing four times in a
row. If there was a copy/paste error it seems like it would be hard to find
it...

I'll be curious what the array function looks like.

Another thing that might be interesting is using Sk4x, although maybe we can't
use it here since we make different decisions for each component.

At the very least, it seems like we should use a macro/function for each
component.

+    // Trim the masks to the allowed number of bits
+    if (fBitsPerPixel < 32) {

[scroggo, 2015/03/06 18:56:13]

If you merged all the masks together, as suggested elsewhere, could this be a
function on BitMasks? It looks like the only reason you need the SkBmpCodec to
get fBitsPerPixel, which could be passed as a parameter. (You also currently
need fBitMasks etc, but after a merge you have all that.)

+        fBitMasks.redMask &= (1 << fBitsPerPixel) - 1;
+        fBitMasks.greenMask &= (1 << fBitsPerPixel) - 1;
+        fBitMasks.blueMask &= (1 << fBitsPerPixel) - 1;
+        fBitMasks.alphaMask &= (1 << fBitsPerPixel) - 1;
+    }
+
+    // Get temporary versions of the inputs masks
+    uint32_t redMask = fBitMasks.redMask;
+    uint32_t greenMask = fBitMasks.greenMask;
+    uint32_t blueMask = fBitMasks.blueMask;
+    uint32_t alphaMask = fBitMasks.alphaMask;
+
+    // Count trailing zeros on masks and the size of masks
+    if (redMask != 0) {
+        for (; (redMask & 1) == 0; redMask >>= 1) {
+            fMaskShifts.redShift++;
+        }
+        for (; redMask & 1; redMask >>= 1) {
+            fMaskSizes.redSize++;
+        }
+        // Truncate masks greater than 8 bits
+        if (fMaskSizes.redSize > 8) {
+            fMaskShifts.redShift += fMaskSizes.redSize - 8;
+            fMaskSizes.redSize = 8;
+        }
+    }
+    if (greenMask != 0) {
+        for (; (greenMask & 1) == 0; greenMask >>= 1) {
+            fMaskShifts.greenShift++;
+        }
+        for (; greenMask & 1; greenMask >>= 1) {
+            fMaskSizes.greenSize++;
+        }
+        // Truncate masks greater than 8 bits
+        if (fMaskSizes.greenSize > 8) {
+            fMaskShifts.greenShift += fMaskSizes.greenSize - 8;
+            fMaskSizes.greenSize = 8;
+        }
+    }
+    if (blueMask != 0) {
+        for (; (blueMask & 1) == 0; blueMask >>= 1) {
+            fMaskShifts.blueShift++;
+        }
+        for (; blueMask & 1; blueMask >>= 1) {
+            fMaskSizes.blueSize++;
+        }
+        // Truncate masks greater than 8 bits
+        if (fMaskSizes.blueSize > 8) {
+            fMaskShifts.blueShift += fMaskSizes.blueSize - 8;
+            fMaskSizes.blueSize = 8;
+        }
+    }
+    if (alphaMask != 0) {
+        for (; (alphaMask & 1) == 0; alphaMask >>= 1) {
+            fMaskShifts.alphaShift++;
+        }
+        for (; alphaMask & 1; alphaMask >>= 1) {
+            fMaskSizes.alphaSize++;
+        }
+        // Truncate masks greater than 8 bits
+        if (fMaskSizes.alphaSize > 8) {
+            fMaskShifts.alphaShift += fMaskSizes.alphaSize - 8;
+            fMaskSizes.alphaSize = 8;
+        }
+    }
+    return;

[scroggo, 2015/03/06 18:56:13]

not needed.

+}
+
+/*
+ *
+ * Performs the bitmap decoding for bit masks input format
+ *
+ */
+SkCodec::Result SkBmpCodec::decodeMask(const SkImageInfo& dstInfo,
+                                       void* dst, uint32_t dstRowBytes) {
+    // Set constant values
+    const int width = dstInfo.width();
+    const int height = dstInfo.height();
+    const uint32_t pixelsPerByte = 8 / fBitsPerPixel;
+    const uint32_t bytesPerPixel = fBitsPerPixel / 8;

[scroggo, 2015/03/06 18:56:13]

what happens if fBitsPerPixel is not an even multiple of 8? Does that ever
happen?

[msarett, 2015/03/07 00:19:50]

It would an error if it happened.  We should probably assert that it won't
happen.

+    const uint32_t unpaddedRowBytes = fBitsPerPixel < 16 ?

[scroggo, 2015/03/06 18:56:13]

I think all you want here is the paddedRowBytes? And you have some similar
calculations down below? I think this is a perfect place for a function:

size_t compute_row_bytes(uint32_t bitsPerPixel, int width) {
  size_t unpaddedRowBytes;
  if (fBitsPerPixel < 16) {
    // compute unpaddedRowBytes using pixelsPerByte
  } else {
    // compute unpaddedRowBytes using bytesPerPixel
  }
  return SkAlign4(unpaddedRowBytes);
}

(Typically we use size_t to a represent number of bytes, hence the return value
of size_t.)

+            (width + pixelsPerByte - 1) / pixelsPerByte : width * bytesPerPixel;
+    const uint32_t paddedRowBytes = SkAlign4(unpaddedRowBytes);
+    const uint32_t alphaMask = fBitMasks.alphaMask;
+
+    // Allocate space for a row buffer and a source for the swizzler
+    SkAutoTDeleteArray<uint8_t> srcBuffer(SkNEW_ARRAY(uint8_t, paddedRowBytes));
+
+    // Define the type of mask row procedures
+    typedef SkSwizzler::ResultAlpha (*RowProc)(void* SK_RESTRICT dstRow,
+                            const uint8_t* SK_RESTRICT src, int width,
+                            BitMasks masks, BitMaskShifts shifts,
+                            BitMaskSizes sizes);
+
+    // Choose the appropriate row procedure
+    RowProc proc = NULL;
+    switch (fBitsPerPixel) {
+        case 16:
+            if (alphaMask == 0) {
+                proc = &swizzle_mask16_to_n32;
+            } else {
+                proc = &swizzle_mask16_alpha_to_n32;
+            }
+            break;
+        case 24:
+            if (alphaMask == 0) {
+                proc = &swizzle_mask24_to_n32;
+            } else {
+                proc = &swizzle_mask24_alpha_to_n32;
+            }
+            break;
+        case 32:
+            if (alphaMask == 0) {
+                proc = &swizzle_mask32_to_n32;
+            } else {
+                proc = &swizzle_mask32_alpha_to_n32;
+            }
+            break;
+        default:
+            SkASSERT(false);
+            return kInvalidInput;
+    }
+
+    // Obtain the size and location of the input bit masks
+    processMasks();
+
+    // Get the destination start row and delta
+    SkColor* dstRow;
+    int32_t delta;
+    if (kTopDown_RowOrder == fRowOrder) {
+        dstRow = (SkColor*) dst;
+        delta = dstRowBytes;
+    } else {
+        dstRow = (SkColor*) SkTAddOffset<void>(dst, (height - 1) * dstRowBytes);
+        delta = -dstRowBytes;
+    }
+
+    // Iterate over rows of the image
+    bool transparent = true;
+    for (uint32_t y = 0; y < height; y++) {
+        // Read a row of the input
+        if (stream()->read(srcBuffer.get(), paddedRowBytes) != paddedRowBytes) {
+            SkDebugf("Warning: incomplete input stream.\n");
+            return kIncompleteInput;
+        }
+
+        // Decode the row in destination format
+        SkSwizzler::ResultAlpha r = proc(dstRow, srcBuffer.get(), width,
+                fBitMasks, fMaskShifts, fMaskSizes);
+        transparent &= SkSwizzler::kTransparent_ResultAlpha == r;

[scroggo, 2015/03/06 18:56:13]

nit: parens around the == check

+
+        // Move to the next row
+        dstRow = SkTAddOffset<SkColor>(dstRow, delta);
+    }
+
+    // Many fully transparent bmp images are intended to be opaque.  Here, we
+    // correct for this possibility.
+    SkColor* dstPtr = (SkColor*) dst;
+    if (transparent) {
+        for (uint32_t y = 0; y < height; y++) {
+            for (uint32_t x = 0; x < width; x++) {
+                dstPtr[y * dstRowBytes + x] |= 0xFF000000;
+            }
+        }
+    }
+
+    // Finished decoding the entire image
+    return kSuccess;
+}
+
+/*
+ *
+ * Set an RLE pixel using the color table
+ *
+ */
+void SkBmpCodec::setRLEPixel(SkColor* dst, uint32_t dstRowBytes, int height,
+                             uint32_t x, uint32_t y, uint8_t index) {
+    if (kBottomUp_RowOrder == fRowOrder) {
+        y = height - y - 1;
+    }
+    SkColor* dstRow = SkTAddOffset<SkColor>(dst, y * dstRowBytes);
+    dstRow[x] = fColorTable.get()[index];
+}
+
+/*
+ *
+ * Performs the bitmap decoding for RLE input format
+ * RLE decoding is performed all at once, rather than a one row at a time
+ *
+ */
+SkCodec::Result SkBmpCodec::decodeRLE(const SkImageInfo& dstInfo,
+                                      void* dst, uint32_t dstRowBytes) {
+    // Set RLE flags
+    static const uint8_t RLE_ESCAPE = 0;
+    static const uint8_t RLE_EOL = 0;
+    static const uint8_t RLE_EOF = 1;
+    static const uint8_t RLE_DELTA = 2;
+
+    // Set constant values
+    const int width = dstInfo.width();
+    const int height = dstInfo.height();
+    const uint32_t pixelsPerByte = 8 / fBitsPerPixel;
+    const uint32_t bytesPerPixel = fBitsPerPixel / 8;
+
+    // Input buffer parameters
+    uint32_t currByte = 0;
+    SkAutoTDeleteArray<uint8_t> buffer(SkNEW_ARRAY(uint8_t, fRemainingBytes));
+    uint32_t totalBytes = stream()->read(buffer.get(), fRemainingBytes);
+    if (totalBytes < fRemainingBytes) {
+        SkDebugf("Warning: incomplete RLE file.\n");
+    } else if (totalBytes <= 0) {
+        SkDebugf("Error: could not read RLE image data.\n");
+        return kInvalidInput;
+    }
+
+    // Destination parameters
+    uint32_t x = 0;
+    uint32_t y = 0;
+    // If the code skips pixels, remaining pixels are transparent or black
+    // TODO: Skip this if memory was already zeroed.
+    memset(dst, 0, dstRowBytes * height);
+    SkColor* dstPtr = (SkColor*) dst;
+
+    while (true) {
+        // Every entry takes at least two bytes
+        if (totalBytes - currByte < 2) {
+            SkDebugf("Warning: incomplete RLE input.\n");
+            return kIncompleteInput;
+        }
+
+        // Read the next two bytes.  These bytes have different meanings

[scroggo, 2015/03/06 18:56:13]

Thanks for the comments! It makes this code a lot easier to follow :)

+        // depending on their values.  In the first interpretation, the first
+        // byte is an escape flag and the second byte indicates what special
+        // task to perform.
+        const uint8_t flag = buffer.get()[currByte++];
+        const uint8_t task = buffer.get()[currByte++];
+
+        // If we have reached a row that is beyond the image size, and the RLE
+        // code does not indicate end of file, abort and signal a warning.
+        if (y >= height && (flag != RLE_ESCAPE || (task != RLE_EOF))) {
+            SkDebugf("Warning: invalid RLE input.\n");
+            return kIncompleteInput;
+        }
+
+        // Perform decoding
+        if (RLE_ESCAPE == flag) {
+            switch (task) {
+                case RLE_EOL:
+                    x = 0;
+                    y++;
+                    break;
+                case RLE_EOF:
+                    return kSuccess;
+                case RLE_DELTA: {
+                    // Two bytes are needed to specify delta
+                    if (totalBytes - currByte < 2) {
+                        SkDebugf("Warning: incomplete RLE input\n");
+                        return kIncompleteInput;
+                    }
+                    // Modify x and y
+                    const uint8_t dx = buffer.get()[currByte++];
+                    const uint8_t dy = buffer.get()[currByte++];
+                    x += dx;
+                    y += dy;
+                    if (x > width || y > height) {
+                        SkDebugf("Warning: invalid RLE input.\n");
+                        return kIncompleteInput;
+                    }
+                    break;
+                }
+                default: {
+                    // If task does not match any of the above signals, it
+                    // indicates that we have a sequence of non-RLE pixels.
+                    // Furthermore, the value of task is equal to the number
+                    // of pixels to interpret.
+                    uint8_t numPixels = task;
+                    const uint32_t unpaddedBytes = fBitsPerPixel < 16 ?
+                            (numPixels + pixelsPerByte - 1) / pixelsPerByte :
+                            numPixels * bytesPerPixel;
+                    const uint32_t paddedBytes = SkAlign2(unpaddedBytes);
+                    if (x + numPixels > width ||
+                            totalBytes - currByte < paddedBytes) {
+                        SkDebugf("Warning: invalid RLE input.\n");
+                        return kIncompleteInput;
+                    }
+                    // Set count number of pixels
+                    while (numPixels > 0) {
+                        switch(fBitsPerPixel) {
+                            case 4: {
+                                uint8_t val = buffer.get()[currByte++];
+                                setRLEPixel(dstPtr, dstRowBytes, height, x++, y,
+                                        val >> 4);
+                                numPixels--;
+                                if (numPixels != 0) {
+                                    setRLEPixel(dstPtr, dstRowBytes, height,
+                                            x++, y, val & 0xF);
+                                    numPixels--;
+                                }
+                                break;
+                            }
+                            case 8:
+                                setRLEPixel(dstPtr, dstRowBytes, height, x++, y,
+                                        buffer.get()[currByte++]);
+                                numPixels--;
+                                break;
+                            case 24: {
+                                uint8_t blue = buffer.get()[currByte++];
+                                uint8_t green = buffer.get()[currByte++];
+                                uint8_t red = buffer.get()[currByte++];
+                                SkColor color = SkColorSetARGBInline(
+                                        0xFF, red, green, blue);
+                                SkColor* dstRow = SkTAddOffset<SkColor>(
+                                        dstPtr, y * dstRowBytes);
+                                dstRow[x++] = color;
+                                numPixels--;
+                            }
+                            default:
+                                SkASSERT(false);
+                                return kInvalidInput;
+                        }
+                    }
+                    // Skip a byte if necessary to maintain alignment
+                    if (unpaddedBytes & 1) {
+                        currByte++;
+                    }
+                    break;
+                }
+            }
+        } else {
+            // If the first byte read is not a flag, it indicates the number of
+            // pixels to set in RLE mode.
+            const uint8_t numPixels = flag;
+            const uint32_t endX = SkTMin<uint32_t>(x + numPixels,
+                                                   (uint32_t) width);
+
+            if (24 == fBitsPerPixel) {
+                // In RLE24, the second byte read is part of the pixel color.
+                // There are two more required bytes to finish encoding the
+                // color.
+                if (totalBytes - currByte < 2) {
+                    SkDebugf("Warning: incomplete RLE input\n");
+                    return kIncompleteInput;
+                }
+
+                // Fill the pixels up to endX with the specified color
+                uint8_t blue = task;
+                uint8_t green = buffer.get()[currByte++];
+                uint8_t red = buffer.get()[currByte++];
+                SkColor color = SkColorSetARGBInline(0xFF, red, green, blue);
+                SkColor* dstRow =
+                        SkTAddOffset<SkColor>(dstPtr, y * dstRowBytes);
+                while (x < endX) {
+                    dstRow[x++] = color;
+                }
+            } else {
+                // In RLE8 or RLE4, the second byte read gives the index in the
+                // color table to look up the pixel color.
+                // RLE8 has one color index that gets repeated
+                // RLE4 has two color indexes in the upper and lower 4 bits of
+                // the bytes, which are alternated
+                uint8_t indices[2] = { task, task };
+                if (4 == fBitsPerPixel) {
+                    indices[0] >>= 4;
+                    indices[1] &= 0xf;
+                }
+
+                // Set the indicated number of pixels
+                for (int which = 0; x < endX; x++) {
+                    setRLEPixel(dstPtr, dstRowBytes, height, x, y,
+                            indices[which]);
+                    which = !which;
+                }
+            }
+        }
+    }
+}
+
+/*
+ *
+ * Performs the bitmap decoding for standard input format
+ *
+ */
+SkCodec::Result SkBmpCodec::decode(const SkImageInfo& dstInfo,
+                                   void* dst, uint32_t dstRowBytes) {
+    // Set constant values
+    const int width = dstInfo.width();
+    const int height = dstInfo.height();
+    const uint32_t pixelsPerByte = 8 / fBitsPerPixel;
+    const uint32_t bytesPerPixel = fBitsPerPixel / 8;
+    const uint32_t unpaddedRowBytes = fBitsPerPixel < 16 ?
+            (width + pixelsPerByte - 1) / pixelsPerByte : width * bytesPerPixel;
+    const uint32_t paddedRowBytes = SkAlign4(unpaddedRowBytes);
+    const uint32_t alphaMask = fBitMasks.alphaMask;
+
+    // Get the destination start row and delta
+    SkColor* dstRow;
+    int32_t delta;
+    if (kTopDown_RowOrder == fRowOrder) {
+        dstRow = (SkColor*) dst;
+        delta = dstRowBytes;
+    } else {
+        dstRow = (SkColor*) SkTAddOffset<void>(dst, (height - 1) * dstRowBytes);
+        delta = -dstRowBytes;
+    }
+
+    // Get swizzler configuration
+    SkSwizzler::SrcConfig config;
+    switch (fBitsPerPixel) {
+        case 1:
+            config = SkSwizzler::kIndex1;
+            break;
+        case 2:
+            config = SkSwizzler::kIndex2;
+            break;
+        case 4:
+            config = SkSwizzler::kIndex4;
+            break;
+        case 8:
+            config = SkSwizzler::kIndex;
+            break;
+        case 24:
+            config = SkSwizzler::kBGR;
+            break;
+        case 32:
+            if (alphaMask == 0) {
+                config = SkSwizzler::kBGRX;
+            } else {
+                config = SkSwizzler::kBGRA;
+            }
+            break;
+        default:
+            SkASSERT(false);
+            return kInvalidInput;
+    }
+
+    // Create swizzler
+    SkSwizzler* swizzler = SkSwizzler::CreateSwizzler(config, fColorTable.get(),
+            dstInfo, dstRow, dstRowBytes, false);
+
+    // Allocate space for a row buffer and a source for the swizzler
+    SkAutoTDeleteArray<uint8_t> srcBuffer(SkNEW_ARRAY(uint8_t, paddedRowBytes));
+
+    // Iterate over rows of the image
+    bool transparent = true;
+    for (uint32_t y = 0; y < height; y++) {
+        // Read a row of the input
+        if (stream()->read(srcBuffer.get(), paddedRowBytes) != paddedRowBytes) {
+            SkDebugf("Warning: incomplete input stream.\n");
+            return kIncompleteInput;
+        }
+
+        // Decode the row in destination format
+        SkSwizzler::ResultAlpha r = swizzler->next(srcBuffer.get(), delta);
+        transparent &= SkSwizzler::kTransparent_ResultAlpha == r;
+    }
+
+    // Now we adjust the output image with some additional behavior that
+    // SkSwizzler does not support.  Firstly, all bmp images that contain
+    // alpha are masked by the alpha mask.  Secondly, many fully transparent
+    // bmp images are intended to be opaque.  Here, we make those corrections.
+    SkColor* dstPtr = (SkColor*) dst;
+    if (alphaMask != 0) {
+        for (uint32_t y = 0; y < height; y++) {
+            for (uint32_t x = 0; x < width; x++) {
+                if (transparent) {
+                    dstPtr[y * dstRowBytes + x] |= 0xFF000000;
+                } else {
+                    dstPtr[y * dstRowBytes + x] &= alphaMask;
+                }
+            }
+        }
+    }
+
+    // Finished decoding the entire image
+    return kSuccess;
+}