Make SkPngCodec support rewinding properly.

Separate out the code for reading the header, and use it to reinitialize
fPng_ptr and fInfo_ptr after a rewind.

Use common code to clean up fPng_ptr and fInfo_ptr, and set them to
NULL and treat them as NULL as appropriate.

Update the test to expect SkPngCodec to succeed.

BUG=skia:3257

Review URL: https://codereview.chromium.org/1048423003

diff --git a/src/codec/SkCodec_libpng.cpp b/src/codec/SkCodec_libpng.cpp
index 9330ac5..18574f7 100644 (file)
--- a/src/codec/SkCodec_libpng.cpp
+++ b/src/codec/SkCodec_libpng.cpp
@@ -201,19 +201,25 @@ bool SkPngCodec::IsPng(SkStream* stream) {
     return true;
 }
 
-SkCodec* SkPngCodec::NewFromStream(SkStream* stream) {
+// Reads the header, and initializes the passed in fields, if not NULL (except
+// stream, which is passed to the read function).
+// Returns true on success, in which case the caller is responsible for calling
+// png_destroy_read_struct. If it returns false, the passed in fields (except
+// stream) are unchanged.
+static bool read_header(SkStream* stream, png_structp* png_ptrp,
+                        png_infop* info_ptrp, SkImageInfo* imageInfo) {
     // 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, NULL,
                                                  sk_error_fn, sk_warning_fn);
     if (!png_ptr) {
-        return NULL;
+        return false;
     }
 
     AutoCleanPng autoClean(png_ptr);
 
     png_infop info_ptr = png_create_info_struct(png_ptr);
     if (info_ptr == NULL) {
-        return NULL;
+        return false;
     }
 
     autoClean.setInfoPtr(info_ptr);
@@ -221,7 +227,7 @@ SkCodec* SkPngCodec::NewFromStream(SkStream* stream) {
     // FIXME: Could we use the return value of setjmp to specify the type of
     // error?
     if (setjmp(png_jmpbuf(png_ptr))) {
-        return NULL;
+        return false;
     }
 
     png_set_read_fn(png_ptr, static_cast<void*>(stream), sk_read_fn);
@@ -244,7 +250,7 @@ SkCodec* SkPngCodec::NewFromStream(SkStream* stream) {
         int64_t size = sk_64_mul(origWidth, origHeight);
         // now check that if we are 4-bytes per pixel, we also don't overflow
         if (size < 0 || size > (0x7FFFFFFF >> 2)) {
-            return NULL;
+            return false;
         }
     }
 
@@ -325,11 +331,28 @@ SkCodec* SkPngCodec::NewFromStream(SkStream* stream) {
 
     // FIXME: Also need to check for sRGB (skbug.com/3471).
 
-    SkImageInfo info = SkImageInfo::Make(origWidth, origHeight, skColorType,
-                                         skAlphaType);
-    SkCodec* codec = SkNEW_ARGS(SkPngCodec, (info, stream, png_ptr, info_ptr));
+    if (imageInfo) {
+        *imageInfo = SkImageInfo::Make(origWidth, origHeight, skColorType,
+                                       skAlphaType);
+    }
     autoClean.detach();
-    return codec;
+    if (png_ptrp) {
+        *png_ptrp = png_ptr;
+    }
+    if (info_ptrp) {
+        *info_ptrp = info_ptr;
+    }
+    return true;
+}
+
+SkCodec* SkPngCodec::NewFromStream(SkStream* stream) {
+    png_structp png_ptr;
+    png_infop info_ptr;
+    SkImageInfo imageInfo;
+    if (read_header(stream, &png_ptr, &info_ptr, &imageInfo)) {
+        return SkNEW_ARGS(SkPngCodec, (imageInfo, stream, png_ptr, info_ptr));
+    }
+    return NULL;
 }
 
 #define INVALID_NUMBER_PASSES -1
@@ -344,7 +367,17 @@ SkPngCodec::SkPngCodec(const SkImageInfo& info, SkStream* stream,
 {}
 
 SkPngCodec::~SkPngCodec() {
-    png_destroy_read_struct(&fPng_ptr, &fInfo_ptr, png_infopp_NULL);
+    this->destroyReadStruct();
+}
+
+void SkPngCodec::destroyReadStruct() {
+    if (fPng_ptr) {
+        // We will never have a NULL fInfo_ptr with a non-NULL fPng_ptr
+        SkASSERT(fInfo_ptr);
+        png_destroy_read_struct(&fPng_ptr, &fInfo_ptr, png_infopp_NULL);
+        fPng_ptr = NULL;
+        fInfo_ptr = NULL;
+    }
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -424,8 +457,20 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void*
     if (rewindState == kCouldNotRewind_RewindState) {
         return kCouldNotRewind;
     } else if (rewindState == kRewound_RewindState) {
-        // TODO(scroggo):  handle rewinds
-        return kCouldNotRewind;
+        // This sets fPng_ptr and fInfo_ptr to NULL. If read_header succeeds,
+        // they will be repopulated, and if it fails, they will remain NULL.
+        // 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();
+        png_structp png_ptr;
+        png_infop info_ptr;
+        if (read_header(this->stream(), &png_ptr, &info_ptr, NULL)) {
+            fPng_ptr = png_ptr;
+            fInfo_ptr = info_ptr;
+        } else {
+            return kCouldNotRewind;
+        }
+
     }
     if (requestedInfo.dimensions() != this->getInfo().dimensions()) {
         return kInvalidScale;

diff --git a/src/codec/SkCodec_libpng.h b/src/codec/SkCodec_libpng.h
index 3afaa39..8e43818 100644 (file)
--- a/src/codec/SkCodec_libpng.h
+++ b/src/codec/SkCodec_libpng.h
@@ -50,6 +50,7 @@ private:
                               size_t rowBytes, const Options&);
     bool decodePalette(bool premultiply);
     void finish();
+    void destroyReadStruct();
 
     friend class SkPngScanlineDecoder;
 

diff --git a/tests/CodexTest.cpp b/tests/CodexTest.cpp
index 94b3b20..f201070 100644 (file)
--- a/tests/CodexTest.cpp
+++ b/tests/CodexTest.cpp
@@ -81,18 +81,17 @@ DEF_TEST(Codec, r) {
     check(r, "color_wheel.ico", SkISize::Make(128, 128), false);
 
     // PNG
-    // TODO (scroggo): SkPngCodec should be able to rewind.
-    check(r, "arrow.png", SkISize::Make(187, 312), false);
-    check(r, "baby_tux.png", SkISize::Make(240, 246), false);
-    check(r, "color_wheel.png", SkISize::Make(128, 128), false);
-    check(r, "half-transparent-white-pixel.png", SkISize::Make(1, 1), false);
-    check(r, "mandrill_128.png", SkISize::Make(128, 128), false);
-    check(r, "mandrill_16.png", SkISize::Make(16, 16), false);
-    check(r, "mandrill_256.png", SkISize::Make(256, 256), false);
-    check(r, "mandrill_32.png", SkISize::Make(32, 32), false);
-    check(r, "mandrill_512.png", SkISize::Make(512, 512), false);
-    check(r, "mandrill_64.png", SkISize::Make(64, 64), false);
-    check(r, "plane.png", SkISize::Make(250, 126), false);
-    check(r, "randPixels.png", SkISize::Make(8, 8), false);
-    check(r, "yellow_rose.png", SkISize::Make(400, 301), false);
+    check(r, "arrow.png", SkISize::Make(187, 312), true);
+    check(r, "baby_tux.png", SkISize::Make(240, 246), true);
+    check(r, "color_wheel.png", SkISize::Make(128, 128), true);
+    check(r, "half-transparent-white-pixel.png", SkISize::Make(1, 1), true);
+    check(r, "mandrill_128.png", SkISize::Make(128, 128), true);
+    check(r, "mandrill_16.png", SkISize::Make(16, 16), true);
+    check(r, "mandrill_256.png", SkISize::Make(256, 256), true);
+    check(r, "mandrill_32.png", SkISize::Make(32, 32), true);
+    check(r, "mandrill_512.png", SkISize::Make(512, 512), true);
+    check(r, "mandrill_64.png", SkISize::Make(64, 64), true);
+    check(r, "plane.png", SkISize::Make(250, 126), true);
+    check(r, "randPixels.png", SkISize::Make(8, 8), true);
+    check(r, "yellow_rose.png", SkISize::Make(400, 301), true);
 }