Make SkPngCodec decode progressively.

src/codec/SkCodec.cpp

Index: src/codec/SkCodec.cpp
diff --git a/src/codec/SkCodec.cpp b/src/codec/SkCodec.cpp
index 6f298700b712c6a4b0f39c5ccd1761048d1384e1..84afc2bdee282b598b76f6f75234d276339712e6 100644
--- a/src/codec/SkCodec.cpp
+++ b/src/codec/SkCodec.cpp
@@ -158,6 +158,8 @@ bool SkCodec::rewindIfNeeded() {
 
     // startScanlineDecode will need to be called before decoding scanlines.
     fCurrScanline = -1;
+    // startIncrementalDecode will need to be called before incrementalDecode.
+    fStartedIncrementalDecode = false;
 
     if (!fStream->rewind()) {
         return false;
@@ -166,6 +168,20 @@ bool SkCodec::rewindIfNeeded() {
     return this->onRewind();
 }
 
+#define CHECK_COLOR_TABLE                                   \
+    if (kIndex_8_SkColorType == info.colorType()) {         \
+        if (nullptr == ctable || nullptr == ctableCount) {  \
+            return SkCodec::kInvalidParameters;             \
+        }                                                   \
+    } else {                                                \
+        if (ctableCount) {                                  \
+            *ctableCount = 0;                               \
+        }                                                   \
+        ctableCount = nullptr;                              \
+        ctable = nullptr;                                   \
+    }
+
+
 SkCodec::Result SkCodec::getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes,
                                    const Options* options, SkPMColor ctable[], int* ctableCount) {
     if (kUnknown_SkColorType == info.colorType()) {
@@ -178,17 +194,7 @@ SkCodec::Result SkCodec::getPixels(const SkImageInfo& info, void* pixels, size_t
         return kInvalidParameters;
     }
 
-    if (kIndex_8_SkColorType == info.colorType()) {
-        if (nullptr == ctable || nullptr == ctableCount) {
-            return kInvalidParameters;
-        }
-    } else {
-        if (ctableCount) {
-            *ctableCount = 0;
-        }
-        ctableCount = nullptr;
-        ctable = nullptr;
-    }
+    CHECK_COLOR_TABLE;
 
     if (!this->rewindIfNeeded()) {
         return kCouldNotRewind;
@@ -213,6 +219,11 @@ SkCodec::Result SkCodec::getPixels(const SkImageInfo& info, void* pixels, size_t
         return kInvalidScale;
     }
 
+    fDstInfo = info;
+    // FIXME: fOptions should be updated to options here, since fillIncompleteImage (called below
+    // in this method) accesses it. Without updating, it uses the old value.
+    //fOptions = *options;

[scroggo_chromium, 2016/09/14 22:01:41]

I haven't figured out what's going on here - the fill code is "working" without
me setting this (so we're using an old value for fOptions - which matters
because fillIncompleteImage uses fSubset), but when I update it (to the correct
subset, I think) we get a memory corruption. (It's pretty consistent, too.
Besides seeing it in
https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-Clang-GCE-CPU-A...,
you can reproduce locally with:

./out/Debug/dm --resourcePath resources/ --images inc1.webp

where inc1.webp is in our test images.)

[msarett, 2016/09/14 22:11:56]

I'll take a look.

[msarett, 2016/09/14 23:20:20]

Additional side note:

We don't have any incomplete pngs on the bots.  This is bad.  Running locally,
it looks like codec currently "works" on incomplete pngs, but doesn't work after
your CL.  Haven't tried to figure out why yet.  I would suggest that we land
this, then add incomplete pngs (with bug fixes).

[msarett, 2016/09/14 23:20:20]

This is an absolute mess (and it's my fault).  We have three different ways of
choosing the "width" to use to fill.  Here they are, by priority.
(1) Whatever the "swizzler" thinks the width is.
(2) Whatever the "subset" width is.
(3) Whatever the "dstInfo" width is.

(1) always seems to work, the swizzler seems to be an absolute source of truth
on the real width.  We run into problems when we don't have a swizzler...

(2) SkJpegCodec needs to use the "subset" width on scanline decodes.  Because
jpegs can do partial scanlines, we may have a case where we have no swizzler,
but are performing a subset decode.  In this case, we should fill with the
"subset" width.

(3) SkWebpCodec needs to *not* use the "subset" width.  The subset will be
specified in terms of the original image (not the scaled image).  We want to
fill with the scaled subset width, which will be stored in dstInfo.

So, by some crazy luck, everything works now - because we only set the subset on
scanline decodes (when jpeg needs the subset width), but not on regular decodes
(when webp needs the dstInfo.width()).

My recommendation would be to not change anything in this CL :).

-----------------------------------------------

How should we actually fix this?  I think we need a consistent interpretation of
what dstInfo and fSubset mean across the entry points to SkCodec.  Potentially
that could be done by changing SkWebpCodec to take the subset in terms of the
scaled image (which, of course, does not match the libwebp API)?

[scroggo_chromium, 2016/09/15 22:34:54]

That's certainly an option, and maybe it's not so bad...

+
     // On an incomplete decode, the subclass will specify the number of scanlines that it decoded
     // successfully.
     int rowsDecoded = 0;
@@ -240,23 +251,78 @@ SkCodec::Result SkCodec::getPixels(const SkImageInfo& info, void* pixels, size_t
     return this->getPixels(info, pixels, rowBytes, nullptr, nullptr, nullptr);
 }
 
-SkCodec::Result SkCodec::startScanlineDecode(const SkImageInfo& dstInfo,
-        const SkCodec::Options* options, SkPMColor ctable[], int* ctableCount) {
-    // Reset fCurrScanline in case of failure.
-    fCurrScanline = -1;
+SkCodec::Result SkCodec::startIncrementalDecode(const SkImageInfo& info, void* pixels,
+        size_t rowBytes, const SkCodec::Options* options, SkPMColor* ctable, int* ctableCount) {
+    fStartedIncrementalDecode = false;
+
+    if (kUnknown_SkColorType == info.colorType()) {
+        return kInvalidConversion;
+    }
+    if (nullptr == pixels) {
+        return kInvalidParameters;
+    }
+
     // Ensure that valid color ptrs are passed in for kIndex8 color type
-    if (kIndex_8_SkColorType == dstInfo.colorType()) {
-        if (nullptr == ctable || nullptr == ctableCount) {
-            return SkCodec::kInvalidParameters;
+    CHECK_COLOR_TABLE;
+
+    // FIXME: If the rows come after the rows of a previous incremental decode,
+    // we might be able to skip the rewind, but only the implementation knows
+    // that. (e.g. PNG will always need to rewind, since we called longjmp, but
+    // a bottom-up BMP could skip rewinding if the new rows are above the old
+    // rows.)
+    if (!this->rewindIfNeeded()) {
+        return kCouldNotRewind;
+    }
+
+    // Set options.
+    Options optsStorage;
+    if (nullptr == options) {
+        options = &optsStorage;
+    } else if (options->fSubset) {
+        SkIRect size = SkIRect::MakeSize(info.dimensions());
+        if (!size.contains(*options->fSubset)) {
+            return kInvalidParameters;
         }
-    } else {
-        if (ctableCount) {
-            *ctableCount = 0;
+
+        const int top = options->fSubset->top();
+        const int bottom = options->fSubset->bottom();
+        if (top < 0 || top >= info.height() || top >= bottom || bottom > info.height()) {
+            return kInvalidParameters;
         }
-        ctableCount = nullptr;
-        ctable = nullptr;
     }
 
+    if (!this->dimensionsSupported(info.dimensions())) {
+        return kInvalidScale;
+    }
+
+    fDstInfo = info;
+    fOptions = *options;
+
+    const Result result = this->onStartIncrementalDecode(info, pixels, rowBytes,
+            fOptions, ctable, ctableCount);
+    if (kSuccess == result) {
+        fStartedIncrementalDecode = true;
+    } else if (kUnimplemented == result) {
+        // FIXME: This is temporarily necessary, until we transition SkCodec
+        // implementations from scanline decoding to incremental decoding.
+        // SkAndroidCodec will first attempt to use incremental decoding, but
+        // will fall back to scanline decoding if incremental returns
+        // kUnimplemented. rewindIfNeeded(), above, set fNeedsRewind to true
+        // (after potentially rewinding), but we do not want the next call to
+        // startScanlineDecode() to do a rewind.
+        fNeedsRewind = false;
+    }
+    return result;
+}
+
+
+SkCodec::Result SkCodec::startScanlineDecode(const SkImageInfo& info,
+        const SkCodec::Options* options, SkPMColor ctable[], int* ctableCount) {
+    // Reset fCurrScanline in case of failure.
+    fCurrScanline = -1;
+    // Ensure that valid color ptrs are passed in for kIndex8 color type
+    CHECK_COLOR_TABLE;
+
     if (!this->rewindIfNeeded()) {
         return kCouldNotRewind;
     }
@@ -266,36 +332,38 @@ SkCodec::Result SkCodec::startScanlineDecode(const SkImageInfo& dstInfo,
     if (nullptr == options) {
         options = &optsStorage;
     } else if (options->fSubset) {
-        SkIRect size = SkIRect::MakeSize(dstInfo.dimensions());
+        SkIRect size = SkIRect::MakeSize(info.dimensions());
         if (!size.contains(*options->fSubset)) {
             return kInvalidInput;
         }
 
         // We only support subsetting in the x-dimension for scanline decoder.
         // Subsetting in the y-dimension can be accomplished using skipScanlines().
-        if (options->fSubset->top() != 0 || options->fSubset->height() != dstInfo.height()) {
+        if (options->fSubset->top() != 0 || options->fSubset->height() != info.height()) {
             return kInvalidInput;
         }
     }
 
     // FIXME: Support subsets somehow?
-    if (!this->dimensionsSupported(dstInfo.dimensions())) {
+    if (!this->dimensionsSupported(info.dimensions())) {
         return kInvalidScale;
     }
 
-    const Result result = this->onStartScanlineDecode(dstInfo, *options, ctable, ctableCount);
+    const Result result = this->onStartScanlineDecode(info, *options, ctable, ctableCount);
     if (result != SkCodec::kSuccess) {
         return result;
     }
 
     fCurrScanline = 0;
-    fDstInfo = dstInfo;
+    fDstInfo = info;
     fOptions = *options;
     return kSuccess;
 }
 
-SkCodec::Result SkCodec::startScanlineDecode(const SkImageInfo& dstInfo) {
-    return this->startScanlineDecode(dstInfo, nullptr, nullptr, nullptr);
+#undef CHECK_COLOR_TABLE
+
+SkCodec::Result SkCodec::startScanlineDecode(const SkImageInfo& info) {
+    return this->startScanlineDecode(info, nullptr, nullptr, nullptr);
 }
 
 int SkCodec::getScanlines(void* dst, int countLines, size_t rowBytes) {
@@ -343,7 +411,6 @@ int SkCodec::outputScanline(int inputScanline) const {
 int SkCodec::onOutputScanline(int inputScanline) const {
     switch (this->getScanlineOrder()) {
         case kTopDown_SkScanlineOrder:
-        case kNone_SkScanlineOrder:
             return inputScanline;
         case kBottomUp_SkScanlineOrder:
             return this->getInfo().height() - inputScanline - 1;
@@ -393,8 +460,7 @@ void SkCodec::fillIncompleteImage(const SkImageInfo& info, void* dst, size_t row
     }
 
     switch (this->getScanlineOrder()) {
-        case kTopDown_SkScanlineOrder:
-        case kNone_SkScanlineOrder: {
+        case kTopDown_SkScanlineOrder: {
             const SkImageInfo fillInfo = info.makeWH(fillWidth, linesRemaining);
             fillDst = SkTAddOffset<void>(dst, linesDecoded * rowBytes);
             fill_proc(fillInfo, fillDst, rowBytes, fillValue, zeroInit, sampler);