Improve the Codec_end test and add fixes

Better imitate the original Android bug. Create a stream with
multiple images in it, and verify that it successfully decodes after
decoding once.

This exposes a bug in SkPngCodec, which did not work for interlaced
images.

Test more formats that also happen to succeed: ICO, BMP, and WBMP

This explicitly does *not* attempt to fix sampled or subset
decodes, which already stopped early when decoding as an
optimization.

Change-Id: Ib0b8918f14ba3fb0fa31e9c71c8100dcbeeb465f
Reviewed-on: https://skia-review.googlesource.com/14104
Reviewed-by: Matt Sarett <msarett@google.com>

2 files changed, 54 insertions, 65 deletions

diff --git a/src/codec/SkPngCodec.cpp b/src/codec/SkPngCodec.cpp
index 1f69159ee6..b6f418fd49 100644
--- a/src/codec/SkPngCodec.cpp
+++ b/src/codec/SkPngCodec.cpp
@@ -647,7 +647,7 @@ private:
     // 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) {
+        if (rowNum < fFirstRow || rowNum > fLastRow || fInterlacedComplete) {
             // Ignore this row
             return;
         }
@@ -663,12 +663,16 @@ private:
         } else {
             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).
+                // Last pass, and we have read all of the rows we care about.
                 fInterlacedComplete = true;
-                // Fake error to stop decoding scanlines.
-                longjmp(PNG_JMPBUF(this->png_ptr()), kStopDecoding);
+                if (fLastRow != this->getInfo().height() - 1 ||
+                        (this->swizzler() && this->swizzler()->sampleY() != 1)) {
+                    // Fake error to stop decoding scanlines. Only stop if we're not decoding the
+                    // whole image, in which case processing the rest of the image might be
+                    // expensive. When decoding the whole image, read through the IEND chunk to
+                    // preserve Android behavior of leaving the input stream in the right place.
+                    longjmp(PNG_JMPBUF(this->png_ptr()), kStopDecoding);
+                }
             }
         }
     }
diff --git a/tests/CodecExactReadTest.cpp b/tests/CodecExactReadTest.cpp
index 7e0d8eaccc..9b6acc8bc9 100644
--- a/tests/CodecExactReadTest.cpp
+++ b/tests/CodecExactReadTest.cpp
@@ -14,89 +14,74 @@
 #include "SkStream.h"
 
 namespace {
-// This class emits a skiatest failure if a client attempts to read beyond its
-// end. Since it is used with complete, valid images, and contains nothing
-// after the encoded image data, it will emit a failure if the client attempts
-// to read beyond the logical end of the data.
-class MyStream : public SkStream {
+// This class wraps another SkStream. It does not own the underlying stream, so
+// that the underlying stream can be reused starting from where the first
+// client left off. This mimics Android's JavaInputStreamAdaptor.
+class UnowningStream : public SkStream {
 public:
-    static MyStream* Make(const char* path, skiatest::Reporter* r) {
-        SkASSERT(path);
-        sk_sp<SkData> data(GetResourceAsData(path));
-        if (!data) {
-            return nullptr;
-        }
-
-        return new MyStream(path, std::move(data), r);
-    }
+    explicit UnowningStream(SkStream* stream)
+        : fStream(stream)
+    {}
 
     size_t read(void* buf, size_t bytes) override {
-        const size_t remaining = fStream.getLength() - fStream.getPosition();
-        if (bytes > remaining) {
-            ERRORF(fReporter, "Tried to read %lu bytes (only %lu remaining) from %s",
-                   bytes, remaining, fPath);
-        }
-        return fStream.read(buf, bytes);
+        return fStream->read(buf, bytes);
     }
 
     bool rewind() override {
-        return fStream.rewind();
+        return fStream->rewind();
     }
 
     bool isAtEnd() const override {
-        return fStream.isAtEnd();
+        return fStream->isAtEnd();
     }
 private:
-    const char* fPath;
-    SkMemoryStream fStream;
-    skiatest::Reporter* fReporter;  // Unowned
-
-    MyStream(const char* path, sk_sp<SkData> data, skiatest::Reporter* r)
-        : fPath(path)
-        , fStream(std::move(data))
-        , fReporter(r)
-    {}
+    SkStream* fStream; // Unowned.
 };
 } // namespace
 
-// Test that SkPngCodec does not attempt to read its input beyond the logical
-// end of its data. Some other SkCodecs do, but some Android apps rely on not
-// doing so for PNGs.
+// Test that some SkCodecs do not attempt to read input beyond the logical
+// end of the data. Some other SkCodecs do, but some Android apps rely on not
+// doing so for PNGs. Test on other formats that work.
 DEF_TEST(Codec_end, r) {
     for (const char* path : { "plane.png",
                               "yellow_rose.png",
-                              "plane_interlaced.png" }) {
-        std::unique_ptr<MyStream> stream(MyStream::Make(path, r));
-        if (!stream) {
+                              "plane_interlaced.png",
+                              "google_chrome.ico",
+                              "color_wheel.ico",
+                              "mandrill.wbmp",
+                              "randPixels.bmp",
+                              }) {
+        sk_sp<SkData> data = GetResourceAsData(path);
+        if (!data) {
             continue;
         }
 
-        std::unique_ptr<SkCodec> codec(SkCodec::NewFromStream(stream.release()));
-        if (!codec) {
-            ERRORF(r, "Failed to create a codec from %s\n", path);
-            continue;
+        const int kNumImages = 2;
+        const size_t size = data->size();
+        sk_sp<SkData> multiData = SkData::MakeUninitialized(size * kNumImages);
+        void* dst = multiData->writable_data();
+        for (int i = 0; i < kNumImages; i++) {
+            memcpy(SkTAddOffset<void>(dst, size * i), data->data(), size);
         }
+        data.reset();
 
-        auto info = codec->getInfo().makeColorType(kN32_SkColorType);
-        SkBitmap bm;
-        bm.allocPixels(info);
+        SkMemoryStream stream(std::move(multiData));
+        for (int i = 0; i < kNumImages; ++i) {
+            std::unique_ptr<SkCodec> codec(SkCodec::NewFromStream(new UnowningStream(&stream)));
+            if (!codec) {
+                ERRORF(r, "Failed to create a codec from %s, iteration %i", path, i);
+                continue;
+            }
 
-        auto result = codec->getPixels(bm.info(), bm.getPixels(), bm.rowBytes());
-        if (result != SkCodec::kSuccess) {
-            ERRORF(r, "Failed to getPixels from %s. error %i", path, result);
-            continue;
-        }
-
-        // Rewind and do an incremental decode.
-        result = codec->startIncrementalDecode(bm.info(), bm.getPixels(), bm.rowBytes());
-        if (result != SkCodec::kSuccess) {
-            ERRORF(r, "Failed to startIncrementalDecode from %s. error %i", path, result);
-            continue;
-        }
+            auto info = codec->getInfo().makeColorType(kN32_SkColorType);
+            SkBitmap bm;
+            bm.allocPixels(info);
 
-        result = codec->incrementalDecode();
-        if (result != SkCodec::kSuccess) {
-            ERRORF(r, "Failed to incrementalDecode from %s. error %i", path, result);
+            auto result = codec->getPixels(bm.info(), bm.getPixels(), bm.rowBytes());
+            if (result != SkCodec::kSuccess) {
+                ERRORF(r, "Failed to getPixels from %s, iteration %i error %i", path, i, result);
+                continue;
+            }
         }
     }
 }