Create a scanline decoder without creating a codec

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 9f9c110cc68bcc06ed9d38c7c272cdecea42341a..b80287c4205ed7ea3211a8fcf143d354749f2141 100644
--- a/src/codec/SkCodec_libpng.cpp
+++ b/src/codec/SkCodec_libpng.cpp
@@ -594,13 +594,40 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void*
 
 class SkPngScanlineDecoder : public SkScanlineDecoder {
 public:
-    SkPngScanlineDecoder(const SkImageInfo& dstInfo, SkPngCodec* codec)
-        : INHERITED(dstInfo)
+    SkPngScanlineDecoder(const SkImageInfo& srcInfo, SkPngCodec* codec)
+        : INHERITED(srcInfo)
         , fCodec(codec)
         , fHasAlpha(false)
+    {}
+
+    SkCodec::Result onReset(const SkImageInfo& dstInfo,

[emmaleer, 2015/07/29 22:40:48]

Is reset required to be called, for the ScanlineDecoder to work?

Could we call move what's in this function to NewSDFromStream, so then the
ScanlineDecoder would work right away, and reset would only be needed if the
user wanted to update the dstInfo to something else.
This would require NewSDFromStream to take extra parameters.

I think if reset is required, it's going to have to be called right after
NewSDFromStream anyways, so we might as well make them one function. 
Or is there a different reason we don't want to pass the extra parameters to
NewSDFromStream?

If we want to keep the reset function, maybe we should rename is start.. (sorry
I know I changed your mind earlier from start to reset in the first place, but I
didn't know reset would be required to be called)

[msarett, 2015/07/30 12:58:46]

NewSDFromStream sets an SkImageInfo with the recommended settings (colorType,
alphaType, dimensions) for the decode.  Then the client can look at these
recommended settings and ask for the type of decode that is desired.

I think asking the client to know all of the information before the call to
NewSDFromStream is a little too much to ask.  They would need to know output
dimensions, color type, alpha type, and if they want a color table all without
any information from the codec.

Looking at dm probably makes this a little confusing because we get the
recommended info from Codec at the top and then modify the info to what we want,
so when we create the scanline decoder, we are ready to call reset() right away
because we have already decided what dstInfo we want.

Having said that, I agree that it would be better to not have to call reset()
before getScanlines().  I just can't think of a way to do it that is better than
what we are already doing.

I would be in favor of SkScanlineDecoder having an additional API called start()
which calls reset().  That way we can call start() to set up a scanline decoder
and reset() to reuse one.

[scroggo, 2015/07/30 15:11:35]

Yes. I considered making reset unnecessary, but if you did *not* want the
default settings, the constructor would do unnecessary work.
We could. One weird thing, as Matt points out as well, is that we do not know
all the pertinent information. We could know whether our memory was zero
initialized, but the dimensions and whether we need a color table would have to
wait until we have some info. (In some ways, this justifies the old two step
creation process for a scanline decoder.)
Just the above. 
I think either way we have a somewhat weird name. If we had two functions, they
would do the same thing (though start could set a couple less variables, if we
set them in the constructor). If start() can be called to start over, maybe
that's less weird than calling reset() to start?

[msarett, 2015/07/30 15:37:55]

Did you have thoughts on having both a start() and a reset() API where start()
calls reset()?  Maybe this is even more confusing?

[emmaleer, 2015/07/30 15:49:48]

I think calling reset to start is less intuitive than calling start to start.
Also, since the function will mostly be used to start, not to reset, I think
start is a better name. It may occasionally be used to reset. 
We could also call it setInfo() or something that describes what it does. 

[scroggo, 2015/07/30 16:48:48]

FWIW, I do not think setInfo is a good name. It sets the destination info (while
leaving the src info unchanged), but it does a lot more than that.

I am not strongly opposed to having two names, but I would prefer to have one,
just for simplicity.

+                            const SkCodec::Options& options,
+                            SkPMColor ctable[], int* ctableCount) override
     {
+        if (!fCodec->handleRewind()) {
+            return SkCodec::kCouldNotRewind;
+        }
+
+        if (!conversion_possible(dstInfo, this->getInfo())) {
+            return SkCodec::kInvalidConversion;
+        }
+
+        // Check to see if scaling was requested.
+        if (dstInfo.dimensions() != this->getInfo().dimensions()) {
+            return SkCodec::kInvalidScale;
+        }
+
+        const SkCodec::Result result = fCodec->initializeSwizzler(dstInfo, options, ctable,
+                                                                  ctableCount);
+        if (result != SkCodec::kSuccess) {
+            return result;
+        }
+
+        fHasAlpha = false;
         fStorage.reset(dstInfo.width() * SkSwizzler::BytesPerPixel(fCodec->fSrcConfig));
         fSrcRow = static_cast<uint8_t*>(fStorage.get());
+
+        return SkCodec::kSuccess;
     }
 
     SkCodec::Result onGetScanlines(void* dst, int count, size_t rowBytes) override {
@@ -648,16 +675,44 @@ private:
 
 class SkPngInterlacedScanlineDecoder : public SkScanlineDecoder {
 public:
-    SkPngInterlacedScanlineDecoder(const SkImageInfo& dstInfo, SkPngCodec* codec)
-        : INHERITED(dstInfo)
+    SkPngInterlacedScanlineDecoder(const SkImageInfo& srcInfo, SkPngCodec* codec)
+        : INHERITED(srcInfo)
         , fCodec(codec)
         , fHasAlpha(false)
         , fCurrentRow(0)
-        , fHeight(dstInfo.height())
+        , fHeight(srcInfo.height())
+    {}
+
+    SkCodec::Result onReset(const SkImageInfo& dstInfo,
+                            const SkCodec::Options& options,
+                            SkPMColor ctable[], int* ctableCount) override
     {
+        // No need to handleRewind here, since it will be done in each call to
+        // getScanlines.
+
+        if (!conversion_possible(dstInfo, this->getInfo())) {
+            return SkCodec::kInvalidConversion;
+        }
+
+        // Check to see if scaling was requested.
+        if (dstInfo.dimensions() != this->getInfo().dimensions()) {
+            return SkCodec::kInvalidScale;
+        }
+
+        const SkCodec::Result result = fCodec->initializeSwizzler(dstInfo, options, ctable,
+                                                                  ctableCount);
+        if (result != SkCodec::kSuccess) {
+            return result;
+        }
+
+        fHasAlpha = false;
+        fCurrentRow = 0;
+        fHeight = dstInfo.height();
         fSrcRowBytes = dstInfo.width() * SkSwizzler::BytesPerPixel(fCodec->fSrcConfig);
         fGarbageRow.reset(fSrcRowBytes);
         fGarbageRowPtr = static_cast<uint8_t*>(fGarbageRow.get());
+
+        return SkCodec::kSuccess;
     }
 
     SkCodec::Result onGetScanlines(void* dst, int count, size_t dstRowBytes) override {
@@ -722,38 +777,21 @@ private:
     typedef SkScanlineDecoder INHERITED;
 };
 
-
-SkScanlineDecoder* SkPngCodec::onGetScanlineDecoder(const SkImageInfo& dstInfo,
-        const Options& options, SkPMColor ctable[], int* ctableCount) {
-    if (!conversion_possible(dstInfo, this->getInfo())) {
-        SkCodecPrintf("no conversion possible\n");
-        return NULL;
-    }
-    // Check to see if scaling was requested.
-    if (dstInfo.dimensions() != this->getInfo().dimensions()) {
-        return NULL;
-    }
-    // Create a new SkPngCodec, to be owned by the scanline decoder.
-    SkStream* stream = this->stream()->duplicate();
-    if (!stream) {
-        return NULL;
-    }
+SkScanlineDecoder* SkPngCodec::NewSDFromStream(SkStream* stream) {
     SkAutoTDelete<SkPngCodec> codec (static_cast<SkPngCodec*>(SkPngCodec::NewFromStream(stream)));

[msarett, 2015/07/30 12:58:46]

I'm noticing that we still need to create a codec in order to create a scanline
decoder, but now we hide it from the client.  I think this is an improvement
over how we did things before.

[scroggo, 2015/07/30 15:11:35]

This is an implementation detail, though. We *could* create some sort of
isolated state machine, which is owned by the codec and the scanline decoder,
but when the scanline decoders were written, we already had codecs with state
machines that were not easily separable, so we just reused them.

The key difference here is that we do not need to do any duplication or
rereading of the header, like we did in the old world.

     if (!codec) {
         return NULL;
     }
 
-    if (codec->initializeSwizzler(dstInfo, options, ctable, ctableCount) != kSuccess) {
-        SkCodecPrintf("failed to initialize the swizzler.\n");
-        return NULL;
-    }
-
+    codec->fNumberPasses = png_set_interlace_handling(codec->fPng_ptr);
     SkASSERT(codec->fNumberPasses != INVALID_NUMBER_PASSES);
+
+    const SkImageInfo& srcInfo = codec->getInfo();
     if (codec->fNumberPasses > 1) {
         // interlaced image
-        return SkNEW_ARGS(SkPngInterlacedScanlineDecoder, (dstInfo, codec.detach()));
+        return SkNEW_ARGS(SkPngInterlacedScanlineDecoder, (srcInfo, codec.detach()));
     }
 
-    return SkNEW_ARGS(SkPngScanlineDecoder, (dstInfo, codec.detach()));
+    return SkNEW_ARGS(SkPngScanlineDecoder, (srcInfo, codec.detach()));
 }