Add nanobench tests for BitmapRegionDecoder

Add nanobench tests for BitmapRegionDecoder

SkBitmapRegionDecoderInterface provides an interface
for multiple implementations of Android's
BitmapRegionDecoder.

We already have correctness tests in DM that will enable us
to compare the quality of our various BRD implementations.

We also need these performance tests to compare the speed
of our various implementations.

BUG=skia:4357
Committed: https://skia.googlesource.com/skia/+/7f69144aaabbedf51ad2a1feddc9e0689f2c5ee9

[msarett, 2015-09-17 17:04:33]

Patchset #1 (id:1) has been deleted

[msarett, 2015-09-17 17:07:25]

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

[scroggo, 2015-09-17 20:05:38]

https://codereview.chromium.org/1344993003/diff/20001/bench/BitmapRegionDecod...
File bench/BitmapRegionDecoderBench.cpp (right):

https://codereview.chromium.org/1344993003/diff/20001/bench/BitmapRegionDecod...
bench/BitmapRegionDecoderBench.cpp:44: case kN32_SkColorType:
This switch statement shows up a lot. Maybe we can unify on something in tools/?

https://codereview.chromium.org/1344993003/diff/20001/bench/BitmapRegionDecod...
File bench/BitmapRegionDecoderBench.h (right):

https://codereview.chromium.org/1344993003/diff/20001/bench/BitmapRegionDecod...
bench/BitmapRegionDecoderBench.h:31: BitmapRegionDecoderBench(SkString basename,
SkData* encoded,
Probably want to use a const ref for basename. (Alternatively, we could use a
const char*)

https://codereview.chromium.org/1344993003/diff/20001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1344993003/diff/20001/bench/nanobench.cpp#new...
bench/nanobench.cpp:580: // We can set the subset rect here, since we now know
the height of the image.  We will choose
Since we're only decoding one subset, and we're comparing
SkImageDecoder::decodeSubset to other implementations, it seems like that
version is going to look bad regardless, since it also needs to call
buildTileIndex.

[msarett, 2015-09-18 13:22:30]

https://codereview.chromium.org/1344993003/diff/20001/bench/BitmapRegionDecod...
File bench/BitmapRegionDecoderBench.cpp (right):

https://codereview.chromium.org/1344993003/diff/20001/bench/BitmapRegionDecod...
bench/BitmapRegionDecoderBench.cpp:44: case kN32_SkColorType:
On 2015/09/17 20:05:38, scroggo wrote:
> This switch statement shows up a lot. Maybe we can unify on something in
tools/?

I moved something into tools, but, on second look, this code doesn't show up
anywhere but nanobench.  In DM, we case on DstColorType and only change the name
for AlwaysGray8 and AlwaysIndex8.

I'm adding SkCodecBenchPriv.h to the bench folder to share this code.

On the other hand, we have come across cases where being able to share code
between some combination of dm, nanobench, codec, tools would be useful. 
get_scaled_dimension() and get_scale_from_sample_size() are two examples, and
there are probably more.

I tried this again when I added the header file to tools - but the compiler
always complains.  Any solution that includes the header file must use all of
the functions defined in the header.  Do you know a way around this?

https://codereview.chromium.org/1344993003/diff/20001/bench/BitmapRegionDecod...
File bench/BitmapRegionDecoderBench.h (right):

https://codereview.chromium.org/1344993003/diff/20001/bench/BitmapRegionDecod...
bench/BitmapRegionDecoderBench.h:31: BitmapRegionDecoderBench(SkString basename,
SkData* encoded,
On 2015/09/17 20:05:38, scroggo wrote:
> Probably want to use a const ref for basename. (Alternatively, we could use a
> const char*)

I like const char*, since that is what we're using anyway.

https://codereview.chromium.org/1344993003/diff/20001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1344993003/diff/20001/bench/nanobench.cpp#new...
bench/nanobench.cpp:580: // We can set the subset rect here, since we now know
the height of the image.  We will choose
On 2015/09/17 20:05:38, scroggo wrote:
> Since we're only decoding one subset, and we're comparing
> SkImageDecoder::decodeSubset to other implementations, it seems like that
> version is going to look bad regardless, since it also needs to call
> buildTileIndex.

Yeah you're right, this is unfair.  I should give SkImageDecoder a real chance. 
My original rationale was that I am more focused on the comparison between
BitmapRegionCanvas and BitmapRegionScaledCodec.

Also, I saw skbug.com/4357 and I agree with you.  We should probably add a test
to the bench/subset folder, but I'm thinking we might as well also use this
test.

I was going to make a change to decode multiple subsets, but instead, I will
keep this as a single subset test and move buildTileIndex to the predraw step.

[mtklein, 2015-09-18 13:26:51]

https://codereview.chromium.org/1344993003/diff/20001/bench/BitmapRegionDecod...
File bench/BitmapRegionDecoderBench.cpp (right):

https://codereview.chromium.org/1344993003/diff/20001/bench/BitmapRegionDecod...
bench/BitmapRegionDecoderBench.cpp:44: case kN32_SkColorType:
> I tried this again when I added the header file to tools - but the compiler
> always complains.  Any solution that includes the header file must use all of
> the functions defined in the header.  Do you know a way around this?

If the function's defined in the header, you'll want to mark it inline.

static int foo(float) { ... }  in .cpp ---> inline int foo(float) { ... } in .h

If you leave it as static, the compiler will include it from the .h into your
.cpp file, then say, hey this static function isn't used inside this .cpp, and
isn't visible outside this .cpp (that's what static means), so why did you
define it in the first place?

Alternatively, declare the function like int foo(float); in the header and
define it in another .cpp file.

[scroggo, 2015-09-18 15:07:23]

https://codereview.chromium.org/1344993003/diff/20001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1344993003/diff/20001/bench/nanobench.cpp#new...
bench/nanobench.cpp:580: // We can set the subset rect here, since we now know
the height of the image.  We will choose
On 2015/09/18 13:22:30, msarett wrote:
> On 2015/09/17 20:05:38, scroggo wrote:
> > Since we're only decoding one subset, and we're comparing
> > SkImageDecoder::decodeSubset to other implementations, it seems like that
> > version is going to look bad regardless, since it also needs to call
> > buildTileIndex.
> 
> Yeah you're right, this is unfair.  I should give SkImageDecoder a real
chance. 
> My original rationale was that I am more focused on the comparison between
> BitmapRegionCanvas and BitmapRegionScaledCodec.
> 
> Also, I saw skbug.com/4357 and I agree with you.  We should probably add a
test
> to the bench/subset folder, but I'm thinking we might as well also use this
> test.
> 
> I was going to make a change to decode multiple subsets, but instead, I will
> keep this as a single subset test and move buildTileIndex to the predraw step.

I'm starting to wonder whether we want to include SkImageDecoder in this
comparison at all, if we've already decided that SkCodec is faster, unless we
think scaling tilts the scale? If we want to check for that it seems like we
would want to run our other subset benchmarks with scaling? (Maybe we already
do?)

This might be an interesting case for skbug.com/4357, and I suppose it couldn't
hurt to check, but I wonder if this is the most meaningful case to check that.
It seems like we really want to compare the cases where BRD is used, and I'm not
sure this particular subset is an interesting one.

https://codereview.chromium.org/1344993003/diff/40001/bench/BitmapRegionDecod...
File bench/BitmapRegionDecoderBench.h (right):

https://codereview.chromium.org/1344993003/diff/40001/bench/BitmapRegionDecod...
bench/BitmapRegionDecoderBench.h:19: *  Time SkBitmapRegionDecoder.
I think these comments could be improved. This one doesn't add much information.
The next paragraph says what this test doesn't do, and then the final paragraph
states what it does test.

I also think the comments should place more emphasis on the fact that this is to
compare scaling approaches.

https://codereview.chromium.org/1344993003/diff/40001/bench/BitmapRegionDecod...
bench/BitmapRegionDecoderBench.h:28: class BitmapRegionDecoderBench : public
Benchmark {
If the primary goal here is to time approaches to scaling, maybe this should
have a name that better reflects that?

[msarett, 2015-09-18 17:41:17]

No new code here - just wanted to respond to the comments.

Thanks for your comments!  They've caused me to think a little bit harder about
what this test is for and why we need it.

This test is broad enough to cover all of the use cases of BitmapRegionDecoder
and should cover all of the use cases.  Because we have moved buildTileIndex()
to onPreDraw(), there is no reason to test more than a single subset at a time,
but we should test all of the subsets that we care about at all of the scales
that we care about.

This test is more powerful than the SubsetBenches because it allows us to test
with scaling, and it allows us to test exactly the code that will be used by
Android.

My original plan was to use the existence of the SubsetBenches as justification
for not devising a more comprehensive set of use cases.  I think that was wrong.

This test should be the test that covers everything.  The use cases will not be
exactly the same as the SubsetBench use cases.  Now that we have scaling, we can
do a better job of matching our test with real uses of BRD.

I will reexamine the use cases of BRD and rewrite this test to cover these use
cases appropriately.

Side Note:
The SubsetBenches are able to test something we cannot from the BRD interface,
which is the ability to reuse the same block of memory for multiple subsets.  I
would guess that this had a huge impact on the positive results we saw initially
for Codec.  This should provide some justification for your improvements to the
API (and also some motivation to improve the API all the way up to the Java
level).

I don't think this test makes the SubsetBenches obsolete.  I think the
SubsetBenches are in need of an update, but we cannot know how to update them
(particularly how to add scaling) until we know which BRD strategy is the
winner.

https://codereview.chromium.org/1344993003/diff/20001/bench/BitmapRegionDecod...
File bench/BitmapRegionDecoderBench.cpp (right):

https://codereview.chromium.org/1344993003/diff/20001/bench/BitmapRegionDecod...
bench/BitmapRegionDecoderBench.cpp:44: case kN32_SkColorType:
On 2015/09/18 13:26:51, mtklein wrote:
> > I tried this again when I added the header file to tools - but the compiler
> > always complains.  Any solution that includes the header file must use all
of
> > the functions defined in the header.  Do you know a way around this?
> 
> If the function's defined in the header, you'll want to mark it inline.
> 
> static int foo(float) { ... }  in .cpp ---> inline int foo(float) { ... } in
.h
> 
> If you leave it as static, the compiler will include it from the .h into your
> .cpp file, then say, hey this static function isn't used inside this .cpp, and
> isn't visible outside this .cpp (that's what static means), so why did you
> define it in the first place?
> 
> Alternatively, declare the function like int foo(float); in the header and
> define it in another .cpp file.

Thanks for your explanation!

https://codereview.chromium.org/1344993003/diff/20001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1344993003/diff/20001/bench/nanobench.cpp#new...
bench/nanobench.cpp:580: // We can set the subset rect here, since we now know
the height of the image.  We will choose
On 2015/09/18 15:07:23, scroggo wrote:
> On 2015/09/18 13:22:30, msarett wrote:
> > On 2015/09/17 20:05:38, scroggo wrote:
> > > Since we're only decoding one subset, and we're comparing
> > > SkImageDecoder::decodeSubset to other implementations, it seems like that
> > > version is going to look bad regardless, since it also needs to call
> > > buildTileIndex.
> > 
> > Yeah you're right, this is unfair.  I should give SkImageDecoder a real
> chance. 
> > My original rationale was that I am more focused on the comparison between
> > BitmapRegionCanvas and BitmapRegionScaledCodec.
> > 
> > Also, I saw skbug.com/4357 and I agree with you.  We should probably add a
> test
> > to the bench/subset folder, but I'm thinking we might as well also use this
> > test.
> > 
> > I was going to make a change to decode multiple subsets, but instead, I will
> > keep this as a single subset test and move buildTileIndex to the predraw
step.
> 
> I'm starting to wonder whether we want to include SkImageDecoder in this
> comparison at all, if we've already decided that SkCodec is faster, unless we
> think scaling tilts the scale?

I think we do want to include SkImageDecoder.  I fully expect SkImageDecoder to
lose, but, as I think you've mentioned before, we haven't really tested with all
of the outer layers on SkCodec.  Before replacing the BitmapRegionDecoder API, I
think we want to do the most fair comparison we can, testing both
implementations from the top level.

My original implementation didn't really give us a fair comparison, so I'm glad
you pointed that out.

If we change our minds, or finally end up deleting SkImageDecoder, turning off
this test is trivial (we just need to remove kOriginal_Strategy from the list of
strategies in nanobench.cpp).

> If we want to check for that it seems like we
> would want to run our other subset benchmarks with scaling? (Maybe we already
> do?)

We absolutely should check the other subset benchmarks with scaling.  I will do
this once I finish implementing scaling in SkScaledCodec.

> This might be an interesting case for skbug.com/4357, and I suppose it
couldn't
> hurt to check, but I wonder if this is the most meaningful case to check that.
> It seems like we really want to compare the cases where BRD is used, and I'm
not
> sure this particular subset is an interesting one.

I agree that this is not the only case for skbug/4357.  But I think it doesn't
hurt to be overly pessimistic when trying to justify replacing the old code with
the new code.

When I rewrite the SubsetBenches to test scaling, I will address 4357 again
there.

In general, I don't completely agree that this subset isn't the most meaningful
to test.  Certainly, it's not as exhaustive as the SubsetBenches.

IMO, this test actually covers a use case that we see quite frequently (but
couldn't test when we didn't have scaling).  Apps like Gallery/Photos/Maps will
scale large original images into many square thumbnails to view on the same
page.

Full disclaimer: Maps only has a TODO to use BitmapRegionDecoder for this.  I
*think* Photos uses BitmapRegionDecoder for this, but I'm not 100% sure.  If
they don't use BRD for this, I think they should.

It's also nice to test scaling performance when scaling a large subset - I
imagine that the influence of other factors is greater when the subsets are
small.

A downfall of this test might be that we don't fix the size of the output (ex:
to 512x512).  This would require cropping differently depending on the scale.  I
preferred not to add this complexity, but now I'm think that this test should be
much more complex...

https://codereview.chromium.org/1344993003/diff/40001/bench/BitmapRegionDecod...
File bench/BitmapRegionDecoderBench.h (right):

https://codereview.chromium.org/1344993003/diff/40001/bench/BitmapRegionDecod...
bench/BitmapRegionDecoderBench.h:19: *  Time SkBitmapRegionDecoder.
On 2015/09/18 15:07:23, scroggo wrote:
> I think these comments could be improved. This one doesn't add much
information.
> The next paragraph says what this test doesn't do, and then the final
paragraph
> states what it does test.
> 
> I also think the comments should place more emphasis on the fact that this is
to
> compare scaling approaches.

You are right these comments are bad and actually misleading.  I do think this
matters as a subset test in addition to a scaling test.  Will fix.

https://codereview.chromium.org/1344993003/diff/40001/bench/BitmapRegionDecod...
bench/BitmapRegionDecoderBench.h:28: class BitmapRegionDecoderBench : public
Benchmark {
On 2015/09/18 15:07:23, scroggo wrote:
> If the primary goal here is to time approaches to scaling, maybe this should
> have a name that better reflects that?

I'd prefer to stick with the name and improve the test.

[msarett, 2015-09-18 21:37:20]

This version tests our use cases much more comprehensively.

I expect this test to put a little bit of pressure on us.  I anticipate that we
will be slower than SkImageDecoder in a number of cases.

[scroggo, 2015-09-22 12:13:09]

https://codereview.chromium.org/1344993003/diff/60001/bench/BitmapRegionDecod...
File bench/BitmapRegionDecoderBench.cpp (right):

https://codereview.chromium.org/1344993003/diff/60001/bench/BitmapRegionDecod...
bench/BitmapRegionDecoderBench.cpp:43: fName.printf("BRD_%s_%s_%s", baseName,
strategyName, colorName);
nit: You could do this always, and then appendf if sampleSize != 1

https://codereview.chromium.org/1344993003/diff/60001/bench/SkCodecBenchPriv.h
File bench/SkCodecBenchPriv.h (right):

https://codereview.chromium.org/1344993003/diff/60001/bench/SkCodecBenchPriv....
bench/SkCodecBenchPriv.h:11: static const char* color_type_to_str(SkColorType
colorType) {
I think this should be inline and not static (see Mike's previous comments)

https://codereview.chromium.org/1344993003/diff/60001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1344993003/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:927: // in the image.  Additionally, power of two scaling is
enabled, so we test on the below
Why only power of two? Is it because BRD claims to only support powers of two,
and JPEG does the wrong thing (skbug.com/4319)?

https://codereview.chromium.org/1344993003/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:931: const uint32_t minOutputSize = 512;
Why did you make this a variable passed to valid_brd_bench, instead of a
constant inside that function?

https://codereview.chromium.org/1344993003/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:951: switch (currentSubsetType) {
You can eliminate a bunch of lines of code by moving the return outside the
switch statement:

SkIRect irect;
switch (currentSubsetType) {
  case kTopLeft_SubsetType:
    basename.append("_TopLeft");
    irect = SkIRect::MakeXYWH(0, 0, subsetSize, subsetSize);
    break;
  ...
  default:
    SkASSERT(false);
    return nullptr;
}

return new BRDBench(basename, encoded, strategy, colorType, sampleSize, irect);

https://codereview.chromium.org/1344993003/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:988: } else {
nit: This will be more readable if you do the "exit" condition first:

if (!valid_brd_bench(...)) {
  break;
}
// The code you currently have in the above block.

[msarett, 2015-09-22 15:27:19]

https://codereview.chromium.org/1344993003/diff/60001/bench/BitmapRegionDecod...
File bench/BitmapRegionDecoderBench.cpp (right):

https://codereview.chromium.org/1344993003/diff/60001/bench/BitmapRegionDecod...
bench/BitmapRegionDecoderBench.cpp:43: fName.printf("BRD_%s_%s_%s", baseName,
strategyName, colorName);
On 2015/09/22 12:13:08, scroggo wrote:
> nit: You could do this always, and then appendf if sampleSize != 1

Done.

https://codereview.chromium.org/1344993003/diff/60001/bench/SkCodecBenchPriv.h
File bench/SkCodecBenchPriv.h (right):

https://codereview.chromium.org/1344993003/diff/60001/bench/SkCodecBenchPriv....
bench/SkCodecBenchPriv.h:11: static const char* color_type_to_str(SkColorType
colorType) {
On 2015/09/22 12:13:08, scroggo wrote:
> I think this should be inline and not static (see Mike's previous comments)

Yes agreed!

Also, I'm renaming this CodecBenchPriv.h

https://codereview.chromium.org/1344993003/diff/60001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1344993003/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:927: // in the image.  Additionally, power of two scaling is
enabled, so we test on the below
On 2015/09/22 12:13:09, scroggo wrote:
> Why only power of two? Is it because BRD claims to only support powers of two,
> and JPEG does the wrong thing (skbug.com/4319)?

I think there are a few good reasons to only test power of 2.
(1) As you said, SkImageDecoder JPEG does the wrong thing, and we don't want to
benchmark code that doesn't work.
(2) All of the uses of BRD I have found only use power of 2, including the
particular use case that I am trying to match.
(3) PNGs always use the same sampling strategy regardless of the sample size, so
I think we get good coverage of PNG performance with this set of sample sizes.

On the other hand, we will definitely want to compare SkScaledCodec and
SkBitmapRegionCanvas for sampling JPEGs, which means we need to test on
non-power of 2 scales.

I am adding a few non-power of two scales and disabling these tests for
SkImageDecoder.  AFAIK, these tests aren't relevant for any current uses but may
be interesting for us to look at.

https://codereview.chromium.org/1344993003/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:931: const uint32_t minOutputSize = 512;
On 2015/09/22 12:13:09, scroggo wrote:
> Why did you make this a variable passed to valid_brd_bench, instead of a
> constant inside that function?

I use it on line 950 to calculate subsetSize and wanted to avoid defining it
twice.  Alternatively, valid_brd_bench could set the subsetSize, but I'm not
sure either approach is better.

https://codereview.chromium.org/1344993003/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:951: switch (currentSubsetType) {
On 2015/09/22 12:13:09, scroggo wrote:
> You can eliminate a bunch of lines of code by moving the return outside the
> switch statement:
> 
> SkIRect irect;
> switch (currentSubsetType) {
>   case kTopLeft_SubsetType:
>     basename.append("_TopLeft");
>     irect = SkIRect::MakeXYWH(0, 0, subsetSize, subsetSize);
>     break;
>   ...
>   default:
>     SkASSERT(false);
>     return nullptr;
> }
> 
> return new BRDBench(basename, encoded, strategy, colorType, sampleSize,
irect);

Yes this is much better!

https://codereview.chromium.org/1344993003/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:988: } else {
On 2015/09/22 12:13:09, scroggo wrote:
> nit: This will be more readable if you do the "exit" condition first:
> 
> if (!valid_brd_bench(...)) {
>   break;
> }
> // The code you currently have in the above block.

Done.

[scroggo, 2015-09-22 16:57:35]

lgtm with some nits

https://codereview.chromium.org/1344993003/diff/60001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1344993003/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:927: // in the image.  Additionally, power of two scaling is
enabled, so we test on the below
On 2015/09/22 15:27:19, msarett wrote:
> On 2015/09/22 12:13:09, scroggo wrote:
> > Why only power of two? Is it because BRD claims to only support powers of
two,
> > and JPEG does the wrong thing (skbug.com/4319)?
> 
> I think there are a few good reasons to only test power of 2.
> (1) As you said, SkImageDecoder JPEG does the wrong thing, and we don't want
to
> benchmark code that doesn't work.
> (2) All of the uses of BRD I have found only use power of 2, including the
> particular use case that I am trying to match.
> (3) PNGs always use the same sampling strategy regardless of the sample size,
so
> I think we get good coverage of PNG performance with this set of sample sizes.
> 
> On the other hand, we will definitely want to compare SkScaledCodec and
> SkBitmapRegionCanvas for sampling JPEGs, which means we need to test on
> non-power of 2 scales.
> 
> I am adding a few non-power of two scales and disabling these tests for
> SkImageDecoder.  AFAIK, these tests aren't relevant for any current uses but
may
> be interesting for us to look at.

Actually, you pose a good argument for only testing powers of two. I'm fine with
sticking with only powers of two (maybe with an explanation similar to the one
in this comment).

https://codereview.chromium.org/1344993003/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:931: const uint32_t minOutputSize = 512;
On 2015/09/22 15:27:19, msarett wrote:
> On 2015/09/22 12:13:09, scroggo wrote:
> > Why did you make this a variable passed to valid_brd_bench, instead of a
> > constant inside that function?
> 
> I use it on line 950 to calculate subsetSize and wanted to avoid defining it
> twice.  Alternatively, valid_brd_bench could set the subsetSize, but I'm not
> sure either approach is better.

Thanks for the explanation. The current way seems fine.

https://codereview.chromium.org/1344993003/diff/80001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1344993003/diff/80001/bench/nanobench.cpp#new...
bench/nanobench.cpp:965: } else {
nit: Part of the beauty of handling the break first is that this need not be an
else statement.

if (!valid_brd_bench(...)) {
  break;
}

// This line can line up with the if statement.
SkString basename ...

[msarett, 2015-09-22 17:53:38]

https://codereview.chromium.org/1344993003/diff/60001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1344993003/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:927: // in the image.  Additionally, power of two scaling is
enabled, so we test on the below
On 2015/09/22 16:57:35, scroggo wrote:
> On 2015/09/22 15:27:19, msarett wrote:
> > On 2015/09/22 12:13:09, scroggo wrote:
> > > Why only power of two? Is it because BRD claims to only support powers of
> two,
> > > and JPEG does the wrong thing (skbug.com/4319)?
> > 
> > I think there are a few good reasons to only test power of 2.
> > (1) As you said, SkImageDecoder JPEG does the wrong thing, and we don't want
> to
> > benchmark code that doesn't work.
> > (2) All of the uses of BRD I have found only use power of 2, including the
> > particular use case that I am trying to match.
> > (3) PNGs always use the same sampling strategy regardless of the sample
size,
> so
> > I think we get good coverage of PNG performance with this set of sample
sizes.
> > 
> > On the other hand, we will definitely want to compare SkScaledCodec and
> > SkBitmapRegionCanvas for sampling JPEGs, which means we need to test on
> > non-power of 2 scales.
> > 
> > I am adding a few non-power of two scales and disabling these tests for
> > SkImageDecoder.  AFAIK, these tests aren't relevant for any current uses but
> may
> > be interesting for us to look at.
> 
> Actually, you pose a good argument for only testing powers of two. I'm fine
with
> sticking with only powers of two (maybe with an explanation similar to the one
> in this comment).

Done.

https://codereview.chromium.org/1344993003/diff/80001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1344993003/diff/80001/bench/nanobench.cpp#new...
bench/nanobench.cpp:965: } else {
On 2015/09/22 16:57:35, scroggo wrote:
> nit: Part of the beauty of handling the break first is that this need not be
an
> else statement.
> 
> if (!valid_brd_bench(...)) {
>   break;
> }
> 
> // This line can line up with the if statement.
> SkString basename ...

Ah yes of course

[msarett, 2015-09-22 17:54:22]

The CQ bit was checked by msarett@google.com

[msarett, 2015-09-22 17:54:22]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1344993003/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-09-22 17:54:32]

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

[commit-bot: I haz the power, 2015-09-22 17:57:02]

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

[commit-bot: I haz the power, 2015-09-22 17:57:03]

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

[msarett, 2015-09-22 18:46:18]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-09-22 18:46:30]

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

[commit-bot: I haz the power, 2015-09-22 18:56:19]

Committed patchset #5 (id:100001) as
https://skia.googlesource.com/skia/+/7f69144aaabbedf51ad2a1feddc9e0689f2c5ee9