[PATCH v2 weston 1/3] gl-renderer: save OpenGL version in renderer context

[Emil Velikov]

Hi Arnaud,

On 11 December 2017 at 12:18, Arnaud Vrac <rawoul at gmail.com> wrote:
> This will allow to make some assumptions in further patches when GLES3
> is available.
>
There's a couple of nitpicks below, but regardless if you opt for them
or not the series is:

Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

> Signed-off-by: Arnaud Vrac <avrac at freebox.fr>
> ---
>  libweston/gl-renderer.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/libweston/gl-renderer.c b/libweston/gl-renderer.c
> index abf556f0..38ae7d60 100644
> --- a/libweston/gl-renderer.c
> +++ b/libweston/gl-renderer.c
> @@ -60,6 +60,12 @@
>  #include "shared/timespec-util.h"
>  #include "weston-egl-ext.h"
>
> +#define GR_GL_VERSION(major, minor) \
> +       (((uint32_t)(major) << 16) | (uint32_t)(minor))
> +
> +#define GR_GL_VERSION_INVALID \
> +       GR_GL_VERSION(0, 0)
> +
>  struct gl_shader {
>         GLuint program;
>         GLuint vertex_shader, fragment_shader;
> @@ -199,6 +205,8 @@ struct gl_renderer {
>
>         EGLSurface dummy_surface;
>
> +       uint32_t gl_version;
> +
>         struct wl_array vertices;
>         struct wl_array vtxcnt;
>
> @@ -3586,6 +3594,22 @@ fan_debug_repaint_binding(struct weston_keyboard *keyboard,
>         weston_compositor_damage_all(compositor);
>  }
>
> +static uint32_t
> +get_gl_version(void)
> +{
> +       const char *version;
> +       int major, minor;
> +
> +       version = (const char *) glGetString(GL_VERSION);
> +       if (version &&
> +           (sscanf(version, "%d.%d", &major, &minor) == 2 ||
> +            sscanf(version, "OpenGL ES %d.%d", &major, &minor) == 2)) {
> +               return GR_GL_VERSION(major, minor);
> +       }
> +
> +       return GR_GL_VERSION_INVALID;
I'm trying to follow the "keep the main codepath straight" approach,
since it tends to be quicker and easier to read ;-)

if (!condition1 || ...)
     // handle failure

// rest of normal code path.

> +}
> +
>  static int
>  gl_renderer_setup(struct weston_compositor *ec, EGLSurface egl_surface)
>  {
> @@ -3626,6 +3650,12 @@ gl_renderer_setup(struct weston_compositor *ec, EGLSurface egl_surface)
>                 return -1;
>         }
>
> +       gr->gl_version = get_gl_version();
> +       if (gr->gl_version == GR_GL_VERSION_INVALID) {
> +               weston_log("failed to get GL version, default to GLES 2.0\n");
I would make the message more obvious or even fail the gl_renderer_setup() call.
Weston uses weston_log("warning: ..."), or even "error" - if you fancy
the former suggestion.

-Emil