Swizzler changes Index8 and 565

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
index 75c5715bda1df787b6e503c3346a1a172590bde7..05689e81178a4161a67b1f1d586dd71e4d9a5995 100644
--- a/src/codec/SkCodec_libbmp.cpp
+++ b/src/codec/SkCodec_libbmp.cpp
@@ -23,12 +23,21 @@ static bool conversion_possible(const SkImageInfo& dst,
         return false;
     }
 
-    // Check for supported color and alpha types
+    // Check for supported alpha types
+    if (src.alphaType() != dst.alphaType() &&
+            (kPremul_SkAlphaType != dst.alphaType() ||
+            kUnpremul_SkAlphaType == src.alphaType())) {
+        return false;
+    }
+
+    // Check for supported color types
     switch (dst.colorType()) {
+        // Allow output to kN32 from any type of input
         case kN32_SkColorType:
-            return src.alphaType() == dst.alphaType() ||
-                    (kPremul_SkAlphaType == dst.alphaType() &&
-                    kUnpremul_SkAlphaType == src.alphaType());
+            return true;
+        // Allow output to kIndex_8 from compatible inputs
+        case kIndex_8_SkColorType:
+            return kIndex_8_SkColorType == src.colorType();
         default:
             return false;
     }
@@ -419,12 +428,17 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool isIco, SkCodec** codecOut) {
     }
     iBuffer.free();
 
-    // Additionally, 32 bit bmp-in-icos use the alpha channel
-    if (isIco && 32 == bitsPerPixel) {
+    // Additionally, 32 bit bmp-in-icos use the alpha channel.
+    // And, RLE inputs may skip pixels, leaving them as transparent.  This
+    // is uncommon, but we cannot be certain that an RLE bmp will be opaque.
+    if ((isIco && 32 == bitsPerPixel) || (kRLE_BitmapInputFormat == inputFormat)) {
         alphaType = kUnpremul_SkAlphaType;
     }
 
-    // Check for valid bits per pixel input
+    // Check for valid bits per pixel.
+    // At the same time, use this information to choose a suggested color type
+    // and to set default masks.
+    SkColorType colorType = kN32_SkColorType;
     switch (bitsPerPixel) {
         // In addition to more standard pixel compression formats, bmp supports
         // the use of bit masks to determine pixel components.  The standard
@@ -439,10 +453,18 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool isIco, SkCodec** codecOut) {
                 inputFormat = kBitMask_BitmapInputFormat;
             }
             break;
+        // We want to decode to kIndex_8 for input formats that are already
+        // designed in index format.
         case 1:
         case 2:
         case 4:
         case 8:
+        	// However, we cannot in RLE format since we may need to leave some

[scroggo, 2015/04/02 19:20:30]

It seems a shame that we cannot support this. It seems like SkColorTable should
be able to support more than 256 colors, except that we make assumptions all
over the place that it only supports 256 (and it would require more than 8 bits
to index...). Oh well :(

[msarett, 2015/04/03 18:01:32]

Agreed.  Wish there was a good way.

+        	// pixels as transparent.  Similarly, we also cannot for ICO images
+        	// since we may need to apply a transparent mask.
+            if (kRLE_BitmapInputFormat != inputFormat && !isIco) {
+                colorType = kIndex_8_SkColorType;
+            }
         case 24:
         case 32:
             break;
@@ -475,11 +497,10 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool isIco, SkCodec** codecOut) {
 
     if (codecOut) {
         // Return the codec
-        // We will use ImageInfo to store width, height, and alpha type. We
-        // will set color type to kN32_SkColorType because that should be the
-        // default output.
+        // We will use ImageInfo to store width, height, suggested color type, and
+        // suggested alpha type.
         const SkImageInfo& imageInfo = SkImageInfo::Make(width, height,
-                kN32_SkColorType, alphaType);
+                colorType, alphaType);
         *codecOut = SkNEW_ARGS(SkBmpCodec, (imageInfo, stream, bitsPerPixel,
                                             inputFormat, masks.detach(),
                                             numColors, bytesPerColor,
@@ -535,8 +556,9 @@ SkBmpCodec::SkBmpCodec(const SkImageInfo& info, SkStream* stream,
  */
 SkCodec::Result SkBmpCodec::onGetPixels(const SkImageInfo& dstInfo,
                                         void* dst, size_t dstRowBytes,
-                                        const Options&,
-                                        SkPMColor*, int*) {
+                                        const Options& opts,
+                                        SkPMColor* inputColorPtr,
+                                        int* inputColorCount) {
     // Check for proper input and output formats
     SkCodec::RewindState rewindState = this->rewindIfNeeded();
     if (rewindState == kCouldNotRewind_RewindState) {
@@ -555,6 +577,11 @@ SkCodec::Result SkBmpCodec::onGetPixels(const SkImageInfo& dstInfo,
         return kInvalidConversion;
     }
 
+    // Get a color table pointer
+    SkPMColor alternateColorPtr[256];
+    fColorTable = get_color_table_ptr(dstInfo.colorType(), inputColorPtr,

[scroggo, 2015/04/02 19:20:31]

This seems fragile. You are setting a pointer field to a local variable, which
will no longer exist when the function returns.

As it happens, we *only* access fColorTable before this function ends (from
decodeMask, decodeRLE, or decode), but if anything were to change, we might
start dereferencing a pointer to arbitrary data.

[msarett, 2015/04/03 18:01:32]

Yeah that concerned me as well.  I think the new approach of using SkColorTable
and copying is better.  I have made this change here and in png.

+            inputColorCount, alternateColorPtr);
+
     // Create the color table if necessary and prepare the stream for decode
     if (!createColorTable(dstInfo.alphaType())) {
         SkCodecPrintf("Error: could not create color table.\n");
@@ -566,7 +593,7 @@ SkCodec::Result SkBmpCodec::onGetPixels(const SkImageInfo& dstInfo,
         case kBitMask_BitmapInputFormat:
             return decodeMask(dstInfo, dst, dstRowBytes);
         case kRLE_BitmapInputFormat:
-            return decodeRLE(dstInfo, dst, dstRowBytes);
+            return decodeRLE(dstInfo, dst, dstRowBytes, opts);
         case kStandard_BitmapInputFormat:
             return decode(dstInfo, dst, dstRowBytes);
         default:
@@ -584,7 +611,6 @@ SkCodec::Result SkBmpCodec::onGetPixels(const SkImageInfo& dstInfo,
     // Allocate memory for color table
     uint32_t colorBytes = 0;
     uint32_t maxColors = 0;
-    SkPMColor colorTable[256];
     if (fBitsPerPixel <= 8) {
         // Zero is a default for maxColors
         // Also set fNumColors to maxColors when it is too large
@@ -629,14 +655,14 @@ SkCodec::Result SkBmpCodec::onGetPixels(const SkImageInfo& dstInfo,
             uint8_t alpha = kOpaque_SkAlphaType == alphaType ? 0xFF :
                     (fMasks->getAlphaMask() >> 24) &
                     get_byte(cBuffer.get(), i*fBytesPerColor + 3);
-            colorTable[i] = packARGB(alpha, red, green, blue);
+            fColorTable[i] = packARGB(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);
+            fColorTable[i] = SkPackARGB32NoCheck(0xFF, 0, 0, 0);
         }
     }
 
@@ -661,8 +687,7 @@ SkCodec::Result SkBmpCodec::onGetPixels(const SkImageInfo& dstInfo,
         }
     }
 
-    // Set the color table and return true on success
-    fColorTable.reset(SkNEW_ARGS(SkColorTable, (colorTable, maxColors)));
+    // Return true on success
     return true;
 }
 
@@ -734,7 +759,7 @@ SkCodec::Result SkBmpCodec::decodeMask(const SkImageInfo& dstInfo,
  * Set an RLE pixel using the color table
  *
  */
-void SkBmpCodec::setRLEPixel(SkPMColor* dst, size_t dstRowBytes,
+void SkBmpCodec::setRLEPixel(void* dst, size_t dstRowBytes,
                              const SkImageInfo& dstInfo, uint32_t x, uint32_t y,
                              uint8_t index) {
     // Set the row
@@ -749,15 +774,9 @@ void SkBmpCodec::setRLEPixel(SkPMColor* dst, size_t dstRowBytes,
     // Set the pixel based on destination color type
     switch (dstInfo.colorType()) {
         case kN32_SkColorType: {
-            SkPMColor* dstRow = SkTAddOffset<SkPMColor>(dst,
+            SkPMColor* dstRow = SkTAddOffset<SkPMColor>((SkPMColor*) dst,
                     row * (int) dstRowBytes);
-            dstRow[x] = fColorTable->operator[](index);
-            break;
-        }
-        case kRGB_565_SkColorType: {
-            uint16_t* dstRow = SkTAddOffset<uint16_t>(dst,
-                    row * (int) dstRowBytes);
-            dstRow[x] = SkPixel32ToPixel16(fColorTable->operator[](index));
+            dstRow[x] = fColorTable[index];
             break;
         }
         default:
@@ -773,7 +792,7 @@ void SkBmpCodec::setRLEPixel(SkPMColor* dst, size_t dstRowBytes,
  * Set an RLE pixel from R, G, B values
  *
  */
-void SkBmpCodec::setRLE24Pixel(SkPMColor* dst, size_t dstRowBytes,
+void SkBmpCodec::setRLE24Pixel(void* dst, size_t dstRowBytes,
                                const SkImageInfo& dstInfo, uint32_t x,
                                uint32_t y, uint8_t red, uint8_t green,
                                uint8_t blue) {
@@ -789,17 +808,11 @@ void SkBmpCodec::setRLE24Pixel(SkPMColor* dst, size_t dstRowBytes,
     // Set the pixel based on destination color type
     switch (dstInfo.colorType()) {
         case kN32_SkColorType: {
-            SkPMColor* dstRow = SkTAddOffset<SkPMColor>(dst,
+            SkPMColor* dstRow = SkTAddOffset<SkPMColor>((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.
@@ -815,7 +828,8 @@ void SkBmpCodec::setRLE24Pixel(SkPMColor* dst, size_t dstRowBytes,
  *
  */
 SkCodec::Result SkBmpCodec::decodeRLE(const SkImageInfo& dstInfo,
-                                      void* dst, size_t dstRowBytes) {
+                                      void* dst, size_t dstRowBytes,
+                                      const Options& opts) {
     // Set RLE flags
     static const uint8_t RLE_ESCAPE = 0;
     static const uint8_t RLE_EOL = 0;
@@ -840,10 +854,15 @@ SkCodec::Result SkBmpCodec::decodeRLE(const SkImageInfo& dstInfo,
     // Destination parameters
     int x = 0;
     int 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);
-    SkPMColor* dstPtr = (SkPMColor*) dst;
+
+    // Set the background as transparent.  Then, if the RLE code skips pixels,
+    // the skipped pixels will be transparent.
+    // Because of the need for transparent pixels, kN32 is the only color
+    // type that makes sense for the destination format.
+    SkASSERT(kN32_SkColorType == dstInfo.colorType());
+    if (kYes_ZeroInitialized == opts.fZeroInitialized) {

[scroggo, 2015/04/02 19:20:30]

kNo_ZeroInitialized.

If it's already zero initialized, you do not need to do this, but if it is, then
you do.

[msarett, 2015/04/03 18:01:32]

Of course!

+        memset(dst, 0, dstRowBytes * height);

[scroggo, 2015/04/02 19:20:30]

Ooh, I missed this on an earlier review.

We allow clients to provide smaller allocations than dstRowBytes * height -
technically, for the last row, you only need width * bytesPerPixel for the last
line. This can allow the client to save a little bit of memory.

We already have a function that calculates this: SkImageInfo::getSafeSize

[msarett, 2015/04/03 18:01:32]

Good to know!  I don't think this is the only place that I have made this
mistake.

+    }
 
     while (true) {
         // Every entry takes at least two bytes
@@ -914,11 +933,11 @@ SkCodec::Result SkBmpCodec::decodeRLE(const SkImageInfo& dstInfo,
                             case 4: {
                                 SkASSERT(currByte < totalBytes);
                                 uint8_t val = buffer.get()[currByte++];
-                                setRLEPixel(dstPtr, dstRowBytes, dstInfo, x++,
+                                setRLEPixel(dst, dstRowBytes, dstInfo, x++,
                                         y, val >> 4);
                                 numPixels--;
                                 if (numPixels != 0) {
-                                    setRLEPixel(dstPtr, dstRowBytes, dstInfo,
+                                    setRLEPixel(dst, dstRowBytes, dstInfo,
                                             x++, y, val & 0xF);
                                     numPixels--;
                                 }
@@ -926,7 +945,7 @@ SkCodec::Result SkBmpCodec::decodeRLE(const SkImageInfo& dstInfo,
                             }
                             case 8:
                                 SkASSERT(currByte < totalBytes);
-                                setRLEPixel(dstPtr, dstRowBytes, dstInfo, x++,
+                                setRLEPixel(dst, dstRowBytes, dstInfo, x++,
                                         y, buffer.get()[currByte++]);
                                 numPixels--;
                                 break;
@@ -935,7 +954,7 @@ SkCodec::Result SkBmpCodec::decodeRLE(const SkImageInfo& dstInfo,
                                 uint8_t blue = buffer.get()[currByte++];
                                 uint8_t green = buffer.get()[currByte++];
                                 uint8_t red = buffer.get()[currByte++];
-                                setRLE24Pixel(dstPtr, dstRowBytes, dstInfo,
+                                setRLE24Pixel(dst, dstRowBytes, dstInfo,
                                             x++, y, red, green, blue);
                                 numPixels--;
                             }
@@ -971,7 +990,7 @@ SkCodec::Result SkBmpCodec::decodeRLE(const SkImageInfo& dstInfo,
                 uint8_t green = buffer.get()[currByte++];
                 uint8_t red = buffer.get()[currByte++];
                 while (x < endX) {
-                    setRLE24Pixel(dstPtr, dstRowBytes, dstInfo, x++, y, red,
+                    setRLE24Pixel(dst, dstRowBytes, dstInfo, x++, y, red,
                             green, blue);
                 }
             } else {
@@ -988,7 +1007,7 @@ SkCodec::Result SkBmpCodec::decodeRLE(const SkImageInfo& dstInfo,
 
                 // Set the indicated number of pixels
                 for (int which = 0; x < endX; x++) {
-                    setRLEPixel(dstPtr, dstRowBytes, dstInfo, x, y,
+                    setRLEPixel(dst, dstRowBytes, dstInfo, x, y,
                             indices[which]);
                     which = !which;
                 }
@@ -1041,7 +1060,7 @@ SkCodec::Result SkBmpCodec::decode(const SkImageInfo& dstInfo,
 
     // Create swizzler
     SkAutoTDelete<SkSwizzler> swizzler(SkSwizzler::CreateSwizzler(config,
-            fColorTable->readColors(), dstInfo, dst, dstRowBytes,
+            fColorTable, dstInfo, dst, dstRowBytes,
             SkImageGenerator::kNo_ZeroInitialized));
 
     // Allocate space for a row buffer and a source for the swizzler
@@ -1078,7 +1097,8 @@ SkCodec::Result SkBmpCodec::decode(const SkImageInfo& dstInfo,
     // 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.
+    // bmp images are intended to be opaque.  Here, we make those corrections
+    // in the kN32 case.
     /*
     SkPMColor* dstRow = (SkPMColor*) dst;
     if (SkSwizzler::kBGRA == config) {