Add timing SkCodec to nanobench.

Add timing SkCodec to nanobench.

CodecBench:
Add new class for timing using SkCodec.

DecodingBench:
Include creating a decoder inside the loop. This is to have a better
comparison against SkCodec. SkCodec's factory function does not
necessarily read the same amount as SkImageDecoder's, so in order to
have a meaningful comparison, read the entire stream from the
beginning. Also for comparison, create a new SkStream from the
SkData each time.
Add a debugging check to make sure we have an SkImageDecoder.
Add include guards.

nanobench.cpp:
Decode using SkCodec.
When decoding using SkImageDecoder, exclude benches where we decoded
to a different color type than requested. SkImageDecoder may decide to
decode to a different type, in which case the name is misleading.

TODOs:
Now that we ignore color types that do not match the desired
color type, we should add Index8. This also means calling the more
complex version of getPixels so CodecBench can support kIndex8.

BUG=skia:3257
Committed: https://skia.googlesource.com/skia/+/60869a42a133942f852dd0f1696444c2a5c9ad83

[scroggo, 2015-03-31 19:20:02]

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

[scroggo, 2015-03-31 19:20:03]

https://codereview.chromium.org/1044363002/diff/1/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1044363002/diff/1/bench/nanobench.cpp#newcode642
bench/nanobench.cpp:642: SkASSERT(codec);
Maybe we should add the filter from DM that says to only consider images with
particular extensions, so this won't fail with non images (e.g. TIMESTAMP).

https://codereview.chromium.org/1044363002/diff/1/bench/nanobench.cpp#newcode666
bench/nanobench.cpp:666: // of SkCodec. All should support rewinding.
All should support rewinding - in the case where we have a rewindable stream -
which we do in this case: an SkMemoryStream.

Another fix would be to create the codec inside the while (fCurrentColorType...
loop, although I think getting rewinding working is the better fix.

[msarett, 2015-04-01 14:44:36]

This looks good.  I'm just curious how much we think calling duplicate() will
affect the bench.

https://codereview.chromium.org/1044363002/diff/1/bench/CodecBench.cpp
File bench/CodecBench.cpp (right):

https://codereview.chromium.org/1044363002/diff/1/bench/CodecBench.cpp#newcode57
bench/CodecBench.cpp:57:
codec.reset(SkCodec::NewFromStream(fStream->duplicate()));
It's unfortunate that we have to duplicate the stream.  How much do you think
that duplicating the stream will dilute the timing of the decode?  Maybe this
isn't a big deal, and if we really care, we can do some sort of profiling on the
bench to see how much time is actually spent in the decode?

Would another option be to only loop once and call detach() instead of
duplicate()?  Can nanobench accurately time a very short bench and/or run a
single bench many times and take the average?

[scroggo, 2015-04-01 14:59:49]

https://codereview.chromium.org/1044363002/diff/1/bench/CodecBench.cpp
File bench/CodecBench.cpp (right):

https://codereview.chromium.org/1044363002/diff/1/bench/CodecBench.cpp#newcode57
bench/CodecBench.cpp:57:
codec.reset(SkCodec::NewFromStream(fStream->duplicate()));
On 2015/04/01 14:44:36, msarett wrote:
> It's unfortunate that we have to duplicate the stream.  How much do you think
> that duplicating the stream will dilute the timing of the decode? 

My guess would be not much. For an SkMemoryStream, we basically do three things:
- call new
- ref the SkData
- initialize fOffset to 0 (and fData to the original)

> Maybe this
> isn't a big deal, and if we really care, we can do some sort of profiling on
the
> bench to see how much time is actually spent in the decode?

I suspect it's not a big deal, but if we're comparing against SkImageDecoder,
which I think is the meaningful comparison (at least for now), it will also pay
the price of duplication.

> 
> Would another option be to only loop once and call detach() instead of
> duplicate()?  Can nanobench accurately time a very short bench and/or run a
> single bench many times and take the average?

My limited understanding of nanobench is that it figures out the best number of
times to loop. Looping only once would seem to defeat that.

[mtklein, 2015-04-01 15:04:09]

> My limited understanding of nanobench is that it figures out the best number
of
> times to loop. Looping only once would seem to defeat that.

Yeah, if you refuse to scale ~linearly with the `loops` variable nanobench gives
you, you're gonna have a bad time.  Generally, you'll get your time printed out
as 0.00593ns, which is garbage.

[mtklein, 2015-04-01 15:17:22]

https://codereview.chromium.org/1044363002/diff/1/bench/CodecBench.cpp
File bench/CodecBench.cpp (right):

https://codereview.chromium.org/1044363002/diff/1/bench/CodecBench.cpp#newcode36
bench/CodecBench.cpp:36: #ifdef SK_DEBUG
Seems fine in Release too.

https://codereview.chromium.org/1044363002/diff/1/bench/CodecBench.cpp#newcode57
bench/CodecBench.cpp:57:
codec.reset(SkCodec::NewFromStream(fStream->duplicate()));
Think things look more clearly cheap if we use SkCodec::NewFromData()?  Seems
like less code too.  Or are we explicitly concerned with benching
NewFromStream()?

https://codereview.chromium.org/1044363002/diff/1/bench/CodecBench.cpp#newcode58
bench/CodecBench.cpp:58: bitmap.allocPixels(codec->getInfo());
Unless you need this to make a fair apples-to-apples comparison, I'd do this
outside the loop, or override onPreDraw() and do it there?

[djsollen, 2015-04-01 16:06:07]

https://codereview.chromium.org/1044363002/diff/1/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1044363002/diff/1/bench/nanobench.cpp#newcode638
bench/nanobench.cpp:638: for (; fCurrentCodec < fImages.count();
fCurrentCodec++) {
range based for loop?  C++11!!

https://codereview.chromium.org/1044363002/diff/1/bench/nanobench.cpp#newcode647
bench/nanobench.cpp:647: while (fCurrentColorType < fColorTypes.count()) {
range based loop

https://codereview.chromium.org/1044363002/diff/1/bench/nanobench.cpp#newcode666
bench/nanobench.cpp:666: // of SkCodec. All should support rewinding.
On 2015/03/31 19:20:03, scroggo wrote:
> All should support rewinding - in the case where we have a rewindable stream -
> which we do in this case: an SkMemoryStream.
> 
> Another fix would be to create the codec inside the while
(fCurrentColorType...
> loop, although I think getting rewinding working is the better fix.

Should this fire an assert then?

[scroggo, 2015-04-01 17:18:04]

New patch set up for review.

https://codereview.chromium.org/1044363002/diff/1/bench/CodecBench.cpp
File bench/CodecBench.cpp (right):

https://codereview.chromium.org/1044363002/diff/1/bench/CodecBench.cpp#newcode36
bench/CodecBench.cpp:36: #ifdef SK_DEBUG
On 2015/04/01 15:17:22, mtklein wrote:
> Seems fine in Release too.

Sure. Just unnecessary, since we do not actually do anything with codec after
this (since SkASSERT is defined to nothing in release).

Not sure if the compiler will give a warning that we do not use codec after
instantiating.

https://codereview.chromium.org/1044363002/diff/1/bench/CodecBench.cpp#newcode57
bench/CodecBench.cpp:57:
codec.reset(SkCodec::NewFromStream(fStream->duplicate()));
On 2015/04/01 15:17:22, mtklein wrote:
> Think things look more clearly cheap if we use SkCodec::NewFromData()?  Seems
> like less code too.  Or are we explicitly concerned with benching
> NewFromStream()?

NewFromData calls NewFromStream, so I don't think that would make a big
difference. That said, it might be more clear to store an SkData here?

https://codereview.chromium.org/1044363002/diff/1/bench/CodecBench.cpp#newcode58
bench/CodecBench.cpp:58: bitmap.allocPixels(codec->getInfo());
On 2015/04/01 15:17:22, mtklein wrote:
> Unless you need this to make a fair apples-to-apples comparison, I'd do this
> outside the loop, or override onPreDraw() and do it there?

Yes, this makes it a more fair comparison. Come to think of it, though, we could
move it outside of the loop and make SkImageDecoder use a custom allocator which
uses preallocated memory. Then we can compare both decodes without worrying
about allocation.

Done, in the latest patch.

https://codereview.chromium.org/1044363002/diff/1/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1044363002/diff/1/bench/nanobench.cpp#newcode638
bench/nanobench.cpp:638: for (; fCurrentCodec < fImages.count();
fCurrentCodec++) {
On 2015/04/01 16:06:07, djsollen wrote:
> range based for loop?  C++11!!

Will that work here? I need to use fCurrentCodec, rather than using a local
variable. (This object is stateful, and each call to next will return the next
new bench mark.)

https://codereview.chromium.org/1044363002/diff/1/bench/nanobench.cpp#newcode647
bench/nanobench.cpp:647: while (fCurrentColorType < fColorTypes.count()) {
On 2015/04/01 16:06:07, djsollen wrote:
> range based loop

Again, I do not think that will work, unless I'm missing something?

https://codereview.chromium.org/1044363002/diff/1/bench/nanobench.cpp#newcode666
bench/nanobench.cpp:666: // of SkCodec. All should support rewinding.
On 2015/04/01 16:06:07, djsollen wrote:
> On 2015/03/31 19:20:03, scroggo wrote:
> > All should support rewinding - in the case where we have a rewindable stream
-
> > which we do in this case: an SkMemoryStream.
> > 
> > Another fix would be to create the codec inside the while
> (fCurrentColorType...
> > loop, although I think getting rewinding working is the better fix.
> 
> Should this fire an assert then?

Once I fix all our SkCodecs to handle rewinding properly (I have written CLs to
fix them all, and I think they've all been approved by now) I can fold this into
the default statement, in which case, yes, it will assert.

[mtklein, 2015-04-01 17:22:19]

https://codereview.chromium.org/1044363002/diff/1/bench/CodecBench.cpp
File bench/CodecBench.cpp (right):

https://codereview.chromium.org/1044363002/diff/1/bench/CodecBench.cpp#newcode57
bench/CodecBench.cpp:57:
codec.reset(SkCodec::NewFromStream(fStream->duplicate()));
On 2015/04/01 17:18:04, scroggo wrote:
> On 2015/04/01 15:17:22, mtklein wrote:
> > Think things look more clearly cheap if we use SkCodec::NewFromData()? 
Seems
> > like less code too.  Or are we explicitly concerned with benching
> > NewFromStream()?
> 
> NewFromData calls NewFromStream, so I don't think that would make a big
> difference. That said, it might be more clear to store an SkData here?

Yeah, seems like it, if only because the nanobench-facing API talks in terms of
SkData.

[djsollen, 2015-04-01 17:23:50]

https://codereview.chromium.org/1044363002/diff/20001/bench/CodecBench.cpp
File bench/CodecBench.cpp (right):

https://codereview.chromium.org/1044363002/diff/20001/bench/CodecBench.cpp#ne...
bench/CodecBench.cpp:39: SkASSERT(codec);
do the assert on line 52 to avoid the extra work.

https://codereview.chromium.org/1044363002/diff/20001/bench/DecodingBench.cpp
File bench/DecodingBench.cpp (right):

https://codereview.chromium.org/1044363002/diff/20001/bench/DecodingBench.cpp...
bench/DecodingBench.cpp:49: SkASSERT(decoder != NULL);
move this to line 64.

[scroggo, 2015-04-01 17:40:54]

https://codereview.chromium.org/1044363002/diff/1/bench/CodecBench.cpp
File bench/CodecBench.cpp (right):

https://codereview.chromium.org/1044363002/diff/1/bench/CodecBench.cpp#newcode57
bench/CodecBench.cpp:57:
codec.reset(SkCodec::NewFromStream(fStream->duplicate()));
On 2015/04/01 17:22:19, mtklein wrote:
> On 2015/04/01 17:18:04, scroggo wrote:
> > On 2015/04/01 15:17:22, mtklein wrote:
> > > Think things look more clearly cheap if we use SkCodec::NewFromData()? 
> Seems
> > > like less code too.  Or are we explicitly concerned with benching
> > > NewFromStream()?
> > 
> > NewFromData calls NewFromStream, so I don't think that would make a big
> > difference. That said, it might be more clear to store an SkData here?
> 
> Yeah, seems like it, if only because the nanobench-facing API talks in terms
of
> SkData.

Done.

https://codereview.chromium.org/1044363002/diff/20001/bench/CodecBench.cpp
File bench/CodecBench.cpp (right):

https://codereview.chromium.org/1044363002/diff/20001/bench/CodecBench.cpp#ne...
bench/CodecBench.cpp:39: SkASSERT(codec);
On 2015/04/01 17:23:50, djsollen wrote:
> do the assert on line 52 to avoid the extra work.

You think that's better than failing earlier?

https://codereview.chromium.org/1044363002/diff/20001/bench/DecodingBench.cpp
File bench/DecodingBench.cpp (right):

https://codereview.chromium.org/1044363002/diff/20001/bench/DecodingBench.cpp...
bench/DecodingBench.cpp:49: SkASSERT(decoder != NULL);
On 2015/04/01 17:23:50, djsollen wrote:
> move this to line 64.

Again, do you think doing less work here is better than failing earlier?

[djsollen, 2015-04-01 17:45:34]

lgtm

[scroggo, 2015-04-01 18:01:52]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-04-01 18:01:52]

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1044363002/#ps80001
(title: "Do not leak fData.")

[commit-bot: I haz the power, 2015-04-01 18:02:21]

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

[commit-bot: I haz the power, 2015-04-01 18:04:03]

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

[commit-bot: I haz the power, 2015-04-01 18:04:03]

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

[scroggo, 2015-04-01 18:22:29]

On 2015/04/01 18:04:03, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
>
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)


Patch set 6 makes CodecBench decode to fColorType, which was the intent.

Thank you trybot for catching a logic bug based on a warning!

[scroggo, 2015-04-01 18:22:34]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-04-01 18:22:34]

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1044363002/#ps100001
(title: "CodecBench decodes to fColorType, as intended.")

[commit-bot: I haz the power, 2015-04-01 18:22:50]

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

[commit-bot: I haz the power, 2015-04-01 19:09:20]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/60869a42a133942f852dd0f1696444c2a5c9ad83