Move all knowledge of X sampling into SkScaledCodec

src/codec/SkCodec.cpp

Index: src/codec/SkCodec.cpp
diff --git a/src/codec/SkCodec.cpp b/src/codec/SkCodec.cpp
index cabcadcccda5c872b69834daa18bbe1701be82fd..54d529d20e143d4727c2ddb56b36ddb4c13b5ed2 100644
--- a/src/codec/SkCodec.cpp
+++ b/src/codec/SkCodec.cpp
@@ -152,7 +152,21 @@ SkCodec::Result SkCodec::getPixels(const SkImageInfo& info, void* pixels, size_t
     Options optsStorage;
     if (nullptr == options) {
         options = &optsStorage;
+    } else if (options->fSubset) {

[msarett, 2015/10/01 19:34:33]

I'm guessing that, for some reason (maybe the kSubset_Mode in DM), it is
necessary to return kUnimplemented for a subset decode before returning
kInvalidScale for a badly scaled decode.  So you have added this to make sure
that the checks are still in the right order?

[scroggo, 2015/10/01 21:16:57]

That is correct. I don't think this is best long term solution necessarily, but
I think this is good enough until we have subsets in SkScaledCodec.

+        SkIRect subset(*options->fSubset);
+        if (!this->onGetValidSubset(&subset) || subset != *options->fSubset) {
+            // FIXME: How to differentiate between not supporting subset at all
+            // and not supporting this particular subset?
+            return kUnimplemented;
+        }
+    }
+
+    // FIXME: Support subsets somehow? Note that this works for SkWebpCodec

[msarett, 2015/10/01 19:34:33]

There's a couple ways we could go about this, I don't think either of them is
too tricky.

We may eventually want to combine dimensionsSupported() and options->fSubset
into one check.

[scroggo, 2015/10/01 21:16:57]

Yeah, I considered that. I decided to punt that until SkScaledCodec supports
subsets.

[msarett, 2015/10/01 22:53:07]

That makes perfect sense.

+    // because it supports arbitrary scaling/subset combinations.
+    if (!this->dimensionsSupported(info.dimensions())) {
+        return kInvalidScale;
     }
+
     const Result result = this->onGetPixels(info, pixels, rowBytes, *options, ctable, ctableCount);
 
     if ((kIncompleteInput == result || kSuccess == result) && ctableCount) {
@@ -190,6 +204,18 @@ SkCodec::Result SkCodec::startScanlineDecode(const SkImageInfo& dstInfo,
     Options optsStorage;
     if (nullptr == options) {
         options = &optsStorage;
+    } else if (options->fSubset) {
+        SkIRect subset(*options->fSubset);
+        if (!this->onGetValidSubset(&subset) || subset != *options->fSubset) {
+            // FIXME: How to differentiate between not supporting subset at all
+            // and not supporting this particular subset?
+            return kUnimplemented;
+        }
+    }
+
+    // FIXME: Support subsets somehow?
+    if (!this->dimensionsSupported(dstInfo.dimensions())) {
+        return kInvalidScale;
     }
 
     const Result result = this->onStartScanlineDecode(dstInfo, *options, ctable, ctableCount);
@@ -213,8 +239,14 @@ SkCodec::Result SkCodec::getScanlines(void* dst, int countLines, size_t rowBytes
     }
 
     SkASSERT(!fDstInfo.isEmpty());
-    if ((rowBytes < fDstInfo.minRowBytes() && countLines > 1 ) || countLines <= 0
-         || fCurrScanline + countLines > fDstInfo.height()) {
+
+    if (rowBytes < fDstInfo.minRowBytes() && countLines > 1) {
+        // FIXME: This check breaks down when we sample without letting SkCodec

[scroggo, 2015/10/01 15:33:14]

The problem shows up when we want to scale down an image using SkScaledCodec.
(Particularly when using an interlaced PNG, since it uses a countLines > 1.)
fDstInfo has the width of the entire image, while the SkScaledCodec is using the
scaled width.

I have a couple of ideas here:
- We could skip the check entirely, which allows smart clients to work
(slightly) faster, and forces dumb clients to figure out why they're failing
(keeping in mind that we may end up being dumb clients...)
- We could provide some kind of function that tells the minimum row bytes, which
computes it based on a sampler.

[msarett, 2015/10/01 19:34:33]

I've deleted this check in my scaled subsetting CL.  Turns out it is also
annoying when we only want to decode part of a scanline.

So yeah, I'm in favor of getting rid of it.

[scroggo, 2015/10/01 21:16:57]

Done.

+        // know about it.
+        // return kInvalidParameters;
+    }
+
+    if (countLines <= 0 || fCurrScanline + countLines > fDstInfo.height()) {
         return kInvalidParameters;
     }