Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1490113005:
Add config options to run different GPU APIs to dm and nanobench (Closed)
|
Created:
5 years ago by Kimmo Kinnunen Modified:
4 years, 12 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@commandbuffer-as-api-03-context-factory-glcontext-type Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAdd config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
Committed: https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1490113005
Committed: https://skia.googlesource.com/skia/+/c8b4336444e7b90382e04e33665fb3b8490b825b
Committed: https://skia.googlesource.com/skia/+/9ebc3f0ee6db215dde461dc4777d85988cf272dd
Committed: https://skia.googlesource.com/skia/+/3e980c3d88fbc509b79e7ccef16ca38f5bbfb180
Patch Set 1 #
Total comments: 5
Patch Set 2 : win angle #
Total comments: 36
Patch Set 3 : address review comments #Patch Set 4 : address review comments #Patch Set 5 : fix errorneous config handling #
Total comments: 10
Patch Set 6 : add a missing hunk #Patch Set 7 : address review comments #Patch Set 8 : win fixes #Patch Set 9 : rebase #Patch Set 10 : fix clang compile errors #Patch Set 11 : msvc fixes #Patch Set 12 : msvc x86_64 fixes #Patch Set 13 : fix chromium build #
Total comments: 1
Patch Set 14 : address review comment #Patch Set 15 : #Patch Set 16 : #
Total comments: 3
Patch Set 17 : rebase for merge #Patch Set 18 : unused param warnding #Patch Set 19 : make tag be always the full --config word #Patch Set 20 : rebase #Patch Set 21 : #Patch Set 22 : remove context iteration from GLProgramsTest #Patch Set 23 : #
Messages
Total messages: 106 (38 generated)
kkinnunen@nvidia.com changed reviewers: + bsalomon@google.com
So here's an option how the command buffer can be selected with NVPR. Feel free to re-specify the config format. I can then incorporate the spec to the patch.
bsalomon@google.com changed reviewers: + joshualitt@google.com, scroggo@google.com
I like this direction a lot. The current set of predefined configs is too rigid. +Leon for command flags stuff +Josh for another set of GPU eyes https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp#newcode615 dm/DM.cpp:615: contextType = GrContextFactory::kANGLE_GL_GLContextType; do we need this conversion? Could GrContextFactory work with command line enum? Or vice-versa, use the grcontextfactory enum in place of the SkCommandLineConfigGpu? https://codereview.chromium.org/1490113005/diff/1/tests/TestConfigParsing.cpp File tests/TestConfigParsing.cpp (right): https://codereview.chromium.org/1490113005/diff/1/tests/TestConfigParsing.cpp... tests/TestConfigParsing.cpp:12: // Parses a normal config and returs correct "tag". returns
On 2015/12/02 23:22:42, bsalomon wrote: > I like this direction a lot. The current set of predefined configs is too rigid. > > +Leon for command flags stuff > +Josh for another set of GPU eyes > > https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp > File dm/DM.cpp (right): > > https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp#newcode615 > dm/DM.cpp:615: contextType = GrContextFactory::kANGLE_GL_GLContextType; > do we need this conversion? Could GrContextFactory work with command line enum? > Or vice-versa, use the grcontextfactory enum in place of the > SkCommandLineConfigGpu? > > https://codereview.chromium.org/1490113005/diff/1/tests/TestConfigParsing.cpp > File tests/TestConfigParsing.cpp (right): > > https://codereview.chromium.org/1490113005/diff/1/tests/TestConfigParsing.cpp... > tests/TestConfigParsing.cpp:12: // Parses a normal config and returs correct > "tag". > returns This is a good change, modulo Brian's comments. Looks good.
https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... File tools/flags/SkCommandLineFlags.cpp (right): https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... tools/flags/SkCommandLineFlags.cpp:27: SkFlagInfo* info = new SkFlagInfo(name, shortName, kString_FlagType, helpString, extendedHelpString); 100 chars. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... tools/flags/SkCommandLineFlags.cpp:162: // Only one line length's worth of text left. This comment no longer applies. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... tools/flags/SkCommandLineFlags.cpp:261: SkDebugf(" Use '--help %s' for more information.\n", allFlags[i]->name().c_str()); 100 chars https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... File tools/flags/SkCommandLineFlags.h (right): https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... tools/flags/SkCommandLineFlags.h:299: const char* extendedHelpString); Can you add documentation for extendedHelpString? https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... tools/flags/SkCommandLineFlags.h:434: SkFlagInfo(const char* name, const char* shortName, FlagTypes type, const char* helpString, const char* extendedHelpString) I think this is over 100 chars. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... File tools/flags/SkCommonFlagsConfig.cpp (right): https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.cpp:41: " or use extended form 'backend(option=value,...)'.\n", 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? https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.cpp:166: } else if (value.equals("false")) { nit: Here and elsewhere - no need to use "else" if the prior condition returned. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.cpp:216: SkCommandLineConfigGpu* parse_command_line_config_gpu(const SkString& tag, const SkTArray<SkString>& vias, const SkString& options) { 100 chars https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.cpp:227: if (options.endsWith(",") || options.startsWith(",")) { Why this check? https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.cpp:262: void ParseConfigs(const SkCommandLineFlags::StringArray& configs, SkCommandLineConfigArray* outResult) { 100 chars https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.cpp:300: for (size_t j = 0; why not: for (auto predefinedConfig : gPredefinedConfigs) { if (!extendedConfigBackend.isEmpty()) { break; } ... } https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... File tools/flags/SkCommonFlagsConfig.h (right): https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.h:15: class SkCommandLineConfigGpu; Should this be in #if SK_SUPPORT_GPU? https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.h:16: // SkCommandLineConfig represents a Skia rendering configuration string. nit: newline before comment block https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.h:62: SkCommandLineConfigGpu(const SkString& tag, const SkTArray<SkString>& viaParts, API api, bool useNVPR, bool useDIText, int samples); 100 chars https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.h:78: void ParseConfigs(const SkCommandLineFlags::StringArray& configList, SkCommandLineConfigArray* outResult); 100 chars
Sorry for the line ends. I try to configure emacs, yet again. https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp#newcode615 dm/DM.cpp:615: contextType = GrContextFactory::kANGLE_GL_GLContextType; On 2015/12/02 23:22:42, bsalomon wrote: > do we need this conversion? Could GrContextFactory work with command line enum? > Or vice-versa, use the grcontextfactory enum in place of the > SkCommandLineConfigGpu? The idea is that these are different things. GrContextFactory::GLContextType represents contexts that GrContextFactory understands. SkCommandLineConfigGpu::API represents the possible values that the parsing is implemented for. Most likely 100% of the time (hopefully?) these are exactly the same. Still, they're a bit different concept. Similarly to "enableNVPR". The SkCommandLineConfigGpu does not refer to "ContextOptions", because "context option" is not a concept of the (user-visible) command line tools options. In practice, it would be confusing to refer from SkCommandLineConfigGpu to GrContextFactory, because they're in different libraries. For example, visualbench does not "use" GrContextFactory, where as it could use the config parsing in the future. Naturally, this being an enum the reference would work. It'd be just confusing. I don't know if it makes a practical difference. For me, there's no upside, since the switch has already been written. I don't know if this is the most important place to start removing apparent redundancy. Of course, I'm not saying that would be a good rationale for not removing. Given the above, feel free to make a decision, I'll act accordingly. https://codereview.chromium.org/1490113005/diff/1/tests/TestConfigParsing.cpp File tests/TestConfigParsing.cpp (right): https://codereview.chromium.org/1490113005/diff/1/tests/TestConfigParsing.cpp... tests/TestConfigParsing.cpp:12: // Parses a normal config and returs correct "tag". On 2015/12/02 23:22:42, bsalomon wrote: > returns Done. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... File tools/flags/SkCommandLineFlags.cpp (right): https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... tools/flags/SkCommandLineFlags.cpp:27: SkFlagInfo* info = new SkFlagInfo(name, shortName, kString_FlagType, helpString, extendedHelpString); On 2015/12/03 19:02:58, scroggo wrote: > 100 chars. Done. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... tools/flags/SkCommandLineFlags.cpp:162: // Only one line length's worth of text left. On 2015/12/03 19:02:58, scroggo wrote: > This comment no longer applies. Done. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... tools/flags/SkCommandLineFlags.cpp:261: SkDebugf(" Use '--help %s' for more information.\n", allFlags[i]->name().c_str()); On 2015/12/03 19:02:58, scroggo wrote: > 100 chars Done. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... File tools/flags/SkCommandLineFlags.h (right): https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... tools/flags/SkCommandLineFlags.h:299: const char* extendedHelpString); On 2015/12/03 19:02:58, scroggo wrote: > Can you add documentation for extendedHelpString? Done. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... tools/flags/SkCommandLineFlags.h:434: SkFlagInfo(const char* name, const char* shortName, FlagTypes type, const char* helpString, const char* extendedHelpString) On 2015/12/03 19:02:58, scroggo wrote: > I think this is over 100 chars. Done. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... File tools/flags/SkCommonFlagsConfig.cpp (right): https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.cpp:41: " or use extended form 'backend(option=value,...)'.\n", On 2015/12/03 19:02:58, scroggo wrote: > 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? 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? https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.cpp:166: } else if (value.equals("false")) { On 2015/12/03 19:02:59, scroggo wrote: > nit: Here and elsewhere - no need to use "else" if the prior condition returned. Done. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.cpp:216: SkCommandLineConfigGpu* parse_command_line_config_gpu(const SkString& tag, const SkTArray<SkString>& vias, const SkString& options) { On 2015/12/03 19:02:58, scroggo wrote: > 100 chars Done. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.cpp:227: if (options.endsWith(",") || options.startsWith(",")) { On 2015/12/03 19:02:59, scroggo wrote: > Why this check? 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. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.cpp:262: void ParseConfigs(const SkCommandLineFlags::StringArray& configs, SkCommandLineConfigArray* outResult) { On 2015/12/03 19:02:59, scroggo wrote: > 100 chars Done. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.cpp:300: for (size_t j = 0; On 2015/12/03 19:02:59, scroggo wrote: > why not: > > for (auto predefinedConfig : gPredefinedConfigs) { > if (!extendedConfigBackend.isEmpty()) { > break; > } > ... > } Done. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... File tools/flags/SkCommonFlagsConfig.h (right): https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.h:15: class SkCommandLineConfigGpu; On 2015/12/03 19:02:59, scroggo wrote: > Should this be in #if SK_SUPPORT_GPU? Done. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.h:16: // SkCommandLineConfig represents a Skia rendering configuration string. On 2015/12/03 19:02:59, scroggo wrote: > nit: newline before comment block Done. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.h:62: SkCommandLineConfigGpu(const SkString& tag, const SkTArray<SkString>& viaParts, API api, bool useNVPR, bool useDIText, int samples); On 2015/12/03 19:02:59, scroggo wrote: > 100 chars Done. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.h:78: void ParseConfigs(const SkCommandLineFlags::StringArray& configList, SkCommandLineConfigArray* outResult); On 2015/12/03 19:02:59, scroggo wrote: > 100 chars Done.
1gtm for the flags changes https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... File tools/flags/SkCommandLineFlags.h (right): https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... tools/flags/SkCommandLineFlags.h:434: SkFlagInfo(const char* name, const char* shortName, FlagTypes type, const char* helpString, const char* extendedHelpString) On 2015/12/04 14:26:19, Kimmo Kinnunen wrote: > On 2015/12/03 19:02:58, scroggo wrote: > > I think this is over 100 chars. > > Done. Hmm, did you forget to upload? This does not appear to be fixed in patch set 3. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... File tools/flags/SkCommonFlagsConfig.cpp (right): https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.cpp:41: " or use extended form 'backend(option=value,...)'.\n", On 2015/12/04 14:26:19, Kimmo Kinnunen wrote: > On 2015/12/03 19:02:58, scroggo wrote: > > 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? > > 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? No, it's not a requirement to land the patch. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.cpp:227: if (options.endsWith(",") || options.startsWith(",")) { On 2015/12/04 14:26:19, Kimmo Kinnunen wrote: > On 2015/12/03 19:02:59, scroggo wrote: > > Why this check? > > 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. Maybe add a comment? If a user does enter one of these, is an error message printed?
https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... File tools/flags/SkCommandLineFlags.h (right): https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommandLi... tools/flags/SkCommandLineFlags.h:434: SkFlagInfo(const char* name, const char* shortName, FlagTypes type, const char* helpString, const char* extendedHelpString) On 2015/12/04 16:40:02, scroggo wrote: > On 2015/12/04 14:26:19, Kimmo Kinnunen wrote: > > On 2015/12/03 19:02:58, scroggo wrote: > > > I think this is over 100 chars. > > > > Done. > > Hmm, did you forget to upload? This does not appear to be fixed in patch set 3. I just somehow missed that. Sorry. Now it's done. https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... File tools/flags/SkCommonFlagsConfig.cpp (right): https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.cpp:227: if (options.endsWith(",") || options.startsWith(",")) { On 2015/12/04 16:40:03, scroggo wrote: > On 2015/12/04 14:26:19, Kimmo Kinnunen wrote: > > On 2015/12/03 19:02:59, scroggo wrote: > > > Why this check? > > > > 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. > > Maybe add a comment? Would it be clearer if I just fixed the root cause, then? PTAL SkString.h/cpp fixes. > If a user does enter one of these, is an error message printed? 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 :) https://codereview.chromium.org/1490113005/diff/80001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1490113005/diff/80001/dm/DM.cpp#newcode687 dm/DM.cpp:687: continue; Changed this ( break -> continue ) https://codereview.chromium.org/1490113005/diff/80001/include/core/SkString.h File include/core/SkString.h (left): https://codereview.chromium.org/1490113005/diff/80001/include/core/SkString.h... include/core/SkString.h:268: } Added these (copied chromium base/strings/string_split.h api)
https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... File tools/flags/SkCommonFlagsConfig.cpp (right): https://codereview.chromium.org/1490113005/diff/20001/tools/flags/SkCommonFla... tools/flags/SkCommonFlagsConfig.cpp:227: if (options.endsWith(",") || options.startsWith(",")) { On 2015/12/07 09:26:48, Kimmo Kinnunen wrote: > On 2015/12/04 16:40:03, scroggo wrote: > > On 2015/12/04 14:26:19, Kimmo Kinnunen wrote: > > > On 2015/12/03 19:02:59, scroggo wrote: > > > > Why this check? > > > > > > 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. > > > > Maybe add a comment? > > Would it be clearer if I just fixed the root cause, then? PTAL SkString.h/cpp > fixes. sgtm > > > If a user does enter one of these, is an error message printed? > > 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 :) Is that the break -> continue? https://codereview.chromium.org/1490113005/diff/80001/include/core/SkString.h File include/core/SkString.h (right): https://codereview.chromium.org/1490113005/diff/80001/include/core/SkString.h... include/core/SkString.h:270: enum SkStrSplitResult { Naming this "Result" makes me think this will be the return value of SkStrSplit. Instead, maybe this should be named SkStrSplitMode? https://codereview.chromium.org/1490113005/diff/80001/include/core/SkString.h... include/core/SkString.h:273: kAll_SkStrSplitResult, I don't find these names to be very descriptive. Better names might be in the comments, though. Something about "strict" and "coalesce"? https://codereview.chromium.org/1490113005/diff/80001/src/core/SkString.cpp File src/core/SkString.cpp (right): https://codereview.chromium.org/1490113005/diff/80001/src/core/SkString.cpp#n... src/core/SkString.cpp:629: if (resultBehavior != kAll_SkStrSplitResult) { Why not if (resultBehavior == kNonEmpty_SkStrSplitResult) This makes it seem like there are multiple resultBehaviors that should hit this case. https://codereview.chromium.org/1490113005/diff/80001/tests/StringTest.cpp File tests/StringTest.cpp (right): https://codereview.chromium.org/1490113005/diff/80001/tests/StringTest.cpp#ne... tests/StringTest.cpp:204: REPORTER_ASSERT(r, results.count() == 0); Is it okay that this default behavior changed? Might anyone be relying on the old behavior?
https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp#newcode615 dm/DM.cpp:615: contextType = GrContextFactory::kANGLE_GL_GLContextType; On 2015/12/04 14:26:18, Kimmo Kinnunen wrote: > On 2015/12/02 23:22:42, bsalomon wrote: > > do we need this conversion? Could GrContextFactory work with command line > enum? > > Or vice-versa, use the grcontextfactory enum in place of the > > SkCommandLineConfigGpu? > > The idea is that these are different things. GrContextFactory::GLContextType > represents contexts that GrContextFactory understands. > SkCommandLineConfigGpu::API represents the possible values that the parsing is > implemented for. Most likely 100% of the time (hopefully?) these are exactly the > same. Still, they're a bit different concept. > > Similarly to "enableNVPR". The SkCommandLineConfigGpu does not refer to > "ContextOptions", because "context option" is not a concept of the > (user-visible) command line tools options. > > In practice, it would be confusing to refer from SkCommandLineConfigGpu to > GrContextFactory, because they're in different libraries. For example, > visualbench does not "use" GrContextFactory, where as it could use the config > parsing in the future. Naturally, this being an enum the reference would work. > It'd be just confusing. > > I don't know if it makes a practical difference. For me, there's no upside, > since the switch has already been written. I don't know if this is the most > important place to start removing apparent redundancy. Of course, I'm not saying > that would be a good rationale for not removing. > > Given the above, feel free to make a decision, I'll act accordingly. GrContextFactory really only exists for our test tools and it should only enumerate the context types that we want to test. We don't necessarily have to leave the enum in GrContextFactory.The enum could live somewhere in tools/. Actually GrContextFactory could live there too as it isn't really part of the gpu target proper, but rather a building block for writing test tools.
https://codereview.chromium.org/1490113005/diff/80001/include/core/SkString.h File include/core/SkString.h (right): https://codereview.chromium.org/1490113005/diff/80001/include/core/SkString.h... include/core/SkString.h:270: enum SkStrSplitResult { On 2015/12/07 13:27:52, scroggo wrote: > Naming this "Result" makes me think this will be the return value of SkStrSplit. > Instead, maybe this should be named SkStrSplitMode? Done. https://codereview.chromium.org/1490113005/diff/80001/include/core/SkString.h... include/core/SkString.h:273: kAll_SkStrSplitResult, On 2015/12/07 13:27:52, scroggo wrote: > I don't find these names to be very descriptive. Better names might be in the > comments, though. Something about "strict" and "coalesce"? I changed them to your suggestions. I don't necessarily agree to the change. From the call site, "strict" and "coalesce" talk about how the thing operates. "All" and "non-empty" talk about what the caller gets. Typically the client thinks about what they need, not how the subroutine is implemented. Or this is how I imagine the original author naming them. https://codereview.chromium.org/1490113005/diff/80001/src/core/SkString.cpp File src/core/SkString.cpp (right): https://codereview.chromium.org/1490113005/diff/80001/src/core/SkString.cpp#n... src/core/SkString.cpp:629: if (resultBehavior != kAll_SkStrSplitResult) { On 2015/12/07 13:27:52, scroggo wrote: > Why not > > if (resultBehavior == kNonEmpty_SkStrSplitResult) > > This makes it seem like there are multiple resultBehaviors that should hit this > case. Done. https://codereview.chromium.org/1490113005/diff/80001/tests/StringTest.cpp File tests/StringTest.cpp (right): https://codereview.chromium.org/1490113005/diff/80001/tests/StringTest.cpp#ne... tests/StringTest.cpp:204: REPORTER_ASSERT(r, results.count() == 0); On 2015/12/07 13:27:52, scroggo wrote: > Is it okay that this default behavior changed? Might anyone be relying on the > old behavior? There's only few places where SkStrSplit is called, and as far as I could see, the old behavior did not make sense in any of them.
Description was changed from
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
null = gpu(api=null)
nullgpu = gpu(api=null)
BUG=skia:2992
==========
to
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
==========
On 2015/12/07 14:33:19, bsalomon wrote: > https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp > File dm/DM.cpp (right): > > https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp#newcode615 > dm/DM.cpp:615: contextType = GrContextFactory::kANGLE_GL_GLContextType; > On 2015/12/04 14:26:18, Kimmo Kinnunen wrote: > > On 2015/12/02 23:22:42, bsalomon wrote: > > > do we need this conversion? Could GrContextFactory work with command line > > enum? > > > Or vice-versa, use the grcontextfactory enum in place of the > > > SkCommandLineConfigGpu? > > > > The idea is that these are different things. GrContextFactory::GLContextType > > represents contexts that GrContextFactory understands. > > SkCommandLineConfigGpu::API represents the possible values that the parsing is > > implemented for. Most likely 100% of the time (hopefully?) these are exactly > the > > same. Still, they're a bit different concept. > > > > Similarly to "enableNVPR". The SkCommandLineConfigGpu does not refer to > > "ContextOptions", because "context option" is not a concept of the > > (user-visible) command line tools options. > > > > In practice, it would be confusing to refer from SkCommandLineConfigGpu to > > GrContextFactory, because they're in different libraries. For example, > > visualbench does not "use" GrContextFactory, where as it could use the config > > parsing in the future. Naturally, this being an enum the reference would work. > > It'd be just confusing. ... > GrContextFactory really only exists for our test tools and it should only > enumerate the context types that we want to test. We don't necessarily have to > leave the enum in GrContextFactory.The enum could live somewhere in tools/. I made the SkCommandLineConfigGpu to return the context type enum. Moving the context factory or the enum does not address the original issues I mentioned.. There's now a gyp change showing the cross-library issue, ptal. > Actually GrContextFactory could live there too as it isn't really part of the > gpu target proper, but rather a building block for writing test tools. As does most of the other gl context related code, SkGLContext and its subclasses. This probably should be a change of its own.
On 2015/12/08 09:31:43, Kimmo Kinnunen wrote: > On 2015/12/07 14:33:19, bsalomon wrote: > > https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp > > File dm/DM.cpp (right): > > > > https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp#newcode615 > > dm/DM.cpp:615: contextType = GrContextFactory::kANGLE_GL_GLContextType; > > On 2015/12/04 14:26:18, Kimmo Kinnunen wrote: > > > On 2015/12/02 23:22:42, bsalomon wrote: > > > > do we need this conversion? Could GrContextFactory work with command line > > > enum? > > > > Or vice-versa, use the grcontextfactory enum in place of the > > > > SkCommandLineConfigGpu? > > > > > > The idea is that these are different things. GrContextFactory::GLContextType > > > represents contexts that GrContextFactory understands. > > > SkCommandLineConfigGpu::API represents the possible values that the parsing > is > > > implemented for. Most likely 100% of the time (hopefully?) these are exactly > > the > > > same. Still, they're a bit different concept. > > > > > > Similarly to "enableNVPR". The SkCommandLineConfigGpu does not refer to > > > "ContextOptions", because "context option" is not a concept of the > > > (user-visible) command line tools options. > > > > > > In practice, it would be confusing to refer from SkCommandLineConfigGpu to > > > GrContextFactory, because they're in different libraries. For example, > > > visualbench does not "use" GrContextFactory, where as it could use the > config > > > parsing in the future. Naturally, this being an enum the reference would > work. > > > It'd be just confusing. > ... > > GrContextFactory really only exists for our test tools and it should only > > enumerate the context types that we want to test. We don't necessarily have to > > leave the enum in GrContextFactory.The enum could live somewhere in tools/. > > I made the SkCommandLineConfigGpu to return the context type enum. > > Moving the context factory or the enum does not address the original issues I > mentioned.. > > There's now a gyp change showing the cross-library issue, ptal. > Leon, is it important that flags doesn't include files from other targets?
On 2015/12/08 15:05:26, bsalomon wrote: > On 2015/12/08 09:31:43, Kimmo Kinnunen wrote: > > On 2015/12/07 14:33:19, bsalomon wrote: > > > https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp > > > File dm/DM.cpp (right): > > > > > > https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp#newcode615 > > > dm/DM.cpp:615: contextType = GrContextFactory::kANGLE_GL_GLContextType; > > > On 2015/12/04 14:26:18, Kimmo Kinnunen wrote: > > > > On 2015/12/02 23:22:42, bsalomon wrote: > > > > > do we need this conversion? Could GrContextFactory work with command > line > > > > enum? > > > > > Or vice-versa, use the grcontextfactory enum in place of the > > > > > SkCommandLineConfigGpu? > > > > > > > > The idea is that these are different things. > GrContextFactory::GLContextType > > > > represents contexts that GrContextFactory understands. > > > > SkCommandLineConfigGpu::API represents the possible values that the > parsing > > is > > > > implemented for. Most likely 100% of the time (hopefully?) these are > exactly > > > the > > > > same. Still, they're a bit different concept. > > > > > > > > Similarly to "enableNVPR". The SkCommandLineConfigGpu does not refer to > > > > "ContextOptions", because "context option" is not a concept of the > > > > (user-visible) command line tools options. > > > > > > > > In practice, it would be confusing to refer from SkCommandLineConfigGpu to > > > > GrContextFactory, because they're in different libraries. For example, > > > > visualbench does not "use" GrContextFactory, where as it could use the > > config > > > > parsing in the future. Naturally, this being an enum the reference would > > work. > > > > It'd be just confusing. > > ... > > > GrContextFactory really only exists for our test tools and it should only > > > enumerate the context types that we want to test. We don't necessarily have > to > > > leave the enum in GrContextFactory.The enum could live somewhere in tools/. > > > > I made the SkCommandLineConfigGpu to return the context type enum. > > > > Moving the context factory or the enum does not address the original issues I > > mentioned.. > > > > There's now a gyp change showing the cross-library issue, ptal. > > > > Leon, is it important that flags doesn't include files from other targets? It would be better if it doesn't, although I don't see an obvious way to remove that, besides duplicating some enums? Can we at least make it only rely on include/gpu (i.e. not src/gpu)?
On 2015/12/08 20:06:49, scroggo wrote: > On 2015/12/08 15:05:26, bsalomon wrote: > > On 2015/12/08 09:31:43, Kimmo Kinnunen wrote: > > > On 2015/12/07 14:33:19, bsalomon wrote: > > > > https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp > > > > File dm/DM.cpp (right): > > > > > > > > https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp#newcode615 > > > > dm/DM.cpp:615: contextType = GrContextFactory::kANGLE_GL_GLContextType; > > > > On 2015/12/04 14:26:18, Kimmo Kinnunen wrote: > > > > > On 2015/12/02 23:22:42, bsalomon wrote: > > > > > > do we need this conversion? Could GrContextFactory work with command > > line > > > > > enum? > > > > > > Or vice-versa, use the grcontextfactory enum in place of the > > > > > > SkCommandLineConfigGpu? > > > > > > > > > > The idea is that these are different things. > > GrContextFactory::GLContextType > > > > > represents contexts that GrContextFactory understands. > > > > > SkCommandLineConfigGpu::API represents the possible values that the > > parsing > > > is > > > > > implemented for. Most likely 100% of the time (hopefully?) these are > > exactly > > > > the > > > > > same. Still, they're a bit different concept. > > > > > > > > > > Similarly to "enableNVPR". The SkCommandLineConfigGpu does not refer to > > > > > "ContextOptions", because "context option" is not a concept of the > > > > > (user-visible) command line tools options. > > > > > > > > > > In practice, it would be confusing to refer from SkCommandLineConfigGpu > to > > > > > GrContextFactory, because they're in different libraries. For example, > > > > > visualbench does not "use" GrContextFactory, where as it could use the > > > config > > > > > parsing in the future. Naturally, this being an enum the reference would > > > work. > > > > > It'd be just confusing. > > > ... > > > > GrContextFactory really only exists for our test tools and it should only > > > > enumerate the context types that we want to test. We don't necessarily > have > > to > > > > leave the enum in GrContextFactory.The enum could live somewhere in > tools/. > > > > > > I made the SkCommandLineConfigGpu to return the context type enum. > > > > > > Moving the context factory or the enum does not address the original issues > I > > > mentioned.. > > > > > > There's now a gyp change showing the cross-library issue, ptal. > > > > > > > Leon, is it important that flags doesn't include files from other targets? > > It would be better if it doesn't, although I don't see an obvious way to remove > that, besides duplicating some enums? Can we at least make it only rely on > include/gpu (i.e. not src/gpu)? The only file it needs is GrContextFactory.h which is currently in src/gpu/. Though, it (and its cpp) should probably be in tools/.
On 2015/12/08 20:14:06, bsalomon wrote: > On 2015/12/08 20:06:49, scroggo wrote: > > On 2015/12/08 15:05:26, bsalomon wrote: > > > On 2015/12/08 09:31:43, Kimmo Kinnunen wrote: > > > > On 2015/12/07 14:33:19, bsalomon wrote: > > > > > https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp > > > > > File dm/DM.cpp (right): > > > > > > > > > > https://codereview.chromium.org/1490113005/diff/1/dm/DM.cpp#newcode615 > > > > > dm/DM.cpp:615: contextType = GrContextFactory::kANGLE_GL_GLContextType; > > > > > On 2015/12/04 14:26:18, Kimmo Kinnunen wrote: > > > > > > On 2015/12/02 23:22:42, bsalomon wrote: > > > > > > > do we need this conversion? Could GrContextFactory work with command > > > line > > > > > > enum? > > > > > > > Or vice-versa, use the grcontextfactory enum in place of the > > > > > > > SkCommandLineConfigGpu? > > > > > > > > > > > > The idea is that these are different things. > > > GrContextFactory::GLContextType > > > > > > represents contexts that GrContextFactory understands. > > > > > > SkCommandLineConfigGpu::API represents the possible values that the > > > parsing > > > > is > > > > > > implemented for. Most likely 100% of the time (hopefully?) these are > > > exactly > > > > > the > > > > > > same. Still, they're a bit different concept. > > > > > > > > > > > > Similarly to "enableNVPR". The SkCommandLineConfigGpu does not refer > to > > > > > > "ContextOptions", because "context option" is not a concept of the > > > > > > (user-visible) command line tools options. > > > > > > > > > > > > In practice, it would be confusing to refer from > SkCommandLineConfigGpu > > to > > > > > > GrContextFactory, because they're in different libraries. For example, > > > > > > visualbench does not "use" GrContextFactory, where as it could use the > > > > config > > > > > > parsing in the future. Naturally, this being an enum the reference > would > > > > work. > > > > > > It'd be just confusing. > > > > ... > > > > > GrContextFactory really only exists for our test tools and it should > only > > > > > enumerate the context types that we want to test. We don't necessarily > > have > > > to > > > > > leave the enum in GrContextFactory.The enum could live somewhere in > > tools/. > > > > > > > > I made the SkCommandLineConfigGpu to return the context type enum. > > > > > > > > Moving the context factory or the enum does not address the original > issues > > I > > > > mentioned.. > > > > > > > > There's now a gyp change showing the cross-library issue, ptal. > > > > > > > > > > Leon, is it important that flags doesn't include files from other targets? > > > > It would be better if it doesn't, although I don't see an obvious way to > remove > > that, besides duplicating some enums? Can we at least make it only rely on > > include/gpu (i.e. not src/gpu)? > > The only file it needs is GrContextFactory.h which is currently in src/gpu/. > Though, it (and its cpp) should probably be in tools/. Great. I would say at least remove include/gpu. I'm okay if we leave moving GrContextFactory into tools/ to another CL, with a FIXME: where we're including src/gpu here.
On 2015/12/08 21:33:44, scroggo wrote: > On 2015/12/08 20:14:06, bsalomon wrote: > > The only file it needs is GrContextFactory.h which is currently in src/gpu/. > > Though, it (and its cpp) should probably be in tools/. > > Great. I would say at least remove include/gpu. I'm okay if we leave moving > GrContextFactory into tools/ to another CL, with a FIXME: where we're including > src/gpu here. I did not add extra directories to include path just for fun :)
On 2015/12/09 07:18:06, Kimmo Kinnunen wrote: > On 2015/12/08 21:33:44, scroggo wrote: > > On 2015/12/08 20:14:06, bsalomon wrote: > > > The only file it needs is GrContextFactory.h which is currently in src/gpu/. > > > Though, it (and its cpp) should probably be in tools/. > > > > Great. I would say at least remove include/gpu. I'm okay if we leave moving > > GrContextFactory into tools/ to another CL, with a FIXME: where we're > including > > src/gpu here. > > I did not add extra directories to include path just for fun :) And by this I mean that include/gpu is needed because GrContextFactory.h uses GrContext and GrContextOptions. I can change GrContextFactory to use SkAutoTUnref<GrContextOptions> instead of GrContextOptions. This looks kind of silly, but should work, if then simultaneously the destructor is moved to the .cpp file and other .gypi files that depend on the header file implementation are then also modified. Should this be the plan?
On 2015/12/09 15:51:17, Kimmo Kinnunen wrote: > On 2015/12/09 07:18:06, Kimmo Kinnunen wrote: > > On 2015/12/08 21:33:44, scroggo wrote: > > > On 2015/12/08 20:14:06, bsalomon wrote: > > > > The only file it needs is GrContextFactory.h which is currently in > src/gpu/. > > > > Though, it (and its cpp) should probably be in tools/. > > > > > > Great. I would say at least remove include/gpu. I'm okay if we leave moving > > > GrContextFactory into tools/ to another CL, with a FIXME: where we're > > including > > > src/gpu here. > > > > I did not add extra directories to include path just for fun :) I'm sure you did not, but sometimes we think it's necessary, or it is necessary but other changes make it unnecessary and we forget to change it back... > > And by this I mean that include/gpu is needed because GrContextFactory.h uses > GrContext and GrContextOptions. > I can change GrContextFactory to use SkAutoTUnref<GrContextOptions> instead of > GrContextOptions. This looks kind of silly, but should work, if then > simultaneously the destructor is moved to the .cpp file and other .gypi files > that depend on the header file implementation are then also modified. Should > this be the plan? I'll defer to Brian on how to deal with GrContextFactory, as I am not very familiar with it.
On 2015/12/09 16:31:18, scroggo wrote: > On 2015/12/09 15:51:17, Kimmo Kinnunen wrote: > > On 2015/12/09 07:18:06, Kimmo Kinnunen wrote: > > > On 2015/12/08 21:33:44, scroggo wrote: > > > > On 2015/12/08 20:14:06, bsalomon wrote: > > > > > The only file it needs is GrContextFactory.h which is currently in > > src/gpu/. > > > > > Though, it (and its cpp) should probably be in tools/. > > > > > > > > Great. I would say at least remove include/gpu. I'm okay if we leave > moving > > > > GrContextFactory into tools/ to another CL, with a FIXME: where we're > > > including > > > > src/gpu here. > > > > > > I did not add extra directories to include path just for fun :) > > I'm sure you did not, but sometimes we think it's necessary, or it is necessary > but other changes make it unnecessary and we forget to change it back... > > > > > And by this I mean that include/gpu is needed because GrContextFactory.h uses > > GrContext and GrContextOptions. > > I can change GrContextFactory to use SkAutoTUnref<GrContextOptions> instead of > > GrContextOptions. This looks kind of silly, but should work, if then > > simultaneously the destructor is moved to the .cpp file and other .gypi files > > that depend on the header file implementation are then also modified. Should > > this be the plan? > > I'll defer to Brian on how to deal with GrContextFactory, as I am not very > familiar with it. Leon, if it's ok to have flags depend on including include/gpu, I'd just leave as is.
On 2015/12/09 17:14:58, bsalomon wrote: > On 2015/12/09 16:31:18, scroggo wrote: > > On 2015/12/09 15:51:17, Kimmo Kinnunen wrote: > > > On 2015/12/09 07:18:06, Kimmo Kinnunen wrote: > > > > On 2015/12/08 21:33:44, scroggo wrote: > > > > > On 2015/12/08 20:14:06, bsalomon wrote: > > > > > > The only file it needs is GrContextFactory.h which is currently in > > > src/gpu/. > > > > > > Though, it (and its cpp) should probably be in tools/. > > > > > > > > > > Great. I would say at least remove include/gpu. I'm okay if we leave > > moving > > > > > GrContextFactory into tools/ to another CL, with a FIXME: where we're > > > > including > > > > > src/gpu here. > > > > > > > > I did not add extra directories to include path just for fun :) > > > > I'm sure you did not, but sometimes we think it's necessary, or it is > necessary > > but other changes make it unnecessary and we forget to change it back... > > > > > > > > And by this I mean that include/gpu is needed because GrContextFactory.h > uses > > > GrContext and GrContextOptions. > > > I can change GrContextFactory to use SkAutoTUnref<GrContextOptions> instead > of > > > GrContextOptions. This looks kind of silly, but should work, if then > > > simultaneously the destructor is moved to the .cpp file and other .gypi > files > > > that depend on the header file implementation are then also modified. Should > > > this be the plan? > > > > I'll defer to Brian on how to deal with GrContextFactory, as I am not very > > familiar with it. > > Leon, if it's ok to have flags depend on including include/gpu, I'd just leave > as is. lgtm, for flags
On 2015/12/09 18:23:28, scroggo wrote: > On 2015/12/09 17:14:58, bsalomon wrote: > > Leon, if it's ok to have flags depend on including include/gpu, I'd just leave > > as is. > > lgtm, for flags Brian, how about the rest?
On 2015/12/11 07:52:34, Kimmo Kinnunen wrote: > On 2015/12/09 18:23:28, scroggo wrote: > > On 2015/12/09 17:14:58, bsalomon wrote: > > > Leon, if it's ok to have flags depend on including include/gpu, I'd just > leave > > > as is. > > > > lgtm, for flags > > Brian, how about the rest? lgtm
The CQ bit was checked by kkinnunen@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1490113005/#ps160001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490113005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490113005/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
The CQ bit was checked by kkinnunen@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1490113005/#ps180001 (title: "fix clang compile errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490113005/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490113005/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...) Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by kkinnunen@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1490113005/#ps200001 (title: "msvc fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490113005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490113005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by kkinnunen@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1490113005/#ps220001 (title: "msvc x86_64 fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490113005/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490113005/220001
Message was sent while issue was closed.
Description was changed from
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
==========
to
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
Committed:
https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/1528473002/ by reed@google.com. The reason for reverting is: speculative revert to see if it unblocks the DEPS roll https://codereview.chromium.org/1529443002.
Message was sent while issue was closed.
On 2015/12/14 at 13:58:03, reed wrote: > A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/1528473002/ by reed@google.com. > > The reason for reverting is: speculative revert to see if it unblocks the DEPS roll > > https://codereview.chromium.org/1529443002. Also seems to have changed the Perf results, seems to mostly affect the Nexus5's. https://perf.skia.org/#4678
Message was sent while issue was closed.
On 2015/12/14 at 17:01:23, jcgregorio wrote: > On 2015/12/14 at 13:58:03, reed wrote: > > A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/1528473002/ by reed@google.com. > > > > The reason for reverting is: speculative revert to see if it unblocks the DEPS roll > > > > https://codereview.chromium.org/1529443002. > > Also seems to have changed the Perf results, seems to mostly affect the Nexus5's. https://perf.skia.org/#4678 And has caused a Perf alert: https://perf.skia.org/cl/1025
Message was sent while issue was closed.
Description was changed from
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
Committed:
https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
==========
to
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
Committed:
https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
==========
On 2015/12/14 17:02:28, jcgregorio wrote: > On 2015/12/14 at 17:01:23, jcgregorio wrote: > > On 2015/12/14 at 13:58:03, reed wrote: > > > A revert of this CL (patchset #12 id:220001) has been created in > https://codereview.chromium.org/1528473002/ by mailto:reed@google.com. > > > > > > The reason for reverting is: speculative revert to see if it unblocks the > DEPS roll I fixed this. > > > https://codereview.chromium.org/1529443002. > > > > Also seems to have changed the Perf results, seems to mostly affect the > Nexus5's. https://perf.skia.org/#4678 > > And has caused a Perf alert: https://perf.skia.org/cl/1025 Is it possible or probable that this could be a temporary degradation? Like, the nexus5 having "a bad moment"? I'm trying to repro it locally with my local .skps, but I'm unable to repro that. However, I don't have access to Nexus 5 atm, only Nexus 5X (which is of course totally different device). I'm using equivalent of: while true; do rm -rf *.log && CLEAN=master ./bin/ac --match skp --config 565 8888 --scales 1.0 1.1; done My SKPs are from era when I still had access to the skp-generating .json files, pretty old ones. I'm getting pretty varying results, some comparison runs returning no real changes, and some returning 0.32x - 2x changes to varying .skp files.
Looking at the graphs now, the rises and falls don't appear related to this change. Feel free to reland.
mtklein@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/1490113005/diff/240001/tests/TestConfigParsin... File tests/TestConfigParsing.cpp (right): https://codereview.chromium.org/1490113005/diff/240001/tests/TestConfigParsin... tests/TestConfigParsing.cpp:7: Before you reland this, please make sure these new tests are thread safe. https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G... I imagine you can't safely register flags inside a function.
On 2015/12/15 23:47:39, mtklein wrote: > https://codereview.chromium.org/1490113005/diff/240001/tests/TestConfigParsin... > File tests/TestConfigParsing.cpp (right): > > https://codereview.chromium.org/1490113005/diff/240001/tests/TestConfigParsin... > tests/TestConfigParsing.cpp:7: > Before you reland this, please make sure these new tests are thread safe. Oh, right. I was trying to avoid changing SkCommandLineFlags::StringArray too much. I uploaded a new version, can you review SkCommandLineFlags.h change?
On 2015/12/16 at 07:02:03, kkinnunen wrote: > On 2015/12/15 23:47:39, mtklein wrote: > > https://codereview.chromium.org/1490113005/diff/240001/tests/TestConfigParsin... > > File tests/TestConfigParsing.cpp (right): > > > > https://codereview.chromium.org/1490113005/diff/240001/tests/TestConfigParsin... > > tests/TestConfigParsing.cpp:7: > > Before you reland this, please make sure these new tests are thread safe. > > Oh, right. I was trying to avoid changing SkCommandLineFlags::StringArray too much. > I uploaded a new version, can you review SkCommandLineFlags.h change? tools/flags/SkCommandLineFlags.h? Looks fine to me... I just see extended help added. Is that what you mean? The changes to the test look good to me too.
https://codereview.chromium.org/1490113005/diff/300001/tools/flags/SkCommonFl... File tools/flags/SkCommonFlagsConfig.h (right): https://codereview.chromium.org/1490113005/diff/300001/tools/flags/SkCommonFl... tools/flags/SkCommonFlagsConfig.h:26: // where 'tag' consists of chars excluding hyphen or "angle-gl" Have you considered calling this angle_gl so that it doesn't need to be an exception?
https://codereview.chromium.org/1490113005/diff/300001/tools/flags/SkCommandL... File tools/flags/SkCommandLineFlags.h (right): https://codereview.chromium.org/1490113005/diff/300001/tools/flags/SkCommandL... tools/flags/SkCommandLineFlags.h:122: StringArray() { } Mike, I meant this addition. This enables the tests to work without DEFINE_string.
https://codereview.chromium.org/1490113005/diff/300001/tools/flags/SkCommandL... File tools/flags/SkCommandLineFlags.h (right): https://codereview.chromium.org/1490113005/diff/300001/tools/flags/SkCommandL... tools/flags/SkCommandLineFlags.h:122: StringArray() { } On 2015/12/17 at 05:57:53, Kimmo Kinnunen wrote: > Mike, I meant this addition. > This enables the tests to work without DEFINE_string. Oh, sure, lgtm!
Description was changed from
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
Committed:
https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
==========
to
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
Committed:
https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========
The CQ bit was checked by kkinnunen@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com, bsalomon@google.com, mtklein@google.com Link to the patchset: https://codereview.chromium.org/1490113005/#ps320001 (title: "rebase for merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490113005/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490113005/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by kkinnunen@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com, scroggo@google.com, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1490113005/#ps340001 (title: "unused param warnding")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490113005/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490113005/340001
Message was sent while issue was closed.
Description was changed from
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
Committed:
https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========
to
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
Committed:
https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/c8b4336444e7b90382e04e33665fb3b8490b825b
==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://skia.googlesource.com/skia/+/c8b4336444e7b90382e04e33665fb3b8490b825b
Message was sent while issue was closed.
On 2015/12/18 11:27:37, commit-bot: I haz the power wrote: > Committed patchset #18 (id:340001) as > https://skia.googlesource.com/skia/+/c8b4336444e7b90382e04e33665fb3b8490b825b Maybe broke arm64 skia_gpu=0 build.. Investigating
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:340001) has been created in https://codereview.chromium.org/1536963002/ by joshualitt@google.com. The reason for reverting is: This CL changed 1200 images on gold, when I wouldn't expect any diffs from the description..
Message was sent while issue was closed.
On 2015/12/18 14:01:49, joshualitt wrote: > A revert of this CL (patchset #18 id:340001) has been created in > https://codereview.chromium.org/1536963002/ by mailto:joshualitt@google.com. > > The reason for reverting is: This CL changed 1200 images on gold, when I > wouldn't expect any diffs from the description.. A quick scan through the diffs shows lots of them are flipped diagonally - e.g. https://gold.skia.org/diff?test=yuv_to_rgb_effect&left=14f9b233d1b026ab9c8a06...
Message was sent while issue was closed.
Description was changed from
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
Committed:
https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/c8b4336444e7b90382e04e33665fb3b8490b825b
==========
to
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
Committed:
https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/c8b4336444e7b90382e04e33665fb3b8490b825b
==========
On 2015/12/18 14:07:32, fmalita_google_do_not_use wrote: > On 2015/12/18 14:01:49, joshualitt wrote: > > A revert of this CL (patchset #18 id:340001) has been created in > > https://codereview.chromium.org/1536963002/ by mailto:joshualitt@google.com. > > > > The reason for reverting is: This CL changed 1200 images on gold, when I > > wouldn't expect any diffs from the description.. > > A quick scan through the diffs shows lots of them are flipped diagonally - e.g. > https://gold.skia.org/diff?test=yuv_to_rgb_effect&left=14f9b233d1b026ab9c8a06... Kimmo, you can run trybots from your cl and view the differences in gold. Also, the actual command lines we run dm with print during runs. For example, the nexus 9 command line can be found here: https://build.chromium.org/p/client.skia.android/builders/Test-Android-GCC-Ne... under "======== To reproduce this run: ========"
On 2015/12/18 14:07:32, fmalita_google_do_not_use wrote: > On 2015/12/18 14:01:49, joshualitt wrote: > > A revert of this CL (patchset #18 id:340001) has been created in > > https://codereview.chromium.org/1536963002/ by mailto:joshualitt@google.com. > > > > The reason for reverting is: This CL changed 1200 images on gold, when I > > wouldn't expect any diffs from the description.. > > A quick scan through the diffs shows lots of them are flipped diagonally - e.g. > https://gold.skia.org/diff?test=yuv_to_rgb_effect&left=14f9b233d1b026ab9c8a06... Kimmo, just FYI, not sure if it'll help for this in particular - you can view image results from try bots using the GOLD_TRYBOT_URL in the description. You can also upload changes that modify tools/dm_flag.py in order to fire off try bots that complete faster by running a reduced set of tests.
Thanks, I will try the trybots. Sorry for the trouble..
On 2015/12/18 14:31:35, Kimmo Kinnunen wrote: > Thanks, I will try the trybots. Sorry for the trouble.. Its no trouble whatsoever, this is a good patch and I'd love to help you land it anyway I can.
The CQ bit was checked by kkinnunen@nvidia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490113005/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490113005/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by kkinnunen@nvidia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490113005/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490113005/400001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
So if I understand correctly: after that dry run, I should see Gold results from GOLD link in the commit description? And if it says "Displaying 0 of 0 Digests that matched your query." then nothing should change? Or did I misunderstand? I don't get why it does not show the case from history where CL changed 1200 images. That had the same GOLD link (same issue). Does the revert or new patch version or something remove the old results from the Gold?
On 2015/12/21 at 13:50:13, kkinnunen wrote: > So if I understand correctly: after that dry run, I should see Gold results from GOLD link in the commit description? And if it says "Displaying 0 of 0 Digests that matched your query." then nothing should change? > > Or did I misunderstand? I don't get why it does not show the case from history where CL changed 1200 images. That had the same GOLD link (same issue). Does the revert or new patch version or something remove the old results from the Gold? Right, it always shows the results of the latest run. 0 of 0 means none are changed as of the last trybot run. Which bots showed those 1200 diffs? If they're not on the default CQ list, you'll want to trigger them manually to make sure you're clean.
The CQ bit was checked by kkinnunen@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com, scroggo@google.com, bsalomon@google.com Link to the patchset: https://chromiumcodereview.appspot.com/1490113005/#ps400001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490113005/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490113005/400001
Message was sent while issue was closed.
Description was changed from
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
Committed:
https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/c8b4336444e7b90382e04e33665fb3b8490b825b
==========
to
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
Committed:
https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/c8b4336444e7b90382e04e33665fb3b8490b825b
Committed:
https://skia.googlesource.com/skia/+/9ebc3f0ee6db215dde461dc4777d85988cf272dd
==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://skia.googlesource.com/skia/+/9ebc3f0ee6db215dde461dc4777d85988cf272dd
Message was sent while issue was closed.
On 2015/12/22 07:48:19, commit-bot: I haz the power wrote: > Committed patchset #21 (id:400001) as > https://skia.googlesource.com/skia/+/9ebc3f0ee6db215dde461dc4777d85988cf272dd Looks like the http://build.chromium.org/p/client.skia/builders/Test-Win8-MSVC-ShuttleB-GPU-... builder has started to fail after this CL. Links to specific builds: http://build.chromium.org/p/client.skia/builders/Test-Win8-MSVC-ShuttleB-GPU-... http://build.chromium.org/p/client.skia/builders/Test-Win8-MSVC-ShuttleB-GPU-...
Message was sent while issue was closed.
A revert of this CL (patchset #21 id:400001) has been created in https://codereview.chromium.org/1548683002/ by rmistry@google.com. The reason for reverting is: The Test-Win8-MSVC-ShuttleB-GPU-HD4600-x86_64-Debug builder fails after this CL. Links to specific builds: http://build.chromium.org/p/client.skia/builders/Test-Win8-MSVC-ShuttleB-GPU-... http://build.chromium.org/p/client.skia/builders/Test-Win8-MSVC-ShuttleB-GPU-... http://build.chromium.org/p/client.skia/builders/Test-Win8-MSVC-ShuttleB-GPU-....
Message was sent while issue was closed.
Description was changed from
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
Committed:
https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/c8b4336444e7b90382e04e33665fb3b8490b825b
Committed:
https://skia.googlesource.com/skia/+/9ebc3f0ee6db215dde461dc4777d85988cf272dd
==========
to
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
Committed:
https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/c8b4336444e7b90382e04e33665fb3b8490b825b
Committed:
https://skia.googlesource.com/skia/+/9ebc3f0ee6db215dde461dc4777d85988cf272dd
==========
The CQ bit was checked by kkinnunen@nvidia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490113005/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490113005/440001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kkinnunen@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com, scroggo@google.com, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1490113005/#ps440001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490113005/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490113005/440001
Message was sent while issue was closed.
Description was changed from
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
Committed:
https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/c8b4336444e7b90382e04e33665fb3b8490b825b
Committed:
https://skia.googlesource.com/skia/+/9ebc3f0ee6db215dde461dc4777d85988cf272dd
==========
to
==========
Add config options to run different GPU APIs to dm and nanobench
Add extended config specification form that can be used to run different
gpu backend with different APIs.
The configs can be specified with the form:
gpu(api=string,dit=bool,nvpr=bool,samples=int)
This replaces and removes the --gpuAPI flag.
All existing configs should still work.
Adds following documentation:
out/Debug/dm --help config
Flags:
--config: type: string default: 565 8888 gpu nonrendering
Options: 565 8888 debug gpu gpudebug gpudft gpunull msaa16 msaa4
nonrendering null nullgpu nvprmsaa16 nvprmsaa4 pdf pdf_poppler skp svg
xps or use extended form 'backend(option=value,...)'.
Extended form: 'backend(option=value,...)'
Possible backends and options:
gpu(api=string,dit=bool,nvpr=bool,samples=int) GPU backend
api type: string default: native.
Select graphics API to use with gpu backend.
Options:
native Use platform default OpenGL or OpenGL ES backend.
gl Use OpenGL.
gles Use OpenGL ES.
debug Use debug OpenGL.
null Use null OpenGL.
dit type: bool default: false.
Use device independent text.
nvpr type: bool default: false.
Use NV_path_rendering OpenGL and OpenGL ES extension.
samples type: int default: 0.
Use multisampling with N samples.
Predefined configs:
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)
BUG=skia:2992
Committed:
https://skia.googlesource.com/skia/+/e13ca329fca4c28cf4e078561f591ab27b743d23
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/c8b4336444e7b90382e04e33665fb3b8490b825b
Committed:
https://skia.googlesource.com/skia/+/9ebc3f0ee6db215dde461dc4777d85988cf272dd
Committed:
https://skia.googlesource.com/skia/+/3e980c3d88fbc509b79e7ccef16ca38f5bbfb180
==========
Message was sent while issue was closed.
Committed patchset #23 (id:440001) as https://skia.googlesource.com/skia/+/3e980c3d88fbc509b79e7ccef16ca38f5bbfb180
Message was sent while issue was closed.
On 2015/12/23 09:33:06, commit-bot: I haz the power wrote: > Committed patchset #23 (id:440001) as > https://skia.googlesource.com/skia/+/3e980c3d88fbc509b79e7ccef16ca38f5bbfb180 Looks like there are still some new images when using NVPR on the Tegra K1: https://gold.skia.org/search2?blame=9ebc3f0ee6db215dde461dc4777d85988cf272dd&... While most of those diffs are pretty small, textblobmixedsizes is very noticeably changed along the upper edge of the lower half of the S. Is that expected?
Message was sent while issue was closed.
On 2015/12/28 18:00:13, mtklein wrote: > On 2015/12/23 09:33:06, commit-bot: I haz the power wrote: > > Committed patchset #23 (id:440001) as > > https://skia.googlesource.com/skia/+/3e980c3d88fbc509b79e7ccef16ca38f5bbfb180 > > Looks like there are still some new images when using NVPR on the Tegra K1: > https://gold.skia.org/search2?blame=9ebc3f0ee6db215dde461dc4777d85988cf272dd&... > > While most of those diffs are pretty small, textblobmixedsizes is very > noticeably changed along the upper edge of the lower half of the S. Is that > expected? Thanks for the inspection. I wasn't aware of DM running with device-independent text. Fixed here: https://codereview.chromium.org/1553453004/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
