Bmp Image Decoding

src/codec/SkCodec_libbmp.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_libbmp.h"
+#include "SkColorTable.h"
+#include "SkEndian.h"
+#include "SkStream.h"
+
+#include <algorithm>
+
+/*
+ *
+ * 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
+}
+
+/*
+ *
+ * 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) {
+    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) {
+    // 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;
+    }
+    //uint16_t signature = get_short(hBuffer, 0);
+
+    // The total bytes in the bmp file
+    const uint32_t totalBytes = get_int(hBuffer.get(), 2);
+    SkASSERT(totalBytes > kBmpHeaderBytes + kBmpOS2V1Bytes);

[scroggo, 2015/02/27 17:04:01]

This is not a valid assert.

Someone could create an invalid BMP file which stores a value here that is too
small. (I can't speak to BMP or this particular case, but in some cases we
deliberately allow certain kinds of bad data, since lots of images have some
mistakes.) If totalBytes is a number that we depend on, we need to handle it
gracefully (i.e. not crash).

The assert I was looking for is something like

SK_COMPILE_ASSERT(2 + 4 <= kBmpHeaderBytes+4, invalid_read);

i.e. that that read was safe to do. 

[msarett, 2015/02/27 21:17:28]

Agreed this has already started causing a problem.

+
+    //uint32_t reserved = get_int(hBuffer, 6);
+
+    // The offset from the start of the file where the pixel data begins
+    const uint32_t offset = get_int(hBuffer.get(), 10);
+    SkASSERT(offset >= kBmpHeaderBytes + kBmpOS2V1Bytes);
+
+    // 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);
+    const uint32_t infoBytesRemaining = infoBytes - 4;

[scroggo, 2015/02/27 17:04:02]

Since infoBytes is read from the buffer, it could be invalid. e.g. it could be <
4, making infoBytesRemaining wrap to a large number. We should probably check
for that rather than attempting to read a large amount from the stream down
below.

+    hBuffer.free();
+
+    // Read the second header
+    SkAutoTDeleteArray<uint8_t> iBuffer(
+            SkNEW_ARRAY(uint8_t, infoBytesRemaining));

[scroggo, 2015/02/27 17:04:01]

Nit: Does this fit on one line? (We don't have a strict rule about line length:

We wrap lines at 100 columns unless it is excessively ugly (use your judgement).

but that looks like it fits in 80 columns?)

+    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 for the many partial versions of the OS 2 header
+        if ((infoBytes <= kBmpOS2V2Bytes && !(infoBytes & 3))
+                || 42 == infoBytes || 46 == infoBytes) {
+            headerType = kOS2VX_BitmapHeaderType;

[scroggo, 2015/02/27 17:04:03]

Since this type is unsupported, can you return here? Or is it possible that one
of the statements below will correct it? (If the latter, can we rearrange things
so that we don't reach this check if it would've been corrected anyway?)

[msarett, 2015/02/27 21:17:28]

This type is supported.  You have caught a bug.  Right now, it will get set to
unknown type below, but it should not.

[scroggo, 2015/02/28 00:25:13]

I'm confused, in patch set 6, it looks like we return NULL on line 346 for
kOS2VX_BitmapHeaderType, or is that only for certain compression methods?

+        }
+        // Check for versions of the Windows headers
+        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;
+            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;
+        }
+        SkASSERT(infoBytesRemaining >= 12);

[scroggo, 2015/02/27 17:04:01]

Again, we need to handle the case where infoBytesRemaining was not large enough.
It looks like if a file reported infoBytes < 36 (in this case), we can return
kInvalidInput.

[msarett, 2015/02/27 21:17:28]

infoBytesRemaining is guaranteed to be large enough.  We do not enter this if
statement unless infoBytes >= 16 which indicates infoBytesRemaining >= 12.  Not
sure if the assert is necessary though.

+        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);
+        }
+        //uint32_t imageBytes = get_int(iBuffer, 16);
+        //uint32_t horizontalResolution = get_int(iBuffer, 20);
+        //uint32_t verticalResolution = get_int(iBuffer, 24);
+
+        // Some versions do not have this field, so we check before
+        // overwriting the default value.
+        if (infoBytesRemaining >= 32) {
+            numColors = get_int(iBuffer.get(), 28);
+        }
+        //uint32_t importantColors = get_int(iBuffer, infoBytes - 4, 32);
+        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);
+        //uint16_t planes = get_short(iBuffer.get(), 4);
+        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
+    static const uint32_t kBmpMaxDim = 1 << 16;
+    bool inverted = true;
+    if (height < 0) {
+        height = -height;
+        inverted = false;
+    }
+    if (width <= 0 || width > kBmpMaxDim || !height || height > kBmpMaxDim) {

[scroggo, 2015/02/27 17:04:01]

Is it possible for width or height to be greater than kBmpMaxDim? Even though
they are ints, you created them with get_short, which returns a uint16_t. Isn't
kBmpMaxDim 17 bits?

[msarett, 2015/02/27 21:17:28]

Yes.  One version of the header reads them as int16.  Later versions provide
them as int32.

[scroggo, 2015/02/28 00:25:13]

Ah, my bad - I missed the other assignment - using get_int.

+        SkDebugf("Error: invalid bitmap dimensions.\n");
+        return NULL;
+    }
+
+    // Determine the input compression format and set bit masks if necessary
+    uint32_t redMask = 0, greenMask = 0, blueMask = 0, alphaMask = 0;

[scroggo, 2015/02/27 17:04:02]

Why not use an SkSwizzler::ColorMask here?

+    uint32_t maskBytes = 0;
+    BitmapInputFormat inputFormat = kUnknown_BitmapInputFormat;
+    switch (compression) {
+        case kNone_BitmapCompressionMethod:
+            inputFormat = kStandard_BitmapInputFormat;
+            // Always respect alpha mask in V4+
+            if (headerType == kInfoV4_BitmapHeaderType ||
+                    headerType == kInfoV5_BitmapHeaderType) {
+                SkASSERT(infoBytesRemaining > 52);

[scroggo, 2015/02/27 17:04:03]

My mistake. I suggested using a compile assert (which is SK_COMPILE_ASSERT;
SkASSERT is a runtime assert), but you actually need to do a check for invalid
data here, since infoBytesRemaining is determined by the input.

[msarett, 2015/02/27 21:17:28]

In this situation, we are guaranteed to have enough infoBytesRemaining because
infoBytes matches this header size.  Still not sure if that means there should
be an assert, check, or neither.

+                alphaMask = get_int(iBuffer.get(), 48);
+            }
+            break;
+        case k8BitRLE_BitmapCompressionMethod:
+            if (bitsPerPixel != 8) {
+                SkDebugf("Warning: correcting invalid bitmap format.\n");
+                bitsPerPixel = 8;
+            }
+            inputFormat = k8BitRLE_BitmapInputFormat;
+            break;
+        case k4BitRLE_BitmapCompressionMethod:
+            if (bitsPerPixel != 4) {
+                SkDebugf("Warning: correcting invalid bitmap format.\n");
+                bitsPerPixel = 4;
+            }
+            inputFormat = k4BitRLE_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;
+                    redMask = get_int(mBuffer.get(), 0);
+                    greenMask = get_int(mBuffer.get(), 4);
+                    blueMask = get_int(mBuffer.get(), 8);
+                    break;
+                }
+                case kInfoV4_BitmapHeaderType:
+                case kInfoV5_BitmapHeaderType:
+                    SkASSERT(infoBytesRemaining >= 52);
+                    alphaMask = get_int(iBuffer.get(), 48);
+                case kInfoV2_BitmapHeaderType:
+                case kInfoV3_BitmapHeaderType:
+                    SkASSERT(infoBytesRemaining >= 48);
+                    redMask = get_int(iBuffer.get(), 36);
+                    greenMask = get_int(iBuffer.get(), 40);
+                    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:
+        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;
+    }
+    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 bmp 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) {
+                redMask = 0x7C00;
+                greenMask = 0x03E0;
+                blueMask = 0x001F;
+            }

[scroggo, 2015/02/27 17:04:01]

Nit: It was hard for me to notice this fall through. Typically we document fall
throughs (except in "trivial" cases, whatever that means), although it seems
like we could just put a break here (since it's what we fall through to).

+        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;
+    }
+
+    // Create mask struct
+    SkSwizzler::ColorMasks* masks = SkNEW(SkSwizzler::ColorMasks);

[scroggo, 2015/02/27 17:04:01]

I'd recommend stack allocating this, and copy constructing it to the SkBmpCodec.
(This will prevent leaks down below. Plus it's only four ints.)

+    masks->redMask = redMask;
+    masks->greenMask = greenMask;
+    masks->blueMask = blueMask;
+    masks->alphaMask = alphaMask;
+
+    // Verify the number of colors for the color table
+    if (bitsPerPixel < 16) {
+        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;
+        }
+    }
+
+    // Construct the color table
+    uint32_t colorBytes = numColors * bytesPerColor;

[scroggo, 2015/02/27 17:04:03]

Can be const?

+    SkPMColor* colorTable = SkNEW_ARRAY(SkPMColor, numColors);

[scroggo, 2015/02/27 17:04:02]

Any reason you're not using an SkColorTable?

Also, this gets leaked if we return NULL a few lines below.

+    if (bitsPerPixel < 16) {
+        SkAutoTDeleteArray<uint8_t> cBuffer(
+                SkNEW_ARRAY(uint8_t, colorBytes));

[scroggo, 2015/02/27 17:04:03]

nit: I think this could fit on one line.

+        if (stream->read(cBuffer.get(), colorBytes) != colorBytes) {
+            SkDebugf("Error: unable to read color table.\n");
+            return NULL;
+        }
+        // We must respect the alpha channel for V4 and V5.  However, if it is
+        // all zeros, we will display the image as opaque rather than
+        // transparent.  This may require redoing some of the processing.
+        bool seenNonZeroAlpha = false;
+        for (uint32_t i = 0; 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 (headerType == kInfoV4_BitmapHeaderType ||
+                    headerType == kInfoV5_BitmapHeaderType) {
+                alpha = (alphaMask >> 24) &
+                        get_byte(cBuffer.get(), i*bytesPerColor + 3);
+                if (!alpha && !seenNonZeroAlpha) {
+                    alpha = 0xFF;
+                } else {
+                    // If we see a non-zero alpha, we restart the loop
+                    seenNonZeroAlpha = true;
+                    i = -1;
+                }
+            }
+            colorTable[i] = SkPreMultiplyColor(SkColorSetARGBInline(alpha,
+                    red, green, blue));
+        }
+    } 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.
+        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;
+    if (stream->skip(offset - bytesRead) != offset - bytesRead) {
+        SkDebugf("Error: unable to skip to image data.\n");
+        return NULL;
+    }
+    const uint32_t remainingBytes = totalBytes - offset;
+
+    // Return the codec
+    // Use of image info for input format does not make sense given
+    // that the possible bitmap input formats do not match up with
+    // Skia color types.  Instead we use ImageInfo for width and height,
+    // and other fields for input format information.
+    const SkImageInfo& imageInfo = SkImageInfo::Make(width, height,
+            kN32_SkColorType, kPremul_SkAlphaType);
+    return SkNEW_ARGS(SkBmpCodec, (imageInfo, stream, bitsPerPixel,
+                                   inputFormat, masks, colorTable, inverted,
+                                   remainingBytes));
+}
+
+/*
+ *
+ * Creates an instance of the decoder
+ * Called only by NewFromStream
+ *
+ */
+SkBmpCodec::SkBmpCodec(const SkImageInfo& info, SkStream* stream,
+                       uint16_t bitsPerPixel, BitmapInputFormat inputFormat,
+                       SkSwizzler::ColorMasks* masks, SkPMColor* colorTable,
+                       bool inverted, const uint32_t remainingBytes)
+    : INHERITED(info, stream)
+    , fBitsPerPixel(bitsPerPixel)
+    , fInputFormat(inputFormat)
+    , fBitMasks(masks)
+    , fColorTable(colorTable)
+    , fInverted(inverted)
+    , fRemainingBytes(remainingBytes)
+{}
+
+/*
+ *
+ * Initiates the bitmap decode
+ *
+ */
+SkCodec::Result SkBmpCodec::onGetPixels(const SkImageInfo& dstInfo,
+                                        void* dst, size_t dstRowBytes,
+                                        SkPMColor*, int*) {
+    // This version of the decoder does not support scaling
+    if (dstInfo.dimensions() != getOriginalInfo().dimensions()) {
+        SkDebugf("Error: scaling not supported.\n");
+        return kInvalidScale;
+    }
+
+    switch (fInputFormat) {
+        case k4BitRLE_BitmapInputFormat:
+        case k8BitRLE_BitmapInputFormat:
+            return decodeRLE(dstInfo, dst, dstRowBytes);
+        case kBitMask_BitmapInputFormat:
+        case kStandard_BitmapInputFormat:
+            return decode(dstInfo, dst, dstRowBytes);
+        default:
+            SkDebugf("Error: unknown bitmap input format.\n");
+            return kInvalidInput;
+    }
+}
+
+/*
+ *
+ * Performs the bitmap decoding for standard and bit masks 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 = (unpaddedRowBytes + 3) & (~3);
+    const uint32_t alphaMask = fBitMasks.get()->alphaMask;

[scroggo, 2015/02/27 17:04:01]

FYI: SkAutoTDelete defines operator->, so this can be written as

fBitMasks->alphaMask

+
+    // 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 16:
+            config = SkSwizzler::kMask16;
+            break;
+        case 24:
+            if (kBitMask_BitmapInputFormat == fInputFormat) {
+                config = SkSwizzler::kMask24;
+            } else {
+                config = SkSwizzler::kBGR;
+            }
+            break;
+        case 32:
+            if (kBitMask_BitmapInputFormat == fInputFormat) {
+                config = SkSwizzler::kMask32;
+            } else if (!alphaMask) {
+                config = SkSwizzler::kBGRX;
+            } else {
+                config = SkSwizzler::kBGRA;
+            }
+            break;
+        default:
+            SkDebugf("Error: default case should be unreachable.\n");

[scroggo, 2015/02/27 17:04:01]

I'd put an SkASSERT(false), if it should never be reached. That will alert us to
the fact that we were wrong - it was reachable.

+            return kInvalidInput;
+    }
+
+    // If zeroAlpha is kNormal, it indicates that the image will be
+    // considered as encoded.  If kTransparentAsOpaque, we will respect the
+    // value of the alpha channel if it is nonzero for any of the pixels.
+    // However, if it is always zero, we will consider the image opaque instead
+    // of transparent. This may require redoing some of the decoding.
+    SkSwizzler::ZeroAlpha zeroAlpha = SkSwizzler::kNormal;
+    if (alphaMask) {
+        zeroAlpha = SkSwizzler::kTransparentAsOpaque;
+    }
+
+    // Create swizzler
+    SkSwizzler* swizzler = SkSwizzler::CreateSwizzler(config, fColorTable.get(),
+            dstInfo, dst, dstRowBytes, false, fBitMasks.get(), zeroAlpha,
+            fInverted ? SkSwizzler::kBottomUp : SkSwizzler::kTopDown);

[scroggo, 2015/02/27 17:04:02]

I'd recommend storing fInverted as this enum.

+
+    // 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
+    for (uint32_t y = 0; y < height; y++) {
+        // Read a row of the input
+        if (stream()->read(srcBuffer.get(), paddedRowBytes) != paddedRowBytes) {
+            return kIncompleteInput;
+        }
+
+        // Decode the row in destination format
+        swizzler->next(srcBuffer.get());
+    }
+
+    // Finished decoding the entire image
+    return kSuccess;
+}
+
+/*
+ *
+ * Set an RLE pixel using the color table
+ *
+ */
+void SkBmpCodec::setPixel(SkPMColor* dst, uint32_t dstRowBytes, int height,

[scroggo, 2015/02/27 17:04:01]

Maybe this should be called setRLEPixel.

Also, not a big deal, but I would assert that fInputType is something that
expects RLE.

+                            uint32_t x, uint32_t y, uint8_t index) {

[scroggo, 2015/02/27 17:04:01]

nit: should line up with SkPMColor.

+    if (fInverted) {
+        y = height - y - 1;
+    }
+    SkPMColor* dstRow = SkTAddOffset<SkPMColor>(dst, y * dstRowBytes);
+    dstRow[x] = fColorTable.get()[index];
+    return;

[scroggo, 2015/02/27 17:04:01]

Not needed.

+}
+
+/*
+ *
+ * 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 ppb = 8 / fBitsPerPixel;
+
+    // Input buffer parameters
+    uint32_t i = 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 hits EOL or EOF early, remaining pixels are transparent

[scroggo, 2015/02/27 17:04:01]

To be fair, they're only transparent in the case of a type with transparency.

As an aside, Android often allocates their memory from the Java heap, meaning
that it was already zeroed. I haven't written an API for that yet, but maybe add
a TODO here to skip this if the memory was already zeroed.

+    memset(dst, 0, dstRowBytes * height);
+    SkPMColor* dstPtr = (SkPMColor*) dst;
+
+    while (true) {
+        // Every entry takes at least two bytes
+        if (totalBytes - i < 2) {
+            SkDebugf("Error: incomplete RLE input.\n");
+            return kIncompleteInput;
+        }
+
+        // Read the two bytes and verify we have not reached end of image
+        const uint8_t count = buffer.get()[i++];
+        const uint8_t code = buffer.get()[i++];
+        if ((count || (code != RLE_EOF)) && y > height) {
+            SkDebugf("Error: invalid RLE input.\n");
+            return kInvalidInput;
+        }
+
+        // Perform decoding
+        if (RLE_ESCAPE == count) {
+            switch (code) {
+                case RLE_EOL:
+                    x = 0;
+                    y++;
+                    break;
+                case RLE_EOF:
+                    return kSuccess;
+                case RLE_DELTA: {
+                    // Two bytes are needed to specify delta
+                    if (totalBytes - i < 2) {
+                        SkDebugf("Error: incomplete RLE input\n");
+                        return kIncompleteInput;
+                    }
+                    // Verify that we are not past the end of row or image
+                    const uint8_t dx = buffer.get()[i++];
+                    const uint8_t dy = buffer.get()[i++];
+                    if (x + dx > width || y + dy > height) {
+                        SkDebugf("Error: invalid RLE input.\n");
+                        return kInvalidInput;
+                    }
+                    // Move to new location
+                    x += dx;

[scroggo, 2015/02/27 17:04:01]

Nit: this is probably premature optimization on my part, but why not instead do
the following:

x += dx;
y += dy;

if (x > width || y > height) {
  return kInvalidInput;
}

+                    y += dy;
+                    break;
+                }
+                default: { // Absolute mode
+                    // Check that we have enough bytes and that there are
+                    // enough pixels remaining in the row
+                    uint32_t unpaddedBytes = (code + ppb - 1) / ppb;
+                    uint32_t paddedBytes = (unpaddedBytes + 1) & ~1;
+                    if (x + code > width || totalBytes - i < paddedBytes) {
+                        SkDebugf("Error: invalid RLE input.\n");
+                        return kInvalidInput;
+                    }
+                    // Use the color table to set the coded number of pixels
+                    uint8_t num = code;
+                    while (num > 0) {
+                        switch(fBitsPerPixel) {
+                            case 4: {
+                                uint8_t val = buffer.get()[i++];
+                                setPixel(dstPtr, dstRowBytes, height, x++, y,
+                                        val >> 4);

[scroggo, 2015/02/27 17:04:03]

nit: This should line up with dstPtr

+                                num--;
+                                if (num) {
+                                    setPixel(dstPtr, dstRowBytes, height,
+                                            x++, y, val & 0xF);
+                                    num--;
+                                }
+                                break;
+                            }
+                            case 8:
+                                setPixel(dstPtr, dstRowBytes, height, x++, y,
+                                        buffer.get()[i++]);
+                                num--;
+                                break;
+                            default:
+                                SkDebugf("Error: invalid RLE bpp.\n");
+                                return kInvalidInput;
+                        }
+                    }
+                    // Skip a byte if necessary to maintain alignment
+                    if (unpaddedBytes & 1) {
+                        i++;
+                    }
+                    break;
+                }
+            }
+        } else { // Encoded mode
+            // Ensure we do not move past the end of the row
+            const int endX = std::min(x + count, (uint32_t) width);

[scroggo, 2015/02/27 17:04:03]

Nit: We typically use our own min functions, defined in SkTypes.h

+
+            // 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 indexes[2] = { code, code };

[scroggo, 2015/02/27 17:04:01]

nit: indices?

+            if (fBitsPerPixel == 4) {
+                indexes[0] >>= 4;
+                indexes[1] &= 0xf;
+            }
+
+            // Set the indicated number of pixels
+            for (int which = 0; x < endX; x++) {
+                setPixel(dstPtr, dstRowBytes, height, x, y, indexes[which]);
+                which = !which;
+            }
+        }
+    }
+}