SkJpegCodec

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 67be0dbe2557678da66ac71df29a4c0f83e1cadf..dda2af9f51539c63134251942a5ecbad361a8106 100644
--- a/src/codec/SkCodec_libbmp.cpp
+++ b/src/codec/SkCodec_libbmp.cpp
@@ -97,7 +97,6 @@ enum BitmapCompressionMethod {
  */
 bool SkBmpCodec::IsBmp(SkStream* stream) {
     // TODO: Support "IC", "PT", "CI", "CP", "BA"
-    // TODO: ICO files may contain a BMP and need to use this decoder
     const char bmpSig[] = { 'B', 'M' };
     char buffer[sizeof(bmpSig)];
     return stream->read(buffer, sizeof(bmpSig)) == sizeof(bmpSig) &&
@@ -773,12 +772,12 @@ SkCodec::Result SkBmpCodec::decodeMask(const SkImageInfo& dstInfo,
         if (stream()->read(srcRow, rowBytes) != rowBytes) {
             SkCodecPrintf("Warning: incomplete input stream.\n");
             // Fill the destination image on failure
-            // By using zero as the fill value, we will fill with transparent
-            // pixels for non-opaque images and white for opaque images.
-            // These are arbitrary choices but allow for consistent behavior.
+            SkPMColor fillColor = dstInfo.alphaType() == kOpaque_SkAlphaType ?
+                    SK_ColorBLACK : SK_ColorTRANSPARENT;
             if (kNo_ZeroInitialized == opts.fZeroInitialized) {
                 void* dstStart = get_dst_start_row(dst, dstRowBytes, y, fRowOrder);
-                SkSwizzler::Fill(dstStart, dstInfo, dstRowBytes, dstInfo.height() - y, 0, NULL);
+                SkSwizzler::Fill(dstStart, dstInfo, dstRowBytes, dstInfo.height() - y, fillColor,
+                        NULL);
             }
             return kIncompleteInput;
         }
@@ -1090,29 +1089,39 @@ SkCodec::Result SkBmpCodec::decode(const SkImageInfo& dstInfo,
     const int height = dstInfo.height();
     const size_t rowBytes = SkAlign4(compute_row_bytes(width, fBitsPerPixel));
 
-    // Get swizzler configuration
+    // Get swizzler configuration and choose the fill value for failures.  We will use
+    // zero as the default palette index, black for opaque images, and transparent for
+    // non-opaque images.
     SkSwizzler::SrcConfig config;
+    uint32_t fillColorOrIndex;
     switch (fBitsPerPixel) {
         case 1:
             config = SkSwizzler::kIndex1;
+            fillColorOrIndex = 0;
             break;
         case 2:
             config = SkSwizzler::kIndex2;
+            fillColorOrIndex = 0;
             break;
         case 4:
             config = SkSwizzler::kIndex4;
+            fillColorOrIndex = 0;
             break;
         case 8:
             config = SkSwizzler::kIndex;
+            fillColorOrIndex = 0;
             break;
         case 24:
             config = SkSwizzler::kBGR;
+            fillColorOrIndex = SK_ColorBLACK;
             break;
         case 32:
             if (kOpaque_SkAlphaType == dstInfo.alphaType()) {
                 config = SkSwizzler::kBGRX;
+                fillColorOrIndex = SK_ColorBLACK;
             } else {
                 config = SkSwizzler::kBGRA;
+                fillColorOrIndex = SK_ColorTRANSPARENT;
             }
             break;
         default:
@@ -1138,14 +1147,10 @@ SkCodec::Result SkBmpCodec::decode(const SkImageInfo& dstInfo,
         if (stream()->read(srcBuffer.get(), rowBytes) != rowBytes) {
             SkCodecPrintf("Warning: incomplete input stream.\n");
             // Fill the destination image on failure
-            // By using zero as the fill value, we will fill with the first
-            // color in the color table for palette images, transparent
-            // pixels for non-opaque images, and white for opaque images.
-            // These are arbitrary choices but allow for consistent behavior.
             if (kNo_ZeroInitialized == opts.fZeroInitialized) {

[scroggo, 2015/04/14 13:10:32]

Since fillColorOrIndex may not be 0, being zero initialized does not necessarily
mean we can skip this.

We either need to do it all the time, or we need to make extra checks to know
whether we can skip (maybe included in the switch statement above).

[msarett, 2015/04/14 19:30:36]

Yes you are right!

                 void* dstStart = get_dst_start_row(dst, dstRowBytes, y, fRowOrder);
-                SkSwizzler::Fill(dstStart, dstInfo, dstRowBytes, dstInfo.height() - y, 0,
-                        colorPtr);
+                SkSwizzler::Fill(dstStart, dstInfo, dstRowBytes, dstInfo.height() - y,
+                        fillColorOrIndex, colorPtr);
             }
             return kIncompleteInput;
         }