Add config options to run different GPU APIs to dm and nanobench

tools/flags/SkCommonFlagsConfig.cpp

+/*
+ * Copyright 2015 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "SkCommonFlagsConfig.h"
+
+#include <stdlib.h>
+
+DEFINE_extended_string(
+    config,
+    "565 8888 gpu nonrendering"
+#if SK_ANGLE
+#if SK_BUILD_FOR_WIN
+    " angle"
+#endif
+#endif
+#ifdef SK_BUILD_FOR_ANDROID_FRAMEWORK
+    " hwui"
+#endif
+    , "Options: 565 8888 debug gpu gpudebug gpudft gpunull "
+    "msaa16 msaa4 nonrendering null nullgpu nvprmsaa16 nvprmsaa4 "
+    "pdf pdf_poppler skp svg xps"
+#if SK_ANGLE
+#if SK_BUILD_FOR_WIN
+    " angle"
+#endif
+    " angle-gl"
+#endif
+#if SK_COMMAND_BUFFER
+    " commandbuffer"
+#endif
+#if SK_MESA
+    " mesa"
+#endif
+#ifdef SK_BUILD_FOR_ANDROID_FRAMEWORK
+    " hwui"
+#endif
+    " or use extended form 'backend(option=value,...)'.\n",

[scroggo, 2015/12/03 19:02:58]

This code looks hard to maintain, especially when we get into all this
formatting. Might there be a better way to do it? e.g. build the options up
individually, rather than trying to put them all in the same string?

[Kimmo Kinnunen, 2015/12/04 14:26:19]

In theory, yes, it can be nicer. It could be either a dynamic string, or a chunk
of preprocessor macros.

Dynamic string has the downside that I would need to start writing yet more
support for command line flags, in form of running dynamic initializers of
extended help message strings.

Preprocessor spaghetti would mean few pages full of preprocessor macros that
transform submacros and do conditional substitutions. Typically non-trivial
macros are quite tedious to write and rarely readable.

I feel that however unmaintainable this code is, it's still better than the
current situation.  From this perspective, I'd rather not do any of the above.
Of course, I'm the author so the perspective might be skewed.

Also, given that my primary focus is not exactly in writing maximally
maintainable extended help message systems, I feel that I've done quite much
already in this domain. From this perspective also I'd rather like to leave this
as-is.

Of course, if cleaning this up further is a requirement for this patch to go in,
I'll do it. I have spent some time working on the prototype preprocessor
version, and I can spend few extra days fixing the loose ends on all the
different compile configs. If it's a must, should it be a dynamic string variant
or a macro thingy?

[scroggo, 2015/12/04 16:40:03]

No, it's not a requirement to land the patch.

+    "Extended form: 'backend(option=value,...)'\n\n"
+    "Possible backends and options:\n"
+#if SK_SUPPORT_GPU
+    "\n"
+    "gpu(api=string,dit=bool,nvpr=bool,samples=int)\tGPU backend\n"
+    "\tapi\ttype: string\tdefault: native.\n"
+    "\t    Select graphics API to use with gpu backend.\n"
+    "\t    Options:\n"
+    "\t\tnative\t\t\tUse platform default OpenGL or OpenGL ES backend.\n"
+    "\t\tgl    \t\t\tUse OpenGL.\n"
+    "\t\tgles  \t\t\tUse OpenGL ES.\n"
+    "\t\tdebug \t\t\tUse debug OpenGL.\n"
+    "\t\tnull  \t\t\tUse null OpenGL.\n"
+#if SK_ANGLE
+#if SK_BUILD_FOR_WIN
+    "\t\tangle\t\t\tUse ANGLE DirectX.\n"
+#endif
+    "\t\tangle-gl\t\t\tUse ANGLE OpenGL.\n"
+#endif
+#if SK_COMMAND_BUFFER
+    "\t\tcommandbuffer\t\tUse command buffer.\n"
+#endif
+#if SK_MESA
+    "\t\tmesa\t\t\tUse MESA.\n"
+#endif
+    "\tdit\ttype: bool\tdefault: false.\n"
+    "\t    Use device independent text.\n"
+    "\tnvpr\ttype: bool\tdefault: false.\n"
+    "\t    Use NV_path_rendering OpenGL and OpenGL ES extension.\n"
+    "\tsamples\ttype: int\tdefault: 0.\n"
+    "\t    Use multisampling with N samples.\n"
+    "\n"
+    "Predefined configs:\n\n"
+    "\tgpu      \t= gpu()\n"
+    "\tmsaa4    \t= gpu(samples=4)\n"
+    "\tmsaa16   \t= gpu(samples=16)\n"
+    "\tnvprmsaa4\t= gpu(nvpr=true,samples=4)\n"
+    "\tnvprmsaa16\t= gpu(nvpr=true,samples=16)\n"
+    "\tgpudft    \t= gpu(dit=true)\n"
+    "\tgpudebug  \t= gpu(api=debug)\n"
+    "\tgpunull   \t= gpu(api=null)\n"
+    "\tdebug     \t= gpu(api=debug)\n"
+    "\tnullgpu   \t= gpu(api=null)\n"
+#if SK_ANGLE
+#if SK_BUILD_FOR_WIN
+    "\tangle     \t= gpu(api=angle)\n"
+#endif
+    "\tangle-gl  \t= gpu(api=angle-gl)\n"
+#endif
+#if SK_COMMAND_BUFFER
+    "\tcommandbuffer\t= gpu(api=commandbuffer)\n"
+#endif
+#if SK_MESA
+    "\tmesa      \t= gpu(api=mesa)\n"
+#endif
+#endif
+    "");
+
+static const struct {
+    const char* predefinedConfig;
+    const char* backend;
+    const char* options;
+} gPredefinedConfigs[] = {
+#if SK_SUPPORT_GPU
+    "gpu",        "gpu", "",
+    "msaa4",      "gpu", "samples=4",
+    "msaa16",     "gpu", "samples=16",
+    "nvprmsaa4",  "gpu", "nvpr=true,samples=4",
+    "nvprmsaa16", "gpu", "nvpr=true,samples=16",
+    "gpudft",     "gpu", "dit=true",
+    "gpudebug",   "gpu", "api=debug",
+    "gpunull",    "gpu", "api=null",
+    "debug",      "gpu", "api=debug",
+    "nullgpu",    "gpu", "api=null",
+#if SK_ANGLE
+#if SK_BUILD_FOR_WIN
+    "angle",      "gpu", "api=angle",
+#endif
+    "angle-gl",   "gpu", "api=angle-gl",
+#endif
+#if SK_COMMAND_BUFFER
+    "commandbuffer", "gpu", "api=commandbuffer",
+#endif
+#if SK_MESA
+    "mesa", "gpu", "api=mesa",
+#endif
+#else
+    "", "", ""
+#endif
+};
+
+SkCommandLineConfig::SkCommandLineConfig(const SkString& tag, const SkTArray<SkString>& viaParts)
+        : fTag(tag)
+        , fViaParts(viaParts) {
+}
+SkCommandLineConfig::~SkCommandLineConfig() {
+}
+
+#if SK_SUPPORT_GPU
+SkCommandLineConfigGpu::SkCommandLineConfigGpu(
+    const SkString& tag, const SkTArray<SkString>& viaParts,
+    API api, bool useNVPR, bool useDIText, int samples)
+        : SkCommandLineConfig(tag, viaParts)
+        , fAPI(api)
+        , fUseNVPR(useNVPR)
+        , fUseDIText(useDIText)
+        , fSamples(samples) {
+}
+static bool parse_option_int(const SkString& value, int* outInt) {
+    if (value.isEmpty()) {
+        return false;
+    }
+    char* endptr = nullptr;
+    long intValue = strtol(value.c_str(), &endptr, 10);
+    if (*endptr != '\0') {
+        return false;
+    }
+    *outInt = static_cast<int>(intValue);
+    return true;
+}
+static bool parse_option_bool(const SkString& value, bool* outBool) {
+    if (value.equals("true")) {
+        *outBool = true;
+        return true;
+    } else if (value.equals("false")) {

[scroggo, 2015/12/03 19:02:59]

nit: Here and elsewhere - no need to use "else" if the prior condition returned.

[Kimmo Kinnunen, 2015/12/04 14:26:19]

Done.

+        *outBool = false;
+        return true;
+    }
+    return false;
+}
+static bool parse_option_gpu_api(const SkString& value, SkCommandLineConfigGpu::API* outAPI) {
+    if (value.equals("native")) {
+        *outAPI = SkCommandLineConfigGpu::kNative_API;
+        return true;
+    } else if (value.equals("gl")) {
+        *outAPI = SkCommandLineConfigGpu::kGL_API;
+        return true;
+    } else if (value.equals("gles")) {
+        *outAPI = SkCommandLineConfigGpu::kGLES_API;
+        return true;
+    } else if (value.equals("debug")) {
+        *outAPI = SkCommandLineConfigGpu::kDebug_API;
+        return true;
+    } else if (value.equals("null")) {
+        *outAPI = SkCommandLineConfigGpu::kNull_API;
+        return true;
+    }
+#if SK_ANGLE
+#if SK_BUILD_FOR_WIN
+    else if (value.equals("angle")) {
+        *outAPI = SkCommandLineConfigGpu::kANGLE_API;
+        return true;
+    }
+#endif
+    else if (value.equals("angle-gl")) {
+        *outAPI = SkCommandLineConfigGpu::kANGLE_GL_API;
+        return true;
+    }
+#endif
+#if SK_COMMAND_BUFFER
+    else if (value.equals("commandbuffer")) {
+        *outAPI = SkCommandLineConfigGpu::kCommandBuffer_API;
+        return true;
+    }
+#endif
+#if SK_MESA
+    else if (value.equals("mesa")) {
+        *outAPI = SkCommandLineConfigGpu::kMESA_API;
+        return true;
+    }
+#endif
+    return false;
+}
+
+SkCommandLineConfigGpu* parse_command_line_config_gpu(const SkString& tag, const SkTArray<SkString>& vias, const SkString& options) {

[scroggo, 2015/12/03 19:02:58]

100 chars

[Kimmo Kinnunen, 2015/12/04 14:26:19]

Done.

+    // Defaults for GPU backend.
+    bool seenAPI = false;
+    SkCommandLineConfigGpu::API api = SkCommandLineConfigGpu::kNative_API;
+    bool seenUseNVPR = false;
+    bool useNVPR = false;
+    bool seenUseDIText =false;
+    bool useDIText = false;
+    bool seenSamples = false;
+    int samples = 0;
+
+    if (options.endsWith(",") || options.startsWith(",")) {

[scroggo, 2015/12/03 19:02:59]

Why this check?

[Kimmo Kinnunen, 2015/12/04 14:26:19]

gpu(,api=gl)
gpu(api=gl,)
gpu(,,)

These look unusual, so user probably did mean something else, and thus these
should be errors.
Currently not possible to detect with SkString split routines.

[scroggo, 2015/12/04 16:40:03]

Maybe add a comment?

If a user does enter one of these, is an error message printed?

[Kimmo Kinnunen, 2015/12/07 09:26:48]

Would it be clearer if I just fixed the root cause, then? PTAL SkString.h/cpp
fixes.
Yeah, they behave in similar way as if user would pass any other incorrect or
unknown config. E.g. it would print out


out/Debug/dm --config  "gpu(,api=gles)" msaa4 "gpu(api=gles)" "gpu(api=gl)"
"gpu(samples=16)" -w a --verbose 

Skipping config gpu(,api=gles): Don't understand 'gpu(,api=gles)'.
475 srcs * 4 sinks + 523 tests == 2423 tasks

(  28/29  MB   2420) 60.3µs	unit test  PathOpsAngleFindSlop
(  28/29  MB   2421) 35.6µs	unit test  PathOpsAngleFindQuadEpsilon

...

Good that you asked, I had to fix this in DM.cpp, PTAL there too :)

[scroggo, 2015/12/07 13:27:51]

sgtm
Is that the break -> continue?

+        return nullptr;
+    }
+    SkTArray<SkString> optionParts;
+    SkStrSplit(options.c_str(), ",", &optionParts);
+    for (int i = 0; i < optionParts.count(); ++i) {
+        SkTArray<SkString> keyValueParts;
+        SkStrSplit(optionParts[i].c_str(), "=", &keyValueParts);
+        if (keyValueParts.count() != 2) {
+            return nullptr;
+        }
+        const SkString& key = keyValueParts[0];
+        const SkString& value = keyValueParts[1];
+        bool valueOk = false;
+        if (key.equals("api") && !seenAPI) {
+            valueOk = parse_option_gpu_api(value, &api);
+            seenAPI = true;
+        } else if (key.equals("nvpr") && !seenUseNVPR) {
+            valueOk = parse_option_bool(value, &useNVPR);
+            seenUseNVPR = true;
+        } else if (key.equals("dit") && !seenUseDIText) {
+            valueOk = parse_option_bool(value, &useDIText);
+            seenUseDIText = true;
+        } else if (key.equals("samples") && !seenSamples) {
+            valueOk = parse_option_int(value, &samples);
+            seenSamples = true;
+        }
+        if (!valueOk) {
+            return nullptr;
+        }
+    }
+    return new SkCommandLineConfigGpu(tag, vias, api, useNVPR, useDIText, samples);
+}
+#endif
+
+void ParseConfigs(const SkCommandLineFlags::StringArray& configs, SkCommandLineConfigArray* outResult) {

[scroggo, 2015/12/03 19:02:59]

100 chars

[Kimmo Kinnunen, 2015/12/04 14:26:19]

Done.

+    outResult->reset();
+    for (int i = 0; i < configs.count(); ++i) {
+        SkString extendedConfigBackend;
+        SkString extendedConfigOptions;
+        SkString tag(configs[i]);
+        SkTArray<SkString> vias;
+
+        SkTArray<SkString> parts;
+        SkStrSplit(tag.c_str(), "(", &parts);
+        if (parts.count() == 2) {
+            SkTArray<SkString> parts2;
+            SkStrSplit(parts[1].c_str(), ")", &parts2);
+            if (parts[1].endsWith(")") && parts2.count() == 1) {
+                SkStrSplit(parts[0].c_str(), "-", &vias);
+                if (vias.count()) {
+                    extendedConfigBackend = vias[vias.count() - 1];
+                    vias.pop_back();
+                } else {
+                    extendedConfigBackend = parts[0];
+                }
+                extendedConfigOptions = parts2[0];
+                tag.printf("%s(%s)", extendedConfigBackend.c_str(), extendedConfigOptions.c_str());
+            }
+        }
+        if (extendedConfigBackend.isEmpty()) {
+            SkStrSplit(tag.c_str(), "-", &vias);
+            if (vias.count()) {
+                tag = vias[vias.count() - 1];
+                vias.pop_back();
+            }
+            // Note: no #if SK_ANGLE: this is a special rule in the via-tag grammar.
+            if (vias.count() && tag.equals("gl") && vias[vias.count() - 1].equals("angle")) {
+                tag = "angle-gl";
+                vias.pop_back();
+            }
+        }
+
+        for (size_t j = 0;

[scroggo, 2015/12/03 19:02:59]

why not:

for (auto predefinedConfig : gPredefinedConfigs) {
  if (!extendedConfigBackend.isEmpty()) {
    break;
  }
  ...
}

[Kimmo Kinnunen, 2015/12/04 14:26:19]

Done.

+             j < SK_ARRAY_COUNT(gPredefinedConfigs) && extendedConfigBackend.isEmpty(); ++j) {
+            if (tag.equals(gPredefinedConfigs[j].predefinedConfig)) {
+                extendedConfigBackend = gPredefinedConfigs[j].backend;
+                extendedConfigOptions = gPredefinedConfigs[j].options;
+            }
+        }
+
+        SkCommandLineConfig* parsedConfig = nullptr;
+#if SK_SUPPORT_GPU
+        if (extendedConfigBackend.equals("gpu")) {
+            parsedConfig = parse_command_line_config_gpu(tag, vias, extendedConfigOptions);
+        }
+#endif
+        if (!parsedConfig) {
+            parsedConfig = new SkCommandLineConfig(tag, vias);
+        }
+        outResult->emplace_back(parsedConfig);
+    }
+}