Report data from bench_pictures in the same fashion as bench.

Report data from bench_pictures in the same fashion as bench.

Move SkBenchLogger into separate files and make bench_pictures use it.
Remove sk_tools::print_msg, since SkBenchLogger is now used instead.

Combine picture_benchmark with bench_pictures, since that is the
only project that uses it.

Refactor the aggregator for bench timer data into its own class and
make bench_pictures use it.

Consolidate the various virtual PictureBenchmark::run functions
into one for reuse.

BUG=https://code.google.com/p/skia/issues/detail?id=822

Committed: https://code.google.com/p/skia/source/detail?r=5432

[EricB, 2012-09-06 12:36:14]

+reed@, +bsalomon@, since I'm not very familiar with this code.  Looks good
overall, with a couple of comments.

https://codereview.appspot.com/6488086/diff/2001/tools/PictureBenchmark.h
File tools/PictureBenchmark.h (right):

https://codereview.appspot.com/6488086/diff/2001/tools/PictureBenchmark.h#new...
tools/PictureBenchmark.h:23: PictureBenchmark() : fLogger(NULL) {}
Should there be a default value for fRepeats?  Or can we rely on setRepeats
always being called?

https://codereview.appspot.com/6488086/diff/2001/tools/bench_pictures_main.cpp
File tools/bench_pictures_main.cpp (right):

https://codereview.appspot.com/6488086/diff/2001/tools/bench_pictures_main.cp...
tools/bench_pictures_main.cpp:117: static void parse_commandline(int argc, char*
const argv[], SkTArray<SkString>* inputs,
Can we include a logFile?  This is necessary for the Android bots, since stdout
and stderr get displayed in the same stream and since Android modifies the log
formatting.

[bsalomon, 2012-09-06 12:56:59]

I think Ben is familiar with bench logging and Rob just recently worked on GPU
timing. I took a look at the latter in this CL and it looked consistent with
what is done in bench to me.

https://codereview.appspot.com/6488086/diff/2001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.appspot.com/6488086/diff/2001/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:52: * Draw to the base canvas.
It looks like this includes the render() functionality and the copy to the base
canvas (as opposed to assuming render() was already called and having this
function just draw the tiles to the base).

It might be helpful to make that obvious in the comments. Or would it make sense
to have a bool param to render() rather than two virtuals?

[robertphillips, 2012-09-06 13:24:44]

I am a bit worried about the loss of the glFinish calls.

https://codereview.appspot.com/6488086/diff/2001/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.appspot.com/6488086/diff/2001/bench/benchmain.cpp#newcode355
bench/benchmain.cpp:355: "    [-match name] [-mode
normal|deferred|record|picturerecord]\n"
[-logFile filename]?

https://codereview.appspot.com/6488086/diff/2001/tools/PictureBenchmark.h
File tools/PictureBenchmark.h (right):

https://codereview.appspot.com/6488086/diff/2001/tools/PictureBenchmark.h#new...
tools/PictureBenchmark.h:44: virtual sk_tools::PictureRenderer* getRenderer() {
pure virtual?

https://codereview.appspot.com/6488086/diff/2001/tools/PictureBenchmark.h#new...
tools/PictureBenchmark.h:69: private:
nit: shouldn't all these classes match this order: data, method, INHERITED?

https://codereview.appspot.com/6488086/diff/2001/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6488086/diff/2001/tools/PictureRenderer.cpp#ne...
tools/PictureRenderer.cpp:136: writer.endRecording();
I think we are losing the call to glFinish here?

[bsalomon, 2012-09-06 13:31:52]

https://codereview.appspot.com/6488086/diff/2001/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6488086/diff/2001/tools/PictureRenderer.cpp#ne...
tools/PictureRenderer.cpp:136: writer.endRecording();
On 2012/09/06 13:24:45, robertphillips wrote:
> I think we are losing the call to glFinish here?

I thought that was deliberate. The loop is now

setup
render // no finish
end_trunc_timer
resetState // finish called
end_timer

[robertphillips, 2012-09-06 13:36:54]

Okay - I missed the glFinish call in resetState.

[Leon, 2012-09-06 18:44:22]

https://codereview.appspot.com/6488086/diff/2001/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.appspot.com/6488086/diff/2001/bench/benchmain.cpp#newcode355
bench/benchmain.cpp:355: "    [-match name] [-mode
normal|deferred|record|picturerecord]\n"
On 2012/09/06 13:24:45, robertphillips wrote:
> [-logFile filename]?

Done.

https://codereview.appspot.com/6488086/diff/2001/tools/PictureBenchmark.h
File tools/PictureBenchmark.h (right):

https://codereview.appspot.com/6488086/diff/2001/tools/PictureBenchmark.h#new...
tools/PictureBenchmark.h:23: PictureBenchmark() : fLogger(NULL) {}
On 2012/09/06 12:36:15, EricB wrote:
> Should there be a default value for fRepeats?  Or can we rely on setRepeats
> always being called?

Done

https://codereview.appspot.com/6488086/diff/2001/tools/PictureBenchmark.h#new...
tools/PictureBenchmark.h:44: virtual sk_tools::PictureRenderer* getRenderer() {
On 2012/09/06 13:24:45, robertphillips wrote:
> pure virtual?

Done.

https://codereview.appspot.com/6488086/diff/2001/tools/PictureBenchmark.h#new...
tools/PictureBenchmark.h:69: private:
On 2012/09/06 13:24:45, robertphillips wrote:
> nit: shouldn't all these classes match this order: data, method, INHERITED?

Done.

https://codereview.appspot.com/6488086/diff/2001/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6488086/diff/2001/tools/PictureRenderer.cpp#ne...
tools/PictureRenderer.cpp:136: writer.endRecording();
On 2012/09/06 13:31:52, bsalomon wrote:
> On 2012/09/06 13:24:45, robertphillips wrote:
> > I think we are losing the call to glFinish here?
> 
> I thought that was deliberate. The loop is now
> 
> setup
> render // no finish
> end_trunc_timer
> resetState // finish called
> end_timer

That is correct.

https://codereview.appspot.com/6488086/diff/2001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.appspot.com/6488086/diff/2001/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:52: * Draw to the base canvas.
On 2012/09/06 12:57:00, bsalomon wrote:
> It looks like this includes the render() functionality and the copy to the
base
> canvas (as opposed to assuming render() was already called and having this
> function just draw the tiles to the base).
> 
> It might be helpful to make that obvious in the comments. Or would it make
sense
> to have a bool param to render() rather than two virtuals?

Done.

https://codereview.appspot.com/6488086/diff/2001/tools/bench_pictures_main.cpp
File tools/bench_pictures_main.cpp (right):

https://codereview.appspot.com/6488086/diff/2001/tools/bench_pictures_main.cp...
tools/bench_pictures_main.cpp:117: static void parse_commandline(int argc, char*
const argv[], SkTArray<SkString>* inputs,
On 2012/09/06 12:36:15, EricB wrote:
> Can we include a logFile?  This is necessary for the Android bots, since
stdout
> and stderr get displayed in the same stream and since Android modifies the log
> formatting.

Done.

[EricB, 2012-09-06 19:36:36]

On 2012/09/06 18:44:22, scroggo-work wrote:
> https://codereview.appspot.com/6488086/diff/2001/bench/benchmain.cpp
> File bench/benchmain.cpp (right):
> 
>
https://codereview.appspot.com/6488086/diff/2001/bench/benchmain.cpp#newcode355
> bench/benchmain.cpp:355: "    [-match name] [-mode
> normal|deferred|record|picturerecord]\n"
> On 2012/09/06 13:24:45, robertphillips wrote:
> > [-logFile filename]?
> 
> Done.
> 
> https://codereview.appspot.com/6488086/diff/2001/tools/PictureBenchmark.h
> File tools/PictureBenchmark.h (right):
> 
>
https://codereview.appspot.com/6488086/diff/2001/tools/PictureBenchmark.h#new...
> tools/PictureBenchmark.h:23: PictureBenchmark() : fLogger(NULL) {}
> On 2012/09/06 12:36:15, EricB wrote:
> > Should there be a default value for fRepeats?  Or can we rely on setRepeats
> > always being called?
> 
> Done
> 
>
https://codereview.appspot.com/6488086/diff/2001/tools/PictureBenchmark.h#new...
> tools/PictureBenchmark.h:44: virtual sk_tools::PictureRenderer* getRenderer()
{
> On 2012/09/06 13:24:45, robertphillips wrote:
> > pure virtual?
> 
> Done.
> 
>
https://codereview.appspot.com/6488086/diff/2001/tools/PictureBenchmark.h#new...
> tools/PictureBenchmark.h:69: private:
> On 2012/09/06 13:24:45, robertphillips wrote:
> > nit: shouldn't all these classes match this order: data, method, INHERITED?
> 
> Done.
> 
> https://codereview.appspot.com/6488086/diff/2001/tools/PictureRenderer.cpp
> File tools/PictureRenderer.cpp (right):
> 
>
https://codereview.appspot.com/6488086/diff/2001/tools/PictureRenderer.cpp#ne...
> tools/PictureRenderer.cpp:136: writer.endRecording();
> On 2012/09/06 13:31:52, bsalomon wrote:
> > On 2012/09/06 13:24:45, robertphillips wrote:
> > > I think we are losing the call to glFinish here?
> > 
> > I thought that was deliberate. The loop is now
> > 
> > setup
> > render // no finish
> > end_trunc_timer
> > resetState // finish called
> > end_timer
> 
> That is correct.
> 
> https://codereview.appspot.com/6488086/diff/2001/tools/PictureRenderer.h
> File tools/PictureRenderer.h (right):
> 
>
https://codereview.appspot.com/6488086/diff/2001/tools/PictureRenderer.h#newc...
> tools/PictureRenderer.h:52: * Draw to the base canvas.
> On 2012/09/06 12:57:00, bsalomon wrote:
> > It looks like this includes the render() functionality and the copy to the
> base
> > canvas (as opposed to assuming render() was already called and having this
> > function just draw the tiles to the base).
> > 
> > It might be helpful to make that obvious in the comments. Or would it make
> sense
> > to have a bool param to render() rather than two virtuals?
> 
> Done.
> 
> https://codereview.appspot.com/6488086/diff/2001/tools/bench_pictures_main.cpp
> File tools/bench_pictures_main.cpp (right):
> 
>
https://codereview.appspot.com/6488086/diff/2001/tools/bench_pictures_main.cp...
> tools/bench_pictures_main.cpp:117: static void parse_commandline(int argc,
char*
> const argv[], SkTArray<SkString>* inputs,
> On 2012/09/06 12:36:15, EricB wrote:
> > Can we include a logFile?  This is necessary for the Android bots, since
> stdout
> > and stderr get displayed in the same stream and since Android modifies the
log
> > formatting.
> 
> Done.

This LGTM but I'd like confirmation from the others.

[bsalomon, 2012-09-06 20:14:12]

LGTM

https://codereview.appspot.com/6488086/diff/19/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.appspot.com/6488086/diff/19/tools/PictureRenderer.h#newcode53
tools/PictureRenderer.h:53: virtual void render(bool
doExtraWorkToDrawToBaseCanvas) = 0;
You know it's ok to be verbose with your parameter names. Please don't hold
back.