Add subsetting to SkScaledCodec

Add subsetting to SkScaledCodec

API Changes:
startScanlineDecoder() now takes a subsetLeft and a
subsetWidth as a parameter in order to decode partial
scanlines.

getScaledSubsetDimensions() has been created in order
to allow the client to request and perform valid
subset decodes.

Major Changes:
SkScaledCodec.cpp onGetPixels() has been significantly
refactored and rewritten.

SkBitmapRegionCodec.cpp implements
SkBitmapRegionDecoderInterface using an SkScaledCodec.

Minor Changes:
Many files have been touched to allow SkSwizzler to
know about subsetLeft and subsetWidth.

BUG=skia:4209

[msarett, 2015-08-27 18:07:01]

Patchset #3 (id:40001) has been deleted

[msarett, 2015-09-17 20:04:28]

Patchset #4 (id:80001) has been deleted

[msarett, 2015-09-23 13:30:48]

Patchset #4 (id:100001) has been deleted

[msarett, 2015-09-23 13:30:57]

Patchset #3 (id:60001) has been deleted

[msarett, 2015-09-23 13:31:05]

Patchset #2 (id:20001) has been deleted

[msarett, 2015-09-23 13:31:13]

Patchset #1 (id:1) has been deleted

[msarett, 2015-09-23 13:32:01]

As promised (well a little later than promised), here is this WIP CL.

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

https://codereview.chromium.org/1321433002/diff/120001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:54: static float snap_to_native_scale(float
inputScale) {
Unused

https://codereview.chromium.org/1321433002/diff/120001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:141: SkISize
SkScaledCodec::onGetScaledSubsetDimensions(float desiredScale,
This function is currently not called by anyone.

https://codereview.chromium.org/1321433002/diff/120001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:286: SkImageInfo scaledInfo =
scaledSubsetInfo.makeWH(
I've been messing around with this code - it may be broken.

[msarett, 2015-10-02 13:46:46]

Patchset #2 (id:140001) has been deleted

[msarett, 2015-10-02 13:46:53]

Patchset #1 (id:120001) has been deleted

[msarett, 2015-10-02 14:37:38]

Patchset #1 (id:160001) has been deleted

[msarett, 2015-10-02 14:44:17]

Patchset #1 (id:160002) has been deleted

[msarett, 2015-10-02 14:49:14]

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

[msarett, 2015-10-02 14:49:15]

Any suggestions for renaming SkScaledCodec (after this
lands)?  I'm not sure about SkScaledSubsetCodec.

Also, this will need a rebase on your dimensionsSupported()
CL once that lands.

https://codereview.chromium.org/1321433002/diff/190001/src/codec/SkBmpCodec.h
File src/codec/SkBmpCodec.h (right):

https://codereview.chromium.org/1321433002/diff/190001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:95: int subsetWidth() const {return fSubsetWidth; }
I need to rename this.

This is different from the field in the base class because it is valid for
scanline decodes and regular decodes.

Ex: For regular decodes, it equals the full width instead of 0.

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

https://codereview.chromium.org/1321433002/diff/190001/src/codec/SkWebpCodec....
src/codec/SkWebpCodec.cpp:231: config.output.u.RGBA.size = rowBytes *
dstInfo.height();
I need to check if I can change this back now that you have updated libwebp.

[scroggo, 2015-10-02 18:27:03]

On 2015/10/02 14:49:15, msarett wrote:
> Any suggestions for renaming SkScaledCodec (after this
> lands)?  I'm not sure about SkScaledSubsetCodec.

Haha. I'm happy to leave it as SkScaledCodec until/unless we come up with a
better name. SkAndroidCodec? SkUtilCodec?

I have a high-level request: This CL does makes two big changes:
- implements subsetting in SkScaledCodec
- implements subsetting in scanline decoding

Would it be possible to split that into two CLs (ideally which build on
crrev.com/1372973002, which I hope to land soon)? The first one will implement
subsets in SkScaledCodec, which we need to do to replace SkImageDecoder. It can
do the simple thing of swizzling the whole row and copying the piece that's
needed. Then we can add the second step, which is really just an optimization.

I haven't dug deeply yet into the meat of this change, but I've made some more
comments inline.

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

https://codereview.chromium.org/1321433002/diff/190001/include/codec/SkCodec....
include/codec/SkCodec.h:193: SkISize         fScaledDimensions;
For fSubset, we decided to make it an unowned pointer, so that a client that
does not want to specify it need not pay the cost of initializing the SkIRect. I
think we should make the same decision for all three options.

https://codereview.chromium.org/1321433002/diff/190001/include/codec/SkCodec....
include/codec/SkCodec.h:202: *                      dimensions.  This may be
modified to a subset
So if I understand correctly, this is analogous to getValidSubset? Why not
combine the two? If someone wants a non-scaled subset, they can always use a
scale of 1.

https://codereview.chromium.org/1321433002/diff/190001/include/codec/SkCodec....
include/codec/SkCodec.h:303: *  @param subsetWidth The number of pixels to
decode in each scanline row.
My first thought was that this could be represented by the dstInfo, but that
represents a scale, so I suppose we do need a separate value for this.

Should these be on the Options object? We already have a long list of parameters
on startScanlineDecode. I suppose moving to the Options object means having a
long list of items there, but it has two advantages:

- The options have names
- It separates the optional from the required

On that note, I think we should consider moving the ctable stuff to the Options
object.

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

https://codereview.chromium.org/1321433002/diff/190001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:246: if (0 > subsetLeft || subsetLeft >= dstInfo.width()
|| 0 > subsetWidth ||
nit: I would lean towards separating these out logically, e.g.

if (0 > subsetLeft || subsetLeft >= dstInfo.width() ||  // boundary check left
    0 > subsetWidth || subsetLeft + subsetWidth > dstInfo.width()) { // right

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

https://codereview.chromium.org/1321433002/diff/190001/src/codec/SkCodecPriv....
src/codec/SkCodecPriv.h:34: inline bool is_valid_subset(const SkIRect* subset,
const SkISize& imageDims) {
Why not:

return SkIRect::MakeSize(imageDims).contains(subset):

?

Also, I think it makes more sense to make subset a const reference (you never
want the caller to pass nullptr, and it's not optional.

https://codereview.chromium.org/1321433002/diff/190001/src/codec/SkCodecPriv....
src/codec/SkCodecPriv.h:83: inline bool is_coord_necessary(int srcCoord, int
sampleFactor, int scaledDim, int subsetOffset) {
I assume scaledDim now means scaled version of subsetWidth?

Is srcCoord scaled? (or subsetOffset, for that matter)

https://codereview.chromium.org/1321433002/diff/190001/src/codec/SkCodec_libg...
File src/codec/SkCodec_libgif.cpp (right):

https://codereview.chromium.org/1321433002/diff/190001/src/codec/SkCodec_libg...
src/codec/SkCodec_libgif.cpp:463: this->getInfo(), subsetLeft, subsetWidth));
If we stick these on options, we can update SkSwizzler to just take the Options,
which will include zeroInit along with subset info.

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

https://codereview.chromium.org/1321433002/diff/190001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:166: if (desiredScale >= 0.9375) {
why did this change?

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

https://codereview.chromium.org/1321433002/diff/190001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:47: static bool is_in_subset(int coord, int offset,
int length) {
I thought we had something like this function, but I may have been thinking of
SkTPin, which is not exactly what you want.

https://codereview.chromium.org/1321433002/diff/190001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:48: if (coord < offset || coord >= offset + length)
{
Why not:

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

?

https://codereview.chromium.org/1321433002/diff/190001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:54: static int get_sample_size(float scale) {
Don't you define this somewhere else?

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

https://codereview.chromium.org/1321433002/diff/190001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:235: // greater than 1. The below loop relies on the
fact that src remains unchanged.
Does the below loop not need to be updated to take this into account?

https://codereview.chromium.org/1321433002/diff/190001/tools/SkBitmapRegionCa...
File tools/SkBitmapRegionCanvas.cpp (right):

https://codereview.chromium.org/1321433002/diff/190001/tools/SkBitmapRegionCa...
tools/SkBitmapRegionCanvas.cpp:36: if (sampleSize < 1) {
Is this just in case? Or were you seeing bad input?

[msarett, 2015-10-06 23:01:27]

"I have a high-level request: This CL does makes two big changes:
- implements subsetting in SkScaledCodec
- implements subsetting in scanline decoding

Would it be possible to split that into two CLs (ideally which build on
crrev.com/1372973002, which I hope to land soon)? The first one will implement
subsets in SkScaledCodec, which we need to do to replace SkImageDecoder. It can
do the simple thing of swizzling the whole row and copying the piece that's
needed. Then we can add the second step, which is really just an optimization."

I'm happy to split this up if that would make it easier to review.  I'm not
convinced that the order you've suggested is the best way to do it.  I think it
seems kind of nasty to write a bunch of code with memcpy() that we will sweep
away anyway.  Any objection to starting with subsetting in SkScanlineDecoder? 
I'm kind of leaning that way.

I know that the other piece is more important to replace SkImageDecoder...but I
think we'll absolutely have to land both anyway.

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

https://codereview.chromium.org/1321433002/diff/190001/include/codec/SkCodec....
include/codec/SkCodec.h:193: SkISize         fScaledDimensions;
On 2015/10/02 18:27:03, scroggo wrote:
> For fSubset, we decided to make it an unowned pointer, so that a client that
> does not want to specify it need not pay the cost of initializing the SkIRect.
I
> think we should make the same decision for all three options.

I'll change to be consistent.

My concern is that a client will pass nullptr for these fields, since we are
responsible for initializing them.  We can't really deal with that because we
want the client to own the memory.

Not sure if it would seem to strange to a client to need to pass in valid
pointers to uninitialized objects.

https://codereview.chromium.org/1321433002/diff/190001/include/codec/SkCodec....
include/codec/SkCodec.h:202: *                      dimensions.  This may be
modified to a subset
On 2015/10/02 18:27:03, scroggo wrote:
> So if I understand correctly, this is analogous to getValidSubset? Why not
> combine the two? If someone wants a non-scaled subset, they can always use a
> scale of 1.

I'm in favor of combining.  I had considered that previously but wasn't sure.

https://codereview.chromium.org/1321433002/diff/190001/include/codec/SkCodec....
include/codec/SkCodec.h:303: *  @param subsetWidth The number of pixels to
decode in each scanline row.
On 2015/10/02 18:27:03, scroggo wrote:
> My first thought was that this could be represented by the dstInfo, but that
> represents a scale, so I suppose we do need a separate value for this.
> 
> Should these be on the Options object? We already have a long list of
parameters
> on startScanlineDecode. I suppose moving to the Options object means having a
> long list of items there, but it has two advantages:
> 
> - The options have names
> - It separates the optional from the required
> 
> On that note, I think we should consider moving the ctable stuff to the
Options
> object.

I'm in favor of putting all of this on the Options object.

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

https://codereview.chromium.org/1321433002/diff/190001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:166: if (desiredScale >= 0.9375) {
On 2015/10/02 18:27:03, scroggo wrote:
> why did this change?

IMO, it used to make sense to always round up, since libjpeg-turbo also always
rounds up, we could be consistent in a sense.

Now we have completing scaling strategies, some of which round up and others of
which round down.  I think we should try to get as close as possible to the
requested scale, since we will be comparing this scaling option with other
scaling options, and choosing the best one.

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

https://codereview.chromium.org/1321433002/diff/190001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:235: // greater than 1. The below loop relies on the
fact that src remains unchanged.
On 2015/10/02 18:27:03, scroggo wrote:
> Does the below loop not need to be updated to take this into account?

Don't think so...

https://codereview.chromium.org/1321433002/diff/190001/tools/SkBitmapRegionCa...
File tools/SkBitmapRegionCanvas.cpp (right):

https://codereview.chromium.org/1321433002/diff/190001/tools/SkBitmapRegionCa...
tools/SkBitmapRegionCanvas.cpp:36: if (sampleSize < 1) {
On 2015/10/02 18:27:03, scroggo wrote:
> Is this just in case? Or were you seeing bad input?

The Android code likes to call decodeRegion with sampleSize = 0 when it really
means 1.  Of course, since Android is not a client of code right now, this seems
unnecessary.

[scroggo, 2015-10-07 17:49:39]

On 2015/10/06 23:01:27, msarett wrote:
> "I have a high-level request: This CL does makes two big changes:
> - implements subsetting in SkScaledCodec
> - implements subsetting in scanline decoding
> 
> Would it be possible to split that into two CLs (ideally which build on
> crrev.com/1372973002, which I hope to land soon)? The first one will implement
> subsets in SkScaledCodec, which we need to do to replace SkImageDecoder. It
can
> do the simple thing of swizzling the whole row and copying the piece that's
> needed. Then we can add the second step, which is really just an
optimization."
> 
> I'm happy to split this up if that would make it easier to review.  I'm not
> convinced that the order you've suggested is the best way to do it.  I think
it
> seems kind of nasty to write a bunch of code with memcpy() that we will sweep
> away anyway.  Any objection to starting with subsetting in SkScanlineDecoder? 
> I'm kind of leaning that way.
> 
> I know that the other piece is more important to replace SkImageDecoder...but
I
> think we'll absolutely have to land both anyway.

I don't think we absolutely have to land both. Subsetting in SkScaledCodec gets
us a step closer to replacing BRD's implementation. subsetting in scanline
decoding seems like an optimization to me.

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

https://codereview.chromium.org/1321433002/diff/190001/include/codec/SkCodec....
include/codec/SkCodec.h:193: SkISize         fScaledDimensions;
On 2015/10/06 23:01:27, msarett wrote:
> On 2015/10/02 18:27:03, scroggo wrote:
> > For fSubset, we decided to make it an unowned pointer, so that a client that
> > does not want to specify it need not pay the cost of initializing the
SkIRect.
> I
> > think we should make the same decision for all three options.
> 
> I'll change to be consistent.
> 
> My concern is that a client will pass nullptr for these fields, since we are
> responsible for initializing them.  We can't really deal with that because we
> want the client to own the memory.
> 
> Not sure if it would seem to strange to a client to need to pass in valid
> pointers to uninitialized objects.

I don't think it's so bad. In this case it will just be an out variable from one
method (we do follow this pattern in other places, where we pass an
uninitialized variable into a method, which returns true if it initialized it),
and then an input variable to another (onGetPixels, I think?).

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

https://codereview.chromium.org/1321433002/diff/190001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:235: // greater than 1. The below loop relies on the
fact that src remains unchanged.
On 2015/10/06 23:01:27, msarett wrote:
> On 2015/10/02 18:27:03, scroggo wrote:
> > Does the below loop not need to be updated to take this into account?
> 
> Don't think so...

Can you help me understand why that is? I thought we previously ran into a
problem where the loop did not match, causing problems...

See Emmalee's comment https://codereview.chromium.org/1260673002#msg92. It looks
like we had to revert patch set 32, and the fix included this assert.

Put another way, did we update dstWidth to take the offset into account? Because
if not, it seems like the memcpy will copy too much, and then the loop will read
too much.

https://codereview.chromium.org/1321433002/diff/190001/tools/SkBitmapRegionCa...
File tools/SkBitmapRegionCanvas.cpp (right):

https://codereview.chromium.org/1321433002/diff/190001/tools/SkBitmapRegionCa...
tools/SkBitmapRegionCanvas.cpp:36: if (sampleSize < 1) {
On 2015/10/06 23:01:27, msarett wrote:
> On 2015/10/02 18:27:03, scroggo wrote:
> > Is this just in case? Or were you seeing bad input?
> 
> The Android code likes to call decodeRegion with sampleSize = 0 when it really
> means 1.  Of course, since Android is not a client of code right now, this
seems
> unnecessary.

Oh, weird, it looks like BitmapFactory.Options.inSampleSize never gets a default
value, so it is 0, and SkImageDecoder does this same fix.

If we do not account for it here, we'll need to make sure the Android code does
not rely on it. I think it's reasonable to account for it here though, in the
interest of being robust.