Implementing filling for SkBmpCodec
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
diff --git a/src/codec/SkCodec_libbmp.cpp b/src/codec/SkCodec_libbmp.cpp
index b553dea..67be0db 100644
--- a/src/codec/SkCodec_libbmp.cpp
+++ b/src/codec/SkCodec_libbmp.cpp
@@ -614,11 +614,11 @@
// 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 @@
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 @@
}
}
- // 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 @@
// 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 @@
// 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::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 @@
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 @@
// 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;
}
diff --git a/src/codec/SkCodec_libbmp.h b/src/codec/SkCodec_libbmp.h
index 8ad613e..a31256c 100644
--- a/src/codec/SkCodec_libbmp.h
+++ b/src/codec/SkCodec_libbmp.h
@@ -114,7 +114,7 @@
*
*/
Result decodeMask(const SkImageInfo& dstInfo, void* dst,
- size_t dstRowBytes);
+ size_t dstRowBytes, const Options& opts);
/*
*
@@ -146,7 +146,7 @@
* 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);
/*
*
diff --git a/src/codec/SkCodec_libgif.cpp b/src/codec/SkCodec_libgif.cpp
index efb3a90..bfa6e03 100644
--- a/src/codec/SkCodec_libgif.cpp
+++ b/src/codec/SkCodec_libgif.cpp
@@ -425,7 +425,7 @@
// 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 @@
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",
diff --git a/src/codec/SkSwizzler.cpp b/src/codec/SkSwizzler.cpp
index 754b088..7913633 100644
--- a/src/codec/SkSwizzler.cpp
+++ b/src/codec/SkSwizzler.cpp
@@ -422,17 +422,13 @@
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 @@
}
// 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");
diff --git a/src/codec/SkSwizzler.h b/src/codec/SkSwizzler.h
index 6044c86..8a471e9 100644
--- a/src/codec/SkSwizzler.h
+++ b/src/codec/SkSwizzler.h
@@ -132,8 +132,11 @@
/**
* 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 @@
* 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 @@
*/
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
diff --git a/tests/SwizzlerTest.cpp b/tests/SwizzlerTest.cpp
index f02f86b..147dfaa 100644
--- a/tests/SwizzlerTest.cpp
+++ b/tests/SwizzlerTest.cpp
@@ -15,6 +15,7 @@
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 @@
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 @@
// 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);
+ }
}
}
}
diff --git a/tools/dm_flags.py b/tools/dm_flags.py
index 368d0a3..c3a44a9 100755
--- a/tools/dm_flags.py
+++ b/tools/dm_flags.py
@@ -73,9 +73,17 @@
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(' '))