[waffle] [PATCH 05/12] waffle: add waffle_display_info_json()

Emil Velikov emil.l.velikov at gmail.com
Sun Apr 24 20:54:32 UTC 2016


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.

Regards,
Emil


More information about the waffle mailing list