In the case of subset decodes, we will often not decode to
the bottom of the image. In our code before this patch,
this would result in cleanup code in onFinish() never being
called.
We can allow subclasses to take ownership of the
SkScanlineDecoder in order to make sure that it is
finished/deleted before deleting the decode manager.
BUG=skia:
Review URL: https://codereview.chromium.org/1212593003
diff --git a/include/codec/SkCodec.h b/include/codec/SkCodec.h
index 2e47a26..e462be2 100644
--- a/include/codec/SkCodec.h
+++ b/include/codec/SkCodec.h
@@ -168,15 +168,32 @@
*/
RewindState SK_WARN_UNUSED_RESULT rewindIfNeeded();
- /*
- *
+ /**
* Get method for the input stream
- *
*/
SkStream* stream() {
return fStream.get();
}
+ /**
+ * If the codec has a scanline decoder, return it (no ownership change occurs)
+ * else return NULL.
+ * The returned decoder is valid while the codec exists and the client has not
+ * created a new scanline decoder.
+ */
+ SkScanlineDecoder* scanlineDecoder() {
+ return fScanlineDecoder.get();
+ }
+
+ /**
+ * Allow the codec subclass to detach and take ownership of the scanline decoder.
+ * This will likely be used when the scanline decoder needs to be destroyed
+ * in the destructor of the subclass.
+ */
+ SkScanlineDecoder* detachScanlineDecoder() {
+ return fScanlineDecoder.detach();
+ }
+
private:
SkAutoTDelete<SkStream> fStream;
bool fNeedsRewind;
diff --git a/include/codec/SkScanlineDecoder.h b/include/codec/SkScanlineDecoder.h
index d7f73dd..eac31ef 100644
--- a/include/codec/SkScanlineDecoder.h
+++ b/include/codec/SkScanlineDecoder.h
@@ -15,8 +15,20 @@
class SkScanlineDecoder : public SkNoncopyable {
public:
- // Note for implementations: An SkScanlineDecoder will be deleted by (and
- // therefore *before*) its associated SkCodec, in case the order matters.
+ /**
+ * Clean up after reading/skipping scanlines.
+ *
+ * It is possible that not all scanlines will have been read/skipped. In
+ * fact, in the case of subset decodes, it is likely that there will be
+ * scanlines at the bottom of the image that have been ignored.
+ *
+ * Note for implementations: An SkScanlineDecoder will be deleted by (and
+ * therefore *before*) its associated SkCodec, in case the order matters.
+ * However, while the SkCodec base class maintains ownership of the
+ * SkScanlineDecoder, the subclass will be deleted before the scanline
+ * decoder. If this is an issue, detachScanlineDecoder() provides
+ * a means for the subclass to take ownership of the SkScanlineDecoder.
+ */
virtual ~SkScanlineDecoder() {}
/**
@@ -34,7 +46,7 @@
return SkImageGenerator::kInvalidParameters;
}
const SkImageGenerator::Result result = this->onGetScanlines(dst, countLines, rowBytes);
- this->checkForFinish(countLines);
+ fCurrScanline += countLines;
return result;
}
@@ -54,7 +66,7 @@
return SkImageGenerator::kInvalidParameters;
}
const SkImageGenerator::Result result = this->onSkipScanlines(countLines);
- this->checkForFinish(countLines);
+ fCurrScanline += countLines;
return result;
}
@@ -97,21 +109,5 @@
virtual SkImageGenerator::Result onGetScanlines(void* dst, int countLines,
size_t rowBytes) = 0;
- /**
- * Called after any set of scanlines read/skipped. Updates fCurrScanline,
- * and, if we are at the end, calls onFinish().
- */
- void checkForFinish(int countLines) {
- fCurrScanline += countLines;
- if (fCurrScanline >= fDstInfo.height()) {
- this->onFinish();
- }
- }
-
- /**
- * This function will be called after reading/skipping all scanlines to do
- * any necessary cleanups.
- */
- virtual void onFinish() {} // Default does nothing.
};
#endif // SkScanlineDecoder_DEFINED
diff --git a/src/codec/SkCodec_libpng.cpp b/src/codec/SkCodec_libpng.cpp
index 444e2ad..3200436 100644
--- a/src/codec/SkCodec_libpng.cpp
+++ b/src/codec/SkCodec_libpng.cpp
@@ -372,6 +372,12 @@
{}
SkPngCodec::~SkPngCodec() {
+ // First, ensure that the scanline decoder is left in a finished state.
+ SkAutoTDelete<SkScanlineDecoder> decoder(this->detachScanlineDecoder());
+ if (NULL != decoder) {
+ this->finish();
+ }
+
this->destroyReadStruct();
}
@@ -514,6 +520,12 @@
SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void* dst,
size_t rowBytes, const Options& options,
SkPMColor ctable[], int* ctableCount) {
+ // Do not allow a regular decode if the caller has asked for a scanline decoder
+ if (NULL != this->scanlineDecoder()) {
+ SkCodecPrintf("cannot getPixels() if a scanline decoder has been created\n");
+ return kInvalidParameters;
+ }
+
if (!this->handleRewind()) {
return kCouldNotRewind;
}
@@ -633,10 +645,6 @@
return SkImageGenerator::kSuccess;
}
- void onFinish() override {
- fCodec->finish();
- }
-
bool onReallyHasAlpha() const override { return fHasAlpha; }
private:
@@ -716,10 +724,6 @@
return SkImageGenerator::kSuccess;
}
- void onFinish() override {
- fCodec->finish();
- }
-
bool onReallyHasAlpha() const override { return fHasAlpha; }
private:
@@ -734,7 +738,7 @@
-
+
typedef SkScanlineDecoder INHERITED;
};
diff --git a/src/codec/SkJpegCodec.cpp b/src/codec/SkJpegCodec.cpp
index 2eaff1a..9c9bd77 100644
--- a/src/codec/SkJpegCodec.cpp
+++ b/src/codec/SkJpegCodec.cpp
@@ -297,9 +297,15 @@
void* dst, size_t dstRowBytes,
const Options& options, SkPMColor*, int*) {
+ // Do not allow a regular decode if the caller has asked for a scanline decoder
+ if (NULL != this->scanlineDecoder()) {
+ return fDecoderMgr->returnFailure("cannot getPixels() if a scanline decoder has been"
+ "created", kInvalidParameters);
+ }
+
// Rewind the stream if needed
if (!this->handleRewind()) {
- fDecoderMgr->returnFailure("could not rewind stream", kCouldNotRewind);
+ return fDecoderMgr->returnFailure("could not rewind stream", kCouldNotRewind);
}
// Get a pointer to the decompress info since we will use it quite frequently
@@ -388,6 +394,25 @@
}
/*
+ * We override the destructor to ensure that the scanline decoder is left in a
+ * finished state before destroying the decode manager.
+ */
+SkJpegCodec::~SkJpegCodec() {
+ SkAutoTDelete<SkScanlineDecoder> decoder(this->detachScanlineDecoder());
+ if (NULL != decoder) {
+ if (setjmp(fDecoderMgr->getJmpBuf())) {
+ SkCodecPrintf("setjmp: Error in libjpeg finish_decompress\n");
+ return;
+ }
+
+ // We may not have decoded the entire image. Prevent libjpeg-turbo from failing on a
+ // partial decode.
+ fDecoderMgr->dinfo()->output_scanline = this->getInfo().height();
+ jpeg_finish_decompress(fDecoderMgr->dinfo());
+ }
+}
+
+/*
* Enable scanline decoding for jpegs
*/
class SkJpegScanlineDecoder : public SkScanlineDecoder {
@@ -443,15 +468,6 @@
return SkImageGenerator::kSuccess;
}
- void onFinish() override {
- if (setjmp(fCodec->fDecoderMgr->getJmpBuf())) {
- SkCodecPrintf("setjmp: Error in libjpeg finish_decompress\n");
- return;
- }
-
- jpeg_finish_decompress(fCodec->fDecoderMgr->dinfo());
- }
-
private:
SkJpegCodec* fCodec; // unowned
SkAutoMalloc fStorage;
diff --git a/src/codec/SkJpegCodec.h b/src/codec/SkJpegCodec.h
index 87c925d..f39bc49 100644
--- a/src/codec/SkJpegCodec.h
+++ b/src/codec/SkJpegCodec.h
@@ -95,6 +95,12 @@
SkJpegCodec(const SkImageInfo& srcInfo, SkStream* stream, JpegDecoderMgr* decoderMgr);
/*
+ * Explicit destructor is used to ensure that the scanline decoder is deleted
+ * before the decode manager.
+ */
+ ~SkJpegCodec() override;
+
+ /*
* Handles rewinding the input stream if it is necessary
*/
bool handleRewind();
diff --git a/tests/CodexTest.cpp b/tests/CodexTest.cpp
index 5c96990..0edd3cf 100644
--- a/tests/CodexTest.cpp
+++ b/tests/CodexTest.cpp
@@ -72,6 +72,10 @@
if (supportsScanlineDecoding) {
bm.eraseColor(SK_ColorYELLOW);
REPORTER_ASSERT(r, scanlineDecoder);
+
+ // Regular decodes should be disabled after creating a scanline decoder
+ result = codec->getPixels(info, bm.getPixels(), bm.rowBytes(), NULL, NULL, NULL);
+ REPORTER_ASSERT(r, SkImageGenerator::kInvalidParameters == result);
for (int y = 0; y < info.height(); y++) {
result = scanlineDecoder->getScanlines(bm.getAddr(0, y), 1, 0);
REPORTER_ASSERT(r, result == SkImageGenerator::kSuccess);