<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Jan 6, 2016 at 2:06 PM, Emil Velikov <<a href="mailto:emil.l.velikov@gmail.com" target="_blank">emil.l.velikov@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Dylan, Perhaps I'm not the best person to comment on the JSON side of things (esp since Frank sent related series a few hours ago), then again a few things, that might be interesting. <div><div class="h5"> On 5 January 2016 at 19:46, <<a href="mailto:baker.dylan.c@gmail.com">baker.dylan.c@gmail.com</a>> wrote: > From: Dylan Baker <<a href="mailto:baker.dylan.c@gmail.com">baker.dylan.c@gmail.com</a>> > > This adds some code to print a JSON formatted version of data provided > by wflinfo. > > Signed-off-by: Dylan Baker <<a href="mailto:dylanx.c.baker@intel.com">dylanx.c.baker@intel.com</a>> > > v2: - Add -j short options (Frank) > - Make context flags always an array (Chad) > - Remove explicit returns from void functions (Chad) > - Fix spaces around "*" in variable definitions > - Nest waffle specific information in a waffle hash and OpenGL > specific information in an OpenGL hash > - use ARRAY_SIZE macro > - rebase on previous changes > --- > src/utils/wflinfo.c | 178 +++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 176 insertions(+), 2 deletions(-) > > diff --git a/src/utils/wflinfo.c b/src/utils/wflinfo.c > index 8ee95c6..456f91b 100644 > --- a/src/utils/wflinfo.c > +++ b/src/utils/wflinfo.c > @@ -85,6 +85,9 @@ static const char *usage_message = > " --debug-context\n" > " Create a debug context.\n" > "\n" > + " -j, --json\n" > + " Print a JSON formatted summary.\n" > + "\n" > " -h, --help\n" > " Print wflinfo usage information.\n" > "\n" > @@ -104,6 +107,7 @@ enum { > OPT_VERBOSE = 'v', > OPT_DEBUG_CONTEXT, > OPT_FORWARD_COMPATIBLE, > + OPT_JSON = 'j', > OPT_HELP = 'h', > }; > > @@ -115,6 +119,7 @@ static const struct option get_opts[] = { > { .name = "verbose", .has_arg = no_argument, .val = OPT_VERBOSE }, > { .name = "debug-context", .has_arg = no_argument, .val = OPT_DEBUG_CONTEXT }, > { .name = "forward-compatible", .has_arg = no_argument, .val = OPT_FORWARD_COMPATIBLE }, > + { .name = "json", .has_arg = no_argument, .val = OPT_JSON }, > { .name = "help", .has_arg = no_argument, .val = OPT_HELP }, > { 0 }, > }; > @@ -252,6 +257,8 @@ struct options { > > bool verbose; > > + bool json; > + > bool context_forward_compatible; > bool context_debug; > > @@ -337,7 +344,7 @@ parse_args(int argc, char *argv[], struct options *opts) > opterr = 0; > > while (loop_get_opt) { > - int opt = getopt_long(argc, argv, "a:hp:vV:", get_opts, NULL); > + int opt = getopt_long(argc, argv, "a:hp:vV:j", get_opts, NULL); > switch (opt) { > case -1: > loop_get_opt = false; > @@ -395,6 +402,9 @@ parse_args(int argc, char *argv[], struct options *opts) > case OPT_DEBUG_CONTEXT: > opts->context_debug = true; > break; > + case OPT_JSON: > + opts->json = true; > + break; > case OPT_HELP: > write_usage_and_exit(stdout, EXIT_SUCCESS); > break; > @@ -563,6 +573,165 @@ print_context_flags(void) > printf("\n"); > } > > +static void > +json_print_context_flags(void) > +{ > + const int flag_count = ARRAY_SIZE(context_flags); </div></div>const unsigned flag_count ? > + GLint gl_context_flags = 0; > + > + printf(" \"context flags\": "); > + > + glGetIntegerv(GL_CONTEXT_FLAGS, &gl_context_flags); > + if (glGetError() != GL_NO_ERROR) { > + printf("\"WFLINFO_GL_ERROR\"\n"); > + return; > + } > + > + if (gl_context_flags == 0) { > + printf("[]"); Missing \n ? </blockquote><div> </div><div>Nope, gets covered later. This is a little odd to look at because that \n gets added later, because of the comma/no-comma madness that JSON enforces. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + return; > + } > + > + printf("[\n"); > + for (int i = 0; i < flag_count; i++) { unsigned i ? </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + if ((context_flags[i].flag & gl_context_flags) != 0) { > + printf(" \"%s\"", context_flags[i].str); > + gl_context_flags = gl_context_flags & ~context_flags[i].flag; > + if (i != flag_count) { This always evaluates to true, doesn't it ? </blockquote><div> </div><div>Doesn't it evaluate to true in every case but the last iteration? </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + printf(",\n"); > + } > + } > + } > + for (int i = 0; gl_context_flags != 0; gl_context_flags >>= 1, i++) { > + if ((gl_context_flags & 1) != 0) { > + printf(",\n"); We'll get two consecutive ",\n" considering the one above ? </blockquote><div> </div><div>nope, because you don't get ",\n" in the last iteration of the above loop. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + printf(" 0x%x", 1 << i); ... and we're missing the \n for the last entry. Easier way might be to do "leading" \n's ? > + } > + } > + printf(" ]"); > +} > + > +static void > +json_print_extensions(bool use_stringi) > +{ > + // Print extensions in JSON format > + printf(" \"extensions\": [\n"); > + if (use_stringi) { > + GLint count = 0, i; > + const char *ext; > + > + glGetIntegerv(GL_NUM_EXTENSIONS, &count); > + if (glGetError() != GL_NO_ERROR) { > + printf("\"WFLINFO_GL_ERROR\""); Intentional difference in tabulation between previous function, here and else statement ? </blockquote><div> </div><div>nope, I'll fix that. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + } else { > + for (i = 0; i < count; i++) { > + ext = (const char *) glGetStringi(GL_EXTENSIONS, i); > + if (glGetError() != GL_NO_ERROR) > + ext = "WFLINFO_GL_ERROR"; > + printf(" \"%s\"%s\n", ext, (i + 1) < count ? "," : ""); > + } > + } > + } else { > + char *extensions = (char *) glGetString(GL_EXTENSIONS); > + if (glGetError() != GL_NO_ERROR) { > + printf(" \"WFLINFO_GL_ERROR\""); > + } else { > + char *splitter = strtok(extensions, " "); > + Note to self: do we care if a driver returns a single extension (and/or empty extensions string) ? > + // The JSON doesn't strictly need to be newline seperated, but > + // it makes it much easier to read. Split the lines and add commas correctly > + while(true) { > + printf(" \"%s\"", splitter); > + splitter = strtok(NULL, " "); > + > + if (splitter != NULL) { > + printf(",\n"); > + } else { > + break; > + } > + } > + } > + printf("\n"); > + } > + printf(" ]\n"); > +} > + > +/// @brief Print JSON formatted OpenGL (ES) information > +static bool > +print_json(const struct options *opts) > +{ > + while(glGetError() != GL_NO_ERROR) { > + /* Clear all errors */ Space between while and (... then again why the while loop ? </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="h5"> > + } > + > + const char *vendor = get_vendor(); > + const char *renderer = get_renderer(); > + const char *version_str = get_version(); > + > + const char *platform = enum_map_to_str(platform_map, opts->platform); > + assert(platform != NULL); > + > + const char *api = enum_map_to_str(context_api_map, opts->context_api); > + assert(api != NULL); > + > + printf("{\n"); > + printf(" \"waffle\": {\n"); > + printf(" \"platform\": \"%s\",\n", platform); > + printf(" \"api\": \"%s\"\n", api); > + printf(" },\n"); > + printf(" \"OpenGL\": {\n"); > + printf(" \"vendor\": \"%s\",\n", vendor); > + printf(" \"renderer\": \"%s\",\n", renderer); > + printf(" \"version\": \"%s\"", version_str); > + // No comma is allowed on the last element. If this context doesn't have > + // context flags or verbose is not requested this will be the last element. > + > + int version = parse_version(version_str); > + > + if (opts->context_api == WAFFLE_CONTEXT_OPENGL && version >= 31) { > + // Add the comma and newline before the context flags. > + printf(",\n"); > + json_print_context_flags(); > + } > + > + // OpenGL and OpenGL ES >= 3.0 support glGetStringi(GL_EXTENSION, i). > + const bool use_getstringi = version >= 30; > + > + if (!glGetStringi && use_getstringi) > + error_get_gl_symbol("glGetStringi"); > + > + if (opts->verbose) { > + // Add the command and newline > + printf(",\n"); > + > + // See the equivalent section in print_wflinfo() for information about > + // this hunk > + const char *language_str = "None"; > + if ((opts->context_api == WAFFLE_CONTEXT_OPENGL && version >= 20) || > + opts->context_api == WAFFLE_CONTEXT_OPENGL_ES2 || > + opts->context_api == WAFFLE_CONTEXT_OPENGL_ES3) { </div></div>Align the last two ones - o (in opts) should be the same column as the ( </blockquote><div> </div><div>that looks like a readability nightmare: if ((opts->context_api == WAFFLE_CONTEXT_OPENGL && version >= 20) || opts->context_api == WAFFLE_CONTEXT_OPENGL_ES2 || opts->context_api == WAFFLE_CONTEXT_OPENGL_ES3) { language_str = (const char *) glGetString(GL_SHADING_LANGUAGE_VERSION); if (glGetError() != GL_NO_ERROR || language_str == NULL) { language_str = "WFLINFO_GL_ERROR"; } } </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + language_str = (const char *) glGetString(GL_SHADING_LANGUAGE_VERSION); > + if (glGetError() != GL_NO_ERROR || language_str == NULL) { > + language_str = "WFLINFO_GL_ERROR"; > + } > + } > + > + printf(" \"shading language version\": \"%s\",\n", language_str); > + > + json_print_extensions(use_getstringi); > + > + } else { > + // If verbose isn't being used, add a newline so that the closing curly > + // brace will be nicely formatted > + printf("\n"); > + } > + > + printf(" }\n"); > + printf("}\n"); > + > + return true; Afaict you can make the function return void ? Seems that most (all?) of the comments here apply to the existing code in wflinfo.c where this was inspired/copied from. As mentioned by others - pondering on reusing the existing functions... I'll give it a quick test tomorrow. A bit tired so do take my comments with a grain of salt. That said (with or without the suggestions in 1 and 3) patches 1-3 are Reviewed-by: Emil Velikov <<a href="mailto:emil.velikov@collabora.com">emil.velikov@collabora.com</a>> -Emil </blockquote></div> </div><div class="gmail_extra">I'll talk to Frank, but I think his series covers everything mine set out to do, if does, I'll just withdraw mine. </div></div>