Add the ability to decode a subset to SkCodec.

Add the ability to decode a subset to SkCodec

This allows codecs that support subsets natively (i.e. WEBP) to do so.

Add a field on SkCodec::Options representing the subset.

Add a method on SkCodec to find a valid subset which approximately
matches a desired subset.

Implement subset decodes in SkWebpCodec.

Add a test in DM for decoding subsets.
Notice that we only start on even boundaries. This is due to the
way libwebp's API works. SkWEBPImageDecoder does not take this into
account, which results in visual artifacts.

FIXME: Subsets with scaling are not pixel identical, but close. (This
may be fine, though - they are not perceptually different. We'll just
need to mark another set of images in gold as valid, once
https://skbug.com/4038 is fixed, so we can tests scaled webp without
generating new images on each run.)

Committed: https://skia.googlesource.com/skia/+/b636b45971bc5e64e3b103169577cbc874bc064d

[scroggo, 2015-07-17 20:03:56]

scroggo@google.com changed reviewers:
+ djsollen@google.com, emmaleer@google.com, msarett@google.com,
scroggo@google.com

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

https://codereview.chromium.org/1240143002/diff/20001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1240143002/diff/20001/dm/DM.cpp#newcode222
dm/DM.cpp:222: case kGray_8_SkColorType:
Should run the Codec Subset test for Gray8 to Gray8 or Index8 to Index8 decodes?
Maybe we should add the Codec Subset test to the kGray_8_SkColorType case, and
the kIndex_8_SkColorType case in this switch statement, to test images which are
Index8 or Gray8.

https://codereview.chromium.org/1240143002/diff/20001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1240143002/diff/20001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:388: decodeInfo = decodeInfo.makeWH(decodeInfo.width() *
fScale,
You could only call decodeInfo.makeWH() once instead of twice.
You would have to create variables for SkTMin(w, W - x) and SkTMin(h, H-y).
decodeInfo.makeWH() creates a new SkImageInfo object and returns it.
I'm not sure how expensive doing that is, but it might be worth changing.

https://codereview.chromium.org/1240143002/diff/20001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:399: break;
What about the case SkCodec::kIncompleteInput.
All of the other tests treat kIncompleteInput as a success.
Maybe add in kIncompleteInput as a success, or add a comment saying why
kIncompleteInput is a failure in this test, but not the other tests.

https://codereview.chromium.org/1240143002/diff/20001/src/codec/SkWebpCodec.cpp
File src/codec/SkWebpCodec.cpp (right):

https://codereview.chromium.org/1240143002/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:165: // supported this, when we really did something
different, report  that we cannot
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

https://codereview.chromium.org/1240143002/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:183: config.options.scaled_height =
dstDimensions.height();
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]

https://codereview.chromium.org/1240143002/diff/20001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1240143002/diff/20001/dm/DM.cpp#newcode222
dm/DM.cpp:222: case kGray_8_SkColorType:
On 2015/07/17 22:31:05, emmaleer wrote:
> Should run the Codec Subset test for Gray8 to Gray8 or Index8 to Index8
decodes?
> Maybe we should add the Codec Subset test to the kGray_8_SkColorType case, and
> the kIndex_8_SkColorType case in this switch statement, to test images which
are
> Index8 or Gray8.

Good question. Currently WEBP is the only codec that supports subsets, and it
will never report gray or index8.

I've added a comment.

https://codereview.chromium.org/1240143002/diff/20001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1240143002/diff/20001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:388: decodeInfo = decodeInfo.makeWH(decodeInfo.width() *
fScale,
On 2015/07/17 22:31:05, emmaleer wrote:
> You could only call decodeInfo.makeWH() once instead of twice.
> You would have to create variables for SkTMin(w, W - x) and SkTMin(h, H-y).
> decodeInfo.makeWH() creates a new SkImageInfo object and returns it.
> I'm not sure how expensive doing that is, but it might be worth changing. 

SkImageInfo::makeWH() is likely pretty cheap (function call, constructor, copy
constructor, without doing any extra calculations), but you're right, it's not
free, and it's actually not even necessary - I'm essentially using it to store
temporary variables.

My excuse for doing it the way I did it was to cut down on variables. That said,
I don't think I've made anything clearer by doing it this way.

Fixed.

https://codereview.chromium.org/1240143002/diff/20001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:399: break;
On 2015/07/17 22:31:05, emmaleer wrote:
> What about the case SkCodec::kIncompleteInput.
> All of the other tests treat kIncompleteInput as a success.
> Maybe add in kIncompleteInput as a success, or add a comment saying why
> kIncompleteInput is a failure in this test, but not the other tests.

I think this goes back to my general concern about kIncompleteInput - where do
we cross the line between a failed decode and an incomplete image? If only a
couple of lines failed to decode, I think it's probably useful to see the
output, but if only one or two *did* decode, it may not be.

I also had a concern about seeing kIncompleteInput on the first subset, and
never getting a chance to attempt any more. That would still be incomplete,
though, so there's probably no good reason to draw the line there. More
importantly, the other cases consider kIncompleteInput a success, so we should
here as well.

Done.

https://codereview.chromium.org/1240143002/diff/20001/src/codec/SkWebpCodec.cpp
File src/codec/SkWebpCodec.cpp (right):

https://codereview.chromium.org/1240143002/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:165: // supported this, when we really did something
different, report  that we cannot
On 2015/07/17 22:31:05, emmaleer wrote:
> 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

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.)

https://codereview.chromium.org/1240143002/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:183: config.options.scaled_height =
dstDimensions.height();
On 2015/07/17 22:31:05, emmaleer wrote:
> 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.

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.

[djsollen, 2015-07-20 14:29:24]

https://codereview.chromium.org/1240143002/diff/20001/dm/DMSrcSink.h
File dm/DMSrcSink.h (right):

https://codereview.chromium.org/1240143002/diff/20001/dm/DMSrcSink.h#newcode102
dm/DMSrcSink.h:102: kSubset_Mode, // For codecs that support subsets directly.
Is there any method for the caller to negotiate with the API?  For example, can
I give it a requested subset and it return a closest match?

[scroggo, 2015-07-20 14:47:02]

https://codereview.chromium.org/1240143002/diff/20001/dm/DMSrcSink.h
File dm/DMSrcSink.h (right):

https://codereview.chromium.org/1240143002/diff/20001/dm/DMSrcSink.h#newcode102
dm/DMSrcSink.h:102: kSubset_Mode, // For codecs that support subsets directly.
On 2015/07/20 14:29:24, djsollen wrote:
> Is there any method for the caller to negotiate with the API?  For example,
can
> I give it a requested subset and it return a closest match?

That's a good idea - much like getScaledDimensions. That said, I'm not sure what
the closest match would be. libwebp decided to snap odd numbers to the left, but
we could just as easily snap odd numbers right. And what do we do about the
width and height? (i.e. if we changed top or left, do we change width or height
so the right/bottom is unchanged? or do we leave them unchanged so that
right/bottom has changed?)

[djsollen, 2015-07-20 16:54:34]

https://codereview.chromium.org/1240143002/diff/20001/dm/DMSrcSink.h
File dm/DMSrcSink.h (right):

https://codereview.chromium.org/1240143002/diff/20001/dm/DMSrcSink.h#newcode102
dm/DMSrcSink.h:102: kSubset_Mode, // For codecs that support subsets directly.
On 2015/07/20 14:47:01, scroggo wrote:
> On 2015/07/20 14:29:24, djsollen wrote:
> > Is there any method for the caller to negotiate with the API?  For example,
> can
> > I give it a requested subset and it return a closest match?
> 
> That's a good idea - much like getScaledDimensions. That said, I'm not sure
what
> the closest match would be. libwebp decided to snap odd numbers to the left,
but
> we could just as easily snap odd numbers right. And what do we do about the
> width and height? (i.e. if we changed top or left, do we change width or
height
> so the right/bottom is unchanged? or do we leave them unchanged so that
> right/bottom has changed?)

I would err on the side of giving them too many pixels and not too few as the
best match.

[scroggo, 2015-07-20 18:55:54]

https://codereview.chromium.org/1240143002/diff/20001/dm/DMSrcSink.h
File dm/DMSrcSink.h (right):

https://codereview.chromium.org/1240143002/diff/20001/dm/DMSrcSink.h#newcode102
dm/DMSrcSink.h:102: kSubset_Mode, // For codecs that support subsets directly.
On 2015/07/20 16:54:34, djsollen wrote:
> On 2015/07/20 14:47:01, scroggo wrote:
> > On 2015/07/20 14:29:24, djsollen wrote:
> > > Is there any method for the caller to negotiate with the API?  For
example,
> > can
> > > I give it a requested subset and it return a closest match?
> > 
> > That's a good idea - much like getScaledDimensions. That said, I'm not sure
> what
> > the closest match would be. libwebp decided to snap odd numbers to the left,
> but
> > we could just as easily snap odd numbers right. And what do we do about the
> > width and height? (i.e. if we changed top or left, do we change width or
> height
> > so the right/bottom is unchanged? or do we leave them unchanged so that
> > right/bottom has changed?)
> 
> I would err on the side of giving them too many pixels and not too few as the
> best match.

Done.

[djsollen, 2015-07-20 19:23:28]

api lgtm

[scroggo, 2015-07-21 14:53:45]

Derek has reviewed the API, can I get a review of the code? Matt or Emmalee,
does anything need to be fixed that I did not cover in the first review?

https://codereview.chromium.org/1240143002/diff/80001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1240143002/diff/80001/include/codec/SkCodec.h...
include/codec/SkCodec.h:164: SkIRect         fSubset;
This might be better as a pointer, where NULL means do not use a subset, and
non-NULL means to use a subset. That way we do not have to initialize/check
empty when it's not used. Thoughts?

[msarett, 2015-07-21 15:19:37]

lgtm plus comments

https://codereview.chromium.org/1240143002/diff/80001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1240143002/diff/80001/include/codec/SkCodec.h...
include/codec/SkCodec.h:164: SkIRect         fSubset;
On 2015/07/21 14:53:45, scroggo wrote:
> This might be better as a pointer, where NULL means do not use a subset, and
> non-NULL means to use a subset. That way we do not have to initialize/check
> empty when it's not used. Thoughts?

I would lean towards using a pointer, but I don't feel strongly.

I could see how we might erroneously decide that an empty subset indicates we
should do a no-op decode.

https://codereview.chromium.org/1240143002/diff/80001/src/codec/SkWebpCodec.cpp
File src/codec/SkWebpCodec.cpp (right):

https://codereview.chromium.org/1240143002/diff/80001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:177: if (!bounds.intersect(options.fSubset)) {
Per our discussion in person, let's explicitly disallow the caller from passing
in a subset that is not fully contained in the image.

[emmaleer, 2015-07-21 15:24:55]

https://codereview.chromium.org/1240143002/diff/80001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1240143002/diff/80001/include/codec/SkCodec.h...
include/codec/SkCodec.h:164: SkIRect         fSubset;
On 2015/07/21 14:53:45, scroggo wrote:
> This might be better as a pointer, where NULL means do not use a subset, and
> non-NULL means to use a subset. That way we do not have to initialize/check
> empty when it's not used. Thoughts?
 
I think making it a pointer makes more sense. Because why would you want to
create the SkIRect at all if you aren't going to use it?

https://codereview.chromium.org/1240143002/diff/80001/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1240143002/diff/80001/tests/CodexTest.cpp#new...
tests/CodexTest.cpp:48: SkASSERT(w > 1 && h > 1);
Should this be SkASSERT(w > 0 && h > 0) ?
Since the comment states "If either dimension is zero"
If it is already correct, we should update this comment so it's not conflicting
the code.

https://codereview.chromium.org/1240143002/diff/80001/tests/CodexTest.cpp#new...
tests/CodexTest.cpp:124: // Do not attempt to decode subsets of an image with a
width and/or height of 1.
Why are we not attempting to decode subsets with a width or height of 1?
Is that because width and height must be even? Could you explain this briefly

[scroggo, 2015-07-21 16:48:56]

https://codereview.chromium.org/1240143002/diff/80001/src/codec/SkWebpCodec.cpp
File src/codec/SkWebpCodec.cpp (right):

https://codereview.chromium.org/1240143002/diff/80001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:177: if (!bounds.intersect(options.fSubset)) {
On 2015/07/21 15:19:37, msarett wrote:
> Per our discussion in person, let's explicitly disallow the caller from
passing
> in a subset that is not fully contained in the image.

Done.

https://codereview.chromium.org/1240143002/diff/80001/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1240143002/diff/80001/tests/CodexTest.cpp#new...
tests/CodexTest.cpp:48: SkASSERT(w > 1 && h > 1);
On 2015/07/21 15:24:55, emmaleer wrote:
> Should this be SkASSERT(w > 0 && h > 0) ?
> Since the comment states "If either dimension is zero"
> If it is already correct, we should update this comment so it's not
conflicting
> the code. 

Ah, I meant either dimension of rect, down below, which will happen if w or h is
1 (since we call rand->nextULessThan(1), which can only return 0.

I agree that this comment is confusing....

I've also changed the function, in response to your comment regarding the early
exit for w or h of 1.

We *could* support a w or h of 1, we just need to change the call made on rand.
In fact, after rewriting the function, this assert is no longer needed (since it
can even support subsets of 1 x 1 sizes without going into the infinite loop).

https://codereview.chromium.org/1240143002/diff/80001/tests/CodexTest.cpp#new...
tests/CodexTest.cpp:124: // Do not attempt to decode subsets of an image with a
width and/or height of 1.
On 2015/07/21 15:24:55, emmaleer wrote:
> Why are we not attempting to decode subsets with a width or height of 1?
> Is that because width and height must be even? Could you explain this briefly 

Originally, I did not attempt to decode subsets of 1 x 1 images (i.e.
half-transparent-white-pixel.png) for two reasons:
- There is no meaningful subset of a 1 x 1 image
- generate_random_rect, as written, would go into an infinite loop on a 1 x 1
image

I then realized that generate_random_rect would *also* go into an infinite loop
for a 1 x h or w x 1 image, so I excluded those as well.

That said, I think it is meaningful to decode a subset of a one-dimensional
image; I just need to rewrite generate_random_rect to support that case.
(Although we do not actually have a one-dimensional image to test on - but we
could someday.) I've updated this (with comments) and fixed
generate_random_rect.

https://codereview.chromium.org/1240143002/diff/140001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1240143002/diff/140001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:311: if (options.fSubset) {
FWIW, we *could* check to see if fSubset matches the bounds, in which case it
means to decode the whole thing. This seems like a silly case to handle, so I
chose to take the fact that a subset was specified to mean that they wanted a
real subset. Let me know if you disagree and think we should handle that case.

[emmaleer, 2015-07-21 18:26:25]

lgtm

https://codereview.chromium.org/1240143002/diff/140001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1240143002/diff/140001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:311: if (options.fSubset) {
On 2015/07/21 16:48:56, scroggo wrote:
> FWIW, we *could* check to see if fSubset matches the bounds, in which case it
> means to decode the whole thing. This seems like a silly case to handle, so I
> chose to take the fact that a subset was specified to mean that they wanted a
> real subset. Let me know if you disagree and think we should handle that case.

I agree. I don't think we should support one specific size subset, and not other
size subsets. That could lead to confusion to the user why a certain subset
worked, and why a different one failed. 
I think it's best to fail for all size subsets.

[scroggo, 2015-07-22 13:41:08]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-07-22 13:41:08]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com,
djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1240143002/#ps160001
(title: "Remove unnecessary assert.")

[commit-bot: I haz the power, 2015-07-22 13:41:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1240143002/160001

[commit-bot: I haz the power, 2015-07-22 13:43:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-22 13:43:36]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[scroggo, 2015-07-22 13:50:43]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-07-22 13:50:44]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com,
emmaleer@google.com, djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1240143002/#ps180001
(title: "Do not treat kInvalidConversion as a failure.")

[commit-bot: I haz the power, 2015-07-22 13:50:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1240143002/180001

[commit-bot: I haz the power, 2015-07-22 13:54:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-22 13:54:25]

Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[scroggo, 2015-07-22 14:08:35]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-07-22 14:08:35]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com,
emmaleer@google.com, djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1240143002/#ps200001
(title: "Fix some conversion warning/errors.")

[commit-bot: I haz the power, 2015-07-22 14:08:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1240143002/200001

[commit-bot: I haz the power, 2015-07-22 14:16:23]

Committed patchset #11 (id:200001) as
https://skia.googlesource.com/skia/+/b636b45971bc5e64e3b103169577cbc874bc064d

[reed2, 2015-07-22 16:04:58]

reed@chromium.org changed reviewers:
+ reed@chromium.org

[reed2, 2015-07-22 16:04:59]

This api is a little unexpected to me. 

What motivates it?

- some codecs would crash on a diff subset?
- some codecs would run slower on a diff subset?

Can the codec make any changes to the subset?
- enlarge?
- shrink?
- offset?

If all of the above, how is the client to easily get what they really wanted
(the original subset)?

[scroggo, 2015-07-22 17:55:58]

On 2015/07/22 16:04:59, reed2 wrote:
> This api is a little unexpected to me. 
> 
> What motivates it?
> 
> - some codecs would crash on a diff subset?
> - some codecs would run slower on a diff subset?
> 
> Can the codec make any changes to the subset?
> - enlarge?
> - shrink?
> - offset?
> 

I suppose you mean getValidDimensions?

This is explained in the implementation of SkWebpCodec, but maybe there should
be an explanation in the header?

libwebp makes a change to the subset, to "snap" an odd top/left to an even
value. The documentation for that can be found at
https://developers.google.com/speed/webp/docs/api#advanced_decoding_api:

  int crop_left, crop_top;          // top-left position for cropping.
                                    // Will be snapped to even values.

They do this silently. So if we did not correct for this, you would end up
getting a slightly different subset than you wanted, scaled differently.
(Internally, I do not know why libwebp made this decision.) In fact, if you look
at the results in Gold, you can see that the webp subset decodes using
SkImageDecoder look different on the boundaries of subsets.

> If all of the above, how is the client to easily get what they really wanted
> (the original subset)?

This is a little tricky, but not terribly so. Webp will return a subset that is
potentially one row taller and one pixel wider. So they can use
SkBitmap::extractSubset in order to get the exact subset.

[reed2, 2015-07-23 10:03:59]

If we possibly can, I'd vote to push webp (the only codec that is forcing the
subset to be larger?) to perform the trim inside, so the caller doesn't have to
deal with this complication, or we perform it in our wrapper code.

[scroggo, 2015-07-23 14:26:49]

On 2015/07/23 10:03:59, reed2 wrote:
> If we possibly can, I'd vote to push webp (the only codec that is forcing the
> subset to be larger?)

(Yes - webp is the only codec that supports subsets natively.)

> to perform the trim inside, so the caller doesn't have to
> deal with this complication, or we perform it in our wrapper code.

I can ask the webp team why they have this restriction, and see if there's any
way around it. That said, I do not see a clear way to do a trim that does not
involve a copy (in SkCodec or in libwebp), given the current APIs [1], which
seems like it would be unnecessarily expensive.

To me, this API feels analogous to getValidDimensions, which lets the client
know what the codec can do efficiently.

[1] An API like SkImageDecoder's would allow hiding this from the caller - since
SkImageDecoder handles deciding how much memory to allocate and uses an
SkBitmap, it could allocate the extra memory and then use extractSubset to
return a bitmap that represents the subset the client actually wanted. On the
other hand, that uses a little bit more memory, and SkCodec's API allows the
client to decide whether to take the memory or speed hit. libwebp's API is
similar to SkCodec, in that the client provides a pointer to memory into which
it writes directly.