Skeleton of DM

dm/DM.cpp

+#include "GrContext.h"

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

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.

[mtklein, 2013/10/15 18:30:53]

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.

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

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.
That part makes sense.

[bsalomon, 2013/10/15 18:58:55]

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?

+#include "GrContextFactory.h"
+#include "SkCommandLineFlags.h"
+#include "SkForceLinking.h"
+#include "SkGraphics.h"
+#include "gm.h"
+
+#include "DMReporter.h"
+#include "DMTask.h"
+#include "DMTaskRunner.h"
+#include "DMCpuTask.h"
+#include "DMGpuTask.h"
+
+using skiagm::GM;
+using skiagm::GMRegistry;
+using skiagm::Expectations;
+using skiagm::ExpectationsSource;
+using skiagm::JsonExpectationsSource;
+
+DEFINE_int32(cpuThreads, -1, "Threads for CPU work. Default NUM_CPUS.");
+DEFINE_int32(gpuThreads, 1, "Threads for GPU work.");
+DEFINE_string(expectations, "", "If set, check expectations against files found at this path.");

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

"check expectations" -> "compare generated images"

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

[mtklein, 2013/10/15 18:30:53]

Done.

+DEFINE_string(resources, "resources", "Path to resources directory.");
+DEFINE_bool(verbose, false, "Print more.");
+DEFINE_string(match, "",  "[~][^]substring[$] [...] of GM name to run.\n"
+                          "Multiple matches may be separated by spaces.\n"
+                          "~ causes a matching GM to always be skipped\n"
+                          "^ requires the start of the GM to match\n"
+                          "$ requires the end of the GM to match\n"
+                          "^ and $ requires an exact match\n"
+                          "If a GM does not match any list entry,\n"
+                          "it is skipped unless some list entry starts with ~\n");
+
+DEFINE_bool(565,      false, "Run 565?");

[bsalomon, 2013/10/10 21:13:07]

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.

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

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.

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

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.
 
I think the answer here is code:

$ mycommand --config elmo

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

[bsalomon, 2013/10/11 21:18:58]

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).

[mtklein, 2013/10/15 18:30:53]

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

+DEFINE_bool(8888,     true,  "Run 8888?");
+DEFINE_bool(gpu,      true,  "Run gpu?");
+DEFINE_bool(msaa4,    false, "Run msaa4?");
+DEFINE_bool(msaa16,   false, "Run msaa16?");
+DEFINE_bool(gpunull,  false, "Run gpunull?");
+DEFINE_bool(gpudebug, false, "Run gpudebug?");
+DEFINE_bool(angle,    false, "Run angle?");
+DEFINE_bool(mesa,     false, "Run mesa?");
+//DEFINE_bool(pdf,      false, "Run pdf?");  // TODO(mtklein)

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

Maybe use an #ifdef to disable PDF, so you don't need to comment/uncomment
things in sync

[mtklein, 2013/10/15 18:30:53]

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

+
+__SK_FORCE_IMAGE_DECODER_LINKING;
+
+void KickOffTasks(const SkTDArray<GMRegistry::Factory>& gms,
+                  const ExpectationsSource& expectations,
+                  DM::Reporter* reporter,
+                  DM::TaskRunner* tasks) {
+    const SkBitmap::Config _565 = SkBitmap::kRGB_565_Config;
+    const SkBitmap::Config _8888 = SkBitmap::kARGB_8888_Config;
+    const GrContextFactory::GLContextType native = GrContextFactory::kNative_GLContextType;
+    const GrContextFactory::GLContextType null   = GrContextFactory::kNull_GLContextType;
+    const GrContextFactory::GLContextType debug  = GrContextFactory::kDebug_GLContextType;
+    const GrContextFactory::GLContextType angle  =
+    #if SK_ANGLE
+        GrContextFactory::kANGLE_GLContextType;
+    #else
+        native;
+    #endif
+    const GrContextFactory::GLContextType mesa   =
+    #if SK_MESA
+        GLContextFactory::kMESA_GLContextType;
+    #else
+        native;
+    #endif
+
+#define START(name, type, ...)                                                     \
+    if (FLAGS_##name) {                                                            \
+        tasks->add(SkNEW_ARGS(DM::type,                                            \
+                    (#name, reporter, tasks, expectations, gms[i], __VA_ARGS__))); \
+    }
+
+    for (int i = 0; i < gms.count(); i++) {
+        SkAutoTDelete<GM> gmForName(gms[i](NULL));
+        if (SkCommandLineFlags::ShouldSkip(FLAGS_match, gmForName->shortName())) continue;
+
+        START(565,      CpuTask, _565);
+        START(8888,     CpuTask, _8888);
+        START(gpu,      GpuTask, _8888, native, 0);
+        START(msaa4,    GpuTask, _8888, native, 4);
+        START(msaa16,   GpuTask, _8888, native, 16);
+        START(gpunull,  GpuTask, _8888, null,   0);
+        START(gpudebug, GpuTask, _8888, debug,  0);
+        START(angle,    GpuTask, _8888, angle,  0);
+        START(mesa,     GpuTask, _8888, mesa,   0);
+        //START(pdf,      PdfTask, _8888);
+    }
+#undef START
+}
+
+void ReportFailures(const DM::Reporter& reporter) {
+    SkTArray<SkString> failures;
+    reporter.getFailures(&failures);
+
+    if (failures.count() == 0) {
+        return;
+    }
+
+    SkDebugf("Failures:\n");
+    for (int i = 0; i < failures.count(); i++) {
+        SkDebugf("  %s\n", failures[i].c_str());
+    }
+}
+
+class NoExpectations : public ExpectationsSource {
+public:
+    Expectations get(const char* /*testName*/) const SK_OVERRIDE {
+        return Expectations(true /*ignore failures*/);
+    }
+};
+
+
+int main(int argc, char** argv) {

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

A few comments within here would be nice

[mtklein, 2013/10/15 18:30:53]

Added a note to the top.

+    SkGraphics::Init();
+    SkCommandLineFlags::Parse(argc, argv);
+    GM::SetResourcePath(FLAGS_resources[0]);
+
+    SkTDArray<GMRegistry::Factory> gms;
+    for (const GMRegistry* reg = GMRegistry::Head(); reg != NULL; reg = reg->next()) {
+        *gms.append() = reg->factory();
+    }
+    SkDebugf("%d GMs\n", gms.count());
+
+    SkAutoTUnref<ExpectationsSource> expectations(SkNEW(NoExpectations));
+    if (FLAGS_expectations.count() > 0) {
+        SkString expectationsPath(FLAGS_expectations[0]);
+        expectationsPath += "/expected-results.json";

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

Why not just allow the user to pass the full path, including the filename?

[mtklein, 2013/10/15 18:30:53]

Done.

+        expectations.reset(SkNEW_ARGS(JsonExpectationsSource, (expectationsPath.c_str())));
+    }
+
+    DM::Reporter reporter;
+    DM::TaskRunner tasks(FLAGS_cpuThreads, FLAGS_gpuThreads);
+    KickOffTasks(gms, *expectations, &reporter, &tasks);
+    tasks.wait();
+
+    reporter.UpdateStatusLine();
+    SkDebugf("\n");
+
+    if (FLAGS_verbose) {
+        ReportFailures(reporter);
+    }
+
+    SkGraphics::Term();
+
+    return reporter.failed() > 0;
+}