Allow creating multiple scanline decoders.

src/codec/SkJpegCodec.cpp

Index: src/codec/SkJpegCodec.cpp
diff --git a/src/codec/SkJpegCodec.cpp b/src/codec/SkJpegCodec.cpp
index 858e305b067132b78dd25bd03403090eadd6d2e5..28e1e12bfc4ed7aa3272947a32662c03d72593c1 100644
--- a/src/codec/SkJpegCodec.cpp
+++ b/src/codec/SkJpegCodec.cpp
@@ -303,13 +303,6 @@ bool SkJpegCodec::scaleToDimensions(uint32_t dstWidth, uint32_t dstHeight) {
 SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo,
                                          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()) {
         return fDecoderMgr->returnFailure("could not rewind stream", kCouldNotRewind);
@@ -380,25 +373,6 @@ SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo,
 }
 
 /*
- * 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();
-        turbo_jpeg_finish_decompress(fDecoderMgr->dinfo());
-    }
-}
-
-/*
  * Enable scanline decoding for jpegs
  */
 class SkJpegScanlineDecoder : public SkScanlineDecoder {
@@ -408,6 +382,18 @@ public:
         , fCodec(codec)
     {}
 
+    virtual ~SkJpegScanlineDecoder() {
+        if (setjmp(fCodec->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.
+        fCodec->fDecoderMgr->dinfo()->output_scanline = fCodec->getInfo().height();
+        turbo_jpeg_finish_decompress(fCodec->fDecoderMgr->dinfo());

[scroggo, 2015/07/09 20:52:31]

Matt, I'm questioning this since I questioned the analogous case in libpng.
According to the documentation, this "release[s] working memory associated with
the JPEG object". Is that memory that would not be released by
jpeg_destroy_decompress?

[msarett, 2015/07/09 21:32:26]

Good question.  I originally chose this function as the place to call free() on
the extra row buffer that was used by jpeg_skip_scanlines().  But this is no
longer the case (we now free in jpeg_destroy_decompress).

So do we actually need to call this function?

Before I implemented skipping, we were getting away with not always calling
finish(), so I would expect that now that skipping does not need this function,
we can stop calling it again.

Here are some things it does:
1) Reads to the end of the image.  Seems clear that we don't care about this.
2) Calls term_source on the data source that we provide.  We currently do not do
anything in our implementation of term_source.  But we would want to be very
clear that we also CANNOT do anything important in term_source if we stop
guaranteeing that it will be called.
3) Calls finish_output_pass().  This wraps up any color quantizing (essentially
conversions to using color tables) that we may have been doing.  I think its
highly unlikely that we will ever care about this feature.
4) Calls jpeg_abort().  This does memory cleanup and prepares the jpeg_struct to
be reused if we desire.  AFAICT, jpeg_destroy() will perform the exact same
memory cleanup if jpeg_abort() hasn't already done it.

So that is a very long-winded way of saying, I think you're right.  I don't
*think* we ever need to call jpeg_finish_decompress().

I think it's probably best to leave this in for now.  Then after I finish fixing
the current jpeg-related valgrind bot errors, maybe I can try to land this and
see what happens.

+    }
+
     SkCodec::Result onGetScanlines(void* dst, int count, size_t rowBytes) override {
         // Set the jump location for libjpeg errors
         if (setjmp(fCodec->fDecoderMgr->getJmpBuf())) {
@@ -460,7 +446,7 @@ public:
     }
 
 private:
-    SkJpegCodec* fCodec; // unowned
+    SkAutoTDelete<SkJpegCodec> fCodec;
 
     typedef SkScanlineDecoder INHERITED;
 };
@@ -480,24 +466,33 @@ SkScanlineDecoder* SkJpegCodec::onGetScanlineDecoder(const SkImageInfo& dstInfo,
         return NULL;
     }
 
+    SkStream* stream = this->stream()->duplicate();
+    if (!stream) {
+        return NULL;
+    }
+    SkAutoTDelete<SkJpegCodec> codec(static_cast<SkJpegCodec*>(SkJpegCodec::NewFromStream(stream)));
+    if (!codec) {
+        return NULL;
+    }
+
     // Check if we can decode to the requested destination and set the output color space
-    if (!this->setOutputColorSpace(dstInfo)) {
+    if (!codec->setOutputColorSpace(dstInfo)) {
         SkCodecPrintf("Cannot convert to output type\n");
         return NULL;
     }
 
     // Perform the necessary scaling
-    if (!this->scaleToDimensions(dstInfo.width(), dstInfo.height())) {
+    if (!codec->scaleToDimensions(dstInfo.width(), dstInfo.height())) {
         SkCodecPrintf("Cannot scale to output dimensions\n");
         return NULL;
     }
 
     // Now, given valid output dimensions, we can start the decompress
-    if (!turbo_jpeg_start_decompress(fDecoderMgr->dinfo())) {
+    if (!turbo_jpeg_start_decompress(codec->fDecoderMgr->dinfo())) {
         SkCodecPrintf("start decompress failed\n");
         return NULL;
     }
 
     // Return the new scanline decoder
-    return SkNEW_ARGS(SkJpegScanlineDecoder, (dstInfo, this));
+    return SkNEW_ARGS(SkJpegScanlineDecoder, (dstInfo, codec.detach()));
 }