On Sun, Apr 24, 2016 at 4:54 PM, Emil Velikov <emil.l.veli...@gmail.com> wrote: > On 24 April 2016 at 20:50, Frank Henigman <fjhenig...@google.com> wrote: >> On Sun, Apr 24, 2016 at 6:42 AM, Emil Velikov <emil.l.veli...@gmail.com> >> wrote: >>> On 21 April 2016 at 21:25, Frank Henigman <fjhenig...@google.com> wrote: >>>> On Fri, Jan 8, 2016 at 7:40 AM, Emil Velikov <emil.l.veli...@gmail.com> >>>> wrote: >>>>> On 6 January 2016 at 21:56, Frank Henigman <fjhenig...@google.com> wrote: >>>>>> Duplicate wflinfo functionality in the API, with the difference that the >>>>>> information is returned in JSON form. >>>>>> The function has a parameter for including platform-specific information, >>>>>> but it is ignored for now. >>>>>> >>>>>> Signed-off-by: Frank Henigman <fjhenig...@google.com> >>>>>> --- >>>>>> include/waffle/waffle.h | 5 + >>>>>> man/waffle_display.3.xml | 19 +++ >>>>>> src/waffle/api/waffle_display.c | 284 >>>>>> +++++++++++++++++++++++++++++++++++++++- >>>>>> src/waffle/waffle.def.in | 1 + >>>>>> 4 files changed, 308 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/include/waffle/waffle.h b/include/waffle/waffle.h >>>>>> index df0218e..1800399 100644 >>>>>> --- a/include/waffle/waffle.h >>>>>> +++ b/include/waffle/waffle.h >>>>>> @@ -214,6 +214,11 @@ bool >>>>>> waffle_display_supports_context_api(struct waffle_display *self, >>>>>> int32_t context_api); >>>>>> >>>>>> +#if WAFFLE_API_VERSION >= 0x0106 >>>>>> +char* >>>>>> +waffle_display_info_json(struct waffle_display *self, bool >>>>>> platform_too); >>>>> The function does not work solely with the display, but it requires a >>>>> (bound) context. Thus it does not really fit waffle naming scheme. I'm >>>>> afraid that I'm short of suggestions though (barring my "returning >>>>> json formatted data sounds iffy, lets use tokens" rant from earlier) >>>>> >>>>> >>>>>> +#endif >>>>>> + >>>>>> union waffle_native_display* >>>>>> waffle_display_get_native(struct waffle_display *self); >>>>>> >>>>>> diff --git a/man/waffle_display.3.xml b/man/waffle_display.3.xml >>>>>> index 9896247..5358472 100644 >>>>>> --- a/man/waffle_display.3.xml >>>>>> +++ b/man/waffle_display.3.xml >>>>>> @@ -24,6 +24,7 @@ >>>>>> <refname>waffle_display</refname> >>>>>> <refname>waffle_display_connect</refname> >>>>>> <refname>waffle_display_disconnect</refname> >>>>>> + <refname>waffle_display_info_json</refname> >>>>>> <refname>waffle_display_supports_context_api</refname> >>>>>> <refname>waffle_display_get_native</refname> >>>>>> <refpurpose>class <classname>waffle_display</classname></refpurpose> >>>>>> @@ -58,6 +59,12 @@ struct waffle_display; >>>>>> </funcprototype> >>>>>> >>>>>> <funcprototype> >>>>>> + <funcdef>char* >>>>>> <function>waffle_display_info_json</function></funcdef> >>>>>> + <paramdef>struct waffle_display >>>>>> *<parameter>self</parameter></paramdef> >>>>>> + <paramdef>bool <parameter>platform_info</parameter></paramdef> >>>>>> + </funcprototype> >>>>>> + >>>>>> + <funcprototype> >>>>>> <funcdef>bool >>>>>> <function>waffle_display_supports_context_api</function></funcdef> >>>>>> <paramdef>struct waffle_display >>>>>> *<parameter>self</parameter></paramdef> >>>>>> <paramdef>int32_t <parameter>context_api</parameter></paramdef> >>>>>> @@ -129,6 +136,18 @@ struct waffle_display; >>>>>> </varlistentry> >>>>>> >>>>>> <varlistentry> >>>>>> + <term><function>waffle_display_info_json()</function></term> >>>>>> + <listitem> >>>>>> + <para> >>>>>> + Return a JSON string containing information about the >>>>>> current context on the given display, including Waffle platform and API, >>>>>> GL version/vendor/renderer and extensions. >>>>>> + If <parameter>platform_info</parameter> is true, >>>>>> platform-specific information (such as GLX or EGL versions and >>>>>> extensions) will be included as available. >>>>>> + Returns <constant>NULL</constant> on error. >>>>>> + The string should be deallocated with >>>>>> <citerefentry><refentrytitle><function>free</function></refentrytitle><manvolnum>3</manvolnum></citerefentry>. >>>>>> + </para> >>>>>> + </listitem> >>>>>> + </varlistentry> >>>>>> + >>>>>> + <varlistentry> >>>>>> >>>>>> <term><function>waffle_display_supports_context_api()</function></term> >>>>>> <listitem> >>>>>> <para> >>>>>> diff --git a/src/waffle/api/waffle_display.c >>>>>> b/src/waffle/api/waffle_display.c >>>>>> index fa19462..7abe2ef 100644 >>>>>> --- a/src/waffle/api/waffle_display.c >>>>>> +++ b/src/waffle/api/waffle_display.c >>>>>> @@ -23,13 +23,61 @@ >>>>>> // OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT >>>>>> OF THE USE >>>>>> // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. >>>>>> >>>>>> +#include <ctype.h> >>>>>> +#include <stdio.h> >>>>>> + >>>>>> #include "api_priv.h" >>>>>> >>>>>> -#include "wcore_error.h" >>>>>> +#include "json.h" >>>>>> + >>>>>> +#include "wcore_context.h" >>>>>> #include "wcore_display.h" >>>>>> +#include "wcore_error.h" >>>>>> #include "wcore_platform.h" >>>>>> #include "wcore_util.h" >>>>>> >>>>>> +typedef unsigned int GLint; >>>>>> +typedef unsigned int GLenum; >>>>>> +typedef unsigned char GLubyte; >>>>>> + >>>>>> +enum { >>>>>> + // Copied from <GL/gl*.h>. >>>>>> + GL_NO_ERROR = 0, >>>>>> + >>>>>> + GL_CONTEXT_FLAGS = 0x821e, >>>>>> + GL_CONTEXT_FLAG_FORWARD_COMPATIBLE_BIT = 0x00000001, >>>>>> + GL_CONTEXT_FLAG_DEBUG_BIT = 0x00000002, >>>>>> + GL_CONTEXT_FLAG_ROBUST_ACCESS_BIT_ARB = 0x00000004, >>>>>> + >>>>>> + GL_VENDOR = 0x1F00, >>>>>> + GL_RENDERER = 0x1F01, >>>>>> + GL_VERSION = 0x1F02, >>>>>> + GL_EXTENSIONS = 0x1F03, >>>>>> + GL_NUM_EXTENSIONS = 0x821D, >>>>>> + GL_SHADING_LANGUAGE_VERSION = 0x8B8C, >>>>>> +}; >>>>>> + >>>>>> +#ifndef _WIN32 >>>>>> +#define APIENTRY >>>>>> +#else >>>>>> +#ifndef APIENTRY >>>>>> +#define APIENTRY __stdcall >>>>>> +#endif >>>>>> +#endif >>>>>> + >>>>>> +static GLenum (APIENTRY *glGetError)(void); >>>>>> +static void (APIENTRY *glGetIntegerv)(GLenum pname, GLint *params); >>>>>> +static const GLubyte * (APIENTRY *glGetString)(GLenum name); >>>>>> +static const GLubyte * (APIENTRY *glGetStringi)(GLenum name, GLint i); >>>>>> + >>>>>> +#if defined(__GNUC__) >>>>>> +#define NORETURN __attribute__((noreturn)) >>>>>> +#elif defined(_MSC_VER) >>>>>> +#define NORETURN __declspec(noreturn) >>>>>> +#else >>>>>> +#define NORETURN >>>>>> +#endif >>>>>> + >>>>>> WAFFLE_API struct waffle_display* >>>>>> waffle_display_connect(const char *name) >>>>>> { >>>>>> @@ -90,6 +138,240 @@ waffle_display_supports_context_api( >>>>>> >>>>>> context_api); >>>>>> } >>>>>> >>>>>> +static int >>>>>> +parse_version(const char *version) >>>>>> +{ >>>>>> + int count, major, minor; >>>>>> + >>>>>> + if (version == NULL) >>>>>> + return 0; >>>>>> + >>>>>> + while (*version != '\0' && !isdigit(*version)) >>>>>> + version++; >>>>>> + >>>>>> + count = sscanf(version, "%d.%d", &major, &minor); >>>>>> + if (count != 2) >>>>>> + return 0; >>>>>> + >>>>>> + if (minor > 9) >>>>>> + return 0; >>>>>> + >>>>>> + return (major * 10) + minor; >>>>>> +} >>>>>> + >>>>>> +static void >>>>>> +add_context_flags(struct json *jj) >>>>>> +{ >>>>>> + static struct { >>>>>> + GLint flag; >>>>>> + char *str; >>>>>> + } flags[] = { >>>>>> + { GL_CONTEXT_FLAG_FORWARD_COMPATIBLE_BIT, "FORWARD_COMPATIBLE" >>>>>> }, >>>>>> + { GL_CONTEXT_FLAG_DEBUG_BIT, "DEBUG" }, >>>>>> + { GL_CONTEXT_FLAG_ROBUST_ACCESS_BIT_ARB, "ROBUST_ACCESS" }, >>>>>> + }; >>>>>> + int flag_count = sizeof(flags) / sizeof(flags[0]); >>>>>> + GLint context_flags = 0; >>>>>> + >>>>>> + glGetIntegerv(GL_CONTEXT_FLAGS, &context_flags); >>>>>> + if (glGetError() != GL_NO_ERROR) >>>>>> + return json_append(jj, json_str("WFLINFO_GL_ERROR")); >>>>>> + >>>>>> + if (context_flags == 0) >>>>>> + return json_append(jj, json_num(0)); >>>>>> + >>>>>> + for (int i = 0; i < flag_count; i++) { >>>>>> + if ((flags[i].flag & context_flags) != 0) { >>>>>> + json_append(jj, json_str(flags[i].str)); >>>>>> + context_flags = context_flags & ~flags[i].flag; >>>>>> + } >>>>>> + } >>>>>> + for (int i = 0; context_flags != 0; context_flags >>= 1, i++) { >>>>>> + if ((context_flags & 1) != 0) { >>>>>> + json_append(jj, json_num(1 << i)); >>>>>> + } >>>>>> + } >>>>>> +} >>>>>> + >>>>>> +static void >>>>>> +add_extensions(struct json *jj, bool use_stringi) >>>>>> +{ >>>>>> + GLint count = 0, i; >>>>>> + const char *ext; >>>>>> + >>>>>> + if (use_stringi) { >>>>>> + glGetIntegerv(GL_NUM_EXTENSIONS, &count); >>>>>> + if (glGetError() != GL_NO_ERROR) { >>>>>> + json_append(jj, json_str("WFLINFO_GL_ERROR")); >>>>>> + } else { >>>>>> + for (i = 0; i < count; i++) { >>>>>> + ext = (const char *) glGetStringi(GL_EXTENSIONS, i); >>>>>> + if (glGetError() != GL_NO_ERROR) >>>>>> + ext = "WFLINFO_GL_ERROR"; >>>>>> + json_append(jj, json_str(ext)); >>>>>> + } >>>>>> + } >>>>>> + } else { >>>>>> + const char *extensions = (const char *) >>>>>> glGetString(GL_EXTENSIONS); >>>>>> + if (glGetError() != GL_NO_ERROR) >>>>>> + json_append(jj, json_str("WFLINFO_GL_ERROR")); >>>>>> + else >>>>>> + json_append(jj, json_split(extensions, " ")); >>>>>> + } >>>>>> +} >>>>>> + >>>>>> +static void >>>>>> +add_generic_info(struct json *jj, struct wcore_context *ctx) >>>>>> +{ >>>>>> + int32_t dl; >>>>>> + //XXX this pattern seems to occur repeatedly - do we need two sets >>>>>> of enums? >>>>> I'm afraid we do. The whole topic is a bit messy, but the gist is that >>>>> - because we have the GL* library that does not imply that we can have >>>>> a context of said API and vice-versa. >>>>> >>>>>> + switch (ctx->context_api) { >>>>>> + case WAFFLE_CONTEXT_OPENGL: dl = WAFFLE_DL_OPENGL; >>>>>> break; >>>>>> + case WAFFLE_CONTEXT_OPENGL_ES1: dl = WAFFLE_DL_OPENGL_ES1; >>>>>> break; >>>>>> + case WAFFLE_CONTEXT_OPENGL_ES2: dl = WAFFLE_DL_OPENGL_ES2; >>>>>> break; >>>>>> + case WAFFLE_CONTEXT_OPENGL_ES3: dl = WAFFLE_DL_OPENGL_ES3; >>>>>> break; >>>>>> + default: >>>>>> + abort(); >>>>> This feels excessive. Set an error and bail out ? >>>> >>>> I'll change to assert(false). I think we want at least that, because >>>> the value has been validated by this point so we should never get >>>> here. >>>> >>> Good point. I've recently went through waffle and unified things to >>> follow this approach - use assert if we've already validated things. >>> Patches are on the mailing list - "... replace wcore_error_internal >>> with assert" - feel free to take a look. >>> >>>>> >>>>>> + break; >>>>>> + } >>>>>> + >>>>>> + glGetError = waffle_dl_sym(dl, "glGetError"); >>>>>> + if (!glGetError) >>>>>> + return json_append(jj, NULL); >>>>>> + >>>>>> + glGetIntegerv = waffle_dl_sym(dl, "glGetIntegerv"); >>>>>> + if (!glGetIntegerv) >>>>>> + return json_append(jj, NULL); >>>>>> + >>>>>> + glGetString = waffle_dl_sym(dl, "glGetString"); >>>>>> + if (!glGetString) >>>>>> + return json_append(jj, NULL); >>>>>> + >>>>>> + // Retrieving GL functions is tricky. When glGetStringi is >>>>>> supported, here >>>>>> + // are some boggling variations as of 2014-11-19: >>>>>> + // - Mali drivers on EGL 1.4 expose glGetStringi statically from >>>>>> + // libGLESv2 but not dynamically from eglGetProcAddress. The >>>>>> EGL 1.4 spec >>>>>> + // permits this behavior. >>>>>> + // - EGL 1.5 requires that eglGetStringi be exposed dynamically >>>>>> through >>>>>> + // eglGetProcAddress. Exposing statically with dlsym is >>>>>> optional. >>>>>> + // - Windows requires that glGetStringi be exposed dynamically >>>>>> from >>>>>> + // wglGetProcAddress. Exposing statically from GetProcAddress >>>>>> (Window's >>>>>> + // dlsym equivalent) is optional. >>>>>> + // - Mesa drivers expose glGetStringi statically from libGL and >>>>>> libGLESv2 >>>>>> + // and dynamically from eglGetProcAddress and glxGetProcAddress. >>>>>> + // - Mac exposes glGetStringi only statically. >>>>>> + // >>>>>> + // Try waffle_dl_sym before waffle_get_proc_address because >>>>>> + // (1) egl/glXProcAddress can return invalid non-null pointers for >>>>>> + // unsupported functions and (2) dlsym returns non-null if and only >>>>>> if the >>>>>> + // library exposes the symbol. >>>>>> + glGetStringi = waffle_dl_sym(dl, "glGetStringi"); >>>>>> + if (!glGetStringi) { >>>>>> + glGetStringi = waffle_get_proc_address("glGetStringi"); >>>>>> + } >>>>>> + >>>>>> + while(glGetError() != GL_NO_ERROR) { >>>>>> + /* Clear all errors */ >>>>>> + } >>>>>> + >>>>> As mentioned elsewhere - why the loop (and yes same question goes for >>>>> the original in wflinfo) ? >>>> >>>> According to the man page "glGetError should always be called in a >>>> loop, until it returns GL_NO_ERROR, if all error flags are to be >>>> reset." >>>> >>> Indeed it does. I wonder if any drivers implement error. queuqueuet >>> sure if this is the right terminology here), which the spec implies >>> with the text. >>> >>> Related: most of piglit (something like 90+%) does not use a loop >>> similar to all of xserver. Haven't looked at dEQP although I'm leaning >>> that it might be in a similar boat. >>> >>>>> >>>>>> + const char *vendor = (const char *) glGetString(GL_VENDOR); >>>>>> + if (glGetError() != GL_NO_ERROR || vendor == NULL) { >>>>>> + vendor = "WFLINFO_GL_ERROR"; >>>>>> + } >>>>>> + >>>>>> + const char *renderer = (const char *) glGetString(GL_RENDERER); >>>>>> + if (glGetError() != GL_NO_ERROR || renderer == NULL) { >>>>>> + renderer = "WFLINFO_GL_ERROR"; >>>>>> + } >>>>>> + >>>>>> + const char *version_str = (const char *) glGetString(GL_VERSION); >>>>>> + if (glGetError() != GL_NO_ERROR || version_str == NULL) { >>>>>> + version_str = "WFLINFO_GL_ERROR"; >>>>>> + } >>>>>> + >>>>> Please drop the extra curly brackets from the last 4 if statements. >>>>> Afaict with MSVC2013 U4 (our min requirement) things should just work >>>>> ? >>>>> >>>>>> + assert(ctx->display->platform->waffle_platform); >>>>>> + const char *platform = >>>>>> + >>>>>> wcore_enum_to_string(ctx->display->platform->waffle_platform); >>>>>> + assert(platform != NULL); >>>>>> + >>>>>> + assert(ctx->context_api); >>>>>> + const char *api = wcore_enum_to_string(ctx->context_api); >>>>>> + assert(api != NULL); >>>>>> + >>>>>> + json_appendv(jj, >>>>>> + "waffle", "{", >>>>>> + "platform", json_str(platform), >>>>>> + "api", json_str(api), >>>>>> + "}", >>>>>> + "opengl", "{", >>>>>> + "vendor", json_str(vendor), >>>>>> + "renderer", json_str(renderer), >>>>>> + "version", json_str(version_str), >>>>>> + "}", ""); >>>>>> + >>>>>> + int version = parse_version(version_str); >>>>>> + >>>>>> + if (ctx->context_api == WAFFLE_CONTEXT_OPENGL && version >= 31) { >>>>>> + json_appendv(jj, "context_flags", "[", ""); >>>>>> + add_context_flags(jj); >>>>>> + json_append(jj, "]"); >>>>>> + } >>>>>> + >>>>>> + // OpenGL and OpenGL ES >= 3.0 support glGetStringi(GL_EXTENSION, >>>>>> i). >>>>>> + const bool use_getstringi = version >= 30; >>>>>> + >>>>>> + if (!glGetStringi && use_getstringi) >>>>>> + return json_append(jj, NULL); >>>>>> + >>>>>> + // There are two exceptional cases where wflinfo may not get a >>>>>> + // version (or a valid version): one is in gles1 and the other >>>>>> + // is GL < 2.0. In these cases do not return WFLINFO_GL_ERROR, >>>>>> + // return None. This is preferable to returning WFLINFO_GL_ERROR >>>>>> + // because it creates a consistant interface for parsers >>>>>> + const char *language_str = "None"; >>>>>> + if ((ctx->context_api == WAFFLE_CONTEXT_OPENGL && version >= 20) || >>>>>> + ctx->context_api == WAFFLE_CONTEXT_OPENGL_ES2 || >>>>>> + ctx->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"; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + json_appendv(jj, "shading_language_version", >>>>>> json_str(language_str), ""); >>>>>> + json_appendv(jj, "extensions", "[", ""); >>>>>> + add_extensions(jj, use_getstringi); >>>>>> + json_append(jj, "]"); >>>>>> +} >>>>>> + >>>>>> +WAFFLE_API char* >>>>> Please add space between char and * >>>>> >>>>>> +waffle_display_info_json(struct waffle_display *self, bool platform_too) >>>>>> +{ >>>>>> + struct wcore_display *wc_self = wcore_display(self); >>>>>> + >>>>>> + const struct api_object *obj_list[] = { >>>>>> + wc_self ? &wc_self->api : NULL, >>>>>> + }; >>>>>> + >>>>>> + if (!api_check_entry(obj_list, 1)) >>>>>> + return NULL; >>>>>> + >>>>>> + if (!wc_self->current_context) { >>>>>> + wcore_errorf(WAFFLE_ERROR_UNKNOWN, "no current context"); >>>>>> + return NULL; >>>>>> + } >>>>>> + >>>>>> + struct json *jj = json_init(); >>>>>> + if (!jj) >>>>> Set the error state ? >>>> >>>> It will only fail if it can't get memory, in which case I don't see >>>> the point in setting error state. >>>> Throughout waffle we just return NULL if calloc() fails, without >>>> setting an error. >>>> >>> We consistently use waffle_[mc]alloc wrappers which themselves set the >>> error state. Although looking at the json implementation... that can >>> only happen in case of -ENOMEM, so if we go with my suggestion (in the >>> json patch) and add a note here that'll be amazing. >>> >>> If you can think of anything better than "Any of the following json >>> functions can only fail due to ENOMEM, and waffle_[mc]alloc already >>> sets the error state", please go ahead. >>> >>>>> >>>>>> + return NULL; >>>>>> + >>>>>> + json_appendv(jj, "{", "generic", "{", ""); >>>>>> + add_generic_info(jj, wc_self->current_context); >>>>>> + json_appendv(jj, "}", "}", ""); >>>>>> + >>>>> A similar question if the json library fails at some point ? >>>> >>>> Similar answer. (: >>>> >>> With the above said, feeding the user some garbage/incomplete data >>> sounds very wrong imho. In a similar way you expect a function to not >>> put data in the output pointer if it fails, right ? >> >> There will be no garbage/incomplete data. My json functions are >> designed so that error checking can be deferred until the end. if an >> error occurs at any point during construction all subsequent calls >> quietly do nothing, then you get a NULL when you try to retrieve the >> finished string. I even tested this by simulating allocation failures >> at each possible point during the construction of a string which >> required over 100 allocations. There were no crashes nor partial >> strings returned. >> > As you can see in the JSON, patch there is a memory leak which I was > fortunate enough to notice by yet unfortunate to misinterpret. > Apologies for bad mouthing your work. Hope I did not manage to > offend/upset you.
Not in the slightest. I value your review. Please don't worry about upsetting me. And I hope you won't mind when I push back on something. It's all for the goal of better code. _______________________________________________ waffle mailing list waffle@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/waffle