Add scaled subset API to SkCodec

Add scaled subset API to SkCodec

The new API is implemented by SkScaledCodec and SkWebpCodec.

BUG=skia:3257

[msarett, 2015-10-12 14:41:30]

Patchset #2 (id:20001) has been deleted

[msarett, 2015-10-12 14:41:37]

Patchset #1 (id:1) has been deleted

[msarett, 2015-10-12 15:56:50]

Patchset #1 (id:40001) has been deleted

[msarett, 2015-10-12 15:56:56]

Patchset #1 (id:60001) has been deleted

[msarett, 2015-10-12 15:57:03]

Patchset #1 (id:80001) has been deleted

[msarett, 2015-10-12 17:32:41]

Patchset #2 (id:120001) has been deleted

[msarett, 2015-10-12 17:32:50]

Patchset #1 (id:100001) has been deleted

[msarett, 2015-10-12 17:37:42]

Patchset #1 (id:140001) has been deleted

[msarett, 2015-10-12 18:27:29]

Patchset #1 (id:160001) has been deleted

[msarett, 2015-10-12 18:33:28]

msarett@google.com changed reviewers:
+ reed@google.com, scroggo@google.com

[msarett, 2015-10-12 18:33:29]

I decided to split this up one more time.  These are the changes to the SkCodec
API without the changes to SkBitmapRegionDecoder.

https://codereview.chromium.org/1395183003/diff/180001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (left):

https://codereview.chromium.org/1395183003/diff/180001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:713: 
I missed this in an earlier CL, but this check is no longer valid.

We may want a ten pixel subset that starts twenty pixels from the left edge of
the image.

This is completely valid, but in this case fX0 = 20 and fSrcWidth = 10.

https://codereview.chromium.org/1395183003/diff/180001/src/codec/SkWebpCodec.cpp
File src/codec/SkWebpCodec.cpp (left):

https://codereview.chromium.org/1395183003/diff/180001/src/codec/SkWebpCodec....
src/codec/SkWebpCodec.cpp:196: #ifdef SK_DEBUG
I tried to keep a debug check here, but it is too tricky now that we use
getScaledSubsetDimensions() instead of getValidSubset().

We would need to somehow calculate the float scale that we are using in order to
call the getScaledSubsetDimensions() API.

[reed1, 2015-10-12 20:21:00]

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

https://codereview.chromium.org/1395183003/diff/180001/include/codec/SkCodec....
include/codec/SkCodec.h:228: bool getScaledSubsetDimensions(float desiredScale,
const Options& options) const;
wow, pretty complicated rules for calling this. Are all combos required for
android?
Can we have 2-3 separate calls (w/ diff names / parameters) to distinguish the
different variations?
e.g.
    getScaledDimensions(float, SkIRect* inOut);
    getSubsetDimensions(float, SkIRect* inOut);
   ...

[msarett, 2015-10-12 20:30:27]

Yeah you're right.

I'll work on splitting this into multiple functions.  In the meantime, how do
you feel about landing the CL that this depends on?
https://codereview.chromium.org/1390213002/

[scroggo, 2015-10-12 20:47:07]

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

https://codereview.chromium.org/1395183003/diff/180001/include/codec/SkCodec....
include/codec/SkCodec.h:172: *      (3) If fSubset is NULL and the dstInfo
dimensions match the
Maybe it is worth noting that (2) and (3) are the same as if there was no
Options object?

https://codereview.chromium.org/1395183003/diff/180001/include/codec/SkCodec....
include/codec/SkCodec.h:176: *  If both are non-NULL we are performing a scaled
subset decode:
Which "both"? fScaledDimensions and fScaledSubset?

https://codereview.chromium.org/1395183003/diff/180001/include/codec/SkCodec....
include/codec/SkCodec.h:208: *                      options->fSubset (in/out)
specifies the
If we wanted to, we could make options an optional parameter (haha) and combine
this with getScaledDimensions. I'm not sure whether that's better. (My suspicion
is that since most codecs do not support subsets at all, it makes sense to keep
them separate, so asking for a scale does not worry about subsetting, but I do
think it's at least worth considering.)

Sort of on the other extreme, it seems like SkCodec needs to do a lot of work to
handle subsets, which are only supported by two subclasses. I wonder if we
should refactor things so that SkCodec doesn't need to know anything about
subsets. The sticky point here is webp, which supports subsets natively instead
of supporting scanline decoding. I think we have the following options:
- This approach, where SkCodec has to deal with subsets
- Sort of a hybrid approach, where SkCodec knows minimally about subsets (in
order to support webp). All of this new functionality goes directly on
SkScaledCodec (which I think will need to pass through to SkWebpCodec). Maybe
SkScaledCodec's API looks very different, as it will be focused on handling
subsets.
- All subset handling is done inside SkScaledCodec. Webp will potentially be
very slow, since we do not have a scanline decoder. (We could write a dumb one
though, which decodes 16 scanlines (the blocks that webp are stored in) and
stores them, and then returns one line at a time).
- Maybe SkScaledCodec has its own interface, for which webp has its own
implementation.

There are probably more options I haven't thought of, and it will probably make
sense to discuss in person.

https://codereview.chromium.org/1395183003/diff/180001/include/codec/SkCodec....
include/codec/SkCodec.h:217: *                      the fSubset in terms of
fScaledDimensions.
"in terms of fScaledDimensions" is not intuitive to me.

Do you mean it specifies fSubset after scaling the entire image to
fScaledDimensions? I think something like this may be more clear.

https://codereview.chromium.org/1395183003/diff/180001/include/codec/SkCodec....
include/codec/SkCodec.h:224: *  @return true if fScaledDimensions and
fScaledSubset have been
You've removed getValidSubset, so I assume the intent is to use this function
with desiredScale == 1.0f, but the comments do not describe that use case.

It seems like there should be an option to pass 1.0f and just a subset, and the
caller need never worry about fScaledDimensions or fScaledSubset, since they are
doing neither.

On that note, you do not mention who owns fScaledDimensions or fScaledSubset.
I'm assuming that the client does the following:

Options options;
options.fSubset = &some_rect;
options.fScaledDimensions = &uninitialized_dim;
options.fScaledSubset = &uninitialized_rect;

if (codec->getScaledSubsetDimensions(scale, options)) {
  ...
}

So they handle the pointers.

https://codereview.chromium.org/1395183003/diff/180001/include/codec/SkCodec....
include/codec/SkCodec.h:228: bool getScaledSubsetDimensions(float desiredScale,
const Options& options) const;
The options object does not appear to be const.

https://codereview.chromium.org/1395183003/diff/180001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1395183003/diff/180001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:141: if (!is_valid_subset(subset,
this->getInfo().dimensions())) {
nit: fInfo.dimensions()

No need to call the accessor function, since this class can access it directly
(unless there was a side effect, and I believe there is not).

https://codereview.chromium.org/1395183003/diff/180001/src/codec/SkCodecPriv.h
File src/codec/SkCodecPriv.h (right):

https://codereview.chromium.org/1395183003/diff/180001/src/codec/SkCodecPriv....
src/codec/SkCodecPriv.h:73: * to name variables using y-coordinate naming
conventions.
Was y picked arbitrarily over x? I understand it makes sense to be less
abstract, but I don't see why y is more clear.

https://codereview.chromium.org/1395183003/diff/180001/src/codec/SkScaledCode...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1395183003/diff/180001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:54: static bool is_in_subset(int coord, int offset,
int length) {
nit: It seems like this is a more generic function. To me it looks like:

  static bool is_in_range(int x, int minInclusive, int maxExclusive)

which is clearer to me. Thoughts?

https://codereview.chromium.org/1395183003/diff/180001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:55: if (coord < offset || coord >= offset + length)
{
nit: This can just be:

  return coord >= offset && coord < offset + length;

(unless you find what you have more clear?)

https://codereview.chromium.org/1395183003/diff/180001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (left):

https://codereview.chromium.org/1395183003/diff/180001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:713: 
On 2015/10/12 18:33:29, msarett wrote:
> I missed this in an earlier CL, but this check is no longer valid.
> 
> We may want a ten pixel subset that starts twenty pixels from the left edge of
> the image.
> 
> This is completely valid, but in this case fX0 = 20 and fSrcWidth = 10.

Wait, does this belong in crrev.com/1390213002? I would prefer it go in with
that one (or by itself) than here.

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

https://codereview.chromium.org/1395183003/diff/180001/src/codec/SkWebpCodec....
src/codec/SkWebpCodec.cpp:124: return !((subset.left() & 1) || (subset.top() &
1));
Could you add a comment explaining this? (Left and top must be even.)

I suppose we don't need to worry about containment, because that was checked in
in subsetSupported?