Unify flag parsing in bench_ and render_pictures.

Created my own flag parser, based off of gflags.

Share common code between bench_ and render_ to set up the PictureRenderer.
Fix an include error in SkPictureRenderer.h.
Simplified parameter passing in render_pictures_main.
Switch to using an SkAutoTUnref for the PictureRenderer.
I also changed the input format somewhat, so the buildbots need to be updated as well: https://codereview.appspot.com/7441044/
Fixed a bug in PictureBenchmark where calling setTimeIndividualTiles(false) sets the member variable to true.
Removed setDeviceType from PictureBenchmark, since only the PictureRenderer needs to know which device type to use.

Some changes to the input format:
'--logPerIter' no longer takes a 1 or 0. Instead, '--logPerIter'
turns it on and '--nologPerIter' turns it off (with off as the
default). (Note that this is for bench_pictures; bench still uses the old format)

Change '--device' to '--config' and 'bitmap' to '8888' to be
the same as gm.

Requires '--r' before inputs (to match gm), though there can be multiple inputs following it.

Changed --enable-deferred-image-decoding (which no one uses but me yet anyway) to --deferImageDecoding, since the former is incompatible with the flag parser.

Changes to behavior:
Show a short error message on failure (rather than the explanation of all
flags).


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

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

[Leon, 2013-01-30 22:30:28]

Overall I like this change because it simplifies bench_pictures and
render_pictures, but it's not without its drawbacks. It changes the format for
some flags (requiring quotes for flags with multiple arguments, so we'll need to
modify the buildbots) and the default help message shows a lot of extraneous
info. --helppackage shows just the flags we care about (I haven't figured out if
there's a way to do this by default, although if we checked in our own version,
which would also help people who don't use gclient, we could modify it to show
--helppackage by default).

Robert, I changed --device to --config (with options 8888 and gpu) to match gm.
Is that what you had in mind?

Edison, I added you because you've added some of the flags to render_pictures,
and I wanted to make sure I didn't mess anything up.

Also, I'm happy to switch the other tools (gm, bench) to using gflags, but
wanted to make sure this is the right track first.

[robertphillips, 2013-01-31 16:27:39]

Leon - there still looks like there is still some pain involved with setting up
the parameters. Is it at least less pain then with the old system?

Otherwise, lgtm

https://codereview.appspot.com/7230053/diff/8002/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.appspot.com/7230053/diff/8002/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:349: 
SK_OVERRIDE

https://codereview.appspot.com/7230053/diff/8002/tools/PictureRenderingFlags.cpp
File tools/PictureRenderingFlags.cpp (right):

https://codereview.appspot.com/7230053/diff/8002/tools/PictureRenderingFlags....
tools/PictureRenderingFlags.cpp:14: 
Do we really need STL? The no-unwind warnings make me nervous.

https://codereview.appspot.com/7230053/diff/8002/tools/PictureRenderingFlags....
tools/PictureRenderingFlags.cpp:43: // to determine which modes to display.
why not fuse 'record' and 'rerecord'?

[edisonn, 2013-01-31 16:39:51]

LGTM with comments

https://codereview.appspot.com/7230053/diff/8002/tools/PictureRenderingFlags.cpp
File tools/PictureRenderingFlags.cpp (right):

https://codereview.appspot.com/7230053/diff/8002/tools/PictureRenderingFlags....
tools/PictureRenderingFlags.cpp:1: /*
nit, optional: probably an "svn cp render_picture_main.cpp
PictureRenderingFlags.h" could have show the code that was just copied.

https://codereview.appspot.com/7230053/diff/8002/tools/render_pictures_main.cpp
File tools/render_pictures_main.cpp (right):

https://codereview.appspot.com/7230053/diff/8002/tools/render_pictures_main.c...
tools/render_pictures_main.cpp:270: inputs->push_back(SkString(*argv));
will it use flags as inputs? or the flags are removed by the time we are here?

[EricB, 2013-01-31 17:14:31]

Tacking on a couple of comments...

https://codereview.appspot.com/7230053/diff/8002/gyp/gflags.gyp
File gyp/gflags.gyp (right):

https://codereview.appspot.com/7230053/diff/8002/gyp/gflags.gyp#newcode19
gyp/gflags.gyp:19: '../third_party/externals/gflags/src/gflags_completions.cc',
I'd suggest including the header files here as well.

https://codereview.appspot.com/7230053/diff/8002/tools/PictureRenderingFlags.cpp
File tools/PictureRenderingFlags.cpp (right):

https://codereview.appspot.com/7230053/diff/8002/tools/PictureRenderingFlags....
tools/PictureRenderingFlags.cpp:14: 
On 2013/01/31 16:27:39, robertphillips wrote:
> Do we really need STL? The no-unwind warnings make me nervous.

+1

[Leon, 2013-02-01 23:44:41]

> Leon - there still looks like there is still some pain 
> involved with setting up the parameters. Is it at least 
> less pain then with the old system?

Good question. I wish I could have completely avoided parsing (i.e. the function
tokenize), but it does make it easier for all variables that don't have multiple
arguments.

https://codereview.appspot.com/7230053/diff/8002/gyp/gflags.gyp
File gyp/gflags.gyp (right):

https://codereview.appspot.com/7230053/diff/8002/gyp/gflags.gyp#newcode19
gyp/gflags.gyp:19: '../third_party/externals/gflags/src/gflags_completions.cc',
On 2013/01/31 17:14:31, EricB wrote:
> I'd suggest including the header files here as well.

Done.

https://codereview.appspot.com/7230053/diff/8002/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.appspot.com/7230053/diff/8002/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:349: 
On 2013/01/31 16:27:39, robertphillips wrote:
> SK_OVERRIDE

Actually, this is the top level for this function, although I could put it on
PictureRenderer as well (which has the benefit that I could call it without
calling getTiledRenderer() first).

https://codereview.appspot.com/7230053/diff/8002/tools/PictureRenderingFlags.cpp
File tools/PictureRenderingFlags.cpp (right):

https://codereview.appspot.com/7230053/diff/8002/tools/PictureRenderingFlags....
tools/PictureRenderingFlags.cpp:1: /*
On 2013/01/31 16:39:51, edisonn wrote:
> nit, optional: probably an "svn cp render_picture_main.cpp
> PictureRenderingFlags.h" could have show the code that was just copied.

I use git, and there's probably a way to get the same behavior with git. Perhaps
if I had done a git cp...

I tried using git cl upload --similarity, which made it think that gflags.gyp
came from effects.gyp.

https://codereview.appspot.com/7230053/diff/8002/tools/PictureRenderingFlags....
tools/PictureRenderingFlags.cpp:14: 
On 2013/01/31 16:27:39, robertphillips wrote:
> Do we really need STL?

We certainly need <string> since gflags provides us with std::string. The others
could be avoided though.

> The no-unwind warnings make me nervous.

Running on linux, I do not see them. What generates them?

(On a side note, I see some from gflags itself, but that does not break our
build, does it?)

https://codereview.appspot.com/7230053/diff/8002/tools/PictureRenderingFlags....
tools/PictureRenderingFlags.cpp:43: // to determine which modes to display.
On 2013/01/31 16:27:39, robertphillips wrote:
> why not fuse 'record' and 'rerecord'?

Because they do different things.

https://codereview.appspot.com/7230053/diff/8002/tools/render_pictures_main.cpp
File tools/render_pictures_main.cpp (right):

https://codereview.appspot.com/7230053/diff/8002/tools/render_pictures_main.c...
tools/render_pictures_main.cpp:270: inputs->push_back(SkString(*argv));
On 2013/01/31 16:39:51, edisonn wrote:
> will it use flags as inputs? or the flags are removed by the time we are here?

The flags are removed. 

(In case you're curious, google::ParseCommandLineFlags has a boolean argument
which determines whether to remove the flags from the command line, and I make
the call with 'true'.)

[Leon, 2013-02-27 23:17:18]

New approach. Instead of using gflags, I wrote my own (simplified) version. This
was mostly because I was having trouble getting it to work on Windows, but it
also means we don't have to change the input format as much. There's no longer a
need to pass multiple arguments to a single flag (so "--mode tile 256 256" does
not need to be "--mode='tile 256 256'"). I did change the format so that the
input files need to be passed to a flag (which is consistent with everything
else, and made it easier for me to assume that all non-flag arguments following
a string flag go with that flag). I also changed numThreads back to multi, as it
was originally. Another advantage to the new approach is the help message only
shows the flags we care about.

https://codereview.appspot.com/7230053/diff/45001/tools/SkFlags.h
File tools/SkFlags.h (right):

https://codereview.appspot.com/7230053/diff/45001/tools/SkFlags.h#newcode331
tools/SkFlags.h:331: SkDebugf("double registering %s\n", name);
I'll remove this printf. I need to check on android to verify this block is
necessary.

[Leon, 2013-02-28 17:13:19]

https://codereview.appspot.com/7230053/diff/45001/tools/SkFlags.h
File tools/SkFlags.h (right):

https://codereview.appspot.com/7230053/diff/45001/tools/SkFlags.h#newcode331
tools/SkFlags.h:331: SkDebugf("double registering %s\n", name);
On 2013/02/27 23:17:18, Leon wrote:
> I'll remove this printf. I need to check on android to verify this block is
> necessary.

Done.

[EricB, 2013-02-28 20:20:34]

This LGTM, with a couple of suggestions about long and short options.  I'd be
more comfortable if another pair of eyeballs took a look as well.

https://codereview.appspot.com/7230053/diff/57002/tools/PictureRenderingFlags...
File tools/PictureRenderingFlags.cpp (right):

https://codereview.appspot.com/7230053/diff/57002/tools/PictureRenderingFlags...
tools/PictureRenderingFlags.cpp:46: // TODO: If bench_pictures and
render_pictures were two separate targets, we could use build flags
Not a bad idea.  I think bench_pictures and render_pictures have outgrown of
their "tools" status.

https://codereview.appspot.com/7230053/diff/57002/tools/PictureRenderingFlags.h
File tools/PictureRenderingFlags.h (right):

https://codereview.appspot.com/7230053/diff/57002/tools/PictureRenderingFlags...
tools/PictureRenderingFlags.h:19: kRender_PictureTool,
I assume this could go away as well, if bench_pictures and render_pictures were
different targets?

https://codereview.appspot.com/7230053/diff/57002/tools/SkFlags.h
File tools/SkFlags.h (right):

https://codereview.appspot.com/7230053/diff/57002/tools/SkFlags.h#newcode212
tools/SkFlags.h:212: if (SkStrStartsWith(string, '-')) {
I think the convention is to use one dash for short flags (single-character) and
two dashes for long.  Allowing use of either is fine though.

https://codereview.appspot.com/7230053/diff/57002/tools/render_pictures_main.cpp
File tools/render_pictures_main.cpp (right):

https://codereview.appspot.com/7230053/diff/57002/tools/render_pictures_main....
tools/render_pictures_main.cpp:30: DECLARE_string(i);
Doesn't GM use "-r" for reading the baselines?  We should either change this to
be "r" or change GM to use "-i" for consistency.

[Leon, 2013-03-01 01:54:19]

Justin, do you mind taking a look at this change as well?

https://codereview.appspot.com/7230053/diff/57002/tools/PictureRenderingFlags...
File tools/PictureRenderingFlags.cpp (right):

https://codereview.appspot.com/7230053/diff/57002/tools/PictureRenderingFlags...
tools/PictureRenderingFlags.cpp:46: // TODO: If bench_pictures and
render_pictures were two separate targets, we could use build flags
On 2013/02/28 20:20:35, EricB wrote:
> Not a bad idea.  I think bench_pictures and render_pictures have outgrown of
> their "tools" status.

Agreed. I've been thinking of moving them into their own directory, too, as
tools is rather big and unfocused. I lean towards making that a separate change
though, as this one is already pretty big.

https://codereview.appspot.com/7230053/diff/57002/tools/PictureRenderingFlags.h
File tools/PictureRenderingFlags.h (right):

https://codereview.appspot.com/7230053/diff/57002/tools/PictureRenderingFlags...
tools/PictureRenderingFlags.h:19: kRender_PictureTool,
On 2013/02/28 20:20:35, EricB wrote:
> I assume this could go away as well, if bench_pictures and render_pictures
were
> different targets?

I had not thought of that, but that is good thinking.

https://codereview.appspot.com/7230053/diff/57002/tools/SkFlags.h
File tools/SkFlags.h (right):

https://codereview.appspot.com/7230053/diff/57002/tools/SkFlags.h#newcode212
tools/SkFlags.h:212: if (SkStrStartsWith(string, '-')) {
On 2013/02/28 20:20:35, EricB wrote:
> I think the convention is to use one dash for short flags (single-character)
and
> two dashes for long.  Allowing use of either is fine though.

I believe that is correct, although gflags allows single and double dashes
interchangeably. I like that, since if the user enters the dashes by convention,
it works as expected, and it also works if the user is not quite right.

https://codereview.appspot.com/7230053/diff/57002/tools/render_pictures_main.cpp
File tools/render_pictures_main.cpp (right):

https://codereview.appspot.com/7230053/diff/57002/tools/render_pictures_main....
tools/render_pictures_main.cpp:30: DECLARE_string(i);
On 2013/02/28 20:20:35, EricB wrote:
> Doesn't GM use "-r" for reading the baselines?  We should either change this
to
> be "r" or change GM to use "-i" for consistency.

I did not realize that, but yes. I'm happy to use the same convention (we should
probably go with -r so we don't need to change the existing one). That actually
raises another question: whether we should provide an option of a shortened
flag. Perhaps I'll do that upon integration with tools that already have
shortened flags.

[EricB, 2013-03-01 13:41:36]

On 2013/03/01 01:54:19, Leon wrote:
> Justin, do you mind taking a look at this change as well?
> 
>
https://codereview.appspot.com/7230053/diff/57002/tools/PictureRenderingFlags...
> File tools/PictureRenderingFlags.cpp (right):
> 
>
https://codereview.appspot.com/7230053/diff/57002/tools/PictureRenderingFlags...
> tools/PictureRenderingFlags.cpp:46: // TODO: If bench_pictures and
> render_pictures were two separate targets, we could use build flags
> On 2013/02/28 20:20:35, EricB wrote:
> > Not a bad idea.  I think bench_pictures and render_pictures have outgrown of
> > their "tools" status.
> 
> Agreed. I've been thinking of moving them into their own directory, too, as
> tools is rather big and unfocused. I lean towards making that a separate
change
> though, as this one is already pretty big.
> 
>
https://codereview.appspot.com/7230053/diff/57002/tools/PictureRenderingFlags.h
> File tools/PictureRenderingFlags.h (right):
> 
>
https://codereview.appspot.com/7230053/diff/57002/tools/PictureRenderingFlags...
> tools/PictureRenderingFlags.h:19: kRender_PictureTool,
> On 2013/02/28 20:20:35, EricB wrote:
> > I assume this could go away as well, if bench_pictures and render_pictures
> were
> > different targets?
> 
> I had not thought of that, but that is good thinking.
> 
> https://codereview.appspot.com/7230053/diff/57002/tools/SkFlags.h
> File tools/SkFlags.h (right):
> 
> https://codereview.appspot.com/7230053/diff/57002/tools/SkFlags.h#newcode212
> tools/SkFlags.h:212: if (SkStrStartsWith(string, '-')) {
> On 2013/02/28 20:20:35, EricB wrote:
> > I think the convention is to use one dash for short flags (single-character)
> and
> > two dashes for long.  Allowing use of either is fine though.
> 
> I believe that is correct, although gflags allows single and double dashes
> interchangeably. I like that, since if the user enters the dashes by
convention,
> it works as expected, and it also works if the user is not quite right.
> 
>
https://codereview.appspot.com/7230053/diff/57002/tools/render_pictures_main.cpp
> File tools/render_pictures_main.cpp (right):
> 
>
https://codereview.appspot.com/7230053/diff/57002/tools/render_pictures_main....
> tools/render_pictures_main.cpp:30: DECLARE_string(i);
> On 2013/02/28 20:20:35, EricB wrote:
> > Doesn't GM use "-r" for reading the baselines?  We should either change this
> to
> > be "r" or change GM to use "-i" for consistency.
> 
> I did not realize that, but yes. I'm happy to use the same convention (we
should
> probably go with -r so we don't need to change the existing one). That
actually
> raises another question: whether we should provide an option of a shortened
> flag. Perhaps I'll do that upon integration with tools that already have
> shortened flags.

I think that's a nice-to-have but not necessary for this change.

[Leon, 2013-03-01 16:35:42]

Updated logPerIter in bench to behave the same as in bench_pictures, so
bench_pictures can inherit the flags from bench.

[robertphillips, 2013-03-02 17:50:01]

Partial (and outdated) review notes - sorry

https://codereview.appspot.com/7230053/diff/45001/tools/PictureRenderingFlags...
File tools/PictureRenderingFlags.cpp (right):

https://codereview.appspot.com/7230053/diff/45001/tools/PictureRenderingFlags...
tools/PictureRenderingFlags.cpp:35: "Has no effect if the provided skp does not
have its images encoded.");
i -> input?

https://codereview.appspot.com/7230053/diff/45001/tools/PictureRenderingFlags.h
File tools/PictureRenderingFlags.h (right):

https://codereview.appspot.com/7230053/diff/45001/tools/PictureRenderingFlags...
tools/PictureRenderingFlags.h:22: /**
Not gFlags any more

[Leon, 2013-03-04 16:19:02]

> Partial (and outdated) review notes - sorry

No problem, Robert. Thanks for taking a look!

https://codereview.appspot.com/7230053/diff/45001/tools/PictureRenderingFlags...
File tools/PictureRenderingFlags.cpp (right):

https://codereview.appspot.com/7230053/diff/45001/tools/PictureRenderingFlags...
tools/PictureRenderingFlags.cpp:35: "Has no effect if the provided skp does not
have its images encoded.");
On 2013/03/02 17:50:01, robertphillips wrote:
> i -> input?

At Eric's suggestion, I changed it to 'r', to match gm (which also allows
'readPath' - We decided to wait for a further change to allow both a single
character and a word to represent the same flag).

https://codereview.appspot.com/7230053/diff/45001/tools/PictureRenderingFlags.h
File tools/PictureRenderingFlags.h (right):

https://codereview.appspot.com/7230053/diff/45001/tools/PictureRenderingFlags...
tools/PictureRenderingFlags.h:22: /**
On 2013/03/02 17:50:01, robertphillips wrote:
> Not gFlags any more

Fixed in the latest patch.

[Leon, 2013-03-04 16:41:29]

Committed patchset #12 manually as r7961 (presubmit successful).