Adding premul and 565 swizzles for bmp:

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 "SkCodecPriv.h"
 #include "SkColorPriv.h"
 #include "SkStream.h"
 
 /*
  *
  * 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) {
-    // All of the swizzles convert to kN32
-    // TODO: Update this when more swizzles are supported
-    if (kN32_SkColorType != dst.colorType()) {
+    // Check for supported color types
+    if (kN32_SkColorType != dst.colorType() &&
+            kRGB_565_SkColorType != dst.colorType()) {
         return false;
     }
     // Support the swizzle if the requested alpha type is the same as our guess
     // for the input alpha type
     if (src.alphaType() == dst.alphaType()) {
         return true;
     }
-    // TODO: Support more swizzles, especially premul
-    return false;
+
+    // Support premul as the destination when the source is unpremul
+    return premul_and_unpremul(dst.alphaType(), src.alphaType());
 }
 
 /*
  *
  * Defines the version and type of the second bitmap header
  *
  */
 enum BitmapHeaderType {
     kInfoV1_BitmapHeaderType,
     kInfoV2_BitmapHeaderType,
@@ skipping 342 matching lines @@
     }
 
     // Check that input bit masks are valid and create the masks object
     SkAutoTDelete<SkMasks>
             masks(SkMasks::CreateMasks(inputMasks, bitsPerPixel));
     if (NULL == masks) {
         SkDebugf("Error: invalid input masks.\n");
         return NULL;
     }
 
-    // Process the color table
-    uint32_t colorBytes = 0;
-    SkPMColor* colorTable = NULL;
-    if (bitsPerPixel < 16) {
-        // Verify the number of colors for the color table
-        const uint32_t 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(SkPMColor, 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 (colors are stored unpremultiplied)
-        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 = (inputMasks.alpha >> 24) &
-                        get_byte(cBuffer.get(), i*bytesPerColor + 3);
-            }
-            // Store the unpremultiplied color
-            colorTable[i] = SkPackARGB32NoCheck(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] = SkPackARGB32NoCheck(0xFF, 0, 0, 0);
-        }
+    // Verify the number of colors in the color table
+    const uint32_t maxColors = 1 << bitsPerPixel;

[scroggo, 2015/03/17 13:27:22]

Could you move all of this into SkBmpCodec's onGetPixels?

Here are the pros and cons, as far as I see it:

pro:
Don't need to pass as many parameters to the creation of the codec (AFAICT).
Instead, we can declare these variables where they're used.

con:
Maybe we won't be able to do the bytesRead > offset check here (unless we
duplicate work).

[msarett, 2015/03/17 16:54:05]

Doing these checks in onGetPixels actually causes us to need another parameter
(bytesRead).

However, I definitely see the pro of keeping all of the color table related work
in one place.  Additionally, we actually have less duplicated code.

The reason I chose the original design was that it would be nice to identify
invalid inputs in NewFromStream rather than onGetPixels.  However, I feel that
this error case is rare enough that it is sufficient to catch the error later.

I agree making this change is a net improvement and have done so.

[scroggo, 2015/03/17 20:02:58]

I think we can get around this...

How about this:

continue to pass "offset - bytesRead" to the constructor as the offset.

In createColorTable, instead of checking:
  if (fBytesRead + colorBytes > fOffset)

check

  if (colorBytes > fOffset)

Then you can skip fOffset - colorBytes.

I want to get rid of fBytesRead for a few reasons:
- It's only needed in one place
- For the rest of an SkBmpCodec's lifetime, it is no longer true
- SkBmpCodec has lots of member variables already
- The constructor of SkBmpCodec has lots of parameters.
- I think we can do without it.

[msarett, 2015/03/18 13:02:52]

Yes great!  That's a much better way to do it.

+    // Zero is a default for maxColors
+    // Also set numColors to maxColors when input is too large
+    if (numColors <= 0 || numColors > maxColors) {
+        numColors = maxColors;
     }
 
-    // Ensure that the stream now points to the start of the pixel array
+    // Account for the color table size
+    uint32_t colorBytes;
+    if (bitsPerPixel < 16) {
+        colorBytes = numColors * bytesPerColor;
+    } else {
+        colorBytes = 0;
+    }
+
+    // Calculate the number of bytes read so far (including the color table)
     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.
+        // rather than try to guess the intended size of the color table.
         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.  We will
-    // choose kN32_SkColorType as the input color type because that is the
-    // expected choice for a destination color type.  In reality, the input
-    // color type has many possible formats.
+    // set color type to kN32_SkColorType because that should be the default
+    // output.
     const SkImageInfo& imageInfo = SkImageInfo::Make(width, height,
             kN32_SkColorType, alphaType);
     return SkNEW_ARGS(SkBmpCodec, (imageInfo, stream, bitsPerPixel,
-                                   inputFormat, masks.detach(), colorTable,
+                                   inputFormat, masks.detach(), numColors,
+                                   bytesPerColor, offset - bytesRead,
                                    rowOrder, remainingBytes));
 }
 
 /*
  *
  * Creates an instance of the decoder
  * Called only by NewFromStream
  *
  */
 SkBmpCodec::SkBmpCodec(const SkImageInfo& info, SkStream* stream,
                        uint16_t bitsPerPixel, BitmapInputFormat inputFormat,
-                       SkMasks* masks, SkPMColor* colorTable,
-                       RowOrder rowOrder,
-                       const uint32_t remainingBytes)
+                       SkMasks* masks, uint32_t numColors,
+                       uint32_t bytesPerColor, uint32_t offset,
+                       RowOrder rowOrder, uint32_t remainingBytes)
     : INHERITED(info, stream)
     , fBitsPerPixel(bitsPerPixel)
     , fInputFormat(inputFormat)
     , fMasks(masks)
-    , fColorTable(colorTable)
+    , fColorTable(NULL)
+    , fNumColors(numColors)
+    , fBytesPerColor(bytesPerColor)
+    , fOffset(offset)
     , fRowOrder(rowOrder)
     , fRemainingBytes(remainingBytes)
 {}
 
 /*
  *
  * Initiates the bitmap decode
  *
  */
 SkCodec::Result SkBmpCodec::onGetPixels(const SkImageInfo& dstInfo,
                                         void* dst, size_t dstRowBytes,
                                         SkPMColor*, int*) {
+    // Check for proper input and output formats
     if (!this->rewindIfNeeded()) {
         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;
     }
 
+    // Create the color table if necessary and prepare the stream for decode
+    if (!createColorTable(dstInfo.alphaType())) {
+        SkDebugf("Error: could not create color table.\n");
+        return kInvalidInput;
+    }
+
+    // Perform the decode
     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;
     }
 }
 
 /*
  *
+ * Process the color table for the bmp input
+ *
+ */
+ bool SkBmpCodec::createColorTable(const SkAlphaType alphaType) {
+    SkPMColor* colorTable = NULL;
+    if (fBitsPerPixel < 16) {
+        // Allocate memory for a color table
+        const uint32_t maxColors = 1 << fBitsPerPixel;
+        colorTable = SkNEW_ARRAY(SkPMColor, maxColors);

[scroggo, 2015/03/17 13:27:22]

colorTable will leak if we return NULL below.

[msarett, 2015/03/17 16:54:06]

Done.

+
+        // Read the color table from the stream
+        uint32_t colorBytes = fNumColors * fBytesPerColor;
+        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 < fNumColors; i++) {
+            uint8_t blue = get_byte(cBuffer.get(), i*fBytesPerColor);
+            uint8_t green = get_byte(cBuffer.get(), i*fBytesPerColor + 1);
+            uint8_t red = get_byte(cBuffer.get(), i*fBytesPerColor + 2);
+            uint8_t alpha;
+            switch (alphaType) {
+                case kOpaque_SkAlphaType:
+                    alpha = 0xFF;
+                    colorTable[i] = SkPackARGB32NoCheck(alpha, red, green,

[scroggo, 2015/03/17 13:27:22]

nit: no need to update the variable alpha. You can just put 0xFF inline here.
I'd be surprised if compiler created an extra assignment, but I think it's more
readable with inlining.

[msarett, 2015/03/17 16:54:05]

Done.

+                            blue);
+                    break;
+                case kUnpremul_SkAlphaType:
+                    alpha = (fMasks->getAlphaMask() >> 24) &
+                            get_byte(cBuffer.get(), i*fBytesPerColor + 3);
+                    colorTable[i] = SkPackARGB32NoCheck(alpha, red, green,
+                            blue);
+                    break;
+                case kPremul_SkAlphaType:
+                    alpha = (fMasks->getAlphaMask() >> 24) &
+                            get_byte(cBuffer.get(), i*fBytesPerColor + 3);
+                    colorTable[i] = SkPreMultiplyARGB(alpha, red, green, blue);
+                    break;
+                default:
+                    // This should not be reached because conversion possible
+                    // should fail if the alpha type is not one of the above
+                    // values.
+                    SkASSERT(false);
+                    break;
+            }
+        }
+
+        // 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] = SkPackARGB32NoCheck(0xFF, 0, 0, 0);
+        }
+    }
+
+    // After reading the color table, skip to the start of the pixel array
+    if (stream()->skip(fOffset) != fOffset) {
+        SkDebugf("Error: unable to skip to image data.\n");
+        return false;
+    }
+
+    // Set the color table and return true on success
+    fColorTable.reset(colorTable);
+    return true;
+}
+
+/*
+ *
  * Performs the bitmap decoding for bit masks input format
  *
  */
 SkCodec::Result SkBmpCodec::decodeMask(const SkImageInfo& dstInfo,
                                        void* dst, size_t dstRowBytes) {
     // Set constant values
     const int width = dstInfo.width();
     const int height = dstInfo.height();
     const size_t rowBytes = SkAlign4(compute_row_bytes(width, fBitsPerPixel));
 
-    // Allocate space for a row buffer and a source for the swizzler
-    SkAutoTDeleteArray<uint8_t> srcBuffer(SkNEW_ARRAY(uint8_t, rowBytes));
+    // Allocate a buffer large enough to hold the full input stream
+    uint8_t* srcBuffer(SkNEW_ARRAY(uint8_t, height*rowBytes));

[msarett, 2015/03/16 20:21:53]

I know that it is preferred to use an SkAutoTDeleteArray.  The reason I am not
using this template is that I need to move row by row through the buffer.  As
far as I could tell this type of pointer arithmetic would be messy using an
AutoTDelete type.  Let me know if there is an option that I am missing.

[scroggo, 2015/03/17 13:27:22]

You should definitely use some sort of auto deleter. (FWIW, in PNG I use an
SkAutoMalloc, and cast the return value of ::get() to the type I want. I don't
know that one is better than the other.)

I don't see the issue with pointer arithmetic. You already use the raw pointer
srcRow; you can do pointer arithmetic on that.

[msarett, 2015/03/17 16:54:06]

Agreed, not sure what I was thinking.

+    uint8_t* srcRow = srcBuffer;
 
     // Get the destination start row and delta
+    SkPMColor* dstStart;
     SkPMColor* dstRow;
     int delta;
     if (kTopDown_RowOrder == fRowOrder) {
-        dstRow = (SkPMColor*) dst;
+        dstStart = (SkPMColor*) dst;
+        dstRow = dstStart;
         delta = (int) dstRowBytes;
     } else {
-        dstRow = (SkPMColor*) SkTAddOffset<void>(dst, (height-1) * dstRowBytes);
+        dstStart = (SkPMColor*) SkTAddOffset<void>(
+                dst, (height - 1) * dstRowBytes);
+        dstRow = dstStart;
         delta = -((int) dstRowBytes);
     }
 
     // Create the swizzler
     SkMaskSwizzler* swizzler = SkMaskSwizzler::CreateMaskSwizzler(
             dstInfo, fMasks, fBitsPerPixel);
 
     // Iterate over rows of the image
     bool transparent = true;
     for (int y = 0; y < height; y++) {
         // Read a row of the input
-        if (stream()->read(srcBuffer.get(), rowBytes) != rowBytes) {
+        if (stream()->read(srcRow, rowBytes) != rowBytes) {
             SkDebugf("Warning: incomplete input stream.\n");
+            SkDELETE_ARRAY(srcBuffer);
             return kIncompleteInput;
         }
 
         // Decode the row in destination format
-        SkSwizzler::ResultAlpha r = swizzler->next(dstRow, srcBuffer.get());
+        SkSwizzler::ResultAlpha r = swizzler->next(dstRow, srcRow);
         transparent &= SkSwizzler::IsTransparent(r);
 
         // Move to the next row
         dstRow = SkTAddOffset<SkPMColor>(dstRow, delta);
+        srcRow = SkTAddOffset<uint8_t>(srcRow, rowBytes);

[scroggo, 2015/03/17 13:27:22]

How did this work before?

[msarett, 2015/03/17 16:54:05]

We used to overwrite the same row buffer for each row that we decoded.  Now we
store all the rows in a src buffer in case we need to redo the decode.

[scroggo, 2015/03/17 20:02:58]

Ah, of course!

     }
 
     // Some fully transparent bmp images are intended to be opaque.  Here, we
     // correct for this possibility.
-    dstRow = (SkPMColor*) dst;
     if (transparent) {
+        const SkImageInfo& newInfo = dstInfo.makeAlphaType(kOpaque_SkAlphaType);

[scroggo, 2015/03/17 13:27:22]

nit: Maybe this should be opaqueInfo? Or maskInfo?

[msarett, 2015/03/17 16:54:06]

Done.

+        SkMaskSwizzler* newSwizzler = SkMaskSwizzler::CreateMaskSwizzler(

[scroggo, 2015/03/17 13:27:22]

This will leak.

[scroggo, 2015/03/17 13:27:22]

nit: Maybe name this maskSwizzler?

[msarett, 2015/03/17 16:54:05]

Yeah it looks like it.  I think this is not the only place.  I have fixed it.

I went with opaqueSwizzler as the name.  I renamed the first swizzler
maskSwizzler.

+            newInfo, fMasks, fBitsPerPixel);

[msarett, 2015/03/16 20:21:53]

Increase indent

+        srcRow = srcBuffer;
+        dstRow = dstStart;
         for (int y = 0; y < height; y++) {
-            for (int x = 0; x < width; x++) {
-                dstRow[x] |= 0xFF000000;
-            }
-            dstRow = SkTAddOffset<SkPMColor>(dstRow, dstRowBytes);
+            // Decode the row in new format
+            newSwizzler->next(dstRow, srcRow);
+
+            // Move to the next row
+            dstRow = SkTAddOffset<SkPMColor>(dstRow, delta);
+            srcRow = SkTAddOffset<uint8_t>(srcRow, rowBytes);
         }
     }
 
+    // Clean up memory
+    SkDELETE_ARRAY(srcBuffer);
+
     // Finished decoding the entire image
     return kSuccess;
 }
 
 /*
  *
  * Set an RLE pixel using the color table
  *
  */
-void SkBmpCodec::setRLEPixel(SkPMColor* dst, size_t dstRowBytes, int height,
-                             uint32_t x, uint32_t y, uint8_t index) {
+void SkBmpCodec::setRLEPixel(SkPMColor* dst, size_t dstRowBytes,
+                             const SkImageInfo& dstInfo, uint32_t x, uint32_t y,
+                             uint8_t index) {
+    // Set the row
+    int height = dstInfo.height();
+    int row;
     if (kBottomUp_RowOrder == fRowOrder) {
-        y = height - y - 1;
+        row = height - y - 1;
+    } else {
+        row = y;
     }
-    SkPMColor* dstRow = SkTAddOffset<SkPMColor>(dst, y * dstRowBytes);
-    dstRow[x] = fColorTable.get()[index];
+
+    // Set the pixel based on destination color type
+    switch (dstInfo.colorType()) {
+        case kN32_SkColorType: {
+            SkPMColor* dstRow = SkTAddOffset<SkPMColor>(dst,
+                    row * (int) dstRowBytes);
+            dstRow[x] = fColorTable.get()[index];
+            break;
+        }
+        case kRGB_565_SkColorType: {
+            uint16_t* dstRow = SkTAddOffset<uint16_t>(dst,
+                    row * (int) dstRowBytes);
+            dstRow[x] = SkPixel32ToPixel16(fColorTable.get()[index]);
+            break;
+        }
+        default:
+            // This case should not be reached.  We should catch an invalid
+            // color type when we check that the conversion is possible.
+            SkASSERT(false);
+            break;
+    }
 }
 
 /*
+ *
+ * Set an RLE pixel from R, G, B values
+ *
+ */
+void SkBmpCodec::setRLE24Pixel(SkPMColor* dst, size_t dstRowBytes,
+                               const SkImageInfo& dstInfo, uint32_t x,
+                               uint32_t y, uint8_t red, uint8_t green,
+                               uint8_t blue) {
+    // Set the row
+    int height = dstInfo.height();
+    int row;
+    if (kBottomUp_RowOrder == fRowOrder) {
+        row = height - y - 1;
+    } else {
+        row = y;
+    }
+
+    // Set the pixel based on destination color type
+    switch (dstInfo.colorType()) {
+        case kN32_SkColorType: {
+            SkPMColor* dstRow = SkTAddOffset<SkPMColor>(dst,
+                    row * (int) dstRowBytes);
+            dstRow[x] = SkPackARGB32NoCheck(0xFF, red, green, blue);
+            break;
+        }
+        case kRGB_565_SkColorType: {
+            uint16_t* dstRow = SkTAddOffset<uint16_t>(dst,
+                    row * (int) dstRowBytes);
+            dstRow[x] = SkPack888ToRGB16(red, green, blue);
+            break;
+        }
+        default:
+            // This case should not be reached.  We should catch an invalid
+            // color type when we check that the conversion is possible.
+            SkASSERT(false);
+            break;
+    }
+}
+
+/*
  *
  * 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, size_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();
 
     // Input buffer parameters
     uint32_t currByte = 0;
     SkAutoTDeleteArray<uint8_t> buffer(SkNEW_ARRAY(uint8_t, fRemainingBytes));
     size_t totalBytes = stream()->read(buffer.get(), fRemainingBytes);
-    if ((uint32_t) totalBytes < 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
     int x = 0;
     int y = 0;
     // If the code skips pixels, remaining pixels are transparent or black
@@ skipping 58 matching lines @@
                             fBitsPerPixel);
                     // Abort if setting numPixels moves us off the edge of the
                     // image.  Also abort if there are not enough bytes
                     // remaining in the stream to set numPixels.
                     if (x + numPixels > width ||
                             (int) totalBytes - currByte < SkAlign2(rowBytes)) {
                         SkDebugf("Warning: invalid RLE input.\n");
                         return kIncompleteInput;
                     }
                     // Set numPixels number of pixels
-                    SkPMColor* dstRow = SkTAddOffset<SkPMColor>(
-                            dstPtr, y * dstRowBytes);
                     while (numPixels > 0) {
                         switch(fBitsPerPixel) {
                             case 4: {
                                 SkASSERT(currByte < totalBytes);
                                 uint8_t val = buffer.get()[currByte++];
-                                setRLEPixel(dstPtr, dstRowBytes, height, x++, y,
-                                        val >> 4);
+                                setRLEPixel(dstPtr, dstRowBytes, dstInfo, x++,
+                                        y, val >> 4);
                                 numPixels--;
                                 if (numPixels != 0) {
-                                    setRLEPixel(dstPtr, dstRowBytes, height,
+                                    setRLEPixel(dstPtr, dstRowBytes, dstInfo,
                                             x++, y, val & 0xF);
                                     numPixels--;
                                 }
                                 break;
                             }
                             case 8:
                                 SkASSERT(currByte < totalBytes);
-                                setRLEPixel(dstPtr, dstRowBytes, height, x++, y,
-                                        buffer.get()[currByte++]);
+                                setRLEPixel(dstPtr, dstRowBytes, dstInfo, x++,
+                                        y, buffer.get()[currByte++]);
                                 numPixels--;
                                 break;
                             case 24: {
                                 SkASSERT(currByte + 2 < totalBytes);
                                 uint8_t blue = buffer.get()[currByte++];
                                 uint8_t green = buffer.get()[currByte++];
                                 uint8_t red = buffer.get()[currByte++];
-                                SkPMColor color = SkPackARGB32NoCheck(
-                                        0xFF, red, green, blue);
-                                dstRow[x++] = color;
+                                setRLE24Pixel(dstPtr, dstRowBytes, dstInfo,
+                                            x++, y, red, green, blue);
                                 numPixels--;
                             }
                             default:
                                 SkASSERT(false);
                                 return kInvalidInput;
                         }
                     }
                     // Skip a byte if necessary to maintain alignment
                     if (!SkIsAlign2(rowBytes)) {
                         currByte++;
@@ skipping 13 matching lines @@
                 // color.
                 if ((int) 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++];
-                SkPMColor color = SkPackARGB32NoCheck(0xFF, red, green, blue);
-                SkPMColor* dstRow =
-                        SkTAddOffset<SkPMColor>(dstPtr, y * dstRowBytes);
                 while (x < endX) {
-                    dstRow[x++] = color;
+                    setRLE24Pixel(dstPtr, dstRowBytes, dstInfo, x++, y, red,
+                            green, blue);
                 }
             } 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,
+                    setRLEPixel(dstPtr, dstRowBytes, dstInfo, x, y,
                             indices[which]);
                     which = !which;
                 }
             }
         }
     }
 }
 
 /*
  *
@@ skipping 90 matching lines @@
                 }
                 dstRow = SkTAddOffset<SkPMColor>(dstRow, dstRowBytes);
             }
         }
     }
     */
 
     // Finished decoding the entire image
     return kSuccess;
 }