Revert of Make SkPngCodec decode progressively. (patchset #18 id:340001 of https://codereview.chromium.org/1997703003/ )
Reason for revert:
This is failing tests and then crashing on Google3 [1]. The crashes are fixed by crrev.com/2026873002, but to fix the builder we'll need to upgrade its version of libpng.
[1] https://sponge.corp.google.com/target?id=e545ef55-4da4-4931-9524-1ac92acb61b1&target=//third_party/skia/HEAD:dm#shard=1|run=1|attempt=1|page=-1
Original issue's description:
> Make SkPngCodec decode progressively.
>
> This is a step towards using SkCodec in Chromium, where progressive
> decoding is necessary.
>
> Switch from using png_read_row (which expects all the data to be
> available) to png_process_data, which uses callbacks when rows are
> available.
>
> Create a new API for SkCodec, which supports progressive decoding and
> scanline decoding. Future changes will switch the other clients off of
> startScanlineDecode and get/skip-Scanlines to the new API.
>
> Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
> PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
> updateCurrScanline(), which was only used by the old implementation for
> interlaced PNG.
>
> DMSrcSink:
> - In CodecSrc::kScanline_Mode, use the new method for scanline decoding
> for the supported formats (just PNG and PNG-in-ICO for now).
>
> fuzz.cpp:
> - Remove reference to kNone_ScanlineOrder
>
> SkCodec:
> - Add new APIs:
> - startIncrementalDecode
> - incrementalDecode
> - Remove kNone_SkScanlineOrder and updateCurrScanline()
>
> SkPngCodec:
> - Implement new APIs
> - Switch from sk_read_fn/png_read_row etc to png_process_data
> - Expand AutoCleanPng's role to decode the header and create the
> SkPngCodec
> - Make the interlaced PNG decoder report how many lines were
> initialized during an incomplete decode
> - Make initializeSwizzler return a bool instead of an SkCodec::Result
> (It only returned kSuccess or kInvalidInput anyway)
>
> SkIcoCodec:
> - Implement the new APIs; supported for PNG in ICO
>
> SkSampledCodec:
> - Call the new method for decoding scanlines, and fall back to the old
> method if the new version is unimplemented
> - Remove references to kNone_SkScanlineOrder
>
> tests/CodecPartial:
> - Add a test which decodes part of an image, then finishes the decode,
> and compares it to the straightforward method
>
> tests/CodecTest:
> - Add a test which decodes all scanlines using the new method
> - Repurpose the Codec_stripes test to decode using the new method in
> sections rather than all at once
> - In the method check(), add a parameter for whether the image supports
> the new method of scanline decoding, and be explicit about whether an
> image supports incomplete
> - Test incomplete PNG decodes. We should have been doing it anyway for
> non-interlaced (except for an image that is too small - one row), but
> the new method supports interlaced incomplete as well
> - Make test_invalid_parameters test the new method
> - Add a test to ensure that it's safe to fall back to scanline decoding without
> rewinding
>
> BUG=skia:4211
>
> The new version was generally faster than the old version (but not significantly so).
>
> Some raw performance differences can be found at https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD-pMgO-FXldlNF9ejo4o/
>
> Design doc can be found at https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_Cu8DF57KNtUmgLM/
>
> GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1997703003
>
> Committed: https://skia.googlesource.com/skia/+/a4b09a117d4d1ba5dda372e6a2323e653766539e
TBR=reed@google.com,msarett@google.com,scroggo@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=skia:4211
Review-Url: https://codereview.chromium.org/2023103002
diff --git a/src/codec/SkSampledCodec.cpp b/src/codec/SkSampledCodec.cpp
index 8c39f27..49c939c 100644
--- a/src/codec/SkSampledCodec.cpp
+++ b/src/codec/SkSampledCodec.cpp
@@ -101,65 +101,37 @@
int scaledSubsetWidth = info.width();
int scaledSubsetHeight = info.height();
- const SkImageInfo scaledInfo = info.makeWH(scaledSize.width(), scaledSize.height());
-
- {
- // Although startScanlineDecode expects the bottom and top to match the
- // SkImageInfo, startIncrementalDecode uses them to determine when to start
- // and end calling the callback.
- SkIRect incrementalSubset = SkIRect::MakeXYWH(scaledSubsetX, scaledSubsetY,
- scaledSubsetWidth, scaledSubsetHeight);
- codecOptions.fSubset = &incrementalSubset;
- const SkCodec::Result startResult = this->codec()->startIncrementalDecode(
- scaledInfo, pixels, rowBytes, &codecOptions,
- options.fColorPtr, options.fColorCount);
- if (SkCodec::kSuccess == startResult) {
- int rowsDecoded;
- const SkCodec::Result incResult = this->codec()->incrementalDecode(&rowsDecoded);
- if (incResult == SkCodec::kSuccess) {
- return SkCodec::kSuccess;
- }
- SkASSERT(SkCodec::kIncompleteInput == incResult);
-
- // FIXME: Can zero initialized be read from SkCodec::fOptions?
- this->codec()->fillIncompleteImage(scaledInfo, pixels, rowBytes,
- options.fZeroInitialized, scaledSubsetHeight, rowsDecoded);
- return SkCodec::kIncompleteInput;
- } else if (startResult != SkCodec::kUnimplemented) {
- return startResult;
- }
- // Otherwise fall down to use the old scanline decoder.
- // codecOptions.fSubset will be reset below, so it will not continue to
- // point to the object that is no longer on the stack.
- }
-
// Start the scanline decode.
SkIRect scanlineSubset = SkIRect::MakeXYWH(scaledSubsetX, 0, scaledSubsetWidth,
scaledSize.height());
codecOptions.fSubset = &scanlineSubset;
-
- SkCodec::Result result = this->codec()->startScanlineDecode(scaledInfo,
- &codecOptions, options.fColorPtr, options.fColorCount);
+ SkCodec::Result result = this->codec()->startScanlineDecode(info.makeWH(scaledSize.width(),
+ scaledSize.height()), &codecOptions, options.fColorPtr, options.fColorCount);
if (SkCodec::kSuccess != result) {
return result;
}
// At this point, we are only concerned with subsetting. Either no scale was
// requested, or the this->codec() is handling the scale.
- // Note that subsetting is only supported for kTopDown, so this code will not be
- // reached for other orders.
- SkASSERT(this->codec()->getScanlineOrder() == SkCodec::kTopDown_SkScanlineOrder);
- if (!this->codec()->skipScanlines(scaledSubsetY)) {
- this->codec()->fillIncompleteImage(info, pixels, rowBytes, options.fZeroInitialized,
- scaledSubsetHeight, 0);
- return SkCodec::kIncompleteInput;
- }
+ switch (this->codec()->getScanlineOrder()) {
+ case SkCodec::kTopDown_SkScanlineOrder:
+ case SkCodec::kNone_SkScanlineOrder: {
+ if (!this->codec()->skipScanlines(scaledSubsetY)) {
+ this->codec()->fillIncompleteImage(info, pixels, rowBytes, options.fZeroInitialized,
+ scaledSubsetHeight, 0);
+ return SkCodec::kIncompleteInput;
+ }
- int decodedLines = this->codec()->getScanlines(pixels, scaledSubsetHeight, rowBytes);
- if (decodedLines != scaledSubsetHeight) {
- return SkCodec::kIncompleteInput;
+ int decodedLines = this->codec()->getScanlines(pixels, scaledSubsetHeight, rowBytes);
+ if (decodedLines != scaledSubsetHeight) {
+ return SkCodec::kIncompleteInput;
+ }
+ return SkCodec::kSuccess;
+ }
+ default:
+ SkASSERT(false);
+ return SkCodec::kUnimplemented;
}
- return SkCodec::kSuccess;
}
@@ -202,70 +174,10 @@
sampledOptions.fSubset = ⊂
}
- // Since we guarantee that output dimensions are always at least one (even if the sampleSize
- // is greater than a given dimension), the input sampleSize is not always the sampleSize that
- // we use in practice.
- const int sampleX = subsetWidth / info.width();
- const int sampleY = subsetHeight / info.height();
-
- const int samplingOffsetY = get_start_coord(sampleY);
- const int startY = samplingOffsetY + subsetY;
- int dstHeight = info.height();
-
- const SkImageInfo nativeInfo = info.makeWH(nativeSize.width(), nativeSize.height());
-
- {
- // Although startScanlineDecode expects the bottom and top to match the
- // SkImageInfo, startIncrementalDecode uses them to determine when to start
- // and end calling the callback.
- SkIRect incrementalSubset;
- incrementalSubset.fTop = startY;
- incrementalSubset.fBottom = startY + (dstHeight - 1) * sampleY + 1;
- if (sampledOptions.fSubset) {
- incrementalSubset.fLeft = sampledOptions.fSubset->fLeft;
- incrementalSubset.fRight = sampledOptions.fSubset->fRight;
- } else {
- incrementalSubset.fLeft = 0;
- incrementalSubset.fRight = nativeSize.width();
- }
- SkCodec::Options incrementalOptions = sampledOptions;
- incrementalOptions.fSubset = &incrementalSubset;
- const SkCodec::Result startResult = this->codec()->startIncrementalDecode(nativeInfo,
- pixels, rowBytes, &incrementalOptions, options.fColorPtr, options.fColorCount);
- if (SkCodec::kSuccess == startResult) {
- SkSampler* sampler = this->codec()->getSampler(true);
- if (!sampler) {
- return SkCodec::kUnimplemented;
- }
-
- if (sampler->setSampleX(sampleX) != info.width()) {
- return SkCodec::kInvalidScale;
- }
- if (get_scaled_dimension(subsetHeight, sampleY) != info.height()) {
- return SkCodec::kInvalidScale;
- }
-
- sampler->setSampleY(sampleY);
-
- int rowsDecoded;
- const SkCodec::Result incResult = this->codec()->incrementalDecode(&rowsDecoded);
- if (incResult == SkCodec::kSuccess) {
- return SkCodec::kSuccess;
- }
- SkASSERT(incResult == SkCodec::kIncompleteInput);
- const int lastRowInOutput = (rowsDecoded - startY) / sampleY;
- // FIXME: Should this be info or nativeInfo? Does it make a difference?
- this->codec()->fillIncompleteImage(info, pixels, rowBytes, options.fZeroInitialized,
- info.height(), lastRowInOutput);
- return SkCodec::kIncompleteInput;
- } else if (startResult != SkCodec::kUnimplemented) {
- return startResult;
- } // kUnimplemented means use the old method.
- }
-
// Start the scanline decode.
- SkCodec::Result result = this->codec()->startScanlineDecode(nativeInfo,
- &sampledOptions, options.fColorPtr, options.fColorCount);
+ SkCodec::Result result = this->codec()->startScanlineDecode(
+ info.makeWH(nativeSize.width(), nativeSize.height()), &sampledOptions,
+ options.fColorPtr, options.fColorCount);
if (SkCodec::kSuccess != result) {
return result;
}
@@ -275,6 +187,11 @@
return SkCodec::kUnimplemented;
}
+ // Since we guarantee that output dimensions are always at least one (even if the sampleSize
+ // is greater than a given dimension), the input sampleSize is not always the sampleSize that
+ // we use in practice.
+ const int sampleX = subsetWidth / info.width();
+ const int sampleY = subsetHeight / info.height();
if (sampler->setSampleX(sampleX) != info.width()) {
return SkCodec::kInvalidScale;
}
@@ -282,6 +199,9 @@
return SkCodec::kInvalidScale;
}
+ const int samplingOffsetY = get_start_coord(sampleY);
+ const int startY = samplingOffsetY + subsetY;
+ int dstHeight = info.height();
switch(this->codec()->getScanlineOrder()) {
case SkCodec::kTopDown_SkScanlineOrder: {
if (!this->codec()->skipScanlines(startY)) {
@@ -346,6 +266,30 @@
}
return SkCodec::kIncompleteInput;
}
+ case SkCodec::kNone_SkScanlineOrder: {
+ const int linesNeeded = subsetHeight - samplingOffsetY;
+ SkAutoTMalloc<uint8_t> storage(linesNeeded * rowBytes);
+ uint8_t* storagePtr = storage.get();
+
+ if (!this->codec()->skipScanlines(startY)) {
+ this->codec()->fillIncompleteImage(info, pixels, rowBytes, options.fZeroInitialized,
+ dstHeight, 0);
+ return SkCodec::kIncompleteInput;
+ }
+ int scanlines = this->codec()->getScanlines(storagePtr, linesNeeded, rowBytes);
+
+ for (int y = 0; y < dstHeight; y++) {
+ memcpy(pixels, storagePtr, info.minRowBytes());
+ storagePtr += sampleY * rowBytes;
+ pixels = SkTAddOffset<void>(pixels, rowBytes);
+ }
+
+ if (scanlines < dstHeight) {
+ // this->codec() has already handled filling uninitialized memory.
+ return SkCodec::kIncompleteInput;
+ }
+ return SkCodec::kSuccess;
+ }
default:
SkASSERT(false);
return SkCodec::kUnimplemented;