From b427db1d457a083f2652756a453fbb91bc6a7447 Mon Sep 17 00:00:00 2001 From: scroggo Date: Wed, 12 Aug 2015 07:24:13 -0700 Subject: [PATCH] Consolidate SkCodec functions for handling rewind Previously, many of our codec implementations followed the same pattern (often in a function named handleRewind): switch (this->rewindIfNeeded()) { case CouldNotRewind: return CouldNotRewind; case NoRewindNecessary: // keep going break; case Rewound: break; } In this CL, remove the enum, and put the piece that happens in the Rewound case into a virtual function, onRewind. rewindIfNeeded now contains the common pieces from various functions named handleRewind. In SkBmpCodec, add a function that returns whether the BMP is in ICO, so it can have a common implementation for onRewind. BUG=skia:3257 Review URL: https://codereview.chromium.org/1288483002 --- include/codec/SkCodec.h | 27 +++++++++--------- src/codec/SkBmpCodec.cpp | 15 ++-------- src/codec/SkBmpCodec.h | 12 ++++++-- src/codec/SkBmpMaskCodec.cpp | 2 +- src/codec/SkBmpRLECodec.cpp | 2 +- src/codec/SkBmpStandardCodec.cpp | 2 +- src/codec/SkBmpStandardCodec.h | 3 ++ src/codec/SkCodec.cpp | 12 +++++--- src/codec/SkCodec_libgif.cpp | 22 +++++++------- src/codec/SkCodec_libgif.h | 2 ++ src/codec/SkCodec_libpng.cpp | 49 +++++++++++++------------------- src/codec/SkCodec_libpng.h | 3 +- src/codec/SkCodec_wbmp.cpp | 16 +++-------- src/codec/SkCodec_wbmp.h | 7 +---- src/codec/SkJpegCodec.cpp | 32 ++++++--------------- src/codec/SkJpegCodec.h | 7 ++--- src/codec/SkWebpCodec.cpp | 12 ++------ 17 files changed, 93 insertions(+), 132 deletions(-) diff --git a/include/codec/SkCodec.h b/include/codec/SkCodec.h index ce581632e1..a0bd18a8e5 100644 --- a/include/codec/SkCodec.h +++ b/include/codec/SkCodec.h @@ -233,26 +233,27 @@ protected: virtual bool onReallyHasAlpha() const { return false; } - enum RewindState { - kRewound_RewindState, - kNoRewindNecessary_RewindState, - kCouldNotRewind_RewindState - }; /** * If the stream was previously read, attempt to rewind. - * @returns: - * kRewound if the stream needed to be rewound, and the - * rewind succeeded. - * kNoRewindNecessary if the stream did not need to be - * rewound. - * kCouldNotRewind if the stream needed to be rewound, and - * rewind failed. + * + * If the stream needed to be rewound, call onRewind. + * @returns true if the codec is at the right position and can be used. + * false if there was a failure to rewind. * * Subclasses MUST call this function before reading the stream (e.g. in * onGetPixels). If it returns false, onGetPixels should return * kCouldNotRewind. */ - RewindState SK_WARN_UNUSED_RESULT rewindIfNeeded(); + bool SK_WARN_UNUSED_RESULT rewindIfNeeded(); + + /** + * Called by rewindIfNeeded, if the stream needed to be rewound. + * + * Subclasses should do any set up needed after a rewind. + */ + virtual bool onRewind() { + return true; + } /** * Get method for the input stream diff --git a/src/codec/SkBmpCodec.cpp b/src/codec/SkBmpCodec.cpp index 4383382d8a..5eb53544b5 100644 --- a/src/codec/SkBmpCodec.cpp +++ b/src/codec/SkBmpCodec.cpp @@ -533,19 +533,8 @@ SkBmpCodec::SkBmpCodec(const SkImageInfo& info, SkStream* stream, , fRowOrder(rowOrder) {} -/* - * Rewinds the image stream if necessary - */ -bool SkBmpCodec::handleRewind(bool inIco) { - SkCodec::RewindState rewindState = this->rewindIfNeeded(); - if (rewindState == kCouldNotRewind_RewindState) { - return false; - } else if (rewindState == kRewound_RewindState) { - if (!SkBmpCodec::ReadHeader(this->stream(), inIco, NULL)) { - return false; - } - } - return true; +bool SkBmpCodec::onRewind() { + return SkBmpCodec::ReadHeader(this->stream(), this->inIco(), NULL); } /* diff --git a/src/codec/SkBmpCodec.h b/src/codec/SkBmpCodec.h index 51d0300091..71006f7315 100644 --- a/src/codec/SkBmpCodec.h +++ b/src/codec/SkBmpCodec.h @@ -63,10 +63,18 @@ protected: */ static bool ReadHeader(SkStream*, bool inIco, SkCodec** codecOut); + bool onRewind() override; + /* - * Rewinds the image stream if necessary + * Returns whether this BMP is part of an ICO image. */ - bool handleRewind(bool inIco); + bool inIco() const { + return this->onInIco(); + } + + virtual bool onInIco() const { + return false; + } /* * Get the destination row to start filling from diff --git a/src/codec/SkBmpMaskCodec.cpp b/src/codec/SkBmpMaskCodec.cpp index f30266b688..7d1706e697 100644 --- a/src/codec/SkBmpMaskCodec.cpp +++ b/src/codec/SkBmpMaskCodec.cpp @@ -56,7 +56,7 @@ SkCodec::Result SkBmpMaskCodec::onGetPixels(const SkImageInfo& dstInfo, const Options& opts, SkPMColor* inputColorPtr, int* inputColorCount) { - if (!this->handleRewind(false)) { + if (!this->rewindIfNeeded()) { return kCouldNotRewind; } if (opts.fSubset) { diff --git a/src/codec/SkBmpRLECodec.cpp b/src/codec/SkBmpRLECodec.cpp index 828871cd54..6be7449ed8 100644 --- a/src/codec/SkBmpRLECodec.cpp +++ b/src/codec/SkBmpRLECodec.cpp @@ -66,7 +66,7 @@ SkCodec::Result SkBmpRLECodec::onGetPixels(const SkImageInfo& dstInfo, const Options& opts, SkPMColor* inputColorPtr, int* inputColorCount) { - if (!this->handleRewind(false)) { + if (!this->rewindIfNeeded()) { return kCouldNotRewind; } if (opts.fSubset) { diff --git a/src/codec/SkBmpStandardCodec.cpp b/src/codec/SkBmpStandardCodec.cpp index 70535a72ac..08146fc3e4 100644 --- a/src/codec/SkBmpStandardCodec.cpp +++ b/src/codec/SkBmpStandardCodec.cpp @@ -66,7 +66,7 @@ SkCodec::Result SkBmpStandardCodec::onGetPixels(const SkImageInfo& dstInfo, const Options& opts, SkPMColor* inputColorPtr, int* inputColorCount) { - if (!this->handleRewind(fInIco)) { + if (!this->rewindIfNeeded()) { return kCouldNotRewind; } if (opts.fSubset) { diff --git a/src/codec/SkBmpStandardCodec.h b/src/codec/SkBmpStandardCodec.h index ff91dcf5e9..45450e6591 100644 --- a/src/codec/SkBmpStandardCodec.h +++ b/src/codec/SkBmpStandardCodec.h @@ -45,6 +45,9 @@ protected: size_t dstRowBytes, const Options&, SkPMColor*, int*) override; + bool onInIco() const override { + return fInIco; + } private: /* diff --git a/src/codec/SkCodec.cpp b/src/codec/SkCodec.cpp index ffe90af539..3e5ebe1850 100644 --- a/src/codec/SkCodec.cpp +++ b/src/codec/SkCodec.cpp @@ -83,16 +83,20 @@ SkCodec::SkCodec(const SkImageInfo& info, SkStream* stream) SkCodec::~SkCodec() {} -SkCodec::RewindState SkCodec::rewindIfNeeded() { +bool SkCodec::rewindIfNeeded() { // Store the value of fNeedsRewind so we can update it. Next read will // require a rewind. const bool needsRewind = fNeedsRewind; fNeedsRewind = true; if (!needsRewind) { - return kNoRewindNecessary_RewindState; + return true; } - return fStream->rewind() ? kRewound_RewindState - : kCouldNotRewind_RewindState; + + if (!fStream->rewind()) { + return false; + } + + return this->onRewind(); } SkCodec::Result SkCodec::getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes, diff --git a/src/codec/SkCodec_libgif.cpp b/src/codec/SkCodec_libgif.cpp index cf935777d1..2e348cc856 100644 --- a/src/codec/SkCodec_libgif.cpp +++ b/src/codec/SkCodec_libgif.cpp @@ -234,6 +234,17 @@ static bool conversion_possible(const SkImageInfo& dst, } } +bool SkGifCodec::onRewind() { + GifFileType* gifOut = NULL; + if (!ReadHeader(this->stream(), NULL, &gifOut)) { + return false; + } + + SkASSERT(NULL != gifOut); + fGif.reset(gifOut); + return true; +} + /* * Initiates the gif decode */ @@ -243,17 +254,8 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo, SkPMColor* inputColorPtr, int* inputColorCount) { // Rewind if necessary - SkCodec::RewindState rewindState = this->rewindIfNeeded(); - if (rewindState == kCouldNotRewind_RewindState) { + if (!this->rewindIfNeeded()) { return kCouldNotRewind; - } else if (rewindState == kRewound_RewindState) { - GifFileType* gifOut = NULL; - if (!ReadHeader(this->stream(), NULL, &gifOut)) { - return kCouldNotRewind; - } else { - SkASSERT(NULL != gifOut); - fGif.reset(gifOut); - } } // Check for valid input parameters diff --git a/src/codec/SkCodec_libgif.h b/src/codec/SkCodec_libgif.h index d805accac3..109020b5cf 100644 --- a/src/codec/SkCodec_libgif.h +++ b/src/codec/SkCodec_libgif.h @@ -64,6 +64,8 @@ protected: return kGIF_SkEncodedFormat; } + bool onRewind() override; + private: /* diff --git a/src/codec/SkCodec_libpng.cpp b/src/codec/SkCodec_libpng.cpp index f23990095b..c94f371399 100644 --- a/src/codec/SkCodec_libpng.cpp +++ b/src/codec/SkCodec_libpng.cpp @@ -471,32 +471,23 @@ SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo, } -bool SkPngCodec::handleRewind() { - switch (this->rewindIfNeeded()) { - case kNoRewindNecessary_RewindState: - return true; - case kCouldNotRewind_RewindState: - return false; - case kRewound_RewindState: { - // 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, NULL)) { - fPng_ptr = png_ptr; - fInfo_ptr = info_ptr; - return true; - } - return false; - } - default: - SkASSERT(false); - return false; +bool SkPngCodec::onRewind() { + // 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, NULL)) { + return false; } + + fPng_ptr = png_ptr; + fInfo_ptr = info_ptr; + return true; } SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void* dst, @@ -512,7 +503,7 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void* if (requestedInfo.dimensions() != this->getInfo().dimensions()) { return kInvalidScale; } - if (!this->handleRewind()) { + if (!this->rewindIfNeeded()) { return kCouldNotRewind; } @@ -593,7 +584,7 @@ public: const SkCodec::Options& options, SkPMColor ctable[], int* ctableCount) override { - if (!fCodec->handleRewind()) { + if (!fCodec->rewindIfNeeded()) { return SkCodec::kCouldNotRewind; } @@ -677,7 +668,7 @@ public: const SkCodec::Options& options, SkPMColor ctable[], int* ctableCount) override { - if (!fCodec->handleRewind()) { + if (!fCodec->rewindIfNeeded()) { return SkCodec::kCouldNotRewind; } @@ -714,7 +705,7 @@ public: // 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->handleRewind()) { + } else if (!fCodec->rewindIfNeeded()) { return SkCodec::kCouldNotRewind; } diff --git a/src/codec/SkCodec_libpng.h b/src/codec/SkCodec_libpng.h index 21bbdadb49..890402200c 100644 --- a/src/codec/SkCodec_libpng.h +++ b/src/codec/SkCodec_libpng.h @@ -35,6 +35,7 @@ protected: Result onGetPixels(const SkImageInfo&, void*, size_t, const Options&, SkPMColor*, int*) override; SkEncodedFormat onGetEncodedFormat() const override { return kPNG_SkEncodedFormat; } + bool onRewind() override; bool onReallyHasAlpha() const override { return fReallyHasAlpha; } private: png_structp fPng_ptr; @@ -56,8 +57,6 @@ private: Result initializeSwizzler(const SkImageInfo& requestedInfo, const Options&, SkPMColor*, int* ctableCount); - // Calls rewindIfNeeded and returns true if the decoder can continue. - bool handleRewind(); bool decodePalette(bool premultiply, int* ctableCount); void destroyReadStruct(); diff --git a/src/codec/SkCodec_wbmp.cpp b/src/codec/SkCodec_wbmp.cpp index 86dce5cc89..52535e1f02 100644 --- a/src/codec/SkCodec_wbmp.cpp +++ b/src/codec/SkCodec_wbmp.cpp @@ -66,16 +66,8 @@ static bool read_header(SkStream* stream, SkISize* size) { return true; } -bool SkWbmpCodec::handleRewind() { - SkCodec::RewindState rewindState = this->rewindIfNeeded(); - if (rewindState == kCouldNotRewind_RewindState) { - return false; - } else if (rewindState == kRewound_RewindState) { - if (!read_header(this->stream(), NULL)) { - return false; - } - } - return true; +bool SkWbmpCodec::onRewind() { + return read_header(this->stream(), NULL); } SkSwizzler* SkWbmpCodec::initializeSwizzler(const SkImageInfo& info, @@ -117,7 +109,7 @@ SkCodec::Result SkWbmpCodec::onGetPixels(const SkImageInfo& info, const Options& options, SkPMColor ctable[], int* ctableCount) { - if (!this->handleRewind()) { + if (!this->rewindIfNeeded()) { return kCouldNotRewind; } if (options.fSubset) { @@ -197,7 +189,7 @@ public: SkCodec::Result onStart(const SkImageInfo& dstInfo, const SkCodec::Options& options, SkPMColor inputColorTable[], int* inputColorCount) { - if (!fCodec->handleRewind()) { + if (!fCodec->rewindIfNeeded()) { return SkCodec::kCouldNotRewind; } if (options.fSubset) { diff --git a/src/codec/SkCodec_wbmp.h b/src/codec/SkCodec_wbmp.h index cec398d34b..8c5466105c 100644 --- a/src/codec/SkCodec_wbmp.h +++ b/src/codec/SkCodec_wbmp.h @@ -33,13 +33,8 @@ protected: SkEncodedFormat onGetEncodedFormat() const override; Result onGetPixels(const SkImageInfo&, void*, size_t, const Options&, SkPMColor[], int*) override; + bool onRewind() override; private: - - /* - * Calls rewindIfNeeded and returns true if the decoder can continue - */ - bool handleRewind(); - /* * Returns a swizzler on success, NULL on failure */ diff --git a/src/codec/SkJpegCodec.cpp b/src/codec/SkJpegCodec.cpp index 0dc2795044..83656da42d 100644 --- a/src/codec/SkJpegCodec.cpp +++ b/src/codec/SkJpegCodec.cpp @@ -189,28 +189,14 @@ SkISize SkJpegCodec::onGetScaledDimensions(float desiredScale) const { return SkISize::Make(dinfo.output_width, dinfo.output_height); } -/* - * Handles rewinding the input stream if it is necessary - */ -bool SkJpegCodec::handleRewind() { - switch(this->rewindIfNeeded()) { - case kCouldNotRewind_RewindState: - return fDecoderMgr->returnFalse("could not rewind"); - case kRewound_RewindState: { - JpegDecoderMgr* decoderMgr = NULL; - if (!ReadHeader(this->stream(), NULL, &decoderMgr)) { - return fDecoderMgr->returnFalse("could not rewind"); - } - SkASSERT(NULL != decoderMgr); - fDecoderMgr.reset(decoderMgr); - return true; - } - case kNoRewindNecessary_RewindState: - return true; - default: - SkASSERT(false); - return false; +bool SkJpegCodec::onRewind() { + JpegDecoderMgr* decoderMgr = NULL; + if (!ReadHeader(this->stream(), NULL, &decoderMgr)) { + return fDecoderMgr->returnFalse("could not rewind"); } + SkASSERT(NULL != decoderMgr); + fDecoderMgr.reset(decoderMgr); + return true; } /* @@ -304,7 +290,7 @@ SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo, void* dst, size_t dstRowBytes, const Options& options, SkPMColor*, int*) { // Rewind the stream if needed - if (!this->handleRewind()) { + if (!this->rewindIfNeeded()) { return fDecoderMgr->returnFailure("could not rewind stream", kCouldNotRewind); } @@ -399,7 +385,7 @@ public: SkPMColor ctable[], int* ctableCount) override { // Rewind the stream if needed - if (!fCodec->handleRewind()) { + if (!fCodec->rewindIfNeeded()) { return SkCodec::kCouldNotRewind; } diff --git a/src/codec/SkJpegCodec.h b/src/codec/SkJpegCodec.h index 5e2135cdff..62a5bbc4cf 100644 --- a/src/codec/SkJpegCodec.h +++ b/src/codec/SkJpegCodec.h @@ -65,6 +65,8 @@ protected: return kJPEG_SkEncodedFormat; } + bool onRewind() override; + private: /* @@ -100,11 +102,6 @@ private: */ SkJpegCodec(const SkImageInfo& srcInfo, SkStream* stream, JpegDecoderMgr* decoderMgr); - /* - * Handles rewinding the input stream if it is necessary - */ - bool handleRewind(); - /* * Checks if the conversion between the input image and the requested output * image has been implemented diff --git a/src/codec/SkWebpCodec.cpp b/src/codec/SkWebpCodec.cpp index 041bfb6b63..624ff74fa0 100644 --- a/src/codec/SkWebpCodec.cpp +++ b/src/codec/SkWebpCodec.cpp @@ -153,16 +153,8 @@ bool SkWebpCodec::onGetValidSubset(SkIRect* desiredSubset) const { SkCodec::Result SkWebpCodec::onGetPixels(const SkImageInfo& dstInfo, void* dst, size_t rowBytes, const Options& options, SkPMColor*, int*) { - switch (this->rewindIfNeeded()) { - case kCouldNotRewind_RewindState: - return kCouldNotRewind; - case kRewound_RewindState: - // Rewound to the beginning. Since creation only does a peek, the stream is at the - // correct position. - break; - case kNoRewindNecessary_RewindState: - // Already at the right spot for decoding. - break; + if (!this->rewindIfNeeded()) { + return kCouldNotRewind; } if (!conversion_possible(dstInfo, this->getInfo())) {