Implementing filling for SkBmpCodec
authormsarett <msarett@google.com>
Fri, 10 Apr 2015 21:36:48 +0000 (14:36 -0700)
committerCommit bot <commit-bot@chromium.org>
Fri, 10 Apr 2015 21:36:49 +0000 (14:36 -0700)
The bmp codec currently returns kIncompleteInput
when the stream is truncated, which we treat as a
partial success.  However, we neglect the fill the
remaining pixels in the image, leaving these
uninitialized.

This CL addresses this problem by initializing the
remaining pixels in the image to default values.

BUG=skia:3257

Review URL: https://codereview.chromium.org/1075243003

src/codec/SkCodec_libbmp.cpp
src/codec/SkCodec_libbmp.h
src/codec/SkCodec_libgif.cpp
src/codec/SkSwizzler.cpp
src/codec/SkSwizzler.h
tests/SwizzlerTest.cpp
tools/dm_flags.py

index b553dea..67be0db 100644 (file)
@@ -614,11 +614,11 @@ SkCodec::Result SkBmpCodec::onGetPixels(const SkImageInfo& dstInfo,
     // Perform the decode
     switch (fInputFormat) {
         case kBitMask_BitmapInputFormat:
-            return decodeMask(dstInfo, dst, dstRowBytes);
+            return decodeMask(dstInfo, dst, dstRowBytes, opts);
         case kRLE_BitmapInputFormat:
             return decodeRLE(dstInfo, dst, dstRowBytes, opts);
         case kStandard_BitmapInputFormat:
-            return decode(dstInfo, dst, dstRowBytes);
+            return decode(dstInfo, dst, dstRowBytes, opts);
         default:
             SkASSERT(false);
             return kInvalidInput;
@@ -701,6 +701,9 @@ SkCodec::Result SkBmpCodec::onGetPixels(const SkImageInfo& dstInfo,
         for (; i < maxColors; i++) {
             colorTable[i] = SkPackARGB32NoCheck(0xFF, 0, 0, 0);
         }
+
+        // Set the color table
+        fColorTable.reset(SkNEW_ARGS(SkColorTable, (colorTable, maxColors)));
     }
 
     // Bmp-in-Ico files do not use an offset to indicate where the pixel data
@@ -724,18 +727,30 @@ 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;
 }
 
 /*
  *
+ * Get the destination row to start filling from
+ * Used to fill the remainder of the image on incomplete input
+ *
+ */
+static inline void* get_dst_start_row(void* dst, size_t dstRowBytes, int32_t y,
+            SkBmpCodec::RowOrder rowOrder) {
+    return (SkBmpCodec::kTopDown_RowOrder == rowOrder) ?
+            SkTAddOffset<void*>(dst, y * dstRowBytes) : dst;
+}
+
+/*
+ *
  * Performs the bitmap decoding for bit masks input format
  *
  */
 SkCodec::Result SkBmpCodec::decodeMask(const SkImageInfo& dstInfo,
-                                       void* dst, size_t dstRowBytes) {
+                                       void* dst, size_t dstRowBytes,
+                                       const Options& opts) {
     // Set constant values
     const int width = dstInfo.width();
     const int height = dstInfo.height();
@@ -757,6 +772,14 @@ SkCodec::Result SkBmpCodec::decodeMask(const SkImageInfo& dstInfo,
         // Read a row of the input
         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.
+            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);
+            }
             return kIncompleteInput;
         }
 
@@ -899,7 +922,7 @@ SkCodec::Result SkBmpCodec::decodeRLE(const SkImageInfo& dstInfo,
     // 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);
+        SkSwizzler::Fill(dst, dstInfo, dstRowBytes, height, SK_ColorTRANSPARENT, NULL);
     }
 
     while (true) {
@@ -1060,7 +1083,8 @@ SkCodec::Result SkBmpCodec::decodeRLE(const SkImageInfo& dstInfo,
  *
  */
 SkCodec::Result SkBmpCodec::decode(const SkImageInfo& dstInfo,
-                                   void* dst, size_t dstRowBytes) {
+                                   void* dst, size_t dstRowBytes,
+                                   const Options& opts) {
     // Set constant values
     const int width = dstInfo.width();
     const int height = dstInfo.height();
@@ -1096,9 +1120,12 @@ SkCodec::Result SkBmpCodec::decode(const SkImageInfo& dstInfo,
             return kInvalidInput;
     }
 
+    // Get a pointer to the color table if it exists
+    const SkPMColor* colorPtr = NULL != fColorTable.get() ? fColorTable->readColors() : NULL;
+
     // Create swizzler
     SkAutoTDelete<SkSwizzler> swizzler(SkSwizzler::CreateSwizzler(config,
-            fColorTable->readColors(), dstInfo, dst, dstRowBytes,
+            colorPtr, dstInfo, dst, dstRowBytes,
             SkImageGenerator::kNo_ZeroInitialized));
 
     // Allocate space for a row buffer and a source for the swizzler
@@ -1110,6 +1137,16 @@ SkCodec::Result SkBmpCodec::decode(const SkImageInfo& dstInfo,
         // Read a row of the input
         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) {
+                void* dstStart = get_dst_start_row(dst, dstRowBytes, y, fRowOrder);
+                SkSwizzler::Fill(dstStart, dstInfo, dstRowBytes, dstInfo.height() - y, 0,
+                        colorPtr);
+            }
             return kIncompleteInput;
         }
 
index 8ad613e..a31256c 100644 (file)
@@ -114,7 +114,7 @@ private:
      *
      */
     Result decodeMask(const SkImageInfo& dstInfo, void* dst,
-                      size_t dstRowBytes);
+                      size_t dstRowBytes, const Options& opts);
 
     /*
      *
@@ -146,7 +146,7 @@ private:
      * Performs the bitmap decoding for standard input format
      *
      */
-    Result decode(const SkImageInfo& dstInfo, void* dst, size_t dstRowBytes);
+    Result decode(const SkImageInfo& dstInfo, void* dst, size_t dstRowBytes, const Options& opts);
 
     /*
      *
index efb3a90..bfa6e03 100644 (file)
@@ -425,7 +425,7 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
                     //        animated gifs where we draw on top of the
                     //        previous frame.
                     if (!skipBackground) {
-                        SkSwizzler::Fill(dst, dstInfo, dstRowBytes, 0, fillIndex, colorTable);
+                        SkSwizzler::Fill(dst, dstInfo, dstRowBytes, height, fillIndex, colorTable);
                     }
 
                     // Modify the dst pointer
@@ -478,8 +478,8 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
                         if (GIF_ERROR == DGifGetLine(fGif, buffer.get(),
                                 innerWidth)) {
                             if (!skipBackground) {
-                                SkSwizzler::Fill(dst, dstInfo, dstRowBytes, y, fillIndex,
-                                        colorTable);
+                                SkSwizzler::Fill(swizzler->getDstRow(), dstInfo, dstRowBytes,
+                                        innerHeight - y, fillIndex, colorTable);
                             }
                             return gif_error(SkStringPrintf(
                                     "Could not decode line %d of %d.\n",
index 754b088..7913633 100644 (file)
@@ -422,17 +422,13 @@ SkSwizzler::ResultAlpha SkSwizzler::next(const uint8_t* SK_RESTRICT src,
             fColorTable);
 }
 
-void SkSwizzler::Fill(void* dst, const SkImageInfo& dstInfo, size_t dstRowBytes, uint32_t y,
-        uint32_t colorOrIndex, SkPMColor* colorTable) {
-    SkASSERT(dst != NULL);
-    SkASSERT(y < (uint32_t) dstInfo.height());
+void SkSwizzler::Fill(void* dstStartRow, const SkImageInfo& dstInfo, size_t dstRowBytes,
+        uint32_t numRows, uint32_t colorOrIndex, const SkPMColor* colorTable) {
+    SkASSERT(dstStartRow != NULL);
+    SkASSERT(numRows <= (uint32_t) dstInfo.height());
 
-    // Get dst start row
-    void* dstRow = SkTAddOffset<void*>(dst, y * dstRowBytes);
-
-    // Calculate remaining bytes.  This is tricky since the final row may not be padded.
-    const size_t totalBytes = dstInfo.getSafeSize(dstRowBytes);
-    const size_t remainingBytes = totalBytes - y * dstRowBytes;
+    // Calculate bytes to fill.  We use getSafeSize since the last row may not be padded.
+    const size_t bytesToFill = dstInfo.makeWH(dstInfo.width(), numRows).getSafeSize(dstRowBytes);
 
     // Use the proper memset routine to fill the remaining bytes
     switch(dstInfo.colorType()) {
@@ -448,25 +444,25 @@ void SkSwizzler::Fill(void* dst, const SkImageInfo& dstInfo, size_t dstRowBytes,
             }
 
             // We must fill row by row in the case of unaligned row bytes
-            if (SkIsAlign4((size_t) dstRow) && SkIsAlign4(dstRowBytes)) {
-                sk_memset32((uint32_t*) dstRow, color,
-                        (uint32_t) remainingBytes / sizeof(SkPMColor));
+            if (SkIsAlign4((size_t) dstStartRow) && SkIsAlign4(dstRowBytes)) {
+                sk_memset32((uint32_t*) dstStartRow, color,
+                        (uint32_t) bytesToFill / sizeof(SkPMColor));
             } else {
                 // This is an unlikely, slow case
                 SkCodecPrintf("Warning: Strange number of row bytes, fill will be slow.\n");
-                for (int32_t row = y; row < dstInfo.height(); row++) {
-                    uint32_t* dstPtr = (uint32_t*) dstRow;
+                uint32_t* dstRow = (uint32_t*) dstStartRow;
+                for (uint32_t row = 0; row < numRows; row++) {
                     for (int32_t col = 0; col < dstInfo.width(); col++) {
-                        dstPtr[col] = color;
+                        dstRow[col] = color;
                     }
-                    dstRow = SkTAddOffset<void*>(dstRow, dstRowBytes);
+                    dstRow = SkTAddOffset<uint32_t>(dstRow, dstRowBytes);
                 }
             }
             break;
         // On an index destination color type, always assume the input is an index
         case kIndex_8_SkColorType:
             SkASSERT(colorOrIndex == (uint8_t) colorOrIndex);
-            memset(dstRow, colorOrIndex, remainingBytes);
+            memset(dstStartRow, colorOrIndex, bytesToFill);
             break;
         default:
             SkCodecPrintf("Error: Unsupported dst color type for fill().  Doing nothing.\n");
index 6044c86..8a471e9 100644 (file)
@@ -132,8 +132,11 @@ public:
     /**
      * Fill the remainder of the destination with a single color
      *
-     * @param y
-     * The starting row for the fill.
+     * @param dstStartRow
+     * The destination row to fill from.
+     *
+     * @param numRows
+     * The number of rows to fill.
      *
      * @param colorOrIndex
      * @param colorTable
@@ -155,8 +158,8 @@ public:
      * Other SkColorTypes are not supported.
      *
      */
-    static void Fill(void* dst, const SkImageInfo& dstInfo, size_t dstRowBytes, uint32_t y,
-            uint32_t colorOrIndex, SkPMColor* colorTable);
+    static void Fill(void* dstStartRow, const SkImageInfo& dstInfo, size_t dstRowBytes,
+            uint32_t numRows, uint32_t colorOrIndex, const SkPMColor* colorTable);
 
     /**
      *  Swizzle the next line. Call height times, once for each row of source.
@@ -187,6 +190,17 @@ public:
      */
     void setDstRow(void* dst) { fDstRow = dst; }
 
+    /**
+     *  Get the next destination row to decode to
+     */
+    void* getDstRow() {
+        // kDesignateRow_NextMode does not update the fDstRow ptr.  This function is
+        // unnecessary in that case since fDstRow will always be equal to the pointer
+        // passed to CreateSwizzler().
+        SkASSERT(kDesignateRow_NextMode != fNextMode);
+        return fDstRow;
+    }
+
 private:
 
 #ifdef SK_DEBUG
index f02f86b..147dfaa 100644 (file)
@@ -15,6 +15,7 @@ static const uint32_t kFillColor = 0x22334455;
 static void check_fill(skiatest::Reporter* r,
                        const SkImageInfo& imageInfo,
                        uint32_t startRow,
+                       uint32_t endRow,
                        size_t rowBytes,
                        uint32_t offset,
                        uint32_t colorOrIndex,
@@ -32,15 +33,17 @@ static void check_fill(skiatest::Reporter* r,
     memset(storage.get(), 0, totalBytes);
     // Adjust the pointer in order to test on different memory alignments
     uint8_t* imageData = storage.get() + offset;
+    uint8_t* imageStart = imageData + rowBytes * startRow;
 
     // Fill image with the fill value starting at the indicated row
-    SkSwizzler::Fill(imageData, imageInfo, rowBytes, startRow, colorOrIndex, colorTable);
+    SkSwizzler::Fill(imageStart, imageInfo, rowBytes, endRow - startRow + 1, colorOrIndex,
+            colorTable);
 
     // Ensure that the pixels are filled properly
     // The bots should catch any memory corruption
     uint8_t* indexPtr = imageData + startRow * rowBytes;
     uint32_t* colorPtr = (uint32_t*) indexPtr;
-    for (int32_t y = startRow; y < imageInfo.height(); y++) {
+    for (uint32_t y = startRow; y <= endRow; y++) {
         for (int32_t x = 0; x < imageInfo.width(); x++) {
             if (kIndex_8_SkColorType == imageInfo.colorType()) {
                 REPORTER_ASSERT(r, kFillIndex == indexPtr[x]);
@@ -90,20 +93,22 @@ DEF_TEST(SwizzlerFill, r) {
                 // If there is padding, we can invent an offset to change the memory alignment
                 for (uint32_t offset = 0; offset <= padding; offset++) {
 
-                    // Test all possible start rows
+                    // Test all possible start rows with all possible end rows
                     for (uint32_t startRow = 0; startRow < height; startRow++) {
+                        for (uint32_t endRow = startRow; endRow < height; endRow++) {
 
-                        // Fill with an index that we use to look up a color
-                        check_fill(r, colorInfo, startRow, colorRowBytes, offset, kFillIndex,
-                               colorTable);
+                            // Fill with an index that we use to look up a color
+                            check_fill(r, colorInfo, startRow, endRow, colorRowBytes, offset,
+                                    kFillIndex, colorTable);
 
-                        // Fill with a color
-                        check_fill(r, colorInfo, startRow, colorRowBytes, offset, kFillColor,
-                                NULL);
+                            // Fill with a color
+                            check_fill(r, colorInfo, startRow, endRow, colorRowBytes, offset,
+                                    kFillColor, NULL);
 
-                        // Fill with an index
-                        check_fill(r, indexInfo, startRow, indexRowBytes, offset, kFillIndex,
-                                NULL);
+                            // Fill with an index
+                            check_fill(r, indexInfo, startRow, endRow, indexRowBytes, offset,
+                                    kFillIndex, NULL);
+                        }
                     }
                 }
             }
index 368d0a3..c3a44a9 100755 (executable)
@@ -73,9 +73,17 @@ def get_args(bot):
   blacklist.extend('_ image decode pal8oversizepal.bmp'.split(' '))
   blacklist.extend('_ image decode pal4rletrns.bmp'.split(' '))
   blacklist.extend('_ image decode pal8rletrns.bmp'.split(' '))
+  blacklist.extend('_ image decode 4bpp-pixeldata-cropped.bmp'.split(' '))
+  blacklist.extend('_ image decode 8bpp-pixeldata-cropped.bmp'.split(' '))
+  blacklist.extend('_ image decode 24bpp-pixeldata-cropped.bmp'.split(' '))
+  blacklist.extend('_ image decode 32bpp-pixeldata-cropped.bmp'.split(' '))
   blacklist.extend('_ image subset rgb24largepal.bmp'.split(' '))
   blacklist.extend('_ image subset pal8os2v2-16.bmp'.split(' '))
   blacklist.extend('_ image subset pal8oversizepal.bmp'.split(' '))
+  blacklist.extend('_ image subset 4bpp-pixeldata-cropped.bmp'.split(' '))
+  blacklist.extend('_ image subset 8bpp-pixeldata-cropped.bmp'.split(' '))
+  blacklist.extend('_ image subset 24bpp-pixeldata-cropped.bmp'.split(' '))
+  blacklist.extend('_ image subset 32bpp-pixeldata-cropped.bmp'.split(' '))
 
   # New ico files that fail on SkImageDecoder
   blacklist.extend('_ image decode Hopstarter-Mac-Folders-Apple.ico'.split(' '))