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..7896cc3bba6df7df2016ce707808b6d6cd3278bd 100644
--- a/src/codec/SkCodec_libpng.cpp
+++ b/src/codec/SkCodec_libpng.cpp
@@ -367,6 +367,7 @@ SkPngCodec::SkPngCodec(const SkImageInfo& info, SkStream* stream,
     : INHERITED(info, stream)
     , fPng_ptr(png_ptr)
     , fInfo_ptr(info_ptr)
+    , fRow(0)
     , fSrcConfig(SkSwizzler::kUnknown)
     , fNumberPasses(INVALID_NUMBER_PASSES)
     , fReallyHasAlpha(false)
@@ -494,8 +495,19 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void*
     // FIXME: Could we use the return value of setjmp to specify the type of
     // error?
     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

[scroggo, 2015/09/22 18:02:48]

Does this mean, for example, that we try to read 8192 bytes, but 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?

I guess the problem is that sk_read_fn has no way to deal with reading partial
data?

[msarett, 2015/09/23 13:22:40]

Yes that is exactly what the problem is.

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

Could you add that to the description?

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

Done.

+        // 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.
+        this->setIncompleteScanlines(requestedInfo.height() - fRow);

[scroggo, 2015/09/22 18:02:48]

I always get confused when dealing with setjmp, so maybe this doesn't work. It
seems weird to me that fRow is a member variable, but it is only used in this
function. What if instead you said:

int row = 0;
if (setjmp())
  ....

for (; row < requestedInfo.height(); row++)
...


(I know fRow is used by scanline decoding, but that tells me it should perhaps
go on that class, rather than SkPngCodec)

[msarett, 2015/09/23 13:22:40]

Yes this works!  Clever!

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

Just to make sure, did you test this? Can we add a test to CodexTest?

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

Yes I tested this.  I have been testing by decoding incomplete pngs in
DMSrcSink.  I intend to upload my test images on the bots when this lands.

Would you be in favor of checking in incomplete images to resources and
asserting that decodes of these images return kIncompleteInput?  I'm not sure
how else to test in CodexTest, but I think the testing in DMSrcSink is
sufficient.

[scroggo, 2015/10/01 14:48:31]

Possibly - the advantage to including it in CodexTest is that we would be
testing that decoding an incomplete image actually does return kIncomplete.

A disadvantage to relying on DM is that if some decode changed from an error to
a Nonfatal Error it seems like we could miss that.

I do like to be conservative about what we add to our resources directory
though.

[msarett, 2015/10/01 18:14:13]

I will add a single incomplete image to resources and verify that it decodes the
same (and returns kIncomplete) for all of our decoding options.

+        return kIncompleteInput;
     }
 
     SkASSERT(fNumberPasses != INVALID_NUMBER_PASSES);
@@ -529,7 +541,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 (fRow = 0; fRow < requestedInfo.height(); fRow++) {
             png_read_rows(fPng_ptr, &srcRow, png_bytepp_NULL, 1);
             fReallyHasAlpha |= !SkSwizzler::IsOpaque(fSwizzler->swizzle(dstRow, srcRow));
             dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
@@ -550,10 +562,18 @@ 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)
     {}
@@ -577,7 +597,7 @@ public:
         }
 
         const SkCodec::Result result = fCodec->initializeSwizzler(dstInfo, options, ctable,
-                                                                  ctableCount);
+                ctableCount);
         if (result != SkCodec::kSuccess) {
             return result;
         }
@@ -589,35 +609,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 fCodec->fRow;
         }
 
         void* dstRow = dst;
-        for (int i = 0; i < count; i++) {
+        for (fCodec->fRow = 0; fCodec->fRow < count; fCodec->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 fCodec->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 fCodec->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 (fCodec->fRow = 0; fCodec->fRow < count; fCodec->fRow++) {
             png_read_rows(fCodec->fPng_ptr, &fSrcRow, png_bytepp_NULL, 1);
         }
-        return SkCodec::kSuccess;
+        return fCodec->fRow;
     }
 
     bool onReallyHasAlpha() const override { return fHasAlpha; }
@@ -628,7 +648,7 @@ public:
 
 
 private:
-    SkAutoTDelete<SkPngCodec>   fCodec;
+    SkPngCodec*                 fCodec; // Owned by parent class
     bool                        fHasAlpha;
     SkAutoMalloc                fStorage;
     uint8_t*                    fSrcRow;
@@ -639,12 +659,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 +688,7 @@ public:
         }
 
         const SkCodec::Result result = fCodec->initializeSwizzler(dstInfo, options, ctable,
-                                                                  ctableCount);
+                ctableCount);
         if (result != SkCodec::kSuccess) {
             return result;
         }
@@ -684,7 +704,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 +712,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 +751,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 +771,7 @@ public:
     }
 
 private:
-    SkAutoTDelete<SkPngCodec>   fCodec;
+    SkPngCodec*                 fCodec; // Owned by parent class
     bool                        fHasAlpha;
     int                         fCurrentRow;
     int                         fHeight;
@@ -777,12 +800,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());
 }