Fix uninitialized bug in SkWuffsCodec for incomplete images
Bug: skia:8579
Zero-initialize the pixel buffer that wuffs uses inside the frame rect.
If the encoded image is incomplete, we will swizzle this memory into the
destination, so this prevents swizzling uninitialized memory.
In addition, zero-initialize the client's memory if the above buffer
will not fill it, and if the frame is independent.
Move decodeFrameConfig into onStartIncrementalDecode. This makes it
clear that we haven't begun decoding the pixels so rowsDecoded is
irrelevant. Update the documentation for onStartIncrementalDecode to
make it clear that it can be called more than once (and when to do so).
If a frame is incomplete and depends on prior frames, do not blend it.
This will overwrite pixels from the prior frames without a way to undo.
Current clients only want to show incomplete images for the first frame
anyway.
Add some debugging information to Codec_partialAnim test, including
writing out pngs when failing the test.
TBR=djsollen@google.com for documentation change in SkCodec.h
Change-Id: I85c8ca4075301306f4738ddfc2f5992a5745108b
Reviewed-on: https://skia-review.googlesource.com/c/174310
Commit-Queue: Leon Scroggins <scroggo@google.com>
Reviewed-by: Nigel Tao <nigeltao@google.com>
diff --git a/include/codec/SkCodec.h b/include/codec/SkCodec.h
index 201cd74..6ce7b99 100644
--- a/include/codec/SkCodec.h
+++ b/include/codec/SkCodec.h
@@ -403,6 +403,9 @@
*
* This may require a rewind.
*
+ * If kIncompleteInput is returned, may be called again after more data has
+ * been provided to the source SkStream.
+ *
* @param dstInfo Info of the destination. If the dimensions do not match
* those of getInfo, this implies a scale.
* @param dst Memory to write to. Needs to be large enough to hold the subset,
@@ -421,10 +424,11 @@
/**
* Start/continue the incremental decode.
*
- * Not valid to call before calling startIncrementalDecode().
+ * Not valid to call before a call to startIncrementalDecode() returns
+ * kSuccess.
*
- * After the first call, should only be called again if more data has been
- * provided to the source SkStream.
+ * If kIncompleteInput is returned, may be called again after more data has
+ * been provided to the source SkStream.
*
* Unlike getPixels and getScanlines, this does not do any filling. This is
* left up to the caller, since they may be skipping lines or continuing the
diff --git a/src/codec/SkWuffsCodec.cpp b/src/codec/SkWuffsCodec.cpp
index 090c234..59b53a5 100644
--- a/src/codec/SkWuffsCodec.cpp
+++ b/src/codec/SkWuffsCodec.cpp
@@ -198,7 +198,6 @@
// Incremental decoding state.
uint8_t* fIncrDecDst;
- bool fIncrDecHaveFrameConfig;
size_t fIncrDecRowBytes;
std::unique_ptr<SkSwizzler> fSwizzler;
@@ -319,7 +318,6 @@
fPixelBuffer(pixbuf),
fIOBuffer((wuffs_base__io_buffer){}),
fIncrDecDst(nullptr),
- fIncrDecHaveFrameConfig(false),
fIncrDecRowBytes(0),
fSwizzler(nullptr),
fNumFullyReceivedFrames(0),
@@ -382,12 +380,19 @@
}
fSpySampler.reset();
- fIncrDecDst = static_cast<uint8_t*>(dst);
- fIncrDecHaveFrameConfig = false;
- fIncrDecRowBytes = rowBytes;
fSwizzler = nullptr;
- return SkCodec::kSuccess;
+ const char* status = this->decodeFrameConfig();
+ if (status == nullptr) {
+ fIncrDecDst = static_cast<uint8_t*>(dst);
+ fIncrDecRowBytes = rowBytes;
+ return SkCodec::kSuccess;
+ } else if (status == wuffs_base__suspension__short_read) {
+ return SkCodec::kIncompleteInput;
+ } else {
+ SkCodecPrintf("decodeFrameConfig: %s", status);
+ return SkCodec::kErrorInInput;
+ }
}
static bool independent_frame(SkCodec* codec, int frameIndex) {
@@ -415,40 +420,30 @@
return SkCodec::kInternalError;
}
- if (!fIncrDecHaveFrameConfig) {
- const char* status = this->decodeFrameConfig();
- if (status == nullptr) {
- // No-op.
- } else if (status == wuffs_base__suspension__short_read) {
- return SkCodec::kIncompleteInput;
- } else {
- SkCodecPrintf("decodeFrameConfig: %s", status);
- return SkCodec::kErrorInInput;
- }
- fIncrDecHaveFrameConfig = true;
- }
-
- SkCodec::Result result = SkCodec::kSuccess;
- const char* status = this->decodeFrame();
- if (status == nullptr) {
- // No-op.
- } else if (status == wuffs_base__suspension__short_read) {
- result = SkCodec::kIncompleteInput;
- } else {
- SkCodecPrintf("decodeFrame: %s", status);
- return SkCodec::kErrorInInput;
- }
+ // In Wuffs, a paletted image is always 1 byte per pixel.
+ static constexpr size_t src_bpp = 1;
+ wuffs_base__table_u8 pixels = fPixelBuffer.plane(0);
const bool independent = independent_frame(this, options().fFrameIndex);
wuffs_base__rect_ie_u32 r = fFrameConfig.bounds();
if (!fSwizzler) {
- SkIRect swizzleRect = SkIRect::MakeLTRB(r.min_incl_x, 0, r.max_excl_x, 1);
- fSwizzler = SkSwizzler::Make(this->getEncodedInfo(), fColorTable, dstInfo(), Options(),
- &swizzleRect);
+ auto bounds = SkIRect::MakeLTRB(r.min_incl_x, r.min_incl_y, r.max_excl_x, r.max_excl_y);
+ fSwizzler = SkSwizzler::Make(this->getEncodedInfo(), fColorTable, dstInfo(),
+ this->options(), &bounds);
fSwizzler->setSampleX(fSpySampler.sampleX());
fSwizzler->setSampleY(fSpySampler.sampleY());
- if (independent) {
+ // Zero-initialize wuffs' buffer covering the frame rect. This will later be used to
+ // determine how we write to the output, even if the image was incomplete. This ensures
+ // that we do not swizzle uninitialized memory.
+ for (uint32_t y = r.min_incl_y; y < r.max_excl_y; y++) {
+ uint8_t* s = pixels.ptr + (y * pixels.stride) + (r.min_incl_x * src_bpp);
+ sk_bzero(s, r.width() * src_bpp);
+ }
+
+ // If the frame rect does not fill the output, ensure that those pixels are not
+ // left uninitialized either.
+ if (independent && bounds != this->bounds()) {
auto fillInfo = dstInfo().makeWH(fSwizzler->fillWidth(),
get_scaled_dimension(this->dstInfo().height(),
fSwizzler->sampleY()));
@@ -456,6 +451,30 @@
}
}
+ SkCodec::Result result = SkCodec::kSuccess;
+ const char* status = this->decodeFrame();
+ if (status != nullptr) {
+ if (status == wuffs_base__suspension__short_read) {
+ result = SkCodec::kIncompleteInput;
+ } else {
+ SkCodecPrintf("decodeFrame: %s", status);
+ result = SkCodec::kErrorInInput;
+ }
+
+ if (!independent) {
+ if (rowsDecoded) {
+ // Though no rows were written by this call, the prior frame
+ // initialized all the rows.
+ *rowsDecoded = get_scaled_dimension(this->dstInfo().height(),
+ fSwizzler->sampleY());
+ }
+ // For a dependent frame, we cannot blend the partial result, since
+ // that will overwrite the contribution from prior frames with all
+ // zeroes that were written to |pixels| above.
+ return result;
+ }
+ }
+
wuffs_base__slice_u8 palette = fPixelBuffer.palette();
SkASSERT(palette.len == 4 * 256);
auto proc = choose_pack_color_proc(false, dstInfo().colorType());
@@ -468,13 +487,9 @@
if (!independent) {
tmpBuffer.reset(new uint8_t[dstInfo().minRowBytes()]);
}
- wuffs_base__table_u8 pixels = fPixelBuffer.plane(0);
const int sampleY = fSwizzler->sampleY();
const int scaledHeight = get_scaled_dimension(dstInfo().height(), sampleY);
for (uint32_t y = r.min_incl_y; y < r.max_excl_y; y++) {
- // In Wuffs, a paletted image is always 1 byte per pixel.
- static constexpr size_t src_bpp = 1;
-
int dstY = y;
if (sampleY != 1) {
if (!fSwizzler->rowNeeded(y)) {
@@ -533,7 +548,6 @@
if (result == SkCodec::kSuccess) {
fSpySampler.reset();
fIncrDecDst = nullptr;
- fIncrDecHaveFrameConfig = false;
fIncrDecRowBytes = 0;
fSwizzler = nullptr;
} else {
diff --git a/tests/CodecPartialTest.cpp b/tests/CodecPartialTest.cpp
index 452ba85..e07432a 100644
--- a/tests/CodecPartialTest.cpp
+++ b/tests/CodecPartialTest.cpp
@@ -5,6 +5,7 @@
* found in the LICENSE file.
*/
+#include "CodecPriv.h"
#include "FakeStreams.h"
#include "Resources.h"
#include "SkBitmap.h"
@@ -40,19 +41,21 @@
return SkCodec::kSuccess == codec->getPixels(info, dst->getPixels(), dst->rowBytes());
}
-static void compare_bitmaps(skiatest::Reporter* r, const SkBitmap& bm1, const SkBitmap& bm2) {
+static bool compare_bitmaps(skiatest::Reporter* r, const SkBitmap& bm1, const SkBitmap& bm2) {
const SkImageInfo& info = bm1.info();
if (info != bm2.info()) {
ERRORF(r, "Bitmaps have different image infos!");
- return;
+ return false;
}
const size_t rowBytes = info.minRowBytes();
for (int i = 0; i < info.height(); i++) {
if (memcmp(bm1.getAddr(0, i), bm2.getAddr(0, i), rowBytes)) {
ERRORF(r, "Bitmaps have different pixels, starting on line %i!", i);
- return;
+ return false;
}
}
+
+ return true;
}
static void test_partial(skiatest::Reporter* r, const char* name, size_t minBytes = 0) {
@@ -289,7 +292,14 @@
frameInfo = partialCodec->getFrameInfo();
REPORTER_ASSERT(r, frameInfo.size() == i + 1);
REPORTER_ASSERT(r, frameInfo[i].fFullyReceived);
- compare_bitmaps(r, frames[i], frame);
+ if (!compare_bitmaps(r, frames[i], frame)) {
+ ERRORF(r, "\tfailure was on frame %i", i);
+ SkString name = SkStringPrintf("expected_%i", i);
+ write_bm(name.c_str(), frames[i]);
+
+ name = SkStringPrintf("actual_%i", i);
+ write_bm(name.c_str(), frame);
+ }
}
}