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..1877d78c26944312a78cf60d712d67c91063b604 100644
--- a/src/codec/SkCodec_libbmp.cpp
+++ b/src/codec/SkCodec_libbmp.cpp
@@ -9,6 +9,7 @@
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
 #include "SkStream.h"
+#include "SkUtils.h"
 
 /*
  *
@@ -23,12 +24,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())) {

[scroggo, 2015/04/06 14:55:11]

I think this is not quite the ! of the original statement, but it is intended to
be, correct?

The prior patch looks like it says:

A || (B && C)

and the new patch says

!A && (!B || C)

I think that

!(A || (B && C))

should be

!A && (!B || !C)

That said, given that we have to think so hard about this multi-conditional
statement, I think we can make this easier to read:

if (src.alphaType() != dst.alphaType()) {
  if (kOpaque_SkAlpha == src.alphaType()) {
    // If the source is opaque, we cannot decode to anything but opaque
    return false;
  }
  // src is opaque.
  switch (dst.alphaType()) {
    case kPremul:
    case kUnpremul:
      // The source is not opaque, so either of these is okay.
      break;
    default:
      // We cannot decode a non-opaque image to opaque (or unknown)
      return false;
  }
}

The switch statement could be an if, and technically you could only check for
premul, since we know the source must be unpremul if it's not opaque. If we
decide to make all our codecs claim their non-opaque images are premul (which is
not correct, but maybe more useful for callers that try to use the default
info), we'd have to go back and change this code if you only check the dst for
unpremul. To catch that, we could assert that src.alphaType() is unpremul.

[msarett, 2015/04/06 18:10:55]

Yeah that statement was confusing and wrong.  Your suggestion is much better.

+        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 +429,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 +454,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
+            // 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 +498,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 +557,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) {
@@ -556,17 +579,26 @@ SkCodec::Result SkBmpCodec::onGetPixels(const SkImageInfo& dstInfo,
     }
 
     // Create the color table if necessary and prepare the stream for decode
-    if (!createColorTable(dstInfo.alphaType())) {
+    // Note that if it is non-NULL, inputColorCount will be modified
+    if (!createColorTable(dstInfo.alphaType(), inputColorCount)) {
         SkCodecPrintf("Error: could not create color table.\n");
         return kInvalidInput;
     }
 
+    // Copy the color table to the client if necessary
+    if (kIndex_8_SkColorType == dstInfo.colorType()) {
+        SkASSERT(NULL != inputColorPtr);
+        SkASSERT(NULL != inputColorCount);
+        SkASSERT(NULL != fColorTable.get());
+        sk_memcpy32(inputColorPtr, fColorTable->readColors(), *inputColorCount);
+    }
+
     // 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:
@@ -580,7 +612,7 @@ SkCodec::Result SkBmpCodec::onGetPixels(const SkImageInfo& dstInfo,
  * Process the color table for the bmp input
  *
  */
- bool SkBmpCodec::createColorTable(SkAlphaType alphaType) {
+ bool SkBmpCodec::createColorTable(SkAlphaType alphaType, int* numColors) {
     // Allocate memory for color table
     uint32_t colorBytes = 0;
     uint32_t maxColors = 0;
@@ -593,6 +625,12 @@ SkCodec::Result SkBmpCodec::onGetPixels(const SkImageInfo& dstInfo,
             fNumColors = maxColors;
         }
 
+        // Inform the caller of the number of colors
+        if (NULL != numColors) {
+            SkASSERT(256 == *numColors);
+            *numColors = maxColors;

[scroggo, 2015/04/06 14:55:11]

Maybe add a note that we set it to maxColors in case the image file has errors
(i.e. fNumColors was wrong).

[msarett, 2015/04/06 18:10:55]

Done.

+        }
+
         // Read the color table from the stream
         colorBytes = fNumColors * fBytesPerColor;
         SkAutoTDeleteArray<uint8_t> cBuffer(SkNEW_ARRAY(uint8_t, colorBytes));
@@ -734,7 +772,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,17 +787,11 @@ 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));
-            break;
-        }
         default:
             // This case should not be reached.  We should catch an invalid
             // color type when we check that the conversion is possible.
@@ -773,7 +805,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 +821,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 +841,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 +867,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 (kNo_ZeroInitialized == opts.fZeroInitialized) {
+        SkSwizzler::Fill(dst, dstInfo, dstRowBytes, 0, SK_ColorTRANSPARENT, NULL);
+    }
 
     while (true) {
         // Every entry takes at least two bytes
@@ -914,11 +946,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 +958,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 +967,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 +1003,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 +1020,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;
                 }
@@ -1078,7 +1110,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) {