Swizzler changes Index8 and 565

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) {
     // Ensure that the profile type is unchanged
     if (dst.profileType() != src.profileType()) {
         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;
     }
 }
 
 /*
  *
  * Defines the version and type of the second bitmap header
  *
  */
@@ skipping 370 matching lines @@
         // Header types are matched based on size.  If the header is
         // V3+, we are guaranteed to be able to read at least this size.
         SkASSERT(infoBytesRemaining > 52);
         inputMasks.alpha = get_int(iBuffer.get(), 48);
         if (inputMasks.alpha != 0) {
             alphaType = kUnpremul_SkAlphaType;
         }
     }
     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
         // 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) {
                 inputMasks.red = 0x7C00;
                 inputMasks.green = 0x03E0;
                 inputMasks.blue = 0x001F;
                 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;
         default:
             SkCodecPrintf("Error: invalid input value for bits per pixel.\n");
             return false;
     }
 
     // Check that input bit masks are valid and create the masks object
     SkAutoTDelete<SkMasks>
@@ skipping 12 matching lines @@
 
     // Calculate the number of bytes read so far
     const uint32_t bytesRead = kBmpHeaderBytes + infoBytes + maskBytes;
     if (!isIco && offset < bytesRead) {
         SkCodecPrintf("Error: pixel data offset less than header size.\n");
         return false;
     }
 
     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,
                                             offset - bytesRead, rowOrder,
                                             RLEBytes, isIco));
     }
     return true;
 }
 
 /*
@@ skipping 35 matching lines @@
 
 {}
 
 /*
  *
  * Initiates the bitmap decode
  *
  */
 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) {
         return kCouldNotRewind;
     } else if (rewindState == kRewound_RewindState) {
         if (!ReadHeader(this->stream(), fIsIco, NULL)) {
             return kCouldNotRewind;
         }
     }
     if (dstInfo.dimensions() != this->getInfo().dimensions()) {
         SkCodecPrintf("Error: scaling not supported.\n");
         return kInvalidScale;
     }
     if (!conversion_possible(dstInfo, this->getInfo())) {
         SkCodecPrintf("Error: cannot convert input type to output type.\n");
         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");
         return kInvalidInput;
     }
 
     // Perform the decode
     switch (fInputFormat) {
         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:
             SkASSERT(false);
             return kInvalidInput;
     }
 }
 
 /*
  *
  * Process the color table for the bmp input
  *
  */
  bool SkBmpCodec::createColorTable(SkAlphaType alphaType) {
     // 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
         maxColors = 1 << fBitsPerPixel;
         if (fNumColors == 0 || fNumColors >= maxColors) {
             fNumColors = maxColors;
         }
 
         // Read the color table from the stream
         colorBytes = fNumColors * fBytesPerColor;
@@ skipping 24 matching lines @@
 
         // 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 = 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);
         }
     }
 
     // Bmp-in-Ico files do not use an offset to indicate where the pixel data
     // begins.  Pixel data always begins immediately after the color table.
     if (!fIsIco) {
         // Check that we have not read past the pixel array offset
         if(fOffset < colorBytes) {
             // 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.
             SkCodecPrintf("Error: pixel data offset less than color table size.\n");
             return false;
         }
 
         // After reading the color table, skip to the start of the pixel array
         if (stream()->skip(fOffset - colorBytes) != fOffset - colorBytes) {
             SkCodecPrintf("Error: unable to skip to image data.\n");
             return false;
         }
     }
 
-    // Set the color table and return true on success
-    fColorTable.reset(SkNEW_ARGS(SkColorTable, (colorTable, maxColors)));
+    // Return true on success
     return true;
 }
 
 /*
  *
  * Performs the bitmap decoding for bit masks input format
  *
  */
 SkCodec::Result SkBmpCodec::decodeMask(const SkImageInfo& dstInfo,
                                        void* dst, size_t dstRowBytes) {
@@ skipping 51 matching lines @@
 
     // Finished decoding the entire image
     return kSuccess;
 }
 
 /*
  *
  * 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
     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,
+            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:
             // 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,
+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) {
     // 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,
+            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.
             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) {
+                                      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;
     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, fRLEBytes));
     size_t totalBytes = stream()->read(buffer.get(), fRLEBytes);
     if (totalBytes < fRLEBytes) {
         SkCodecPrintf("Warning: incomplete RLE file.\n");
     } else if (totalBytes <= 0) {
         SkCodecPrintf("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
-    // 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
         if ((int) totalBytes - currByte < 2) {
             SkCodecPrintf("Warning: incomplete RLE input.\n");
             return kIncompleteInput;
         }
 
         // Read the next two bytes.  These bytes have different meanings
         // depending on their values.  In the first interpretation, the first
@@ skipping 50 matching lines @@
                             (int) totalBytes - currByte < SkAlign2(rowBytes)) {
                         SkCodecPrintf("Warning: invalid RLE input.\n");
                         return kIncompleteInput;
                     }
                     // Set numPixels number of pixels
                     while (numPixels > 0) {
                         switch(fBitsPerPixel) {
                             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--;
                                 }
                                 break;
                             }
                             case 8:
                                 SkASSERT(currByte < totalBytes);
-                                setRLEPixel(dstPtr, dstRowBytes, dstInfo, x++,
+                                setRLEPixel(dst, 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++];
-                                setRLE24Pixel(dstPtr, dstRowBytes, dstInfo,
+                                setRLE24Pixel(dst, dstRowBytes, dstInfo,
                                             x++, y, red, green, blue);
                                 numPixels--;
                             }
                             default:
                                 SkASSERT(false);
                                 return kInvalidInput;
                         }
                     }
                     // Skip a byte if necessary to maintain alignment
                     if (!SkIsAlign2(rowBytes)) {
@@ skipping 15 matching lines @@
                 if ((int) totalBytes - currByte < 2) {
                     SkCodecPrintf("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++];
                 while (x < endX) {
-                    setRLE24Pixel(dstPtr, dstRowBytes, dstInfo, x++, y, red,
+                    setRLE24Pixel(dst, 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, dstInfo, x, y,
+                    setRLEPixel(dst, dstRowBytes, dstInfo, x, y,
                             indices[which]);
                     which = !which;
                 }
             }
         }
     }
 }
 
 /*
  *
@@ skipping 32 matching lines @@
                 config = SkSwizzler::kBGRA;
             }
             break;
         default:
             SkASSERT(false);
             return kInvalidInput;
     }
 
     // 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
     SkAutoTDeleteArray<uint8_t> srcBuffer(SkNEW_ARRAY(uint8_t, rowBytes));
 
     // Iterate over rows of the image
     // FIXME: bool transparent = true;
     for (int y = 0; y < height; y++) {
         // Read a row of the input
         if (stream()->read(srcBuffer.get(), rowBytes) != rowBytes) {
@@ skipping 16 matching lines @@
     }
 
     // FIXME: This code exists to match the behavior in the chromium decoder
     // and to follow the bmp specification as it relates to alpha masks.  It is
     // commented out because we have yet to discover a test image that provides
     // an alpha mask and uses this decode mode.
 
     // 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) {
         for (int y = 0; y < height; y++) {
             for (int x = 0; x < width; x++) {
                 if (transparent) {
                     dstRow[x] |= 0xFF000000;
                 } else {
                     dstRow[x] &= alphaMask;
                 }
@@ skipping 34 matching lines @@
                 uint32_t alphaBit =
                         (srcBuffer.get()[quotient] >> shift) & 0x1;
                 dstRow[x] &= alphaBit - 1;
             }
         }
     }
 
     // Finished decoding the entire image
     return kSuccess;
 }