Incremental decode: only use subset for subsetting

In the initial patch, we modified the subset passed to incremental
decoding to account for sampling in Y. This allowed SkSampledCodec to
use a tighter bounds. But it also means that the implementation cannot
distinguish between lines that are excluded due to subsetting and those
excluded at the beginning and end due to sampling. This becomes
problematic in GIF (crrev.com/2045293002), which may need to fill,
requiring it to reconstruct the actual destination. In truth,
SkGifCodec does not need to support subsets, but cannot distinguish
between the tighter bounds and a true subset.

Fix this by passing the scaled subset to incremental decode, without
using the tighter bounds.

Make SkSampler::rowNeeded take the starting coordinate into account.

In SkPngCodec, compute the number of rows needed in the output, and use
that as a signal to stop.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2420843003

Review-Url: https://codereview.chromium.org/2420843003

3 files changed, 23 insertions, 33 deletions

diff --git a/src/codec/SkPngCodec.cpp b/src/codec/SkPngCodec.cpp
index 9f8722f787..7083137d58 100644
--- a/src/codec/SkPngCodec.cpp
+++ b/src/codec/SkPngCodec.cpp
@@ -458,7 +458,6 @@ public:
     SkPngNormalDecoder(const SkEncodedInfo& info, const SkImageInfo& imageInfo, SkStream* stream,
             SkPngChunkReader* reader, png_structp png_ptr, png_infop info_ptr, int bitDepth)
         : INHERITED(info, imageInfo, stream, reader, png_ptr, info_ptr, bitDepth)
-        , fLinesDecoded(0)
         , fRowsWrittenToOutput(0)
         , fDst(nullptr)
         , fRowBytes(0)
@@ -481,12 +480,6 @@ public:
 #endif
 
 private:
-    // This represents the number of lines reported by libpng, minus any we skipped at the
-    // beginning. Only used when we are skipping lines (i.e. not in decodeAllRows).
-    int                         fLinesDecoded;
-    // While fLinesDecoded include lines that we skipped, this only includes lines written to the
-    // output so we can report it to the caller for filling.
-    // FIXME: Can we remove fLinesDecoded and just rely on fRowsWrittenToOutput?
     int                         fRowsWrittenToOutput;
     void*                       fDst;
     size_t                      fRowBytes;
@@ -494,6 +487,7 @@ private:
     // Variables for partial decode
     int                         fFirstRow;  // FIXME: Move to baseclass?
     int                         fLastRow;
+    int                         fRowsNeeded;
 
     typedef SkPngCodec INHERITED;
 
@@ -545,14 +539,18 @@ private:
         fLastRow = lastRow;
         fDst = dst;
         fRowBytes = rowBytes;
-        fLinesDecoded = 0;
         fRowsWrittenToOutput = 0;
+        fRowsNeeded = fLastRow - fFirstRow + 1;
     }
 
     SkCodec::Result decode(int* rowsDecoded) override {
+        if (this->swizzler()) {
+            const int sampleY = this->swizzler()->sampleY();
+            fRowsNeeded = get_scaled_dimension(fLastRow - fFirstRow + 1, sampleY);
+        }
         this->processData();
 
-        if (fLinesDecoded == fLastRow - fFirstRow + 1) {
+        if (fRowsWrittenToOutput == fRowsNeeded) {
             return SkCodec::kSuccess;
         }
 
@@ -570,17 +568,16 @@ private:
         }
 
         SkASSERT(rowNum <= fLastRow);
+        SkASSERT(fRowsWrittenToOutput < fRowsNeeded);
 
         // If there is no swizzler, all rows are needed.
-        if (!this->swizzler() || this->swizzler()->rowNeeded(fLinesDecoded)) {
+        if (!this->swizzler() || this->swizzler()->rowNeeded(rowNum - fFirstRow)) {
             this->applyXformRow(fDst, row);
             fDst = SkTAddOffset<void>(fDst, fRowBytes);
             fRowsWrittenToOutput++;
         }
 
-        fLinesDecoded++;
-
-        if (rowNum == fLastRow) {
+        if (fRowsWrittenToOutput == fRowsNeeded) {
             // Fake error to stop decoding scanlines.
             longjmp(PNG_JMPBUF(this->png_ptr()), kStopDecoding);
         }
@@ -722,17 +719,17 @@ private:
             }
             return SkCodec::kIncompleteInput;
         }
-        const int lastRow = fLinesDecoded + fFirstRow - 1;
-        SkASSERT(lastRow <= fLastRow);
 
+        const int sampleY = this->swizzler() ? this->swizzler()->sampleY() : 1;
+        const int rowsNeeded = get_scaled_dimension(fLastRow - fFirstRow + 1, sampleY);
         int rowsWrittenToOutput = 0;
 
         // FIXME: For resuming interlace, we may swizzle a row that hasn't changed. But it
         // may be too tricky/expensive to handle that correctly.
         png_bytep srcRow = fInterlaceBuffer.get();
-        const int sampleY = this->swizzler() ? this->swizzler()->sampleY() : 1;
         void* dst = fDst;
-        for (int rowNum = fFirstRow; rowNum <= lastRow; rowNum += sampleY) {
+        for (int rowNum = fFirstRow + get_start_coord(sampleY); rowsWrittenToOutput < rowsNeeded;
+                rowNum += sampleY) {
             this->applyXformRow(dst, srcRow);
             dst = SkTAddOffset<void>(dst, fRowBytes);
             srcRow = SkTAddOffset<png_byte>(srcRow, fPng_rowbytes * sampleY);
diff --git a/src/codec/SkSampledCodec.cpp b/src/codec/SkSampledCodec.cpp
index 46286cff9d..22b5cfae5a 100644
--- a/src/codec/SkSampledCodec.cpp
+++ b/src/codec/SkSampledCodec.cpp
@@ -186,7 +186,7 @@ SkCodec::Result SkSampledCodec::sampledDecode(const SkImageInfo& info, void* pix
         // We will need to know about subsetting in the y-dimension in order to use the
         // scanline decoder.
         // Update the subset to account for scaling done by this->codec().
-        SkIRect* subsetPtr = options.fSubset;
+        const SkIRect* subsetPtr = options.fSubset;
 
         // Do the divide ourselves, instead of calling get_scaled_dimension. If
         // X and Y are 0, they should remain 0, rather than being upgraded to 1
@@ -210,7 +210,7 @@ SkCodec::Result SkSampledCodec::sampledDecode(const SkImageInfo& info, void* pix
 
     const int samplingOffsetY = get_start_coord(sampleY);
     const int startY = samplingOffsetY + subsetY;
-    int dstHeight = info.height();
+    const int dstHeight = info.height();
 
     const SkImageInfo nativeInfo = info.makeWH(nativeSize.width(), nativeSize.height());
 
@@ -218,22 +218,15 @@ SkCodec::Result SkSampledCodec::sampledDecode(const SkImageInfo& info, void* pix
         // Although startScanlineDecode expects the bottom and top to match the
         // SkImageInfo, startIncrementalDecode uses them to determine which rows to
         // decode.
-        // Note: We *could* use "subsetY" and "subsetHeight" (calculated above) for
-        // incrementalSubset, but this code gives us a tighter bounds on the subset,
-        // meaning that we can start with the first row actually needed by the output,
-        // and stop when we've decoded the last row needed by the output.
+        SkCodec::Options incrementalOptions = sampledOptions;
         SkIRect incrementalSubset;
-        incrementalSubset.fTop = startY;
-        incrementalSubset.fBottom = startY + (dstHeight - 1) * sampleY + 1;
         if (sampledOptions.fSubset) {
+            incrementalSubset.fTop = subsetY;
+            incrementalSubset.fBottom = subsetY + subsetHeight;
             incrementalSubset.fLeft = sampledOptions.fSubset->fLeft;
             incrementalSubset.fRight = sampledOptions.fSubset->fRight;
-        } else {
-            incrementalSubset.fLeft = 0;
-            incrementalSubset.fRight = nativeSize.width();
+            incrementalOptions.fSubset = &incrementalSubset;
         }
-        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) {
diff --git a/src/codec/SkSampler.h b/src/codec/SkSampler.h
index 00015585a1..e7bec0c035 100644
--- a/src/codec/SkSampler.h
+++ b/src/codec/SkSampler.h
@@ -8,6 +8,7 @@
 #define SkSampler_DEFINED
 
 #include "SkCodec.h"
+#include "SkCodecPriv.h"
 #include "SkTypes.h"
 
 class SkSampler : public SkNoncopyable {
@@ -37,11 +38,10 @@ public:
     /**
      *  Based on fSampleY, return whether this row belongs in the output.
      *
-     *  @param row Row of the image, starting with the first row used in the
-     *      output.
+     *  @param row Row of the image, starting with the first row in the subset.
      */
     bool rowNeeded(int row) const {
-        return row % fSampleY == 0;
+        return (row + get_start_coord(fSampleY)) % fSampleY == 0;
     }
 
     /**