Combine native sampling with sampling

Combine native sampling with sampling

In SkSampledCodec, allow the native codec to do its scaling first, then
sample on top of that. Since the only codec which can do native scaling
is JPEG, and we know what it can do, hard-code for JPEG. Check to see
if the sampleSize is something JPEG supports, or a multiple of
something it supports. If so, use JPEG directly or combine them.

BUG=skia:4320
Committed: https://skia.googlesource.com/skia/+/501b7344f116ccc821d437d324aa7883d7ce97bf

[scroggo_chromium, 2015-10-29 21:17:26]

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

[scroggo_chromium, 2015-10-29 21:17:27]

I thought I was going to need to change getScaledSubsetDimensions to call a
virtual, but I think the base class version remains fine.

I also need to add tests for higher sampleSizes (e.g. 16, 24, 12). I've verified
that it correctly combines for sampleSize of 6 (in DM). I'll probably add more
scales to DM, unless you think there's a better way. (More images in gold to
look through. Yay!)

Otherwise this is ready for review.

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.h
File src/codec/SkSampledCodec.h (right):

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.h#...
src/codec/SkSampledCodec.h:48: SkISize accountForNativeScaling(int* sampleSize,
int* nativeSampleSize = nullptr) const;
I am open to suggestion for names of this method or its parameters. I don't like
these necessarily, but couldn't think of anything better.

[msarett, 2015-10-29 22:36:33]

As far as testing, I think we should add it everywhere.

I definitely think it belongs in DMSrcSink, and I don't think it'll kill us in
Gold, because a lot of the images will be too small to draw.  Having said that,
we should probably add a few more large pngs to the bots, since the only large
pngs we have are those three interlaced images.

As far as testing scaling on small images, there's a test in CodexTest for that.
 I don't think we care what the outputs look like, but just want to make sure
that we don't decode zero size images or seg fault.  We should probably add a
few scales to that test.

Also, I'd be in favor of adding 1 or 2 to nanobench now that we're competitive
with the old implementation.

LGTM

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.cpp
File src/codec/SkSampledCodec.cpp (left):

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.cp...
src/codec/SkSampledCodec.cpp:26: // Check if the codec can provide the scaling
natively.
I'm quite happy that this block of code is gone.

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.cpp
File src/codec/SkSampledCodec.cpp (right):

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.cp...
src/codec/SkSampledCodec.cpp:33: if (fCodec->getEncodedFormat() ==
kJPEG_SkEncodedFormat) {
Admitting that we know a little bit about the internals of jpeg in this class
doesn't seem so bad to me.  Maybe we want to keep this confined to our strange
Android support classes or maybe we could benefit from this elsewhere?

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.cp...
src/codec/SkSampledCodec.cpp:72: // I guess it would be a dumb client to call
with sampleSize of 1...
I think it's worth thinking about.  We call this with sampleSize = 1 in our test
code.  Are we a dumb client?

Unfortunately, we have three different entry points to this code. 
getSampledDimensions(), onGetSampledDimensions(), and accountForNativeScaling().
 Seems a shame to have to check in all 3, but they all would benefit from a
check.

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.cp...
src/codec/SkSampledCodec.cpp:97: // FIXME: Can we avoid redoing this?
I don't think that this is a redo.

We only make this call here on a subset decode.  And I would assume the client
is calling getSampledSubsetDimensions() on a subset decode.

So this is probably the first time we calculate this.

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.cp...
src/codec/SkSampledCodec.cpp:153: // FIXME: This was already called by
onGetAndroidPixels. Can we reduce that?
If we are performing a subset decode, we have already called this in
onGetAndroidPixels().  If we are not performing a subset decode, we have
probably already called with in getScaledDimensions().

This feels like one of those cases (similar to SkCodec) where we should think
about caching, but haven't done it yet.  I'm not sure how much it's worth
compared to decoding an image.

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.h
File src/codec/SkSampledCodec.h (right):

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.h#...
src/codec/SkSampledCodec.h:48: SkISize accountForNativeScaling(int* sampleSize,
int* nativeSampleSize = nullptr) const;
On 2015/10/29 21:17:26, scroggo_chromium wrote:
> I am open to suggestion for names of this method or its parameters. I don't
like
> these necessarily, but couldn't think of anything better.

combineScalingMethods()?
applyNativeScaling()?

It's fine as is, I don't really feel strongly.

[scroggo_chromium, 2015-11-02 21:22:10]

PTAL

On 2015/10/29 22:36:33, msarett wrote:
> As far as testing, I think we should add it everywhere.
> 
> I definitely think it belongs in DMSrcSink, and I don't think it'll kill us in
> Gold, because a lot of the images will be too small to draw. 

Added to DMSrcSink, which caught a bug (fixed in patch set 4).

> Having said that,
> we should probably add a few more large pngs to the bots, since the only large
> pngs we have are those three interlaced images.

Sounds like a good idea. I'll add some from Emmalee's set she was testing on.

> 
> As far as testing scaling on small images, there's a test in CodexTest for
that.
>  I don't think we care what the outputs look like, but just want to make sure
> that we don't decode zero size images or seg fault.  We should probably add a
> few scales to that test.

I've added some more scales to Codec_Dimensions - up to 32. Maybe we should go
further, since apps may use higher sample sizes?

> 
> Also, I'd be in favor of adding 1 or 2 to nanobench now that we're competitive
> with the old implementation.

I think if we add images to our GoogleStorage bucket they should automatically
be tested with nanobench. Is there something you think that test is missing?

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.cpp
File src/codec/SkSampledCodec.cpp (left):

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.cp...
src/codec/SkSampledCodec.cpp:26: // Check if the codec can provide the scaling
natively.
On 2015/10/29 22:36:33, msarett wrote:
> I'm quite happy that this block of code is gone.

Haha, yeah, I think the new code is much cleaner.

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.cpp
File src/codec/SkSampledCodec.cpp (right):

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.cp...
src/codec/SkSampledCodec.cpp:33: if (fCodec->getEncodedFormat() ==
kJPEG_SkEncodedFormat) {
On 2015/10/29 22:36:33, msarett wrote:
> Admitting that we know a little bit about the internals of jpeg in this class
> doesn't seem so bad to me.  Maybe we want to keep this confined to our strange
> Android support classes or maybe we could benefit from this elsewhere?

We can certainly consider adding this elsewhere, but I see no rush to.

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.cp...
src/codec/SkSampledCodec.cpp:72: // I guess it would be a dumb client to call
with sampleSize of 1...
On 2015/10/29 22:36:33, msarett wrote:
> I think it's worth thinking about.  We call this with sampleSize = 1 in our
test
> code.  Are we a dumb client?

Well, that makes a good point. If we don't special case it, we're assuming the
client will special case. I think it does make sense to add here.

Done. As long as I'm making the check, I made it <= 1, catching anyone who uses
a negative number or 0.

EDIT: I just realized that we already do this check - you had it in
onGetSampledDimensions, and I moved it to accountForNativeSampling. But the
other caller of accountForNativeSampling knows it has a sampleSize > 1, and I
think it makes more sense to move this into the base class, skipping ever
calling onGetSampledDimensions. This means SkWebpAdapterCodec will also do the
check.

> 
> Unfortunately, we have three different entry points to this code. 
> getSampledDimensions(), onGetSampledDimensions(), and
accountForNativeScaling().
>  Seems a shame to have to check in all 3, but they all would benefit from a
> check.

https://codereview.chromium.org/1417583009/diff/1/src/codec/SkSampledCodec.cp...
src/codec/SkSampledCodec.cpp:97: // FIXME: Can we avoid redoing this?
On 2015/10/29 22:36:33, msarett wrote:
> I don't think that this is a redo.
> 
> We only make this call here on a subset decode.  And I would assume the client
> is calling getSampledSubsetDimensions() on a subset decode.
> 
> So this is probably the first time we calculate this.

Good point. Removed.

https://codereview.chromium.org/1417583009/diff/60001/src/codec/SkAndroidCode...
File src/codec/SkAndroidCodec.cpp (right):

https://codereview.chromium.org/1417583009/diff/60001/src/codec/SkAndroidCode...
src/codec/SkAndroidCodec.cpp:114: // The caller wants the whole thing, rather
than a subset. Modify
This is an important fix: without it, I end up reporting failure for some of the
images we test on the bots.

The problem is that we didn't match the dimensions just right, due to rounding
differences.

When I get down into the code that checks whether setSampleX(sampleX)_==
info.height(), they do not match, so we return kInvalidScale. (FWIW, even with
your change to use options.fSampleX, modified to be the right
post-native-scaling sample size, the two do not match.)

https://codereview.chromium.org/1417583009/diff/60001/src/codec/SkAndroidCode...
src/codec/SkAndroidCodec.cpp:117: defaultOptions = *options;
"options" is not const, so I could have modified it directly. I think it should
be const, but didn't want that to slow down this change, so I'm treating it as
const, which is safer than modifying it behind the client's back. SkCodec's
options are also passed via a const pointer (was it deliberate that this one was
not?).

[msarett, 2015-11-02 22:31:57]

> > Also, I'd be in favor of adding 1 or 2 to nanobench now that we're
competitive
> > with the old implementation.
> 
> I think if we add images to our GoogleStorage bucket they should automatically
> be tested with nanobench. Is there something you think that test is missing?

Sorry for being unclear.  I meant that I would be in favor of adding a 1 or 2
additional sampleSizes to the nanobench tests.

lgtm

https://codereview.chromium.org/1417583009/diff/60001/src/codec/SkAndroidCode...
File src/codec/SkAndroidCodec.cpp (right):

https://codereview.chromium.org/1417583009/diff/60001/src/codec/SkAndroidCode...
src/codec/SkAndroidCodec.cpp:114: // The caller wants the whole thing, rather
than a subset. Modify
On 2015/11/02 21:22:10, scroggo_chromium wrote:
> This is an important fix: without it, I end up reporting failure for some of
the
> images we test on the bots.
> 
> The problem is that we didn't match the dimensions just right, due to rounding
> differences.
> 
> When I get down into the code that checks whether setSampleX(sampleX)_==
> info.height(), they do not match, so we return kInvalidScale. (FWIW, even with
> your change to use options.fSampleX, modified to be the right
> post-native-scaling sample size, the two do not match.)

Acknowledged.

https://codereview.chromium.org/1417583009/diff/60001/src/codec/SkAndroidCode...
src/codec/SkAndroidCodec.cpp:117: defaultOptions = *options;
On 2015/11/02 21:22:10, scroggo_chromium wrote:
> "options" is not const, so I could have modified it directly. I think it
should
> be const, but didn't want that to slow down this change, so I'm treating it as
> const, which is safer than modifying it behind the client's back. SkCodec's
> options are also passed via a const pointer (was it deliberate that this one
was
> not?).

Yes I think you are right, it should be const.

https://codereview.chromium.org/1417583009/diff/60001/src/codec/SkSampledCode...
File src/codec/SkSampledCodec.cpp (right):

https://codereview.chromium.org/1417583009/diff/60001/src/codec/SkSampledCode...
src/codec/SkSampledCodec.cpp:169: subsetWidth =
get_scaled_dimension(subsetPtr->width(), nativeSampleSize);
Ahh this is where it doesn't work.  Because the native scaling may not match the
behavior of get_scaled_dimension().

I think I can convince myself that this does work properly when the subset is
not the entire image.  Because getSampledSubsetDimensions() also calls
get_scaled_dimension().

[scroggo, 2015-11-03 15:17:37]

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

[scroggo, 2015-11-03 15:17:38]

On 2015/11/02 22:31:57, msarett wrote:
> > > Also, I'd be in favor of adding 1 or 2 to nanobench now that we're
> competitive
> > > with the old implementation.
> > 
> > I think if we add images to our GoogleStorage bucket they should
automatically
> > be tested with nanobench. Is there something you think that test is missing?
> 
> Sorry for being unclear.  I meant that I would be in favor of adding a 1 or 2
> additional sampleSizes to the nanobench tests.

Done. In keeping with the comment there, I only added power of two sizes (32 and
64).

https://codereview.chromium.org/1417583009/diff/60001/src/codec/SkSampledCode...
File src/codec/SkSampledCodec.cpp (right):

https://codereview.chromium.org/1417583009/diff/60001/src/codec/SkSampledCode...
src/codec/SkSampledCodec.cpp:169: subsetWidth =
get_scaled_dimension(subsetPtr->width(), nativeSampleSize);
On 2015/11/02 22:31:57, msarett wrote:
> Ahh this is where it doesn't work.  Because the native scaling may not match
the
> behavior of get_scaled_dimension().
> 
> I think I can convince myself that this does work properly when the subset is
> not the entire image.  Because getSampledSubsetDimensions() also calls
> get_scaled_dimension().

I think there's another wrinkle here, too. Our sampling method is pretty
flexible. We recompute the sampleX and sampleY down below. I could imagine that
if we tried to use the sampleSize suggested by the caller (divided by whatever
native sampling was done e.g. for a sampleSize of 24, libjpeg does 8, so if we
set sampleX and sampleY to 3) the math may not work out.

[msarett, 2015-11-03 15:23:29]

lgtm

https://codereview.chromium.org/1417583009/diff/60001/src/codec/SkSampledCode...
File src/codec/SkSampledCodec.cpp (right):

https://codereview.chromium.org/1417583009/diff/60001/src/codec/SkSampledCode...
src/codec/SkSampledCodec.cpp:169: subsetWidth =
get_scaled_dimension(subsetPtr->width(), nativeSampleSize);
On 2015/11/03 15:17:37, scroggo wrote:
> On 2015/11/02 22:31:57, msarett wrote:
> > Ahh this is where it doesn't work.  Because the native scaling may not match
> the
> > behavior of get_scaled_dimension().
> > 
> > I think I can convince myself that this does work properly when the subset
is
> > not the entire image.  Because getSampledSubsetDimensions() also calls
> > get_scaled_dimension().
> 
> I think there's another wrinkle here, too. Our sampling method is pretty
> flexible. We recompute the sampleX and sampleY down below. I could imagine
that
> if we tried to use the sampleSize suggested by the caller (divided by whatever
> native sampling was done e.g. for a sampleSize of 24, libjpeg does 8, so if we
> set sampleX and sampleY to 3) the math may not work out.

Acknowledged.

[scroggo, 2015-11-03 15:35:49]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2015-11-03 15:35:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1417583009/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1417583009/100001

[commit-bot: I haz the power, 2015-11-03 15:37:15]

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

[commit-bot: I haz the power, 2015-11-03 15:37:16]

Try jobs failed on following builders:
  Build-Mac10.8-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.8-Clang-...)

[scroggo, 2015-11-03 15:41:25]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-11-03 15:41:26]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com
Link to the patchset: https://codereview.chromium.org/1417583009/#ps120001
(title: "Work around missing initializer_list on mac")

[commit-bot: I haz the power, 2015-11-03 15:41:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1417583009/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1417583009/120001

[commit-bot: I haz the power, 2015-11-03 15:55:17]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/501b7344f116ccc821d437d324aa7883d7ce97bf