[waffle] [PATCH 05/12] waffle: add waffle_display_info_json()
Frank Henigman
fjhenigman at google.com
Mon Apr 25 03:19:42 UTC 2016
On Sun, Apr 24, 2016 at 4:54 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 24 April 2016 at 20:50, Frank Henigman <fjhenigman at google.com> wrote:
>> On Sun, Apr 24, 2016 at 6:42 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>> On 21 April 2016 at 21:25, Frank Henigman <fjhenigman at google.com> wrote:
>>>> On Fri, Jan 8, 2016 at 7:40 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>> On 6 January 2016 at 21:56, Frank Henigman <fjhenigman at 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 <fjhenigman at 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.
More information about the waffle
mailing list