Fill incomplete images in SkCodec parent class

src/codec/SkCodec_libpng.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 "SkCodec_libpng.h"
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
@@ skipping 349 matching lines @@
     }
     return nullptr;
 }
 
 #define INVALID_NUMBER_PASSES -1
 SkPngCodec::SkPngCodec(const SkImageInfo& info, SkStream* stream,
                        png_structp png_ptr, png_infop info_ptr, int bitDepth)
     : INHERITED(info, stream)
     , fPng_ptr(png_ptr)
     , fInfo_ptr(info_ptr)
+    , fRow(0)
     , fSrcConfig(SkSwizzler::kUnknown)
     , fNumberPasses(INVALID_NUMBER_PASSES)
     , fReallyHasAlpha(false)
     , fBitDepth(bitDepth)
 {}
 
 SkPngCodec::~SkPngCodec() {
     this->destroyReadStruct();
 }
 
@@ skipping 107 matching lines @@
 
     // 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;
     }
     // 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);
     SkAutoMalloc storage;
     void* dstRow = dst;
     if (fNumberPasses > 1) {
         const int width = requestedInfo.width();
         const int height = requestedInfo.height();
         const int bpp = SkSwizzler::BytesPerPixel(fSrcConfig);
         const size_t srcRowBytes = width * bpp;
@@ skipping 13 matching lines @@
         // Now swizzle it.
         uint8_t* srcRow = base;
         for (int y = 0; y < height; y++) {
             fReallyHasAlpha |= !SkSwizzler::IsOpaque(fSwizzler->swizzle(dstRow, srcRow));
             dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
             srcRow += srcRowBytes;
         }
     } 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);
         }
     }
 
     // FIXME: do we need substituteTranspColor? Note that we cannot do it for
     // scanline decoding, but we could do it here. Alternatively, we could do
     // it as we go, instead of in post-processing like SkPNGImageDecoder.
 
     if (setjmp(png_jmpbuf(fPng_ptr))) {
         // We've already read all the scanlines. This is a success.
         return kSuccess;
     }
 
     // read rest of file, and get additional comment and time chunks in info_ptr
     png_read_end(fPng_ptr, fInfo_ptr);
     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)
     {}
 
     SkCodec::Result onStart(const SkImageInfo& dstInfo,
                             const SkCodec::Options& options,
                             SkPMColor ctable[], int* ctableCount) override {
         if (!fCodec->rewindIfNeeded()) {
             return SkCodec::kCouldNotRewind;
         }
 
         if (!conversion_possible(dstInfo, this->getInfo())) {
             return SkCodec::kInvalidConversion;
         }
 
         // Check to see if scaling was requested.
         if (dstInfo.dimensions() != this->getInfo().dimensions()) {
             if (!SkScaledCodec::DimensionsSupportedForSampling(this->getInfo(), dstInfo)) {
                 return SkCodec::kInvalidScale;   
             }
         }
 
         const SkCodec::Result result = fCodec->initializeSwizzler(dstInfo, options, ctable,
-                                                                  ctableCount);
+                ctableCount);
         if (result != SkCodec::kSuccess) {
             return result;
         }
 
         fHasAlpha = false;
         fStorage.reset(this->getInfo().width() * SkSwizzler::BytesPerPixel(fCodec->fSrcConfig));
         fSrcRow = static_cast<uint8_t*>(fStorage.get());
 
         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; }
 
     SkEncodedFormat onGetEncodedFormat() const override { 
         return kPNG_SkEncodedFormat;
     }
 
 
 private:
-    SkAutoTDelete<SkPngCodec>   fCodec;
+    SkPngCodec*                 fCodec; // Owned by parent class
     bool                        fHasAlpha;
     SkAutoMalloc                fStorage;
     uint8_t*                    fSrcRow;
 
     typedef SkScanlineDecoder INHERITED;
 };
 
 
 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)
     {}
 
     SkCodec::Result onStart(const SkImageInfo& dstInfo,
                             const SkCodec::Options& options,
                             SkPMColor ctable[], int* ctableCount) override
     {
         if (!fCodec->rewindIfNeeded()) {
             return SkCodec::kCouldNotRewind;
         }
 
         if (!conversion_possible(dstInfo, this->getInfo())) {
             return SkCodec::kInvalidConversion;    
         }
 
         // Check to see if scaling was requested.
         if (dstInfo.dimensions() != this->getInfo().dimensions()) {
             if (!SkScaledCodec::DimensionsSupportedForSampling(this->getInfo(), dstInfo)) {
                 return SkCodec::kInvalidScale;
             }
         }
 
         const SkCodec::Result result = fCodec->initializeSwizzler(dstInfo, options, ctable,
-                                                                  ctableCount);
+                ctableCount);
         if (result != SkCodec::kSuccess) {
             return result;
         }
 
         fHasAlpha = false;
         fCurrentRow = 0;
         fHeight = dstInfo.height();
         fSrcRowBytes = this->getInfo().width() * SkSwizzler::BytesPerPixel(fCodec->fSrcConfig);
         fGarbageRow.reset(fSrcRowBytes);
         fGarbageRowPtr = static_cast<uint8_t*>(fGarbageRow.get());
         fCanSkipRewind = true;
 
         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) {
             // We already rewound in onStart, so there is no reason to rewind.
             // 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);
         uint8_t* storagePtr = static_cast<uint8_t*>(storage.get());
         uint8_t* srcRow;
         for (int i = 0; i < number_passes; i++) {
             //read rows we planned to skip into garbage row
             for (int y = 0; y < fCurrentRow; y++){
                 png_read_rows(fCodec->fPng_ptr, &fGarbageRowPtr, png_bytepp_NULL, 1);
             }
@@ skipping 10 matching lines @@
         }
         //swizzle the rows we care about
         srcRow = storagePtr;
         void* dstRow = dst;
         for (int y = 0; y < count; y++) {
             fHasAlpha |= !SkSwizzler::IsOpaque(fCodec->fSwizzler->swizzle(dstRow, srcRow));
             dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
             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; }
 
     SkScanlineOrder onGetScanlineOrder() const override {
         return kNone_SkScanlineOrder;
     }
 
     SkEncodedFormat onGetEncodedFormat() const override { 
         return kPNG_SkEncodedFormat;
     }
 
 private:
-    SkAutoTDelete<SkPngCodec>   fCodec;
+    SkPngCodec*                 fCodec; // Owned by parent class
     bool                        fHasAlpha;
     int                         fCurrentRow;
     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 onStart
     // followed by onGetScanlines does *not* require a rewind. Since
     // rewindIfNeeded does not have this flexibility, we need to add another
     // layer.
     bool                        fCanSkipRewind;
 
     typedef SkScanlineDecoder INHERITED;
 };
 
 SkScanlineDecoder* SkPngCodec::NewSDFromStream(SkStream* stream) {
     SkAutoTDelete<SkPngCodec> codec (static_cast<SkPngCodec*>(SkPngCodec::NewFromStream(stream)));
     if (!codec) {
         return nullptr;
     }
 
     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());
 }