Skeleton of DM

dm is like gm, but faster and with fewer features.

This is sort of the near-minimal proof-of-concept skeleton.

  - It can run existing GMs.
  - It supports most configs (just not PDF).
  - --replay is the only "fancy" feature it currently supports

Hopefully you will be disturbed by its speed.

BUG=
R=epoger@google.com

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

[mtklein, 2013-10-10 18:23:58]

You guys are the union of folks who's eyes lit up when I mentioned this and
those who usually seem to have something to say about GM.  I won't be offended
if you drop off, and feel free to add other eyes.

[bsalomon, 2013-10-10 21:13:06]

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode34
dm/DM.cpp:34: DEFINE_bool(565,      false, "Run 565?");
Hmm... most of our other tools work like

--config gpu 8888

rather than --gpu --8888

I'm not arguing against the latter, but rather for consistency.

https://codereview.chromium.org/22839016/diff/53001/dm/README
File dm/README (right):

https://codereview.chromium.org/22839016/diff/53001/dm/README#newcode9
dm/README:9: --deferred
We probably want most of these replay features. I'm not sure what
--mismatchPath, --missingExpectationsPath do. I doubt we'd miss
--writePicturePath initially (if ever).

[mtklein, 2013-10-10 22:32:19]

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode34
dm/DM.cpp:34: DEFINE_bool(565,      false, "Run 565?");
On 2013/10/10 21:13:07, bsalomon wrote:
> Hmm... most of our other tools work like
> 
> --config gpu 8888
> 
> rather than --gpu --8888
> 
> I'm not arguing against the latter, but rather for consistency.

Yeah, either way works.  This one was a bit of an intentional push.  My thought
was to let this style stew here and maybe get folks to see the light over time.

The things I like about doing it this way are that parsing and --help are free,
and especially it's that hard to make a silent typo.  I have trouble remembering
whether it's "--config GPU" or "--config gpu".

I guess I doubt we'd have written --config to work the way it does if we had
Leon's flags code from the start.

https://codereview.chromium.org/22839016/diff/53001/dm/README
File dm/README (right):

https://codereview.chromium.org/22839016/diff/53001/dm/README#newcode9
dm/README:9: --deferred
On 2013/10/10 21:13:07, bsalomon wrote:
> We probably want most of these replay features. I'm not sure what
> --mismatchPath, --missingExpectationsPath do. I doubt we'd miss
> --writePicturePath initially (if ever).

Roger roger.  Will come around and flesh these all out once I get a consensus on
the framework.

Looks to me that --mismatchPath and --missingExpecationsPath are pretty handy
when you're looking at this from a bot's perspective.  For now, I'm ignoring
bots as users, but one day something like that may show up here.

[epoger, 2013-10-11 17:23:23]

I haven't had a chance to review this yet, but I will definitely do so today. 
Thanks for including me.

[epoger, 2013-10-11 21:13:13]

Big picture: I like it!  Definitely a breath of fresh air compared to the giant
crufty gmmain.cpp

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode1
dm/DM.cpp:1: #include "GrContext.h"
High-level suggestion: Explore using the existing gm self-tests
(gm/tests/run.sh) to see how the behavior of this tool varies from gm.

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode22
dm/DM.cpp:22: DEFINE_string(expectations, "", "If set, check expectations
against files found at this path.");
"check expectations" -> "compare generated images"

More importantly, does this specify a JSON expectations file or a directory with
image files in it?

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode34
dm/DM.cpp:34: DEFINE_bool(565,      false, "Run 565?");
On 2013/10/10 22:32:19, mtklein wrote:
> On 2013/10/10 21:13:07, bsalomon wrote:
> > Hmm... most of our other tools work like
> > 
> > --config gpu 8888
> > 
> > rather than --gpu --8888
> > 
> > I'm not arguing against the latter, but rather for consistency.
> 
> Yeah, either way works.  This one was a bit of an intentional push.  My
thought
> was to let this style stew here and maybe get folks to see the light over
time.

Personally, I prefer "--configs gpu 8888" to "--gpu --8888".  Yes, there is more
implementation work, but:
1. looking at a command line that is being run, you can quickly tell which
arguments are config-related
2. the --help output can show the flags in alphabetical order (which is
generally useful) but you don't have config-related arguments spread out all
over the place in that --help output

In other words, I haven't seen the light yet.
 
> The things I like about doing it this way are that parsing and --help are
free,
> and especially it's that hard to make a silent typo.  I have trouble
remembering
> whether it's "--config GPU" or "--config gpu".

I think the answer here is code:

$ mycommand --config elmo

Error: unknown config 'elmo'. allowed configs are: bert, ernie

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode43
dm/DM.cpp:43: //DEFINE_bool(pdf,      false, "Run pdf?");  // TODO(mtklein)
Maybe use an #ifdef to disable PDF, so you don't need to comment/uncomment
things in sync

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode115
dm/DM.cpp:115: int main(int argc, char** argv) {
A few comments within here would be nice

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode129
dm/DM.cpp:129: expectationsPath += "/expected-results.json";
Why not just allow the user to pass the full path, including the filename?

https://codereview.chromium.org/22839016/diff/53001/dm/DMCpuTask.cpp
File dm/DMCpuTask.cpp (right):

https://codereview.chromium.org/22839016/diff/53001/dm/DMCpuTask.cpp#newcode38
dm/DMCpuTask.cpp:38: this->fail();
So, are different types of failures (no_comparison, failed, failure_ignored)
still tallied separately?

https://codereview.chromium.org/22839016/diff/53001/dm/DMCpuTask.cpp#newcode48
dm/DMCpuTask.cpp:48: if (SkBitmap::kRGB_565_Config == fConfig &&
(fGM->getFlags() & skiagm::GM::kSkip565_Flag)) {
Why is this the only config we check whether to skip?  Shouldn't it be a more
complete list of possible configs?

https://codereview.chromium.org/22839016/diff/53001/dm/DMTaskRunner.cpp
File dm/DMTaskRunner.cpp (right):

https://codereview.chromium.org/22839016/diff/53001/dm/DMTaskRunner.cpp#newco...
dm/DMTaskRunner.cpp:20: // These free calls block until the threadpool is done. 
We don't allow
Thanks for the commentary!

https://codereview.chromium.org/22839016/diff/53001/dm/README
File dm/README (right):

https://codereview.chromium.org/22839016/diff/53001/dm/README#newcode9
dm/README:9: --deferred
On 2013/10/10 22:32:19, mtklein wrote:
> Looks to me that --mismatchPath and --missingExpecationsPath are pretty handy
> when you're looking at this from a bot's perspective.  For now, I'm ignoring
> bots as users, but one day something like that may show up here.

Yup, we definitely need those on the bots.  Fine with me to focus on non-bot
users first, but we should generally aim to have developers run the same tools
that the bots do.

[bsalomon, 2013-10-11 21:18:57]

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode34
dm/DM.cpp:34: DEFINE_bool(565,      false, "Run 565?");
On 2013/10/11 21:13:13, epoger wrote:
> On 2013/10/10 22:32:19, mtklein wrote:
> > On 2013/10/10 21:13:07, bsalomon wrote:
> > > Hmm... most of our other tools work like
> > > 
> > > --config gpu 8888
> > > 
> > > rather than --gpu --8888
> > > 
> > > I'm not arguing against the latter, but rather for consistency.
> > 
> > Yeah, either way works.  This one was a bit of an intentional push.  My
> thought
> > was to let this style stew here and maybe get folks to see the light over
> time.
> 
> Personally, I prefer "--configs gpu 8888" to "--gpu --8888".  Yes, there is
more
> implementation work, but:
> 1. looking at a command line that is being run, you can quickly tell which
> arguments are config-related
> 2. the --help output can show the flags in alphabetical order (which is
> generally useful) but you don't have config-related arguments spread out all
> over the place in that --help output
> 
> In other words, I haven't seen the light yet.
>  
> > The things I like about doing it this way are that parsing and --help are
> free,
> > and especially it's that hard to make a silent typo.  I have trouble
> remembering
> > whether it's "--config GPU" or "--config gpu".
> 
> I think the answer here is code:
> 
> $ mycommand --config elmo
> 
> Error: unknown config 'elmo'. allowed configs are: bert, ernie

I'd just add that the gpu vs GPU naming is kind of a separate issue. We should
just change bench to use lower case config names (or maybe make it case
insensitive for a little bit of time to allow the bot scripts to be updated).

[scroggo, 2013-10-11 21:20:48]

+1 to short, specific files!

https://codereview.chromium.org/22839016/diff/53001/dm/DMTaskRunner.h
File dm/DMTaskRunner.h (right):

https://codereview.chromium.org/22839016/diff/53001/dm/DMTaskRunner.h#newcode24
dm/DMTaskRunner.h:24: SkAutoTDelete<SkThreadPool> fMain, fGpu;
Now that you've added wait() to SkThreadPool, you no longer need the
SkAutoTDelete.

https://codereview.chromium.org/22839016/diff/53001/dm/README
File dm/README (right):

https://codereview.chromium.org/22839016/diff/53001/dm/README#newcode11
dm/README:11: --pipe
Pipe is currently only used by deferred, so it might be redundant to run it as
well.

https://codereview.chromium.org/22839016/diff/53001/dm/README#newcode15
dm/README:15: --tiledPipe
tiledPipe is an experiment that we are not currently using/pursuing.

[mtklein, 2013-10-15 16:01:43]

reup

[mtklein, 2013-10-15 18:10:07]

reup

[mtklein, 2013-10-15 18:30:52]

I _think_ I addressed all comments.  Let me know if I missed something.

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode1
dm/DM.cpp:1: #include "GrContext.h"
On 2013/10/11 21:13:13, epoger wrote:
> High-level suggestion: Explore using the existing gm self-tests
> (gm/tests/run.sh) to see how the behavior of this tool varies from gm.

Uh, that's a nice idea but I'm explicitly not caring about this.  DM varies from
GM a lot.  I will happily add separate unit and system tests for DM; haven't
added them yet in case this review leads to major reorganization.

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode22
dm/DM.cpp:22: DEFINE_string(expectations, "", "If set, check expectations
against files found at this path.");
On 2013/10/11 21:13:13, epoger wrote:
> "check expectations" -> "compare generated images"
> 
> More importantly, does this specify a JSON expectations file or a directory
with
> image files in it?

Done.

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode34
dm/DM.cpp:34: DEFINE_bool(565,      false, "Run 565?");
On 2013/10/11 21:18:58, bsalomon wrote:
> On 2013/10/11 21:13:13, epoger wrote:
> > On 2013/10/10 22:32:19, mtklein wrote:
> > > On 2013/10/10 21:13:07, bsalomon wrote:
> > > > Hmm... most of our other tools work like
> > > > 
> > > > --config gpu 8888
> > > > 
> > > > rather than --gpu --8888
> > > > 
> > > > I'm not arguing against the latter, but rather for consistency.
> > > 
> > > Yeah, either way works.  This one was a bit of an intentional push.  My
> > thought
> > > was to let this style stew here and maybe get folks to see the light over
> > time.
> > 
> > Personally, I prefer "--configs gpu 8888" to "--gpu --8888".  Yes, there is
> more
> > implementation work, but:
> > 1. looking at a command line that is being run, you can quickly tell which
> > arguments are config-related
> > 2. the --help output can show the flags in alphabetical order (which is
> > generally useful) but you don't have config-related arguments spread out all
> > over the place in that --help output
> > 
> > In other words, I haven't seen the light yet.
> >  
> > > The things I like about doing it this way are that parsing and --help are
> > free,
> > > and especially it's that hard to make a silent typo.  I have trouble
> > remembering
> > > whether it's "--config GPU" or "--config gpu".
> > 
> > I think the answer here is code:
> > 
> > $ mycommand --config elmo
> > 
> > Error: unknown config 'elmo'. allowed configs are: bert, ernie
> 
> I'd just add that the gpu vs GPU naming is kind of a separate issue. We should
> just change bench to use lower case config names (or maybe make it case
> insensitive for a little bit of time to allow the bot scripts to be updated).

Okay.  I've switched to a single --config.  I've punted on defaults and ~ for
now.

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode43
dm/DM.cpp:43: //DEFINE_bool(pdf,      false, "Run pdf?");  // TODO(mtklein)
On 2013/10/11 21:13:13, epoger wrote:
> Maybe use an #ifdef to disable PDF, so you don't need to comment/uncomment
> things in sync

Thanks.  I think I'll be able to handle the comments.

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode115
dm/DM.cpp:115: int main(int argc, char** argv) {
On 2013/10/11 21:13:13, epoger wrote:
> A few comments within here would be nice

Added a note to the top.

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode129
dm/DM.cpp:129: expectationsPath += "/expected-results.json";
On 2013/10/11 21:13:13, epoger wrote:
> Why not just allow the user to pass the full path, including the filename?

Done.

https://codereview.chromium.org/22839016/diff/53001/dm/DMCpuTask.cpp
File dm/DMCpuTask.cpp (right):

https://codereview.chromium.org/22839016/diff/53001/dm/DMCpuTask.cpp#newcode38
dm/DMCpuTask.cpp:38: this->fail();
On 2013/10/11 21:13:13, epoger wrote:
> So, are different types of failures (no_comparison, failed, failure_ignored)
> still tallied separately?

Ah, good catch.  I intended to count failures but none of the rest, but I think
the no_comparisons were probably getting lumped in there with the failures. 
I've updated DMUtil.cpp's meetsExpectations to only fail on a non-ignored
non-degenerate mismatch.

https://codereview.chromium.org/22839016/diff/53001/dm/DMCpuTask.cpp#newcode48
dm/DMCpuTask.cpp:48: if (SkBitmap::kRGB_565_Config == fConfig &&
(fGM->getFlags() & skiagm::GM::kSkip565_Flag)) {
On 2013/10/11 21:13:13, epoger wrote:
> Why is this the only config we check whether to skip?  Shouldn't it be a more
> complete list of possible configs?

Hmm.  Looks like I was being a little cute here.  I've updated it to maybe be
clearer?

I think this is correct.  The only things we need to check are that we don't
want to run 565 when we say to skip it, and that we don't run GPU-only tasks. 
The other flags don't affect this task as far as I can tell.

https://codereview.chromium.org/22839016/diff/53001/dm/DMTaskRunner.h
File dm/DMTaskRunner.h (right):

https://codereview.chromium.org/22839016/diff/53001/dm/DMTaskRunner.h#newcode24
dm/DMTaskRunner.h:24: SkAutoTDelete<SkThreadPool> fMain, fGpu;
On 2013/10/11 21:20:48, scroggo wrote:
> Now that you've added wait() to SkThreadPool, you no longer need the
> SkAutoTDelete.

Absolutely.  Done.

https://codereview.chromium.org/22839016/diff/53001/dm/README
File dm/README (right):

https://codereview.chromium.org/22839016/diff/53001/dm/README#newcode11
dm/README:11: --pipe
On 2013/10/11 21:20:48, scroggo wrote:
> Pipe is currently only used by deferred, so it might be redundant to run it as
> well.

Great.  I will keep this in mind.

https://codereview.chromium.org/22839016/diff/53001/dm/README#newcode15
dm/README:15: --tiledPipe
On 2013/10/11 21:20:48, scroggo wrote:
> tiledPipe is an experiment that we are not currently using/pursuing.

Sweet.  Gone.

[epoger, 2013-10-15 18:52:37]

LGTM at patchset 21; a couple of suggested TODOs below.

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode1
dm/DM.cpp:1: #include "GrContext.h"
On 2013/10/15 18:30:53, mtklein wrote:
> On 2013/10/11 21:13:13, epoger wrote:
> > High-level suggestion: Explore using the existing gm self-tests
> > (gm/tests/run.sh) to see how the behavior of this tool varies from gm.
> 
> Uh, that's a nice idea but I'm explicitly not caring about this.  DM varies
from
> GM a lot.  

Well, at some point I suppose the goal is to switch our current uses of GM over
to DM instead.  Which I look forward to.  But before we can do that, we'll need
to know how this tool differs from the old one.

> I will happily add separate unit and system tests for DM; haven't
> added them yet in case this review leads to major reorganization.

That part makes sense.

https://codereview.chromium.org/22839016/diff/53001/dm/DMCpuTask.cpp
File dm/DMCpuTask.cpp (right):

https://codereview.chromium.org/22839016/diff/53001/dm/DMCpuTask.cpp#newcode48
dm/DMCpuTask.cpp:48: if (SkBitmap::kRGB_565_Config == fConfig &&
(fGM->getFlags() & skiagm::GM::kSkip565_Flag)) {
On 2013/10/15 18:30:53, mtklein wrote:
> On 2013/10/11 21:13:13, epoger wrote:
> > Why is this the only config we check whether to skip?  Shouldn't it be a
more
> > complete list of possible configs?
> 
> Hmm.  Looks like I was being a little cute here.  I've updated it to maybe be
> clearer?
> 
> I think this is correct.  The only things we need to check are that we don't
> want to run 565 when we say to skip it, and that we don't run GPU-only tasks. 
> The other flags don't affect this task as far as I can tell.

Easier to follow now, thanks.

https://codereview.chromium.org/22839016/diff/77001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/22839016/diff/77001/dm/DM.cpp#newcode43
dm/DM.cpp:43: static void split(const char* str, const char* delimiters,
SkTArray<SkString>* out) {
Is there somewhere we can add this function such that it will be (1) usable
generally and (2) unit-tested?  Like maybe in SkString?

(I'm fine for this to be a TODO; this CL is plenty big already)

https://codereview.chromium.org/22839016/diff/77001/dm/DM.cpp#newcode56
dm/DM.cpp:56: static SkString lowercase(SkString s) {
ditto

[bsalomon, 2013-10-15 18:58:54]

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode1
dm/DM.cpp:1: #include "GrContext.h"
On 2013/10/15 18:52:37, epoger wrote:
> On 2013/10/15 18:30:53, mtklein wrote:
> > I will happily add separate unit and system tests for DM; haven't
> > added them yet in case this review leads to major reorganization.
> 
> That part makes sense.
> 


When we get to that step, can we please make DM emit its unit-testable summary
of failures, passes, missing-expectations, etc as a json/xml/etc file rather
using stdout/stderr so that it is kept separate from human readable output?

[mtklein, 2013-10-15 19:42:31]

On 2013/10/15 18:58:54, bsalomon wrote:
> https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp
> File dm/DM.cpp (right):
> 
> https://codereview.chromium.org/22839016/diff/53001/dm/DM.cpp#newcode1
> dm/DM.cpp:1: #include "GrContext.h"
> On 2013/10/15 18:52:37, epoger wrote:
> > On 2013/10/15 18:30:53, mtklein wrote:
> > > I will happily add separate unit and system tests for DM; haven't
> > > added them yet in case this review leads to major reorganization.
> > 
> > That part makes sense.
> > 
> 
> 
> When we get to that step, can we please make DM emit its unit-testable summary
> of failures, passes, missing-expectations, etc as a json/xml/etc file rather
> using stdout/stderr so that it is kept separate from human readable output?

Absolutely.  My thought was to have whatever end-to-end test comes out of this
be built into the binary.  That way its test output can be completely disjoint
from its human-readable output.

[mtklein, 2013-10-15 19:46:44]

https://codereview.chromium.org/22839016/diff/77001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/22839016/diff/77001/dm/DM.cpp#newcode43
dm/DM.cpp:43: static void split(const char* str, const char* delimiters,
SkTArray<SkString>* out) {
On 2013/10/15 18:52:38, epoger wrote:
> Is there somewhere we can add this function such that it will be (1) usable
> generally and (2) unit-tested?  Like maybe in SkString?
> 
> (I'm fine for this to be a TODO; this CL is plenty big already)

Yeah, we could put this in SkString.  I'll definitely move this somewhere unit
testable once I start adding those.

https://codereview.chromium.org/22839016/diff/77001/dm/DM.cpp#newcode56
dm/DM.cpp:56: static SkString lowercase(SkString s) {
On 2013/10/15 18:52:38, epoger wrote:
> ditto

Ditto for moving to somewhere unittestable, but I'd hesitate to put this in
SkString.  Bringing the concept of case (and so locale, encoding, etc.) into
SkString seems like a big bag of worms.

[mtklein, 2013-10-16 13:02:22]

Committed patchset #22 manually as r11802 (presubmit successful).