onGetScanlines and onSkipScanlines for interlaced pngs

src/codec/SkCodec_libpng.cpp

Index: src/codec/SkCodec_libpng.cpp
diff --git a/src/codec/SkCodec_libpng.cpp b/src/codec/SkCodec_libpng.cpp
index c2a032e84ea8d98017998bdf5e6f9e12bf059685..c02990c1c6a201ae7955853c2a4bd32855adc20e 100644
--- a/src/codec/SkCodec_libpng.cpp
+++ b/src/codec/SkCodec_libpng.cpp
@@ -648,6 +648,88 @@ private:
     typedef SkScanlineDecoder INHERITED;
 };
 
+
+class SkPngInterlacedScanlineDecoder : public SkScanlineDecoder {
+public:
+    SkPngInterlacedScanlineDecoder(const SkImageInfo& dstInfo, SkPngCodec* codec)
+        : INHERITED(dstInfo)
+        , fCodec(codec)
+        , fHasAlpha(false)
+    {
+        fCurrentRow = 0;

[emmaleer, 2015/06/18 21:05:41]

Should these variables be defined in the cosntructor, or in onGetScanlines()?
They could be defined in either place 

[msarett, 2015/06/19 13:35:54]

I prefer them here.  For interlaced png, you probably only intend for
onGetScanlines() to be called once, but it looks like it can be called multiple
times.  If the variables are in the constructor, we are sure that the
initialization only happens once.

Also, I think it would be nice to use the initializer list to define them.
For example,
...
, fHasAlpha(false)
, fCurrentRow(0)
, fHeight(dstInfo.height())
...
is equivalent to what you wrote.

[emmaleer, 2015/06/19 14:28:08]

Done.

[scroggo, 2015/06/19 15:45:53]

Taking that a step further, I believe you can even put fStorageGarbage in the
initializer list, since it has a constructor that does the same thing as
reset(). It can be a little tricky though, since you want to initialize it with
the value of another member variable, and member variables will get initialized
in the order they are declared, rather than their ordering in the initializer
list. I think we have the warning turned on (as an error) that fires if the two
orderings do not match (although I don't think you'd get a warning if the
orderings did match, but you initialized them in the wrong order).

All that said, for some member variables (i.e. those with dependencies on each
other), it might be preferable to initialize in the body of the constructor.

[emmaleer, 2015/06/19 22:12:20]

I left fStorageGarbage in the body of the constructor, since all the other
scanlineDecoder classes do this, and I wanted to keep them consistent for
readability.  

+        fHeight = dstInfo.height();
+        fSrcRowBytes = dstInfo.minRowBytes();
+        fStorageGarbage.reset(fSrcRowBytes);
+        fGarbageRow = static_cast<uint8_t*>(fStorageGarbage.get());
+    }
+
+    SkImageGenerator::Result onGetScanlines(void* dst, int count, size_t dstRowBytes) override {
+        //rewind stream since we need entire progressive image to get scanlines
+        fCodec->handleRewind();

[scroggo, 2015/06/19 15:45:53]

If handleRewind returns false (which it can for a stream that cannot be rewound,
although we will typically use it with a stream that can be), this should return
kCouldNotRewind.

[emmaleer, 2015/06/19 22:12:20]

Done.

+        if (setjmp(png_jmpbuf(fCodec->fPng_ptr))) {
+            SkCodecPrintf("setjmp long jump!\n");
+            return SkImageGenerator::kInvalidInput;
+        }
+        const int number_passes = png_set_interlace_handling(fCodec->fPng_ptr);
+        fStorageData.reset(count * fSrcRowBytes);

[msarett, 2015/06/19 13:35:54]

Maybe this belongs on the constructor as well?  Do we need both fStorageData and
fBuffer to be fields?  Same question for fStorageGarbage and fGarbageRow.

[scroggo, 2015/06/19 15:45:53]

(FYI: If you put ">"s at the beginning of each line in your response, it looks
like it's quoting text, making it hard to distinguish between what you wrote and
what you're replying to.)

I believe what Matt is saying is that instead of making fStorageData and fBuffer
"fields" (another word for member variables), you could just put them on the
stack, since they're only used inside this function.

fStorageGarbage should arguably remain a field, though, since you'll use the
same one for the next call to onGetScanlines (if there is another).

[emmaleer, 2015/06/19 22:12:20]

Good to know, I thought ">" was used for a new line.
fStorage and fStoragePtr are no longer fields 

+        fBuffer = static_cast<uint8_t*>(fStorageData.get());
+
+        for (int i = 0; i < number_passes; i++) {
+            //read rows we planned to skip into garbage row
+            for (int y = 0; y < fCurrentRow; y++){
+                png_read_rows(fCodec->fPng_ptr, &fGarbageRow, png_bytepp_NULL, 1);
+            }
+            //read rows we care about into buffer
+            fSrcRow = fBuffer;
+            for (int y = 0; y < count; y++) {
+                png_read_rows(fCodec->fPng_ptr, &fSrcRow, png_bytepp_NULL, 1);
+                fSrcRow += fSrcRowBytes;
+            }
+            //read rows we don't want into garbage buffer
+            for (int y = 0; y < fHeight - fCurrentRow - count; y++) {
+                png_read_rows(fCodec->fPng_ptr, &fGarbageRow, png_bytepp_NULL, 1);
+            }
+        }
+        //swizzle the rows we care about
+        fSrcRow = fBuffer;
+        for (int y = 0; y < count; y++) {
+            fCodec->fSwizzler->setDstRow(dst);
+            fHasAlpha |= !SkSwizzler::IsOpaque(fCodec->fSwizzler->next(fSrcRow));
+            dst = SkTAddOffset<void>(dst, dstRowBytes);
+           fSrcRow += fSrcRowBytes;
+        }
+        fCurrentRow += count;
+        return SkImageGenerator::kSuccess;
+    }
+
+    SkImageGenerator::Result onSkipScanlines(int count) override {
+        //when ongetScanlines is called it will skip to fCurrentRow
+        fCurrentRow += count;

[scroggo, 2015/06/19 15:45:53]

I think if we've already read rows, you'll want to do a rewind here as well. (If
we've only skipped rows previously, you do not need to rewind, but given that
that is presumably an uncommon use case, you may not need to take it into
consideration - but maybe add a comment that it wouldn't be necessary.)

The tricky thing is that this is a little bit of a mismatch between my previous
thoughts on rewinding and this particular situation:

A) (old thinking) The base class SkCodec has a function called rewindIfNeeded.
It does a couple of things:
  - It attempts to rewind if the stream has already been read (past initial
setup)
  - It makes a note that the stream has already been read

B) (this situation) You want to rewind the stream if it has already been read,
but you do *not* want to mark it as read (since you don't want the next call to
getScanlines to require a rewind - it would still work, but it would be less
efficient). (Come to think of it, solving this problem will solve the earlier
problem I mentioned, where you don't want to rewind if all you've done so far is
skip.)

I think the simplest solution will be to use a boolean in this class that keeps
track of whether you've read any data yet. Maybe fCurrentRow is enough? It
doesn't solve the consecutive calls to skipScanlines, but as I mentioned
earlier, I think that's an okay case to handle poorly.

This method might give us the wrong behavior if someone tried to do the
following:

SkCodec* codec = SkCodec::NewFromStream();
SkScanlineDecoder* sd = codec->getScanlineDecoder();
codec->getPixels(); // After this call we need to rewind
sd->getScanlines(); // If we only check our local info, rather than
rewindIfNeeded
                    // sd won't know to rewind.

Maybe that's a dumb use case, although I think it is currently supported.

The other way would be to plumb an option to rewindIfNeeded to only check if a
rewind is necessary, but not tell it the next call will require a rewind. This
would support the dumb use case (and might be more straightforward).

That was kind of a brain dump of my thoughts on this, so I may not have
explained it well... Let me know if you want to discuss in person.

[scroggo, 2015/06/19 16:00:40]

And, I was confused...

The way the other scanline decoders are written, you do *not* need to call
handleRewind inside onGetScanlines - that is called inside getScanlineDecoder -
the expectation is that you will call it again if you want to start from the
beginning.

But you will need to do a rewind if you get some scanlines, and then attempt to
get more (even if they are not the following ones).

[emmaleer, 2015/06/19 22:12:20]

I added the boolean fRewindNeeded to keep track if we've called onGetScanlines
before and need to rewind 

+        return SkImageGenerator::kSuccess;
+    }
+
+    void onFinish() override {
+        fCodec->finish();
+    }
+
+    bool onReallyHasAlpha() const override { return fHasAlpha; }
+
+private:
+    SkPngCodec*         fCodec;     // Unowned.
+    bool                fHasAlpha;
+    SkAutoMalloc        fStorageData;
+    SkAutoMalloc        fStorageGarbage;
+    uint8_t*            fBuffer;
+    uint8_t*            fSrcRow;
+    uint8_t*            fGarbageRow;
+    int                 fHeight;
+    int                 fSrcRowBytes;
+    int                 fCurrentRow;
+
+    typedef SkScanlineDecoder INHERITED;
+};
+
+
 SkScanlineDecoder* SkPngCodec::onGetScanlineDecoder(const SkImageInfo& dstInfo,
         const Options& options, SkPMColor ctable[], int* ctableCount) {
     if (!this->handleRewind()) {
@@ -675,8 +757,8 @@ SkScanlineDecoder* SkPngCodec::onGetScanlineDecoder(const SkImageInfo& dstInfo,
 
     SkASSERT(fNumberPasses != INVALID_NUMBER_PASSES);
     if (fNumberPasses > 1) {
-        // We cannot efficiently do scanline decoding.
-        return NULL;
+        // interlaced image
+        return SkNEW_ARGS(SkPngInterlacedScanlineDecoder, (dstInfo, this));
     }
 
     return SkNEW_ARGS(SkPngScanlineDecoder, (dstInfo, this));