SkScaledCodec class

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 553233de12117ea8ccebcdfd261a720b8e721412..ca7c73fb0bb0012595ee2316d2aec05a9f2321fd 100644
--- a/src/codec/SkCodec_libpng.cpp
+++ b/src/codec/SkCodec_libpng.cpp
@@ -424,6 +424,14 @@ static bool conversion_possible(const SkImageInfo& dst, const SkImageInfo& src)
     }
 }
 
+bool SkPngCodec::onIsInterlaced() {
+    if (1 == png_set_interlace_handling(fPng_ptr)){

[scroggo, 2015/07/27 15:09:19]

You actually do not need a conditional here. You can just say:

return 1 != png_set_interlace_handling(fPng_ptr);

Maybe add a comment that we can not depend on fNumberPasses, since it may not
have been set yet.

[emmaleer, 2015/07/27 18:31:38]

Acknowledged.

+        return false;
+    } else {
+        return true;
+    }
+}
+
 SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
                                                void* dst, size_t rowBytes,
                                                const Options& options,
@@ -593,14 +601,19 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void*
 class SkPngScanlineDecoder : public SkScanlineDecoder {
 public:
     SkPngScanlineDecoder(const SkImageInfo& dstInfo, SkPngCodec* codec)
-        : INHERITED(dstInfo)
+        : INHERITED(dstInfo, codec->getInfo())
         , fCodec(codec)
         , fHasAlpha(false)
     {
-        fStorage.reset(dstInfo.width() * SkSwizzler::BytesPerPixel(fCodec->fSrcConfig));
+        fStorage.reset(fCodec->getInfo().width() * SkSwizzler::BytesPerPixel(fCodec->fSrcConfig));
         fSrcRow = static_cast<uint8_t*>(fStorage.get());
     }
 
+    bool onSetSampleX(int sampleX) override {
+        fCodec->fSwizzler->setSampleX(sampleX);
+        return true;
+    }
+
     SkCodec::Result onGetScanlines(void* dst, int count, size_t rowBytes) override {
         if (setjmp(png_jmpbuf(fCodec->fPng_ptr))) {
             SkCodecPrintf("setjmp long jump!\n");
@@ -647,17 +660,22 @@ private:
 class SkPngInterlacedScanlineDecoder : public SkScanlineDecoder {
 public:
     SkPngInterlacedScanlineDecoder(const SkImageInfo& dstInfo, SkPngCodec* codec)
-        : INHERITED(dstInfo)
+        : INHERITED(dstInfo, codec->getInfo())
         , fCodec(codec)
         , fHasAlpha(false)
         , fCurrentRow(0)
         , fHeight(dstInfo.height())
     {
-        fSrcRowBytes = dstInfo.width() * SkSwizzler::BytesPerPixel(fCodec->fSrcConfig);
+        fSrcRowBytes = codec->getInfo().width() * SkSwizzler::BytesPerPixel(fCodec->fSrcConfig);
         fGarbageRow.reset(fSrcRowBytes);
         fGarbageRowPtr = static_cast<uint8_t*>(fGarbageRow.get());
     }
 
+    bool onSetSampleX(int sampleX) override {
+        fCodec->fSwizzler->setSampleX(sampleX);
+        return true;
+    }
+
     SkCodec::Result onGetScanlines(void* dst, int count, size_t dstRowBytes) override {
         //rewind stream if have previously called onGetScanlines, 
         //since we need entire progressive image to get scanlines
@@ -728,8 +746,10 @@ SkScanlineDecoder* SkPngCodec::onGetScanlineDecoder(const SkImageInfo& dstInfo,
         return NULL;
     }
     // Check to see if scaling was requested.
-    if (dstInfo.dimensions() != this->getInfo().dimensions()) {
-        return NULL;
+    if(options.fScaled == false) {

[scroggo, 2015/07/27 15:09:19]

Please add a comment explaining this. Something like:

// If the caller is sampling, it is okay for the dimensions to not match.

(Which makes me wonder if the variable should be called fSampled?)

That said, IIUC, the width may be smaller (due to sampling), but it needs to be
a supported width, and the height should be the same?

[emmaleer, 2015/07/27 18:31:38]

Changed to fSampled.

For normal images the dstHeight and dstWidth can both be smaller. These are the
dimensions the user requests. They are checked in SkScaledCodec.cpp in
onGetPixels() by calling scaling_supported()

For interlaced images, the dstHeight must be the same. (as we do the y sampling
after)
In SkScaledCodec we set the dstHeight = srcHeight if the image is interlaced,
before creating the scanline decoder.

I could add a check saying: If interlaced and sampled, make sure dstHeight and
srcHeight are equal.
But since SkScaledCodec previously set them equal, I don't think it's really
necessary.
Let me know if you think it's still beneficial, and I can add the check. 

[scroggo, 2015/07/27 19:29:56]

Does the SkCodec need to know that dstHeight is smaller though? It seems like
from this SkCodec's perspective, the dstHeight is unchanged - the caller needs
to call skip/get scanline for each scanline in the original height, right?
If there is a condition that we assume in a function, then that is a place we
can add an assert. It checks that you were right about your assumption, and
allows a reader to know you assume that.

That said, the info passed in comes from a public function, so someone could
call with an invalid value, right?

[emmaleer, 2015/07/28 14:19:16]

Yes, I have changed the height to be the original height
I added a check that returns NULL if the heights are no equal when scaling

+        if (dstInfo.dimensions() != this->getInfo().dimensions()) {
+            return NULL;
+        }
     }
     // Create a new SkPngCodec, to be owned by the scanline decoder.
     SkStream* stream = this->stream()->duplicate();