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

tools/bench_record.cpp

Index: tools/bench_record.cpp
diff --git a/tools/bench_record.cpp b/tools/bench_record.cpp
index 8faf81bbea7c58ba05d9c6dc6b95c3ae80f07caf..07bb8252cb74658000be45f3bc6522af985bfdaf 100644
--- a/tools/bench_record.cpp
+++ b/tools/bench_record.cpp
@@ -10,6 +10,7 @@
 #include "SkGraphics.h"
 #include "SkOSFile.h"
 #include "SkPicture.h"
+#include "SkQuadTreePicture.h"
 #include "SkStream.h"
 #include "SkString.h"
 #include "SkTileGridPicture.h"
@@ -26,9 +27,10 @@ DEFINE_int32(loops, 900, "Number of times to re-record each SKP.");
 DEFINE_int32(flags, SkPicture::kUsePathBoundsForClip_RecordingFlag, "RecordingFlags to use.");
 DEFINE_bool(endRecording, true, "If false, don't time SkPicture::endRecording()");
 DEFINE_int32(nullSize, 1000, "Pretend dimension of null source picture.");
-DEFINE_int32(tileGridSize, 0, "Set the tile grid size, if non zero switches to tile grid picture and enables kOptimizeForClippedPlayback_RecordingFlag.");
+DEFINE_int32(tileGridSize, 512, "Set the tile grid size. Has no effect if bbh is not set to tilegrid.");
+DEFINE_string(bbh, "bbh", "Turn on the bbh and select the type");

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

If I'm reading correctly, this sets the default value of bbh to "bbh"?

[mtklein, 2014/03/03 15:20:13]

Yeah, why default this to an invalid choice?

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

[iancottrell, 2014/03/03 16:03:47]

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

 
-static void bench_record(SkPicture* src, const char* name) {
+static bool bench_record(SkPicture* src, const char* name) {
     const SkMSec start = SkTime::GetMSecs();
     const int width  = src ? src->width()  : FLAGS_nullSize;
     const int height = src ? src->height() : FLAGS_nullSize;
@@ -36,15 +38,27 @@ static void bench_record(SkPicture* src, const char* name) {
     for (int i = 0; i < FLAGS_loops; i++) {
         SkAutoTUnref<SkPicture> dst;
         int recordingFlags = FLAGS_flags;
-        if (FLAGS_tileGridSize > 0) {
+        if (FLAGS_bbh.isEmpty()) {

[mtklein, 2014/03/03 15:20:13]

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:47]

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?

+            dst.reset(SkNEW(SkPicture));
+        } else if (FLAGS_bbh.count() !=1 ) {
+            SkDebugf("More than one bbh type specified.\n");
+            return false;
+        } else if (FLAGS_bbh.contains("rtree")) {
+            recordingFlags |= SkPicture::kOptimizeForClippedPlayback_RecordingFlag;
+            dst.reset(SkNEW(SkPicture));
+        } else if (FLAGS_bbh.contains("tilegrid")) {
+            recordingFlags |= SkPicture::kOptimizeForClippedPlayback_RecordingFlag;
             SkTileGridPicture::TileGridInfo info;
             info.fTileInterval.set(FLAGS_tileGridSize, FLAGS_tileGridSize);
             info.fMargin.setEmpty();
             info.fOffset.setZero();
             dst.reset(SkNEW_ARGS(SkTileGridPicture, (width, height, info)));
+        } else if (FLAGS_bbh.contains("quadtree")) {
             recordingFlags |= SkPicture::kOptimizeForClippedPlayback_RecordingFlag;
+            dst.reset(SkNEW_ARGS(SkQuadTreePicture, (SkIRect::MakeWH(width, height))));
         } else {
-            dst.reset(SkNEW(SkPicture));
+            SkDebugf("Invalid bbh type %s, must be one of rtree, tilegrid, quadtree.\n", FLAGS_bbh[0]);
+            return false;
         }
         SkCanvas* canvas = dst->beginRecording(width, height, recordingFlags);
         if (src) src->draw(canvas);
@@ -54,6 +68,7 @@ static void bench_record(SkPicture* src, const char* name) {
     const SkMSec elapsed = SkTime::GetMSecs() - start;
     const double msPerLoop = elapsed / (double)FLAGS_loops;
     printf("%.2g\t%s\n", msPerLoop, name);
+    return true;
 }
 
 int tool_main(int argc, char** argv);
@@ -61,7 +76,9 @@ int tool_main(int argc, char** argv) {
     SkCommandLineFlags::Parse(argc, argv);
     SkAutoGraphics autoGraphics;
 
-    bench_record(NULL, "NULL");
+    if (!bench_record(NULL, "NULL")) {
+        return 1;
+    }
     if (FLAGS_skps.isEmpty()) return 0;
 
     SkOSFile::Iter it(FLAGS_skps[0], ".skp");