From 3eada2a49ff2814833afcbd61329aa451d86da9c Mon Sep 17 00:00:00 2001 From: scroggo Date: Wed, 1 Apr 2015 09:33:23 -0700 Subject: 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 --- src/codec/SkCodec_libpng.cpp | 69 ++++++++++++++++++++++++++++++++++++-------- src/codec/SkCodec_libpng.h | 1 + tests/CodexTest.cpp | 27 +++++++++-------- 3 files changed, 71 insertions(+), 26 deletions(-) diff --git a/src/codec/SkCodec_libpng.cpp b/src/codec/SkCodec_libpng.cpp index 9330ac5aee..18574f70be 100644 --- 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(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 3afaa39bf4..8e43818204 100644 --- 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 94b3b20683..f2010708ed 100644 --- 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); } -- cgit v1.2.3