Pause Wuffs' getFrameCount in incremental decode
Bug: skia:8235
Bug: skia:8750
Change-Id: I7b8a6a1aba38ba2f16bd184253fbef2a37000cef
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/223016
Reviewed-by: Leon Scroggins <scroggo@google.com>
Commit-Queue: Leon Scroggins <scroggo@google.com>
diff --git a/src/codec/SkWuffsCodec.cpp b/src/codec/SkWuffsCodec.cpp
index 57704f6..72120a3 100644
--- a/src/codec/SkWuffsCodec.cpp
+++ b/src/codec/SkWuffsCodec.cpp
@@ -316,6 +316,9 @@
void* dst,
size_t rowBytes,
const SkCodec::Options& options) {
+ if (!dst) {
+ return SkCodec::kInvalidParameters;
+ }
if (options.fSubset) {
return SkCodec::kUnimplemented;
}
@@ -481,7 +484,37 @@
}
int SkWuffsCodec::onGetFrameCount() {
- if (!fFramesComplete) {
+ // It is valid, in terms of the SkCodec API, to call SkCodec::getFrameCount
+ // while in an incremental decode (after onStartIncrementalDecode returns
+ // and before onIncrementalDecode returns kSuccess).
+ //
+ // We should not advance the SkWuffsCodec' stream while doing so, even
+ // though other SkCodec implementations can return increasing values from
+ // onGetFrameCount when given more data. If we tried to do so, the
+ // subsequent resume of the incremental decode would continue reading from
+ // a different position in the I/O stream, leading to an incorrect error.
+ //
+ // Other SkCodec implementations can move the stream forward during
+ // onGetFrameCount because they assume that the stream is rewindable /
+ // seekable. For example, an alternative GIF implementation may choose to
+ // store, for each frame walked past when merely counting the number of
+ // frames, the I/O position of each of the frame's GIF data blocks. (A GIF
+ // frame's compressed data can have multiple data blocks, each at most 255
+ // bytes in length). Obviously, this can require O(numberOfFrames) extra
+ // memory to store these I/O positions. The constant factor is small, but
+ // it's still O(N), not O(1).
+ //
+ // Wuffs and SkWuffsCodec tries to minimize relying on the rewindable /
+ // seekable assumption. By design, Wuffs per se aims for O(1) memory use
+ // (after any pixel buffers are allocated) instead of O(N), and its I/O
+ // type, wuffs_base__io_buffer, is not necessarily rewindable or seekable.
+ //
+ // The Wuffs API provides a limited, optional form of seeking, to the start
+ // of an animation frame's data, but does not provide arbitrary save and
+ // load of its internal state whilst in the middle of an animation frame.
+ bool incrementalDecodeIsInProgress = fIncrDecDst != nullptr;
+
+ if (!fFramesComplete && !incrementalDecodeIsInProgress) {
this->readFrames();
this->updateNumFullyReceivedFrames();
}
diff --git a/tests/CodecPartialTest.cpp b/tests/CodecPartialTest.cpp
index dc3508b..0ff5435 100644
--- a/tests/CodecPartialTest.cpp
+++ b/tests/CodecPartialTest.cpp
@@ -101,9 +101,8 @@
while (true) {
// This imitates how Chromium calls getFrameCount before resuming a decode.
- // Without this line, the test passes. With it, it fails when skia_use_wuffs
- // is true.
partialCodec->getFrameCount();
+
const SkCodec::Result result = partialCodec->incrementalDecode();
if (result == SkCodec::kSuccess) {
