Revert of Make SkPngCodec decode progressively. (patchset #26 id:520001 of https://codereview.chromium.org/1997703003/ )
Reason for revert:
Still causing problems in Google3, e.g.
https://test.corp.google.com/ui#cl=124138817&flags=CAMQBQ==&id=OCL:124138817:BASE:124139560:1465227435491:219ffbdb&t=//third_party/skia/HEAD:dm
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
>
> Committed: https://skia.googlesource.com/skia/+/30e78c9737ff4861dc4e3fa1e4cd010680ed6965
>
> Committed: https://skia.googlesource.com/skia/+/6fb2391b2cc83ee2160b4e994faa8128975acc1f
TBR=reed@google.com,msarett@google.com,scroggo@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=skia:4211
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2044573002
Review-Url: https://codereview.chromium.org/2044573002
diff --git a/tests/CodecTest.cpp b/tests/CodecTest.cpp
index 52ddbff..12d6ac8 100644
--- a/tests/CodecTest.cpp
+++ b/tests/CodecTest.cpp
@@ -83,62 +83,6 @@
return rect;
}
-static void test_incremental_decode(skiatest::Reporter* r, SkCodec* codec, const SkImageInfo& info,
- const SkMD5::Digest& goodDigest) {
- SkBitmap bm;
- bm.allocPixels(info);
- SkAutoLockPixels autoLockPixels(bm);
-
- REPORTER_ASSERT(r, SkCodec::kSuccess == codec->startIncrementalDecode(info, bm.getPixels(),
- bm.rowBytes()));
-
- REPORTER_ASSERT(r, SkCodec::kSuccess == codec->incrementalDecode());
-
- compare_to_good_digest(r, goodDigest, bm);
-}
-
-// Test in stripes, similar to DM's kStripe_Mode
-static void test_in_stripes(skiatest::Reporter* r, SkCodec* codec, const SkImageInfo& info,
- const SkMD5::Digest& goodDigest) {
- SkBitmap bm;
- bm.allocPixels(info);
- bm.eraseColor(SK_ColorYELLOW);
-
- const int height = info.height();
- // Note that if numStripes does not evenly divide height there will be an extra
- // stripe.
- const int numStripes = 4;
-
- if (numStripes > height) {
- // Image is too small.
- return;
- }
-
- const int stripeHeight = height / numStripes;
-
- // Iterate through the image twice. Once to decode odd stripes, and once for even.
- for (int oddEven = 1; oddEven >= 0; oddEven--) {
- for (int y = oddEven * stripeHeight; y < height; y += 2 * stripeHeight) {
- SkIRect subset = SkIRect::MakeLTRB(0, y, info.width(),
- SkTMin(y + stripeHeight, height));
- SkCodec::Options options;
- options.fSubset = ⊂
- if (SkCodec::kSuccess != codec->startIncrementalDecode(info, bm.getAddr(0, y),
- bm.rowBytes(), &options)) {
- ERRORF(r, "failed to start incremental decode!\ttop: %i\tbottom%i\n",
- subset.top(), subset.bottom());
- return;
- }
- if (SkCodec::kSuccess != codec->incrementalDecode()) {
- ERRORF(r, "failed incremental decode starting from line %i\n", y);
- return;
- }
- }
- }
-
- compare_to_good_digest(r, goodDigest, bm);
-}
-
template<typename Codec>
static void test_codec(skiatest::Reporter* r, Codec* codec, SkBitmap& bm, const SkImageInfo& info,
const SkISize& size, SkCodec::Result expectedResult, SkMD5::Digest* digest,
@@ -247,14 +191,12 @@
return false;
}
-// FIXME: Break up this giant function
static void check(skiatest::Reporter* r,
const char path[],
SkISize size,
bool supportsScanlineDecoding,
bool supportsSubsetDecoding,
- bool supportsIncomplete,
- bool supportsNewScanlineDecoding = false) {
+ bool supportsIncomplete = true) {
SkAutoTDelete<SkStream> stream(resource(path));
if (!stream) {
@@ -284,15 +226,12 @@
test_codec(r, codec.get(), bm, info, size, expectedResult, &codecDigest, nullptr);
// Scanline decoding follows.
-
- if (supportsNewScanlineDecoding && !isIncomplete) {
- test_incremental_decode(r, codec, info, codecDigest);
- test_in_stripes(r, codec, info, codecDigest);
- }
-
// Need to call startScanlineDecode() first.
- REPORTER_ASSERT(r, codec->getScanlines(bm.getAddr(0, 0), 1, 0) == 0);
- REPORTER_ASSERT(r, !codec->skipScanlines(1));
+ REPORTER_ASSERT(r, codec->getScanlines(bm.getAddr(0, 0), 1, 0)
+ == 0);
+ REPORTER_ASSERT(r, codec->skipScanlines(1)
+ == 0);
+
const SkCodec::Result startResult = codec->startScanlineDecode(info);
if (supportsScanlineDecoding) {
bm.eraseColor(SK_ColorYELLOW);
@@ -387,7 +326,7 @@
}
// SkAndroidCodec tests
- if (supportsScanlineDecoding || supportsSubsetDecoding || supportsNewScanlineDecoding) {
+ if (supportsScanlineDecoding || supportsSubsetDecoding) {
SkAutoTDelete<SkStream> stream(resource(path));
if (!stream) {
@@ -425,23 +364,14 @@
REPORTER_ASSERT(r, gen->getPixels(info, bm.getPixels(), bm.rowBytes()));
compare_to_good_digest(r, codecDigest, bm);
-#if PNG_LIBPNG_VER_MAJOR == 1 && PNG_LIBPNG_VER_MINOR < 5
- // FIXME: With older versions of libpng, SkPngCodec requires being able to call
- // SkStream::move(), which is not supported by SkFrontBufferedStream. (Android
- // has a more recent version of libpng which uses png_process_data_pause to
- // avoid calling move().)
- if (!SkStrEndsWith(path, ".png"))
-#endif
- {
- // Test using SkFrontBufferedStream, as Android does
- SkStream* bufferedStream = SkFrontBufferedStream::Create(new SkMemoryStream(fullData),
- SkCodec::MinBufferedBytesNeeded());
- REPORTER_ASSERT(r, bufferedStream);
- codec.reset(SkCodec::NewFromStream(bufferedStream));
- REPORTER_ASSERT(r, codec);
- if (codec) {
- test_info(r, codec.get(), info, SkCodec::kSuccess, &codecDigest);
- }
+ // Test using SkFrontBufferedStream, as Android does
+ SkStream* bufferedStream = SkFrontBufferedStream::Create(new SkMemoryStream(fullData),
+ SkCodec::MinBufferedBytesNeeded());
+ REPORTER_ASSERT(r, bufferedStream);
+ codec.reset(SkCodec::NewFromStream(bufferedStream));
+ REPORTER_ASSERT(r, codec);
+ if (codec) {
+ test_info(r, codec.get(), info, SkCodec::kSuccess, &codecDigest);
}
}
@@ -453,16 +383,16 @@
DEF_TEST(Codec, r) {
// WBMP
- check(r, "mandrill.wbmp", SkISize::Make(512, 512), true, false, true);
+ check(r, "mandrill.wbmp", SkISize::Make(512, 512), true, false);
// WEBP
- check(r, "baby_tux.webp", SkISize::Make(386, 395), false, true, true);
- check(r, "color_wheel.webp", SkISize::Make(128, 128), false, true, true);
- check(r, "yellow_rose.webp", SkISize::Make(400, 301), false, true, true);
+ check(r, "baby_tux.webp", SkISize::Make(386, 395), false, true);
+ check(r, "color_wheel.webp", SkISize::Make(128, 128), false, true);
+ check(r, "yellow_rose.webp", SkISize::Make(400, 301), false, true);
// BMP
- check(r, "randPixels.bmp", SkISize::Make(8, 8), true, false, true);
- check(r, "rle.bmp", SkISize::Make(320, 240), true, false, true);
+ check(r, "randPixels.bmp", SkISize::Make(8, 8), true, false);
+ check(r, "rle.bmp", SkISize::Make(320, 240), true, false);
// ICO
// FIXME: We are not ready to test incomplete ICOs
@@ -470,7 +400,7 @@
// Decodes an embedded BMP image
check(r, "color_wheel.ico", SkISize::Make(128, 128), true, false, false);
// Decodes an embedded PNG image
- check(r, "google_chrome.ico", SkISize::Make(256, 256), false, false, false, true);
+ check(r, "google_chrome.ico", SkISize::Make(256, 256), true, false, false);
// GIF
// FIXME: We are not ready to test incomplete GIFs
@@ -480,30 +410,30 @@
check(r, "randPixels.gif", SkISize::Make(8, 8), true, false, false);
// JPG
- check(r, "CMYK.jpg", SkISize::Make(642, 516), true, false, true);
- check(r, "color_wheel.jpg", SkISize::Make(128, 128), true, false, true);
+ check(r, "CMYK.jpg", SkISize::Make(642, 516), true, false);
+ check(r, "color_wheel.jpg", SkISize::Make(128, 128), true, false);
// grayscale.jpg is too small to test incomplete
check(r, "grayscale.jpg", SkISize::Make(128, 128), true, false, false);
- check(r, "mandrill_512_q075.jpg", SkISize::Make(512, 512), true, false, true);
+ check(r, "mandrill_512_q075.jpg", SkISize::Make(512, 512), true, false);
// randPixels.jpg is too small to test incomplete
check(r, "randPixels.jpg", SkISize::Make(8, 8), true, false, false);
// PNG
- check(r, "arrow.png", SkISize::Make(187, 312), false, false, true, true);
- check(r, "baby_tux.png", SkISize::Make(240, 246), false, false, true, true);
- check(r, "color_wheel.png", SkISize::Make(128, 128), false, false, true, true);
- // half-transparent-white-pixel.png is too small to test incomplete
- check(r, "half-transparent-white-pixel.png", SkISize::Make(1, 1), false, false, false, true);
- check(r, "mandrill_128.png", SkISize::Make(128, 128), false, false, true, true);
- check(r, "mandrill_16.png", SkISize::Make(16, 16), false, false, true, true);
- check(r, "mandrill_256.png", SkISize::Make(256, 256), false, false, true, true);
- check(r, "mandrill_32.png", SkISize::Make(32, 32), false, false, true, true);
- check(r, "mandrill_512.png", SkISize::Make(512, 512), false, false, true, true);
- check(r, "mandrill_64.png", SkISize::Make(64, 64), false, false, true, true);
- check(r, "plane.png", SkISize::Make(250, 126), false, false, true, true);
- check(r, "plane_interlaced.png", SkISize::Make(250, 126), false, false, true, true);
- check(r, "randPixels.png", SkISize::Make(8, 8), false, false, true, true);
- check(r, "yellow_rose.png", SkISize::Make(400, 301), false, false, true, true);
+ check(r, "arrow.png", SkISize::Make(187, 312), true, false, false);
+ check(r, "baby_tux.png", SkISize::Make(240, 246), true, false, false);
+ check(r, "color_wheel.png", SkISize::Make(128, 128), true, false, false);
+ check(r, "half-transparent-white-pixel.png", SkISize::Make(1, 1), true, false, false);
+ check(r, "mandrill_128.png", SkISize::Make(128, 128), true, false, false);
+ check(r, "mandrill_16.png", SkISize::Make(16, 16), true, false, false);
+ check(r, "mandrill_256.png", SkISize::Make(256, 256), true, false, false);
+ check(r, "mandrill_32.png", SkISize::Make(32, 32), true, false, false);
+ check(r, "mandrill_512.png", SkISize::Make(512, 512), true, false, false);
+ check(r, "mandrill_64.png", SkISize::Make(64, 64), true, false, false);
+ check(r, "plane.png", SkISize::Make(250, 126), true, false, false);
+ // FIXME: We are not ready to test incomplete interlaced pngs
+ check(r, "plane_interlaced.png", SkISize::Make(250, 126), true, false, false);
+ check(r, "randPixels.png", SkISize::Make(8, 8), true, false, false);
+ check(r, "yellow_rose.png", SkISize::Make(400, 301), true, false, false);
// RAW
// Disable RAW tests for Win32.
@@ -514,6 +444,97 @@
#endif
}
+// Test interlaced PNG in stripes, similar to DM's kStripe_Mode
+DEF_TEST(Codec_stripes, r) {
+ const char * path = "plane_interlaced.png";
+ SkAutoTDelete<SkStream> stream(resource(path));
+ if (!stream) {
+ SkDebugf("Missing resource '%s'\n", path);
+ }
+
+ SkAutoTDelete<SkCodec> codec(SkCodec::NewFromStream(stream.release()));
+ REPORTER_ASSERT(r, codec);
+
+ if (!codec) {
+ return;
+ }
+
+ switch (codec->getScanlineOrder()) {
+ case SkCodec::kBottomUp_SkScanlineOrder:
+ case SkCodec::kOutOfOrder_SkScanlineOrder:
+ ERRORF(r, "This scanline order will not match the original.");
+ return;
+ default:
+ break;
+ }
+
+ // Baseline for what the image should look like, using N32.
+ const SkImageInfo info = codec->getInfo().makeColorType(kN32_SkColorType);
+
+ SkBitmap bm;
+ bm.allocPixels(info);
+ SkAutoLockPixels autoLockPixels(bm);
+ SkCodec::Result result = codec->getPixels(info, bm.getPixels(), bm.rowBytes());
+ REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+
+ SkMD5::Digest digest;
+ md5(bm, &digest);
+
+ // Now decode in stripes
+ const int height = info.height();
+ const int numStripes = 4;
+ int stripeHeight;
+ int remainingLines;
+ SkTDivMod(height, numStripes, &stripeHeight, &remainingLines);
+
+ bm.eraseColor(SK_ColorYELLOW);
+
+ result = codec->startScanlineDecode(info);
+ REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+
+ // Odd stripes
+ for (int i = 1; i < numStripes; i += 2) {
+ // Skip the even stripes
+ bool skipResult = codec->skipScanlines(stripeHeight);
+ REPORTER_ASSERT(r, skipResult);
+
+ int linesDecoded = codec->getScanlines(bm.getAddr(0, i * stripeHeight), stripeHeight,
+ bm.rowBytes());
+ REPORTER_ASSERT(r, linesDecoded == stripeHeight);
+ }
+
+ // Even stripes
+ result = codec->startScanlineDecode(info);
+ REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+
+ for (int i = 0; i < numStripes; i += 2) {
+ int linesDecoded = codec->getScanlines(bm.getAddr(0, i * stripeHeight), stripeHeight,
+ bm.rowBytes());
+ REPORTER_ASSERT(r, linesDecoded == stripeHeight);
+
+ // Skip the odd stripes
+ if (i + 1 < numStripes) {
+ bool skipResult = codec->skipScanlines(stripeHeight);
+ REPORTER_ASSERT(r, skipResult);
+ }
+ }
+
+ // Remainder at the end
+ if (remainingLines > 0) {
+ result = codec->startScanlineDecode(info);
+ REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+
+ bool skipResult = codec->skipScanlines(height - remainingLines);
+ REPORTER_ASSERT(r, skipResult);
+
+ int linesDecoded = codec->getScanlines(bm.getAddr(0, height - remainingLines),
+ remainingLines, bm.rowBytes());
+ REPORTER_ASSERT(r, linesDecoded == remainingLines);
+ }
+
+ compare_to_good_digest(r, digest, bm);
+}
+
static void test_invalid_stream(skiatest::Reporter* r, const void* stream, size_t len) {
// Neither of these calls should return a codec. Bots should catch us if we leaked anything.
SkCodec* codec = SkCodec::NewFromStream(new SkMemoryStream(stream, len, false));
@@ -656,41 +677,26 @@
return;
}
SkAutoTDelete<SkCodec> decoder(SkCodec::NewFromStream(stream.release()));
- if (!decoder) {
- SkDebugf("Missing codec for %s\n", path);
- return;
- }
-
- const SkImageInfo info = decoder->getInfo().makeColorType(kIndex_8_SkColorType);
// This should return kSuccess because kIndex8 is supported.
SkPMColor colorStorage[256];
int colorCount;
- SkCodec::Result result = decoder->startScanlineDecode(info, nullptr, colorStorage,
- &colorCount);
- if (SkCodec::kSuccess == result) {
- // This should return kInvalidParameters because, in kIndex_8 mode, we must pass in a valid
- // colorPtr and a valid colorCountPtr.
- result = decoder->startScanlineDecode(info, nullptr, nullptr, nullptr);
- REPORTER_ASSERT(r, SkCodec::kInvalidParameters == result);
- result = decoder->startScanlineDecode(info);
- REPORTER_ASSERT(r, SkCodec::kInvalidParameters == result);
- } else if (SkCodec::kUnimplemented == result) {
- // New method should be supported:
- SkBitmap bm;
- sk_sp<SkColorTable> colorTable(new SkColorTable(colorStorage, 256));
- bm.allocPixels(info, nullptr, colorTable.get());
- result = decoder->startIncrementalDecode(info, bm.getPixels(), bm.rowBytes(), nullptr,
- colorStorage, &colorCount);
- REPORTER_ASSERT(r, SkCodec::kSuccess == result);
- result = decoder->startIncrementalDecode(info, bm.getPixels(), bm.rowBytes());
- REPORTER_ASSERT(r, SkCodec::kInvalidParameters == result);
- } else {
- // The test is uninteresting if kIndex8 is not supported
- ERRORF(r, "Should not call test_invalid_parameters for non-Index8 file: %s\n", path);
+ SkCodec::Result result = decoder->startScanlineDecode(
+ decoder->getInfo().makeColorType(kIndex_8_SkColorType), nullptr, colorStorage, &colorCount);
+ REPORTER_ASSERT(r, SkCodec::kSuccess == result);
+ // The rest of the test is uninteresting if kIndex8 is not supported
+ if (SkCodec::kSuccess != result) {
return;
}
+ // This should return kInvalidParameters because, in kIndex_8 mode, we must pass in a valid
+ // colorPtr and a valid colorCountPtr.
+ result = decoder->startScanlineDecode(
+ decoder->getInfo().makeColorType(kIndex_8_SkColorType), nullptr, nullptr, nullptr);
+ REPORTER_ASSERT(r, SkCodec::kInvalidParameters == result);
+ result = decoder->startScanlineDecode(
+ decoder->getInfo().makeColorType(kIndex_8_SkColorType));
+ REPORTER_ASSERT(r, SkCodec::kInvalidParameters == result);
}
DEF_TEST(Codec_Params, r) {
@@ -1006,91 +1012,3 @@
REPORTER_ASSERT(r, !codec);
}
-
-// Only rewinds up to a limit.
-class LimitedRewindingStream : public SkStream {
-public:
- static SkStream* Make(const char path[], size_t limit) {
- SkStream* stream = resource(path);
- if (!stream) {
- return nullptr;
- }
- return new LimitedRewindingStream(stream, limit);
- }
-
- size_t read(void* buffer, size_t size) override {
- const size_t bytes = fStream->read(buffer, size);
- fPosition += bytes;
- return bytes;
- }
-
- bool isAtEnd() const override {
- return fStream->isAtEnd();
- }
-
- bool rewind() override {
- if (fPosition <= fLimit && fStream->rewind()) {
- fPosition = 0;
- return true;
- }
-
- return false;
- }
-
-private:
- SkAutoTDelete<SkStream> fStream;
- const size_t fLimit;
- size_t fPosition;
-
- LimitedRewindingStream(SkStream* stream, size_t limit)
- : fStream(stream)
- , fLimit(limit)
- , fPosition(0)
- {
- SkASSERT(fStream);
- }
-};
-
-DEF_TEST(Codec_fallBack, r) {
- // SkAndroidCodec needs to be able to fall back to scanline decoding
- // if incremental decoding does not work. Make sure this does not
- // require a rewind.
-
- // Formats that currently do not support incremental decoding
- auto files = {
- "box.gif",
- "CMYK.jpg",
- "color_wheel.ico",
- "mandrill.wbmp",
- "randPixels.bmp",
- };
- for (auto file : files) {
- SkStream* stream = LimitedRewindingStream::Make(file, 14);
- if (!stream) {
- SkDebugf("Missing resources (%s). Set --resourcePath.\n", file);
- return;
- }
-
- SkAutoTDelete<SkCodec> codec(SkCodec::NewFromStream(stream));
- if (!codec) {
- ERRORF(r, "Failed to create codec for %s,", file);
- continue;
- }
-
- SkImageInfo info = codec->getInfo().makeColorType(kN32_SkColorType);
- SkBitmap bm;
- bm.allocPixels(info);
-
- if (SkCodec::kUnimplemented != codec->startIncrementalDecode(info, bm.getPixels(),
- bm.rowBytes())) {
- ERRORF(r, "Is scanline decoding now implemented for %s?", file);
- continue;
- }
-
- // Scanline decoding should not require a rewind.
- SkCodec::Result result = codec->startScanlineDecode(info);
- if (SkCodec::kSuccess != result) {
- ERRORF(r, "Scanline decoding failed for %s with %i", file, result);
- }
- }
-}