Add the ability to decode a subset to SkCodec.

src/codec/SkWebpCodec.cpp

Index: src/codec/SkWebpCodec.cpp
diff --git a/src/codec/SkWebpCodec.cpp b/src/codec/SkWebpCodec.cpp
index 32a8b78b14f2487e69305c6d90c89533319435bb..bbfd153ecba85bea8b7735f2d563539afc2f3325 100644
--- a/src/codec/SkWebpCodec.cpp
+++ b/src/codec/SkWebpCodec.cpp
@@ -126,7 +126,7 @@ static WEBP_CSP_MODE webp_decode_mode(SkColorType ct, bool premultiply) {
 static const size_t BUFFER_SIZE = 4096;
 
 SkCodec::Result SkWebpCodec::onGetPixels(const SkImageInfo& dstInfo, void* dst, size_t rowBytes,
-                                         const Options&, SkPMColor*, int*) {
+                                         const Options& options, SkPMColor*, int*) {
     switch (this->rewindIfNeeded()) {
         case kCouldNotRewind_RewindState:
             return kCouldNotRewind;
@@ -153,12 +153,34 @@ SkCodec::Result SkWebpCodec::onGetPixels(const SkImageInfo& dstInfo, void* dst,
     // Free any memory associated with the buffer. Must be called last, so we declare it first.
     SkAutoTCallVProc<WebPDecBuffer, WebPFreeDecBuffer> autoFree(&(config.output));
 
-    SkISize dimensions = dstInfo.dimensions();
-    if (this->getInfo().dimensions() != dimensions) {
+    SkIRect bounds = SkIRect::MakeSize(this->getInfo().dimensions());
+    if (!options.fSubset.isEmpty()) {
+        // Caller is requesting a subset.
+        if (!bounds.intersect(options.fSubset)) {
+            // The subset is out of bounds.
+            return kInvalidParameters;
+        }
+
+        // This is tricky. libwebp snaps the top and left to even values. Rather than claim we
+        // supported this, when we really did something different, report  that we cannot

[emmaleer, 2015/07/17 22:31:05]

What is the different thing we did, instead of crop the top and left to even
values?
You could add what that different thing is to the comment so it's more clear

[scroggo, 2015/07/20 14:25:42]

Done. Please take a look and let me know if you think this is more clear.

I think this raises a good question about the API contract. As it stands, the
header does not mention any caveats about what kinds of subsets are valid (I
should at least mention that it should intersect with the original bounds,
unless that is obvious...). 

Although we're in control of the main client (BitmapRegionDecoder on Android),
other clients may want to programmatically decide what subsets to use, and only
luck will determine whether they choose an odd top/left. In order to figure out
why they received kInvalidParameters, they'll need to debug or search through
the code.

It does seem weird to have a restriction that depends on a particular
implementation. On the other hand, it is currently the only implementation.
(We've talked about making the default version use the scanline decoder; if we
decided to take that approach, the other implementations would not have this
restriction.) That said, I do not have a better solution, and have updated the
header.

(As a side note, SkImageDecoder's API allows a little bit more flexibility here.
Since it allocates the memory itself, it could always allocate a larger amount
of memory (for the extra row/column that libwebp snapped to) and return an
SkBitmap using SkBitmap::extractSubset or copy the data without the extra
row/column, depending on whether the client was more concerned about the extra
time or extra memory. On the other hand, the API of SkCodec forces the client to
make those decisions, rather than leaving us to guess.)

+        // handle it.
+        if (!SkIsAlign2(bounds.fLeft) || !SkIsAlign2(bounds.fTop)) {
+            return kInvalidParameters;
+        }
+
+        config.options.use_cropping = 1;
+        config.options.crop_left = bounds.fLeft;
+        config.options.crop_top = bounds.fTop;
+        config.options.crop_width = bounds.width();
+        config.options.crop_height = bounds.height();
+    }
+
+    SkISize dstDimensions = dstInfo.dimensions();
+    if (bounds.size() != dstDimensions) {
         // Caller is requesting scaling.
         config.options.use_scaling = 1;
-        config.options.scaled_width = dimensions.width();
-        config.options.scaled_height = dimensions.height();
+        config.options.scaled_width = dstDimensions.width();
+        config.options.scaled_height = dstDimensions.height();

[emmaleer, 2015/07/17 22:31:05]

Should we check that the width and height are being scaled by the same number?
Here we just set the width and height to the dstDimensions. 
But what if the caller didn't correctly scale these values, or scaled width and
height by different values?
Maybe we should do some sort of checks before setting the scaled_width and
scaled_height.

[scroggo, 2015/07/20 14:25:42]

Not sure. As I understand it, libwebp allows scaling width and height by
different factors. Although our other implementations require using the same
factor, I am not sure there's harm in letting libwebp do its thing. I do have a
concern about requiring the same factor though - if we say something like the
following must be true:

origW / scaledW == origH / scaledH

and we rounded differently from the client, we may say they are not using the
same scale factor in each dimension.

     }
 
     config.output.colorspace = webp_decode_mode(dstInfo.colorType(),