Add the ability to select bbh type, and include quadtree support

Add the ability to select bbh type, and include quadtree support.
This is a breaking change to tileGridSize flag added in 184543003, as it now defaults to a sensible value but does not implicitly turn on the tile grid bbh.

BUG=skia:2242
Committed: http://code.google.com/p/skia/source/detail?r=13641

[tomhudson, 2014-03-03 15:00:03]

LGTM modulo nit.

https://codereview.chromium.org/183763022/diff/1/tools/bench_record.cpp
File tools/bench_record.cpp (right):

https://codereview.chromium.org/183763022/diff/1/tools/bench_record.cpp#newco...
tools/bench_record.cpp:31: DEFINE_string(bbh, "bbh", "Turn on the bbh and select
the type");
If I'm reading correctly, this sets the default value of bbh to "bbh"?

[mtklein, 2014-03-03 15:20:12]

https://codereview.chromium.org/183763022/diff/1/tools/bench_record.cpp
File tools/bench_record.cpp (right):

https://codereview.chromium.org/183763022/diff/1/tools/bench_record.cpp#newco...
tools/bench_record.cpp:31: DEFINE_string(bbh, "bbh", "Turn on the bbh and select
the type");
Yeah, why default this to an invalid choice?

List the options in the help string so they show up in --help?

https://codereview.chromium.org/183763022/diff/1/tools/bench_record.cpp#newco...
tools/bench_record.cpp:41: if (FLAGS_bbh.isEmpty()) {
This is getting a little hard to digest.

Let's move the FLAGS_bbh validation logic to main just after
SkCommandLineFlags::Parse().  It seems weird to be doing it in here, and weirder
still to be doing it every time around the loop.

You might consider an approach like this:

typedef SkPicture* (*PictureFactory)();
static SkPicture* vanilla_factory() { return SkNEW(SkPicture); }
static SKPicture* rtree_factory() { ... }
...

static PictureFactory* parse_FLAGS_bbh() {
  if (FLAGS_bbh.isEmpty()) { return vanilla_factory; }
  else if (FLAGS_bbh.count != 1) { return NULL; }
  else if (...) { FLAGS_flags |= ...; return rtree_factory; }
  ....
  else { return NULL; }
}

...

int tool_main(int argc, char** argv) {
  SkCommandLineFlags::Parse(argc, argv);
  PictureFactory* factory = parse_FLAGS_bbh();
  if (!factory) { SkDebugf(...); exit(2); }

  bench_record(NULL, "NULL", factory);
  ...
}

[iancottrell, 2014-03-03 16:03:46]

https://codereview.chromium.org/183763022/diff/1/tools/bench_record.cpp
File tools/bench_record.cpp (right):

https://codereview.chromium.org/183763022/diff/1/tools/bench_record.cpp#newco...
tools/bench_record.cpp:31: DEFINE_string(bbh, "bbh", "Turn on the bbh and select
the type");
On 2014/03/03 15:20:13, mtklein wrote:
> Yeah, why default this to an invalid choice?
> 
> List the options in the help string so they show up in --help?

sorry, was meant to be the empty string so it defaults to none like before.

https://codereview.chromium.org/183763022/diff/1/tools/bench_record.cpp#newco...
tools/bench_record.cpp:41: if (FLAGS_bbh.isEmpty()) {
On 2014/03/03 15:20:13, mtklein wrote:
> This is getting a little hard to digest.
> 
> Let's move the FLAGS_bbh validation logic to main just after
> SkCommandLineFlags::Parse().  It seems weird to be doing it in here, and
weirder
> still to be doing it every time around the loop.
> 
> You might consider an approach like this:
> 
> typedef SkPicture* (*PictureFactory)();
> static SkPicture* vanilla_factory() { return SkNEW(SkPicture); }
> static SKPicture* rtree_factory() { ... }
> ...
> 
> static PictureFactory* parse_FLAGS_bbh() {
>   if (FLAGS_bbh.isEmpty()) { return vanilla_factory; }
>   else if (FLAGS_bbh.count != 1) { return NULL; }
>   else if (...) { FLAGS_flags |= ...; return rtree_factory; }
>   ....
>   else { return NULL; }
> }
> 
> ...
> 
> int tool_main(int argc, char** argv) {
>   SkCommandLineFlags::Parse(argc, argv);
>   PictureFactory* factory = parse_FLAGS_bbh();
>   if (!factory) { SkDebugf(...); exit(2); }
> 
>   bench_record(NULL, "NULL", factory);
>   ...
> }

Done, although it is a little more complicated as it has to be passed the
construction arguments.

BTW as an aside, I really don't like that we have to select a subclass of
SkPicture with a different constructor signature just to change the bbh
hierarchy implementation.
Seems like it should be possible to hide all this behind SkPicture itself.
I would like to see use take BbhType from gmmain and make it part of the
SkPicture, so you can just build an SkPicture and select what bbh it is going to
use.
It would also mean you could remove kOptimizeForClippedPlayback_RecordingFlag as
selecting kNone_BbhType gives you the same effect.
Would you be open to a cl that made such a change?

[mtklein, 2014-03-03 16:35:12]

Cool cool, just nits now.

Yes, I'd be so happy if the bounding box hierarchy were an injectable dependency
of SkPicture.

https://codereview.chromium.org/183763022/diff/20001/tools/bench_record.cpp
File tools/bench_record.cpp (right):

https://codereview.chromium.org/183763022/diff/20001/tools/bench_record.cpp#n...
tools/bench_record.cpp:78: SkAutoTUnref<SkPicture> dst(pictureFactory(width,
height, recordingFlags));
Can you make recordingFlags an int* instead of an int&?  Skia's not very good
about enforcing it, but we do try to make output parameters pointers so its
obvious at the call site.

https://codereview.chromium.org/183763022/diff/20001/tools/bench_record.cpp#n...
tools/bench_record.cpp:87: return true;
I think we can revert this back to static void now. :)

[iancottrell, 2014-03-03 16:52:50]

I will build a separate cl as a proposal for removing SkTileGridPicture and
SkQuadTreePicture

https://codereview.chromium.org/183763022/diff/20001/tools/bench_record.cpp
File tools/bench_record.cpp (right):

https://codereview.chromium.org/183763022/diff/20001/tools/bench_record.cpp#n...
tools/bench_record.cpp:78: SkAutoTUnref<SkPicture> dst(pictureFactory(width,
height, recordingFlags));
On 2014/03/03 16:35:13, mtklein wrote:
> Can you make recordingFlags an int* instead of an int&?  Skia's not very good
> about enforcing it, but we do try to make output parameters pointers so its
> obvious at the call site.

Done.

https://codereview.chromium.org/183763022/diff/20001/tools/bench_record.cpp#n...
tools/bench_record.cpp:87: return true;
On 2014/03/03 16:35:13, mtklein wrote:
> I think we can revert this back to static void now. :)

Done.

[mtklein, 2014-03-03 17:04:03]

lgtm

[iancottrell, 2014-03-03 17:06:10]

The CQ bit was checked by iancottrell@google.com

[commit-bot: I haz the power, 2014-03-03 17:06:18]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/iancottrell@google.com/183763022/40001

[commit-bot: I haz the power, 2014-03-03 17:51:14]

Change committed as 13641