Fill incomplete images in SkCodec parent class

src/codec/SkCodec_libpng.cpp

Index: src/codec/SkCodec_libpng.cpp
diff --git a/src/codec/SkCodec_libpng.cpp b/src/codec/SkCodec_libpng.cpp
index 2ba2d5bb9f2bed42209d9c2ca5b0ce4ce127bba0..074f0ad3d520176af5f6ec90b2f8babe4784a461 100644
--- a/src/codec/SkCodec_libpng.cpp
+++ b/src/codec/SkCodec_libpng.cpp
@@ -470,7 +470,8 @@ bool SkPngCodec::onRewind() {
 
 SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void* dst,
                                         size_t dstRowBytes, const Options& options,
-                                        SkPMColor ctable[], int* ctableCount) {
+                                        SkPMColor ctable[], int* ctableCount,
+                                        int* incompleteScanlines) {
     if (!conversion_possible(requestedInfo, this->getInfo())) {
         return kInvalidConversion;
     }
@@ -493,9 +494,21 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void*
     }
     // FIXME: Could we use the return value of setjmp to specify the type of
     // error?
+    int row = 0;
     if (setjmp(png_jmpbuf(fPng_ptr))) {
-        SkCodecPrintf("setjmp long jump!\n");
-        return kInvalidInput;
+        // 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 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.  Making our buffer size
+        // smaller would help, but what impact would it have on regular decode performance?  Should
+        // we investigate using a different API than png_read_row(s)?  Chromium uses
+        // png_process_data.
+        *incompleteScanlines = requestedInfo.height() - row;
+        return kIncompleteInput;
     }
 
     SkASSERT(fNumberPasses != INVALID_NUMBER_PASSES);
@@ -529,7 +542,7 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void*
     } else {
         storage.reset(requestedInfo.width() * SkSwizzler::BytesPerPixel(fSrcConfig));
         uint8_t* srcRow = static_cast<uint8_t*>(storage.get());
-        for (int y = 0; y < requestedInfo.height(); y++) {
+        for (; row < requestedInfo.height(); row++) {
             png_read_rows(fPng_ptr, &srcRow, png_bytepp_NULL, 1);
             fReallyHasAlpha |= !SkSwizzler::IsOpaque(fSwizzler->swizzle(dstRow, srcRow));
             dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
@@ -550,12 +563,21 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void*
     return kSuccess;
 }
 
+uint32_t SkPngCodec::onGetFillValue(const SkImageInfo& dstInfo) const {
+    const SkPMColor* colorPtr = get_color_ptr(fColorTable.get());
+    if (colorPtr) {
+        return get_color_table_fill_value(dstInfo.colorType(), colorPtr, 0);
+    }
+    return INHERITED::onGetFillValue(dstInfo);
+}
+
 class SkPngScanlineDecoder : public SkScanlineDecoder {
 public:
-    SkPngScanlineDecoder(const SkImageInfo& srcInfo, SkPngCodec* codec)
-        : INHERITED(srcInfo)
+    SkPngScanlineDecoder(SkPngCodec* codec)
+        : INHERITED(codec, codec->getInfo())
         , fCodec(codec)
         , fHasAlpha(false)
+        , fRow(0)
     {}
 
     SkCodec::Result onStart(const SkImageInfo& dstInfo,
@@ -577,7 +599,7 @@ public:
         }
 
         const SkCodec::Result result = fCodec->initializeSwizzler(dstInfo, options, ctable,
-                                                                  ctableCount);
+                ctableCount);
         if (result != SkCodec::kSuccess) {
             return result;
         }
@@ -589,35 +611,35 @@ public:
         return SkCodec::kSuccess;
     }
 
-    SkCodec::Result onGetScanlines(void* dst, int count, size_t rowBytes) override {
+    uint32_t onGetScanlines(void* dst, int count, size_t rowBytes) override {
+        // Assume that an error in libpng indicates an incomplete input.
         if (setjmp(png_jmpbuf(fCodec->fPng_ptr))) {
             SkCodecPrintf("setjmp long jump!\n");
-            return SkCodec::kInvalidInput;
+            return fRow;

[scroggo, 2015/09/25 15:55:06]

Oh, wait, you said that "this works", and I thought you meant using a local
variable. That would be even better, if setjmp doesn't ruin it for us.

[msarett, 2015/10/01 12:44:52]

It does work...For some reason I only thought it would only work in
onGetPixels(), but clearly it will work here as well - we always start over at
zero.

I will fix.

         }
 
         void* dstRow = dst;
-        for (int i = 0; i < count; i++) {
+        for (fRow = 0; fRow < count; fRow++) {
             png_read_rows(fCodec->fPng_ptr, &fSrcRow, png_bytepp_NULL, 1);
             fHasAlpha |= !SkSwizzler::IsOpaque(fCodec->fSwizzler->swizzle(dstRow, fSrcRow));
             dstRow = SkTAddOffset<void>(dstRow, rowBytes);
         }
-        return SkCodec::kSuccess;
+        return fRow;
     }
 
-    SkCodec::Result onSkipScanlines(int count) override {
-        // FIXME: Could we use the return value of setjmp to specify the type of
-        // error?
+    uint32_t onSkipScanlines(int count) override {
+        // Assume that an error in libpng indicates an incomplete input.
         if (setjmp(png_jmpbuf(fCodec->fPng_ptr))) {
             SkCodecPrintf("setjmp long jump!\n");
-            return SkCodec::kInvalidInput;
+            return fRow;
         }
         //there is a potential tradeoff of memory vs speed created by putting this in a loop. 
         //calling png_read_rows in a loop is insignificantly slower than calling it once with count 
         //as png_read_rows has it's own loop which calls png_read_row count times.
-        for (int i = 0; i < count; i++) {
+        for (fRow = 0; fRow < count; fRow++) {
             png_read_rows(fCodec->fPng_ptr, &fSrcRow, png_bytepp_NULL, 1);
         }
-        return SkCodec::kSuccess;
+        return fRow;
     }
 
     bool onReallyHasAlpha() const override { return fHasAlpha; }
@@ -628,8 +650,9 @@ public:
 
 
 private:
-    SkAutoTDelete<SkPngCodec>   fCodec;
+    SkPngCodec*                 fCodec; // Owned by parent class
     bool                        fHasAlpha;
+    uint32_t                    fRow;
     SkAutoMalloc                fStorage;
     uint8_t*                    fSrcRow;
 
@@ -639,12 +662,12 @@ private:
 
 class SkPngInterlacedScanlineDecoder : public SkScanlineDecoder {
 public:
-    SkPngInterlacedScanlineDecoder(const SkImageInfo& srcInfo, SkPngCodec* codec)
-        : INHERITED(srcInfo)
+    SkPngInterlacedScanlineDecoder(SkPngCodec* codec)
+        : INHERITED(codec, codec->getInfo())
         , fCodec(codec)
         , fHasAlpha(false)
         , fCurrentRow(0)
-        , fHeight(srcInfo.height())
+        , fHeight(codec->getInfo().height())
         , fCanSkipRewind(false)
     {}
 
@@ -668,7 +691,7 @@ public:
         }
 
         const SkCodec::Result result = fCodec->initializeSwizzler(dstInfo, options, ctable,
-                                                                  ctableCount);
+                ctableCount);
         if (result != SkCodec::kSuccess) {
             return result;
         }
@@ -684,7 +707,7 @@ public:
         return SkCodec::kSuccess;
     }
 
-    SkCodec::Result onGetScanlines(void* dst, int count, size_t dstRowBytes) override {
+    uint32_t 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) {
@@ -692,12 +715,15 @@ public:
             // Next time onGetScanlines is called, we will need to rewind.
             fCanSkipRewind = false;
         } else if (!fCodec->rewindIfNeeded()) {
-            return SkCodec::kCouldNotRewind;
+            return 0;
         }
 
         if (setjmp(png_jmpbuf(fCodec->fPng_ptr))) {
             SkCodecPrintf("setjmp long jump!\n");
-            return SkCodec::kInvalidInput;
+            // 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;
         }
         const int number_passes = png_set_interlace_handling(fCodec->fPng_ptr);
         SkAutoMalloc storage(count * fSrcRowBytes);
@@ -728,13 +754,13 @@ public:
             srcRow += fSrcRowBytes;
         }
         fCurrentRow += count;
-        return SkCodec::kSuccess;
+        return count;
     }
 
-    SkCodec::Result onSkipScanlines(int count) override {
+    uint32_t onSkipScanlines(int count) override {
         //when ongetScanlines is called it will skip to fCurrentRow
         fCurrentRow += count;
-        return SkCodec::kSuccess;
+        return count;
     }
 
     bool onReallyHasAlpha() const override { return fHasAlpha; }
@@ -748,7 +774,7 @@ public:
     }
 
 private:
-    SkAutoTDelete<SkPngCodec>   fCodec;
+    SkPngCodec*                 fCodec; // Owned by parent class
     bool                        fHasAlpha;
     int                         fCurrentRow;
     int                         fHeight;
@@ -777,12 +803,11 @@ SkScanlineDecoder* SkPngCodec::NewSDFromStream(SkStream* stream) {
     codec->fNumberPasses = png_set_interlace_handling(codec->fPng_ptr);
     SkASSERT(codec->fNumberPasses != INVALID_NUMBER_PASSES);
 
-    const SkImageInfo& srcInfo = codec->getInfo();
     if (codec->fNumberPasses > 1) {
         // interlaced image
-        return new SkPngInterlacedScanlineDecoder(srcInfo, codec.detach());
+        return new SkPngInterlacedScanlineDecoder(codec.detach());
     }
 
-    return new SkPngScanlineDecoder(srcInfo, codec.detach());
+    return new SkPngScanlineDecoder(codec.detach());
 }