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 ? Thanks Emil _______________________________________________ waffle mailing list waffle@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/waffle