Make SkPngCodec decode progressively.

src/codec/SkPngCodec.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkBitmap.h"
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
@@ skipping 14 matching lines @@
 
 static void sk_error_fn(png_structp png_ptr, png_const_charp msg) {
     SkCodecPrintf("------ png error %s\n", msg);
     longjmp(png_jmpbuf(png_ptr), 1);
 }
 
 void sk_warning_fn(png_structp, png_const_charp msg) {
     SkCodecPrintf("----- png warning %s\n", msg);
 }
 
-static void sk_read_fn(png_structp png_ptr, png_bytep data,
-                       png_size_t length) {
-    SkStream* stream = static_cast<SkStream*>(png_get_io_ptr(png_ptr));
-    const size_t bytes = stream->read(data, length);
-    if (bytes != length) {
-        // FIXME: We want to report the fact that the stream was truncated.
-        // One way to do that might be to pass a enum to longjmp so setjmp can
-        // specify the failure.
-        png_error(png_ptr, "Read Error!");
-    }
-}
-
 #ifdef PNG_READ_UNKNOWN_CHUNKS_SUPPORTED
 static int sk_read_user_chunk(png_structp png_ptr, png_unknown_chunkp chunk) {
     SkPngChunkReader* chunkReader = (SkPngChunkReader*)png_get_user_chunk_ptr(png_ptr);
     // readChunk() returning true means continue decoding
     return chunkReader->readChunk((const char*)chunk->name, chunk->data, chunk->size) ? 1 : -1;
 }
 #endif
 
 ///////////////////////////////////////////////////////////////////////////////
 // Helpers
 ///////////////////////////////////////////////////////////////////////////////
 
 class AutoCleanPng : public SkNoncopyable {
 public:
-    AutoCleanPng(png_structp png_ptr)
+    /*
+     *  This class does not take ownership of stream or reader, but if codecPtr
+     *  is non-NULL, and decodeBounds succeeds, it will have created a new
+     *  SkCodec (pointed to by *codecPtr) which will own them, as well as the

[msarett, 2016/05/20 15:04:40]

Codec refs but doesn't own the chunkReader?

[scroggo_chromium, 2016/05/20 16:51:59]

I'm not sure what you're asking. Do you want a clearer comment here?

Technically, yes, the Codec will ref the chunkReader, and may not be the only
owner.  How about this:

"... a new SkCodec (pointed to by *codecPtr) which will own/ref them, ..."

[msarett, 2016/05/20 18:21:38]

Yes thanks.

+     *  png_ptr and info_ptr.
+     */
+    AutoCleanPng(png_structp png_ptr, SkStream* stream, SkPngChunkReader* reader,
+            SkCodec** codecPtr)
         : fPng_ptr(png_ptr)
-        , fInfo_ptr(nullptr) {}
+        , fInfo_ptr(nullptr)
+        , fDecodedBounds(false)
+        , fStream(stream)
+        , fChunkReader(reader)
+        , fOutCodec(codecPtr)
+    {}
 
     ~AutoCleanPng() {
         // fInfo_ptr will never be non-nullptr unless fPng_ptr is.
         if (fPng_ptr) {
             png_infopp info_pp = fInfo_ptr ? &fInfo_ptr : nullptr;
             png_destroy_read_struct(&fPng_ptr, info_pp, nullptr);
         }
     }
 
     void setInfoPtr(png_infop info_ptr) {
         SkASSERT(nullptr == fInfo_ptr);
         fInfo_ptr = info_ptr;
     }
 
-    void release() {
+    /**
+     *  Reads enough of the input stream to decode the bounds.
+     *  @return false if the stream is not a valid PNG (or too short).
+     *          true if it read enough of the stream to determine the bounds.
+     *          In the latter case, the stream may have been read beyond the
+     *          point to determine the bounds, and the png_ptr will have saved
+     *          any extra data. Further, if the codecPtr supplied to the
+     *          constructor was not NULL, it will now point to a new SkCodec,
+     *          which owns (or refs, in the case of the SkPngChunkReader) the
+     *          inputs. If codecPtr was NULL, the png_ptr and info_ptr are
+     *          unowned, and it is up to the caller to destroy them.
+     */
+    bool decodeBounds();
+
+    /**
+     *  Supplied to libpng to call when it has read enough data to determine
+     *  bounds.
+     */
+    static void InfoCallback(png_structp png_ptr, png_infop info_ptr) {
+        static_cast<AutoCleanPng*>(png_get_progressive_ptr(png_ptr))->infoCallback();
+    }
+
+private:
+    png_structp         fPng_ptr;
+    png_infop           fInfo_ptr;
+    bool                fDecodedBounds;
+    SkStream*           fStream;
+    SkPngChunkReader*   fChunkReader;
+    SkCodec**           fOutCodec;
+
+    void infoCallback();
+
+    void releasePngPtrs() {
         fPng_ptr = nullptr;
         fInfo_ptr = nullptr;
     }
-
-private:
-    png_structp     fPng_ptr;
-    png_infop       fInfo_ptr;
 };
 #define AutoCleanPng(...) SK_REQUIRE_LOCAL_VAR(AutoCleanPng)
 
+bool AutoCleanPng::decodeBounds() {
+    if (setjmp(png_jmpbuf(fPng_ptr))) {
+            return false;
+    }
+
+    // Arbitrary buffer size, though note that it matches (below)
+    // SkPngCodec::processData(). FIXME: Can we better suit this to the size of
+    // the PNG header?
+    constexpr size_t kBufferSize = 4096;
+    char buffer[kBufferSize];
+
+    while (true) {
+        const size_t bytesRead = fStream->read(buffer, kBufferSize);
+        if (!bytesRead) {
+            // We have read to the end of the input without decoding bounds.
+            return false;
+        }
+
+        png_process_data(fPng_ptr, fInfo_ptr, (png_bytep) buffer, bytesRead);
+        if (fDecodedBounds) {
+            return true;
+        }
+    }
+}
+
+void SkPngCodec::processData() {
+    if (setjmp(png_jmpbuf(fPng_ptr))) {
+        // FIXME: I think we need to throw out the png_ptr at this point.
+        // A straightforward way to do that might be to set a flag telling
+        // ourselves that we need to rewind. This makes sense if we
+        // deliberately called png_error (i.e. when we decoded as many
+        // scanlines as we wanted to), but maybe not otherwise (so maybe
+        // we should do something there? what to do here?)
+        return;
+    }
+
+    // Arbitrary buffer size
+    constexpr size_t kBufferSize = 4096;
+    char buffer[kBufferSize];
+
+    while (true) {
+        const size_t bytesRead = this->stream()->read(buffer, kBufferSize);
+        png_process_data(fPng_ptr, fInfo_ptr, (png_bytep) buffer, bytesRead);
+
+        if (!bytesRead) {
+            // We have read to the end of the input. Note that we quit *after*
+            // calling png_process_data, because decodeBounds may have told
+            // libpng to save the remainder of the buffer, in which case
+            // png_process_data will process the saved buffer, though the
+            // stream has no more to read.
+            break;
+        }
+    }
+}
+
 // Note: SkColorTable claims to store SkPMColors, which is not necessarily
 // the case here.
-// TODO: If we add support for non-native swizzles, we'll need to handle that here.
 bool SkPngCodec::createColorTable(SkColorType dstColorType, bool premultiply, int* ctableCount) {
 
     int numColors;
     png_color* palette;
     if (!png_get_PLTE(fPng_ptr, fInfo_ptr, &palette, &numColors)) {
         return false;
     }
 
     // Note: These are not necessarily SkPMColors.
     SkPMColor colorPtr[256];
@@ skipping 152 matching lines @@
 
 #endif // LIBPNG >= 1.6
 
     // Finally, what should we do if there is no color space information in the PNG?
     // The specification says that this indicates "gamma is unknown" and that the
     // "color is device dependent".  I'm assuming we can represent this with NULL.
     // But should we guess sRGB?  Most images are sRGB, even if they don't specify.
     return nullptr;
 }
 
-static int bytes_per_pixel(int bitsPerPixel) {
-    // Note that we will have to change this implementation if we start
-    // supporting outputs from libpng that are less than 8-bits per component.
-    return bitsPerPixel / 8;
-}
-
-// Subclass of SkPngCodec which supports scanline decoding
-class SkPngScanlineDecoder : public SkPngCodec {
+class SkPngNormalDecoder : public SkPngCodec {
 public:
-    SkPngScanlineDecoder(int width, int height, const SkEncodedInfo& info, SkStream* stream,
-            SkPngChunkReader* chunkReader, png_structp png_ptr, png_infop info_ptr, int bitDepth,
+    SkPngNormalDecoder(int width, int height, const SkEncodedInfo& info, SkStream* stream,
+            SkPngChunkReader* reader, png_structp png_ptr, png_infop info_ptr, int bitDepth,
             sk_sp<SkColorSpace> colorSpace)
-        : INHERITED(width, height, info, stream, chunkReader, png_ptr, info_ptr, bitDepth, 1,
-                    colorSpace)
-        , fSrcRow(nullptr)
+        : INHERITED(width, height, info, stream, reader, png_ptr, info_ptr, bitDepth,
+                std::move(colorSpace))

[msarett, 2016/05/20 15:04:40]

Why std::move?

[scroggo_chromium, 2016/05/20 16:51:59]

My understanding of sk_sp is that if I do not std::move it, there will be
unnecessary ref count churn:

The constructor (INHERITED) takes an sk_sp<SkColorSpace>. If I pass it
colorSpace, it will call the copy constructor which will call ref. At the end of
this constructor, colorSpace will go out of scope and call unref().

On the other hand, if I call std::move, INHERITED will call the move
constructor, which will not call ref/unref.

Is my understanding correct?

[msarett, 2016/05/20 18:21:38]

That makes sense to me - thanks for the explanation.  I should be more
thoughtful about using std::move also...

+        , fLinesDecoded(0)
+        , fDst(nullptr)
+        , fRowBytes(0)
+        , fFirstRow(0)
+        , fLastRow(0)
+        , fCallback(nullptr)
     {}
 
-    Result onStartScanlineDecode(const SkImageInfo& dstInfo, const Options& options,
-            SkPMColor ctable[], int* ctableCount) override {
-        if (!conversion_possible(dstInfo, this->getInfo())) {
-            return kInvalidConversion;
-        }
-
-        const Result result = this->initializeSwizzler(dstInfo, options, ctable,
-                                                       ctableCount);
-        if (result != kSuccess) {
-            return result;
-        }
-
-        fStorage.reset(this->getInfo().width() *
-                (bytes_per_pixel(this->getEncodedInfo().bitsPerPixel())));
-        fSrcRow = fStorage.get();
-
-        return kSuccess;
-    }
-
-    int onGetScanlines(void* dst, int count, size_t rowBytes) override {
-        // Assume that an error in libpng indicates an incomplete input.
-        int row = 0;
-        if (setjmp(png_jmpbuf(this->png_ptr()))) {
-            SkCodecPrintf("setjmp long jump!\n");
-            return row;
-        }
-
-        void* dstRow = dst;
-        for (; row < count; row++) {
-            png_read_row(this->png_ptr(), fSrcRow, nullptr);
-            this->swizzler()->swizzle(dstRow, fSrcRow);
-            dstRow = SkTAddOffset<void>(dstRow, rowBytes);
-        }
-
-        return row;
-    }
-
-    bool onSkipScanlines(int count) override {
-        // Assume that an error in libpng indicates an incomplete input.
-        if (setjmp(png_jmpbuf(this->png_ptr()))) {
-            SkCodecPrintf("setjmp long jump!\n");
-            return false;
-        }
-
-        for (int row = 0; row < count; row++) {
-            png_read_row(this->png_ptr(), fSrcRow, nullptr);
-        }
-        return true;
-    }
-
+    static void AllRowsCallback(png_structp png_ptr, png_bytep row, png_uint_32 rowNum, int /*pass*/) {
+        GetDecoder(png_ptr)->allRowsCallback(row, rowNum);
+    }
+
+    static void RowCallback(png_structp png_ptr, png_bytep row, png_uint_32 rowNum, int /*pass*/) {
+        GetDecoder(png_ptr)->rowCallback(row, rowNum);
+    }
 private:
-    SkAutoTMalloc<uint8_t>      fStorage;
-    uint8_t*                    fSrcRow;
+    int                         fLinesDecoded; // FIXME: Move to baseclass?
+
+    // Variables for decodeAllRows/allRowsCallback
+    void*                       fDst;
+    size_t                      fRowBytes;
+
+    // Variables for partial decode
+    int                         fFirstRow;  // FIXME: Move to baseclass?
+    int                         fLastRow;
+    std::function<void*(int)>   fCallback;
 
     typedef SkPngCodec INHERITED;
+
+    static SkPngNormalDecoder* GetDecoder(png_structp png_ptr) {
+        return static_cast<SkPngNormalDecoder*>(png_get_progressive_ptr(png_ptr));

[msarett, 2016/05/20 15:04:41]

I think I've figured it out, but maybe comment on what does
png_get_progressive_ptr does?

[scroggo_chromium, 2016/05/20 16:51:59]

I'm not sure it's necessary to document the methods we call in other libraries
(libpng has its own documentation for this method, FWIW). As a compromise, I've
added a comment the first time I use it.

You also made me question the fact that read_header set the progressive_ptr, and
AutoCleanPng uses it. I've moved that instance into AutoCleanPng.

[msarett, 2016/05/20 18:21:38]

Acknowledged.

+    }
+
+    Result decodeAllRows(void* dst, size_t rowBytes, int* rowsDecoded) override {
+        const int height = this->getInfo().height();
+        png_set_progressive_read_fn(this->png_ptr(), this, nullptr, AllRowsCallback, nullptr);
+        fDst = dst;
+        fRowBytes = rowBytes;
+
+        // FIXME: Make it possible to continue decoding all rows?

[msarett, 2016/05/20 15:04:40]

What does this mean?

[scroggo_chromium, 2016/05/20 16:51:59]

This comment is probably out of place here. Removed.

(FWIW, it means that I think it would be nice to have a way for the client to
say "I want all rows, incrementally". I imagined they might do that by calling
getPixels(), but we don't support that yet (a little more thoughts at [1]). If
we were to add that support, and continue calling through decodeAllRows, we
would need to not reset this here.)

[1]
https://docs.google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_Cu8DF57KNtUmg...

[msarett, 2016/05/20 18:21:38]

Acknowledged.

+        fLinesDecoded = 0;
+
+        this->processData();
+
+        if (fLinesDecoded == height) {
+            return SkCodec::kSuccess;
+        }
+
+        if (rowsDecoded) {

[msarett, 2016/05/20 15:04:41]

Should we set rowDecoded even if it's a success?

[scroggo_chromium, 2016/05/20 16:51:59]

I don't think so. On success the client need not read this variable.

FWIW, onGetPixels already behaves this way. My comment in the header does not
specify this, though, so I have updated it.

[msarett, 2016/05/20 18:21:38]

Acknowledged.

+            *rowsDecoded = fLinesDecoded;
+        }
+
+        return SkCodec::kIncompleteInput;
+    }
+
+    void allRowsCallback(png_bytep row, int rowNum) {
+        SkASSERT(rowNum - fFirstRow == fLinesDecoded);
+        fLinesDecoded++;
+        this->swizzler()->swizzle(fDst, row);
+        fDst = SkTAddOffset<void>(fDst, fRowBytes);
+    }
+
+    void setRange(int firstRow, int lastRow) override {
+        png_set_progressive_read_fn(this->png_ptr(), this, nullptr, RowCallback, nullptr);
+        fFirstRow = firstRow;
+        fLastRow = lastRow;
+        fLinesDecoded = 0;
+    }
+
+    SkCodec::Result decode(std::function<void*(int)> callback, int* rowsDecoded) override {
+        fCallback = callback;
+
+        this->processData();
+
+        if (fLinesDecoded == fLastRow - fFirstRow + 1) {
+            return SkCodec::kSuccess;
+        }
+
+        if (rowsDecoded) {
+            *rowsDecoded = fLinesDecoded;
+        }
+
+        return SkCodec::kIncompleteInput;
+    }
+
+    void rowCallback(png_bytep row, int rowNum) {
+        if (rowNum < fFirstRow) {
+            // Ignore this row.
+            return;
+        }
+
+        SkASSERT(rowNum <= fLastRow);
+
+        fLinesDecoded++;
+        void* dst = fCallback(rowNum);
+        if (dst) {
+            this->swizzler()->swizzle(dst, row);
+        }
+
+        if (rowNum == fLastRow) {
+            png_error(this->png_ptr(), "Fake error to stop decoding scanlines.");

[msarett, 2016/05/20 15:04:41]

Can we pass more info to this call so, when we catch it in process data, we know
that we are the ones who called it?

Will that change how we handle it?

I'm happy for this to be a follow up.

[scroggo_chromium, 2016/05/20 16:51:59]

Not to png_error, but we could call longjmp directly, which would allow us to
pass a special value. (png_error calls a method we specify. We make it call
sk_error_fn, which arbitrarily uses the value 1. We could specify 1 there and
something else here.)
I'm not sure...
sgtm.

[scroggo_chromium, 2016/05/23 21:00:10]

Patch set 6 calls longjmp directly, and uses different values. 

I turned on SkCodecPrintf, and saw a lot of output from all those png_errors, so
I switched to longjmp. As long as I was doing that switch, I used different
values for actual error versus stop decoding. PTAL.

+        }
+    }
 };
 
-
-class SkPngInterlacedScanlineDecoder : public SkPngCodec {
+class SkPngInterlacedDecoder : public SkPngCodec {
 public:
-    SkPngInterlacedScanlineDecoder(int width, int height, const SkEncodedInfo& info,
-            SkStream* stream, SkPngChunkReader* chunkReader, png_structp png_ptr,
-            png_infop info_ptr, int bitDepth, int numberPasses, sk_sp<SkColorSpace> colorSpace)
-        : INHERITED(width, height, info, stream, chunkReader, png_ptr, info_ptr, bitDepth,
-                    numberPasses, colorSpace)
-        , fHeight(-1)
-        , fCanSkipRewind(false)
-    {
-        SkASSERT(numberPasses != 1);
-    }
-
-    Result onStartScanlineDecode(const SkImageInfo& dstInfo, const Options& options,
-            SkPMColor ctable[], int* ctableCount) override {
-        if (!conversion_possible(dstInfo, this->getInfo())) {
-            return kInvalidConversion;
-        }
-
-        const Result result = this->initializeSwizzler(dstInfo, options, ctable,
-                                                       ctableCount);
-        if (result != kSuccess) {
-            return result;
-        }
-
-        fHeight = dstInfo.height();
-        // FIXME: This need not be called on a second call to onStartScanlineDecode.
-        fSrcRowBytes = this->getInfo().width() *
-                (bytes_per_pixel(this->getEncodedInfo().bitsPerPixel()));
-        fGarbageRow.reset(fSrcRowBytes);
-        fGarbageRowPtr = static_cast<uint8_t*>(fGarbageRow.get());
-        fCanSkipRewind = true;
-
-        return SkCodec::kSuccess;
-    }
-
-    int onGetScanlines(void* dst, int count, size_t dstRowBytes) override {
-        // rewind stream if have previously called onGetScanlines,
-        // since we need entire progressive image to get scanlines
-        if (fCanSkipRewind) {
-            // We already rewound in onStartScanlineDecode, so there is no reason to rewind.
-            // Next time onGetScanlines is called, we will need to rewind.
-            fCanSkipRewind = false;
+    SkPngInterlacedDecoder(int width, int height, const SkEncodedInfo& info, SkStream* stream,
+            SkPngChunkReader* reader, png_structp png_ptr, png_infop info_ptr, int bitDepth,
+            sk_sp<SkColorSpace> colorSpace, int numberPasses)
+        : INHERITED(width, height, info, stream, reader, png_ptr, info_ptr, bitDepth,
+                std::move(colorSpace))
+        , fNumberPasses(numberPasses)
+        , fFirstRow(0)
+        , fLastRow(0)
+        , fLinesDecoded(0)
+        , fInterlacedComplete(false)
+        , fPng_rowbytes(0)
+    {}
+
+    static void InterlacedRowCallback(png_structp png_ptr, png_bytep row, png_uint_32 rowNum, int pass) {
+        auto decoder = static_cast<SkPngInterlacedDecoder*>(png_get_progressive_ptr(png_ptr));
+        decoder->interlacedRowCallback(row, rowNum, pass);
+    }
+
+private:
+    const int               fNumberPasses;
+    int                     fFirstRow;
+    int                     fLastRow;
+    int                     fLinesDecoded;
+    bool                    fInterlacedComplete;
+    png_uint_32             fPng_rowbytes;
+    SkAutoTMalloc<png_byte> fInterlaceBuffer;
+
+    typedef SkPngCodec INHERITED;
+
+    // FIXME: Currently sharing interlaced callback for all rows and subset. It's not
+    // as expensive as the subset version of non-interlaced, but it still does extra
+    // work.
+    void interlacedRowCallback(png_bytep row, int rowNum, int pass) {
+        if (rowNum < fFirstRow || rowNum > fLastRow) {
+            // Ignore this row
+            return;
+        }
+
+        png_bytep oldRow = fInterlaceBuffer.get() + (rowNum - fFirstRow) * fPng_rowbytes;
+        png_progressive_combine_row(this->png_ptr(), oldRow, row);
+
+        if (0 == pass) {
+            // The first pass initializes all rows.
+            SkASSERT(row);
+            SkASSERT(fLinesDecoded == rowNum - fFirstRow);
+            fLinesDecoded++;
         } else {
-            // rewindIfNeeded resets fCurrScanline, since it assumes that start
-            // needs to be called again before scanline decoding. PNG scanline
-            // decoding is the exception, since it needs to rewind between
-            // calls to getScanlines. Keep track of fCurrScanline, to undo the
-            // reset.
-            const int currScanline = this->nextScanline();
-            // This method would never be called if currScanline is -1
-            SkASSERT(currScanline != -1);
-
-            if (!this->rewindIfNeeded()) {
-                return kCouldNotRewind;
+            SkASSERT(fLinesDecoded == fLastRow - fFirstRow + 1);
+            if (fNumberPasses - 1 == pass && rowNum == fLastRow) {
+                // Last pass, and we have read all of the rows we care about. Note that
+                // we do not care about reading anything beyond the end of the image (or
+                // beyond the last scanline requested).
+                fInterlacedComplete = true;
+                png_error(this->png_ptr(), "Fake error to stop interlaced scanline decoding");
             }
-            this->updateCurrScanline(currScanline);
-        }
-
-        if (setjmp(png_jmpbuf(this->png_ptr()))) {
-            SkCodecPrintf("setjmp long jump!\n");
-            // FIXME (msarett): Returning 0 is pessimistic.  If we can complete a single pass,
-            // we may be able to report that all of the memory has been initialized.  Even if we
-            // fail on the first pass, we can still report than some scanlines are initialized.
-            return 0;
-        }
-        SkAutoTMalloc<uint8_t> storage(count * fSrcRowBytes);
-        uint8_t* storagePtr = storage.get();
-        uint8_t* srcRow;
-        const int startRow = this->nextScanline();
-        for (int i = 0; i < this->numberPasses(); i++) {
-            // read rows we planned to skip into garbage row
-            for (int y = 0; y < startRow; y++){
-                png_read_row(this->png_ptr(), fGarbageRowPtr, nullptr);
+        }
+    }
+
+    SkCodec::Result decodeAllRows(void* dst, size_t rowBytes, int* rowsDecoded) override {
+        const int height = this->getInfo().height();
+        this->setUpInterlaceBuffer(height);
+        png_set_progressive_read_fn(this->png_ptr(), this, nullptr, InterlacedRowCallback, nullptr);
+
+        // FIXME: Make it possible to continue decoding all rows?
+        fFirstRow = 0;
+        fLastRow = height - 1;
+        fLinesDecoded = 0;
+
+        this->processData();
+
+        png_bytep srcRow = fInterlaceBuffer.get();
+        // FIXME: When resuming, this may rewrite rows that did not change.
+        for (int rowNum = 0; rowNum <= fLastRow; rowNum++) {
+            this->swizzler()->swizzle(dst, srcRow);
+            dst = SkTAddOffset<void>(dst, rowBytes);
+            srcRow = SkTAddOffset<png_byte>(srcRow, fPng_rowbytes);
+        }
+        if (fInterlacedComplete) {
+            return SkCodec::kSuccess;
+        }
+
+        if (rowsDecoded) {

[msarett, 2016/05/20 15:04:40]

Same question as above...  Should we still set rowsDecoded on a success?

[scroggo_chromium, 2016/05/20 16:51:59]

Same answer as above :) I don't think we need to, since there's no reason for
the client to read it on kSuccess.

+            *rowsDecoded = fLinesDecoded;

[msarett, 2016/05/20 15:04:40]

This is what we want for filling...  But is it weird for a client to think that
we have decoded most/all of the rows when there are more interlaced passes to
come.

[scroggo_chromium, 2016/05/20 16:51:59]

Perhaps weird, but I don't know why else the client would want to know how many
rows were decoded.

I've updated the comment in the header to discuss this behavior. Perhaps I
should go further and change the name to rowsInitialized?

[msarett, 2016/05/20 18:21:38]

Thanks for the comment.  I don't feel strongly about the name.

+        }
+
+        return SkCodec::kIncompleteInput;
+    }
+
+    void setRange(int firstRow, int lastRow) override {
+        this->setUpInterlaceBuffer(lastRow - firstRow + 1);
+        png_set_progressive_read_fn(this->png_ptr(), this, nullptr, InterlacedRowCallback, nullptr);
+        fFirstRow = firstRow;
+        fLastRow = lastRow;
+        fLinesDecoded = 0;
+    }
+
+    SkCodec::Result decode(std::function<void*(int)> callback, int* rowsDecoded) override {
+        this->processData();
+
+        // Now call the callback on all the rows that were decoded.
+        if (!fLinesDecoded) {
+            return SkCodec::kIncompleteInput;
+        }
+        const int lastRow = fLinesDecoded + fFirstRow - 1;
+        SkASSERT(lastRow <= fLastRow);
+
+        // 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();
+        for (int rowNum = fFirstRow; rowNum <= lastRow; rowNum++, srcRow += fPng_rowbytes) {
+            void* dst = callback(rowNum);
+            if (dst) {
+                this->swizzler()->swizzle(dst, srcRow);
             }
-            // read rows we care about into buffer
-            srcRow = storagePtr;
-            for (int y = 0; y < count; y++) {
-                png_read_row(this->png_ptr(), srcRow, nullptr);
-                srcRow += fSrcRowBytes;
-            }
-            // read rows we don't want into garbage buffer
-            for (int y = 0; y < fHeight - startRow - count; y++) {
-                png_read_row(this->png_ptr(), fGarbageRowPtr, nullptr);
-            }
-        }
-        //swizzle the rows we care about
-        srcRow = storagePtr;
-        void* dstRow = dst;
-        for (int y = 0; y < count; y++) {
-            this->swizzler()->swizzle(dstRow, srcRow);
-            dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
-            srcRow += fSrcRowBytes;
-        }
-
-        return count;
-    }
-
-    bool onSkipScanlines(int count) override {
-        // The non-virtual version will update fCurrScanline.
-        return true;
-    }
-
-    SkScanlineOrder onGetScanlineOrder() const override {
-        return kNone_SkScanlineOrder;
-    }
-
-private:
-    int                         fHeight;
-    size_t                      fSrcRowBytes;
-    SkAutoMalloc                fGarbageRow;
-    uint8_t*                    fGarbageRowPtr;
-    // FIXME: This imitates behavior in SkCodec::rewindIfNeeded. That function
-    // is called whenever some action is taken that reads the stream and
-    // therefore the next call will require a rewind. So it modifies a boolean
-    // to note that the *next* time it is called a rewind is needed.
-    // SkPngInterlacedScanlineDecoder has an extra wrinkle - calling
-    // onStartScanlineDecode followed by onGetScanlines does *not* require a
-    // rewind. Since rewindIfNeeded does not have this flexibility, we need to
-    // add another layer.
-    bool                        fCanSkipRewind;
-
-    typedef SkPngCodec INHERITED;
+        }
+
+        if (fInterlacedComplete) {
+            return SkCodec::kSuccess;
+        }
+
+        if (rowsDecoded) {
+            *rowsDecoded = fLinesDecoded;
+        }
+        return SkCodec::kIncompleteInput;
+    }
+
+    void setUpInterlaceBuffer(int height) {
+        fPng_rowbytes = png_get_rowbytes(this->png_ptr(), this->info_ptr());
+        fInterlaceBuffer.reset(fPng_rowbytes * height);
+        fInterlacedComplete = false;
+    }
 };
 
 // Reads the header and initializes the output fields, if not NULL.
 //
 // @param stream Input data. Will be read to get enough information to properly
 //      setup the codec.
 // @param chunkReader SkPngChunkReader, for reading unknown chunks. May be NULL.
 //      If not NULL, png_ptr will hold an *unowned* pointer to it. The caller is
 //      expected to continue to own it for the lifetime of the png_ptr.
 // @param outCodec Optional output variable.  If non-NULL, will be set to a new
 //      SkPngCodec on success.
 // @param png_ptrp Optional output variable. If non-NULL, will be set to a new
 //      png_structp on success.
 // @param info_ptrp Optional output variable. If non-NULL, will be set to a new
 //      png_infop on success;
 // @return true on success, in which case the caller is responsible for calling
 //      png_destroy_read_struct(png_ptrp, info_ptrp).
 //      If it returns false, the passed in fields (except stream) are unchanged.
 static bool read_header(SkStream* stream, SkPngChunkReader* chunkReader, SkCodec** outCodec,
                         png_structp* png_ptrp, png_infop* info_ptrp) {
     // The image is known to be a PNG. Decode enough to know the SkImageInfo.
     png_structp png_ptr = png_create_read_struct(PNG_LIBPNG_VER_STRING, nullptr,
                                                  sk_error_fn, sk_warning_fn);
     if (!png_ptr) {
         return false;
     }
 
-    AutoCleanPng autoClean(png_ptr);
+    AutoCleanPng autoClean(png_ptr, stream, chunkReader, outCodec);
 
     png_infop info_ptr = png_create_info_struct(png_ptr);
     if (info_ptr == nullptr) {
         return false;
     }
 
     autoClean.setInfoPtr(info_ptr);
 
     // FIXME: Could we use the return value of setjmp to specify the type of
     // error?
     if (setjmp(png_jmpbuf(png_ptr))) {
         return false;
     }
 
-    png_set_read_fn(png_ptr, static_cast<void*>(stream), sk_read_fn);
-
 #ifdef PNG_READ_UNKNOWN_CHUNKS_SUPPORTED
     // Hookup our chunkReader so we can see any user-chunks the caller may be interested in.
     // This needs to be installed before we read the png header.  Android may store ninepatch
     // chunks in the header.
     if (chunkReader) {
         png_set_keep_unknown_chunks(png_ptr, PNG_HANDLE_CHUNK_ALWAYS, (png_byte*)"", 0);
         png_set_read_user_chunk_fn(png_ptr, (png_voidp) chunkReader, sk_read_user_chunk);
     }
 #endif
 
-    // The call to png_read_info() gives us all of the information from the
-    // PNG file before the first IDAT (image data chunk).
-    png_read_info(png_ptr, info_ptr);
+    png_set_progressive_read_fn(png_ptr, &autoClean, AutoCleanPng::InfoCallback, nullptr, nullptr);
+    const bool decodedBounds = autoClean.decodeBounds();
+    // For safety, clear the pointers to autoClean.
+    png_set_progressive_read_fn(png_ptr, nullptr, nullptr, nullptr, nullptr);
+
+    if (!decodedBounds) {
+        return false;
+    }
+
+    // On success, decodeBounds releases ownership of png_ptr and info_ptr.
+    if (png_ptrp) {
+        *png_ptrp = png_ptr;
+    }
+    if (info_ptrp) {
+        *info_ptrp = info_ptr;
+    }
+
+    // decodeBounds takes care of setting outCodec
+    if (outCodec) {
+        SkASSERT(*outCodec);
+    }
+    return true;
+}
+
+void AutoCleanPng::infoCallback() {
     png_uint_32 origWidth, origHeight;
     int bitDepth, encodedColorType;
-    png_get_IHDR(png_ptr, info_ptr, &origWidth, &origHeight, &bitDepth,
+    png_get_IHDR(fPng_ptr, fInfo_ptr, &origWidth, &origHeight, &bitDepth,
                  &encodedColorType, nullptr, nullptr, nullptr);
 
     // Tell libpng to strip 16 bit/color files down to 8 bits/color.
     // TODO: Should we handle this in SkSwizzler?  Could this also benefit
     //       RAW decodes?
     if (bitDepth == 16) {
         SkASSERT(PNG_COLOR_TYPE_PALETTE != encodedColorType);
-        png_set_strip_16(png_ptr);
+        png_set_strip_16(fPng_ptr);
     }
 
     // Now determine the default colorType and alphaType and set the required transforms.
     // Often, we depend on SkSwizzler to perform any transforms that we need.  However, we
     // still depend on libpng for many of the rare and PNG-specific cases.
     SkEncodedInfo::Color color;
     SkEncodedInfo::Alpha alpha;
     switch (encodedColorType) {
         case PNG_COLOR_TYPE_PALETTE:
             // Extract multiple pixels with bit depths of 1, 2, and 4 from a single
             // byte into separate bytes (useful for paletted and grayscale images).
             if (bitDepth < 8) {
                 // TODO: Should we use SkSwizzler here?
-                png_set_packing(png_ptr);
+                png_set_packing(fPng_ptr);
             }
 
             color = SkEncodedInfo::kPalette_Color;
             // Set the alpha depending on if a transparency chunk exists.
-            alpha = png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS) ?
+            alpha = png_get_valid(fPng_ptr, fInfo_ptr, PNG_INFO_tRNS) ?
                     SkEncodedInfo::kUnpremul_Alpha : SkEncodedInfo::kOpaque_Alpha;
             break;
         case PNG_COLOR_TYPE_RGB:
-            if (png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS)) {
+            if (png_get_valid(fPng_ptr, fInfo_ptr, PNG_INFO_tRNS)) {
                 // Convert to RGBA if transparency chunk exists.
-                png_set_tRNS_to_alpha(png_ptr);
+                png_set_tRNS_to_alpha(fPng_ptr);
                 color = SkEncodedInfo::kRGBA_Color;
                 alpha = SkEncodedInfo::kBinary_Alpha;
             } else {
                 color = SkEncodedInfo::kRGB_Color;
                 alpha = SkEncodedInfo::kOpaque_Alpha;
             }
             break;
         case PNG_COLOR_TYPE_GRAY:
             // Expand grayscale images to the full 8 bits from 1, 2, or 4 bits/pixel.
             if (bitDepth < 8) {
                 // TODO: Should we use SkSwizzler here?
-                png_set_expand_gray_1_2_4_to_8(png_ptr);
+                png_set_expand_gray_1_2_4_to_8(fPng_ptr);
             }
 
-            if (png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS)) {
-                png_set_tRNS_to_alpha(png_ptr);
+            if (png_get_valid(fPng_ptr, fInfo_ptr, PNG_INFO_tRNS)) {
+                png_set_tRNS_to_alpha(fPng_ptr);
                 color = SkEncodedInfo::kGrayAlpha_Color;
                 alpha = SkEncodedInfo::kBinary_Alpha;
             } else {
                 color = SkEncodedInfo::kGray_Color;
                 alpha = SkEncodedInfo::kOpaque_Alpha;
             }
             break;
         case PNG_COLOR_TYPE_GRAY_ALPHA:
             color = SkEncodedInfo::kGrayAlpha_Color;
             alpha = SkEncodedInfo::kUnpremul_Alpha;
             break;
         case PNG_COLOR_TYPE_RGBA:
             color = SkEncodedInfo::kRGBA_Color;
             alpha = SkEncodedInfo::kUnpremul_Alpha;
             break;
         default:
             // All the color types have been covered above.
             SkASSERT(false);
             color = SkEncodedInfo::kRGBA_Color;
             alpha = SkEncodedInfo::kUnpremul_Alpha;
     }
 
-    int numberPasses = png_set_interlace_handling(png_ptr);
+    const int numberPasses = png_set_interlace_handling(fPng_ptr);
 
-    autoClean.release();
-    if (png_ptrp) {
-        *png_ptrp = png_ptr;
-    }
-    if (info_ptrp) {
-        *info_ptrp = info_ptr;
-    }
-
-    if (outCodec) {
-        sk_sp<SkColorSpace> colorSpace = read_color_space(png_ptr, info_ptr);
+    if (fOutCodec) {
+        SkASSERT(nullptr == *fOutCodec);
+        sk_sp<SkColorSpace> colorSpace = read_color_space(fPng_ptr, fInfo_ptr);
         SkEncodedInfo info = SkEncodedInfo::Make(color, alpha, 8);
-
         if (1 == numberPasses) {
-            *outCodec = new SkPngScanlineDecoder(origWidth, origHeight, info, stream,
-                    chunkReader, png_ptr, info_ptr, bitDepth, colorSpace);
+            *fOutCodec = new SkPngNormalDecoder(origWidth, origHeight, info, fStream,
+                    fChunkReader, fPng_ptr, fInfo_ptr, bitDepth, std::move(colorSpace));
         } else {
-            *outCodec = new SkPngInterlacedScanlineDecoder(origWidth, origHeight, info, stream,
-                    chunkReader, png_ptr, info_ptr, bitDepth, numberPasses, colorSpace);
+            *fOutCodec = new SkPngInterlacedDecoder(origWidth, origHeight, info, fStream,
+                    fChunkReader, fPng_ptr, fInfo_ptr, bitDepth, std::move(colorSpace),
+                    numberPasses);
         }
     }
 
-    return true;
+    fDecodedBounds = true;
+    // 1 tells libpng to save any extra data. We may be able to be more efficient by saving
+    // it ourselves.
+    png_process_data_pause(fPng_ptr, 1);
+
+    // Release the pointers, which are now owned by the codec or the caller is expected to
+    // take ownership.
+    this->releasePngPtrs();
 }
 
 SkPngCodec::SkPngCodec(int width, int height, const SkEncodedInfo& info, SkStream* stream,
                        SkPngChunkReader* chunkReader, png_structp png_ptr, png_infop info_ptr,
-                       int bitDepth, int numberPasses, sk_sp<SkColorSpace> colorSpace)
+                       int bitDepth, sk_sp<SkColorSpace> colorSpace)
     : INHERITED(width, height, info, stream, colorSpace)
     , fPngChunkReader(SkSafeRef(chunkReader))
     , fPng_ptr(png_ptr)
     , fInfo_ptr(info_ptr)
-    , fNumberPasses(numberPasses)
     , fBitDepth(bitDepth)
 {}
 
 SkPngCodec::~SkPngCodec() {
     this->destroyReadStruct();
 }
 
 void SkPngCodec::destroyReadStruct() {
     if (fPng_ptr) {
         // We will never have a nullptr fInfo_ptr with a non-nullptr fPng_ptr
         SkASSERT(fInfo_ptr);
         png_destroy_read_struct(&fPng_ptr, &fInfo_ptr, nullptr);
         fPng_ptr = nullptr;
         fInfo_ptr = nullptr;
     }
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // Getting the pixels
 ///////////////////////////////////////////////////////////////////////////////
 
-SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
-                                               const Options& options,
-                                               SkPMColor ctable[],
-                                               int* ctableCount) {
-    // FIXME: Could we use the return value of setjmp to specify the type of
-    // error?
+bool SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
+                                    const Options& options,
+                                    SkPMColor ctable[],
+                                    int* ctableCount) {
     if (setjmp(png_jmpbuf(fPng_ptr))) {
-        SkCodecPrintf("setjmp long jump!\n");
-        return kInvalidInput;
+        return false;
     }
     png_read_update_info(fPng_ptr, fInfo_ptr);
 
     if (SkEncodedInfo::kPalette_Color == this->getEncodedInfo().color()) {
         if (!this->createColorTable(requestedInfo.colorType(),
                 kPremul_SkAlphaType == requestedInfo.alphaType(), ctableCount)) {
-            return kInvalidInput;
+            return false;
         }
     }
 
     // Copy the color table to the client if they request kIndex8 mode
     copy_color_table(requestedInfo, fColorTable, ctable, ctableCount);
 
     // Create the swizzler.  SkPngCodec retains ownership of the color table.
     const SkPMColor* colors = get_color_ptr(fColorTable.get());
     fSwizzler.reset(SkSwizzler::CreateSwizzler(this->getEncodedInfo(), colors, requestedInfo,
             options));
     SkASSERT(fSwizzler);
 
-    return kSuccess;
+    return true;
 }
 
 
 bool SkPngCodec::onRewind() {
     // This sets fPng_ptr and fInfo_ptr to nullptr. If read_header
     // succeeds, they will be repopulated, and if it fails, they will
     // remain nullptr. Any future accesses to fPng_ptr and fInfo_ptr will
     // come through this function which will rewind and again attempt
     // to reinitialize them.
     this->destroyReadStruct();
@@ skipping 15 matching lines @@
                                         int* rowsDecoded) {
     if (!conversion_possible(requestedInfo, this->getInfo())) {
         return kInvalidConversion;
     }
     if (options.fSubset) {
         // Subsets are not supported.
         return kUnimplemented;
     }
 
     // Note that ctable and ctableCount may be modified if there is a color table
-    const Result result = this->initializeSwizzler(requestedInfo, options, ctable, ctableCount);
-    if (result != kSuccess) {
-        return result;
+    if (!this->initializeSwizzler(requestedInfo, options, ctable, ctableCount)) {
+        return kInvalidInput;   // or parameters?
     }
 
-    const int width = requestedInfo.width();
-    const int height = requestedInfo.height();
-    const int bpp = bytes_per_pixel(this->getEncodedInfo().bitsPerPixel());
-    const size_t srcRowBytes = width * bpp;
+    return this->decodeAllRows(dst, dstRowBytes, rowsDecoded);
+}
 
-    // FIXME: Could we use the return value of setjmp to specify the type of
-    // error?
-    int row = 0;
-    // This must be declared above the call to setjmp to avoid memory leaks on incomplete images.
-    SkAutoTMalloc<uint8_t> storage;
-    if (setjmp(png_jmpbuf(fPng_ptr))) {
-        // Assume that any error that occurs while reading rows is caused by an incomplete input.
-        if (fNumberPasses > 1) {
-            // FIXME (msarett): Handle incomplete interlaced pngs.
-            return (row == height) ? kSuccess : kInvalidInput;
-        }
-        // FIXME: We do a poor job on incomplete pngs compared to other decoders (ex: Chromium,
-        // Ubuntu Image Viewer).  This is because we use the default buffer size in libpng (8192
-        // bytes), and if we can't fill the buffer, we immediately fail.
-        // For example, if we try to read 8192 bytes, and the image (incorrectly) only contains
-        // half that, which may have been enough to contain a non-zero number of lines, we fail
-        // when we could have decoded a few more lines and then failed.
-        // The read function that we provide for libpng has no way of indicating that we have
-        // made a partial read.
-        // Making our buffer size smaller improves our incomplete decodes, but what impact does
-        // it have on regular decode performance?  Should we investigate using a different API
-        // instead of png_read_row?  Chromium uses png_process_data.
-        *rowsDecoded = row;
-        return (row == height) ? kSuccess : kIncompleteInput;
+SkCodec::Result SkPngCodec::onStartIncrementalDecode(const SkImageInfo& dstInfo,
+        const SkCodec::Options& options, SkPMColor* ctable, int* ctableCount) {
+    if (!conversion_possible(dstInfo, this->getInfo())) {
+        return kInvalidConversion;
     }
 
-    // FIXME: We could split these out based on subclass.
-    void* dstRow = dst;
-    if (fNumberPasses > 1) {
-        storage.reset(height * srcRowBytes);
-        uint8_t* const base = storage.get();
-
-        for (int i = 0; i < fNumberPasses; i++) {
-            uint8_t* srcRow = base;
-            for (int y = 0; y < height; y++) {
-                png_read_row(fPng_ptr, srcRow, nullptr);
-                srcRow += srcRowBytes;
-            }
-        }
-
-        // Now swizzle it.
-        uint8_t* srcRow = base;
-        for (; row < height; row++) {
-            fSwizzler->swizzle(dstRow, srcRow);
-            dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
-            srcRow += srcRowBytes;
-        }
-    } else {
-        storage.reset(srcRowBytes);
-        uint8_t* srcRow = storage.get();
-        for (; row < height; row++) {
-            png_read_row(fPng_ptr, srcRow, nullptr);
-            fSwizzler->swizzle(dstRow, srcRow);
-            dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
-        }
+    if (!this->initializeSwizzler(dstInfo, options, ctable, ctableCount)) {
+        return kInvalidInput;
     }
 
-    // read rest of file, and get additional comment and time chunks in info_ptr
-    png_read_end(fPng_ptr, fInfo_ptr);
+    int firstRow, lastRow;
+    if (options.fSubset) {
+        firstRow = options.fSubset->top();
+        lastRow = options.fSubset->bottom() - 1;
+    } else {
+        firstRow = 0;
+        lastRow = dstInfo.height() - 1;
+    }
+    this->setRange(firstRow, lastRow);
+    return kSuccess;
+}
 
-    return kSuccess;
+SkCodec::Result SkPngCodec::onIncrementalDecode(std::function<void*(int)> callback,
+        int* rowsDecoded) {
+    return this->decode(callback, rowsDecoded);
 }
 
 uint32_t SkPngCodec::onGetFillValue(SkColorType colorType) const {
     const SkPMColor* colorPtr = get_color_ptr(fColorTable.get());
     if (colorPtr) {
         return get_color_table_fill_value(colorType, colorPtr, 0);
     }
     return INHERITED::onGetFillValue(colorType);
 }
 
 SkCodec* SkPngCodec::NewFromStream(SkStream* stream, SkPngChunkReader* chunkReader) {
     SkAutoTDelete<SkStream> streamDeleter(stream);
 
-    SkCodec* outCodec;
+    SkCodec* outCodec = nullptr;
     if (read_header(stream, chunkReader, &outCodec, nullptr, nullptr)) {
         // Codec has taken ownership of the stream.
         SkASSERT(outCodec);
         streamDeleter.release();
         return outCodec;
     }
 
     return nullptr;
 }