[RFCv2 3/9] gl-renderer: Add optional support for desktop OpenGL.
Pekka Paalanen
ppaalanen at gmail.com
Mon Nov 19 00:21:28 PST 2012
On Sat, 17 Nov 2012 03:23:54 +0100
John Kåre Alsaker <john.kare.alsaker at gmail.com> wrote:
> This adds support for desktop OpenGL which can be enabled by with
> ./configure --enable-opengl.
>
> Most of the differences in API between OpenGL and OpenGL ES is hidden by
> the new gl_renderer fields.
>
> It also accesses GLES2 extensions by including GLES2/gl2platform.h directly.
Hi John,
I'm only commenting on some details of this patch, I'm concerned about
the differences between desktop GL and GLESv2. I'm assuming the
general approach is fine, and not evaluating that.
> ---
> configure.ac | 13 +++++++++-
> src/Makefile.am | 4 +--
> src/compositor-rpi.c | 2 +-
> src/compositor.h | 1 +
> src/gl-internal.h | 7 +++---
> src/gl-renderer.c | 69 +++++++++++++++++++++++++++++++++++-----------------
> src/gl-renderer.h | 19 +++++++++++++++
> src/gl-shaders.c | 5 ++--
> 8 files changed, 89 insertions(+), 31 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 7329c06..df43c75 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -34,7 +34,18 @@ AC_CHECK_HEADERS([execinfo.h])
> AC_CHECK_FUNCS([mkostemp strchrnul])
>
> PKG_CHECK_MODULES(COMPOSITOR,
> - [wayland-server egl >= 7.10 glesv2 xkbcommon pixman-1])
> + [wayland-server egl >= 7.10 xkbcommon pixman-1])
> +
> +
> +AC_ARG_ENABLE(opengl, [ --enable-opengl],,
> + enable_opengl=no)
> +AM_CONDITIONAL(ENABLE_OPENGL, test x$enable_opengl = xyes)
> +if test x$enable_opengl = xyes; then
> + PKG_CHECK_MODULES([OPENGL], gl)
> + AC_DEFINE([BUILD_OPENGL], [1], [Build using desktop OpenGL])
> +else
> + PKG_CHECK_MODULES([OPENGL], glesv2)
> +fi
This produces some strange looking results. If you are on GLESv2, then
BUILD_OPENGL will not be defined, yet OPENGL_{LIBS,CFLAGS} will be used
and needed. Perhaps rename BUILD_OPENGL to HAVE_DESKTOP_OPENGL or
something, so it's less confusing.
> AC_ARG_ENABLE(setuid-install, [ --enable-setuid-install],,
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 0c315cd..bea769c 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -7,8 +7,8 @@ AM_CPPFLAGS = \
> -DLIBEXECDIR='"$(libexecdir)"'
>
> weston_LDFLAGS = -export-dynamic
> -weston_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS)
> -weston_LDADD = $(COMPOSITOR_LIBS) $(DLOPEN_LIBS) -lm ../shared/libshared.la
> +weston_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS) $(OPENGL_CFLAGS)
> +weston_LDADD = $(COMPOSITOR_LIBS) $(OPENGL_LIBS) $(DLOPEN_LIBS) -lm ../shared/libshared.la
>
> weston_SOURCES = \
> git-version.h \
> diff --git a/src/compositor-rpi.c b/src/compositor-rpi.c
> index 339032a..d3ae952 100644
> --- a/src/compositor-rpi.c
> +++ b/src/compositor-rpi.c
> @@ -1457,7 +1457,7 @@ rpi_compositor_create(struct wl_display *display, int argc, char *argv[],
> EGL_GREEN_SIZE, 1,
> EGL_BLUE_SIZE, 1,
> EGL_ALPHA_SIZE, 0,
> - EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
> + EGL_RENDERABLE_TYPE, GL_EGL_OPENGL_BIT,
This looks wrong at first, it appears that you are using a GL value in
an EGL call. Even if you invented GL_EGL_OPENGL_BIT yourself (google
does not know about it), the name should be clearly distinguishable from
official GL and EGL macros.
Yeah, IIRC there was another similar thing already in window.c, and
that's not good either.
So, we'd need a name that is clearly our own. How about WESTON_EGL_*
and WESTON_GL_* name spaces?
> EGL_NONE
> };
>
> diff --git a/src/compositor.h b/src/compositor.h
> index e5c579b..a5dd3c6 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -24,6 +24,7 @@
> #ifndef _WAYLAND_SYSTEM_COMPOSITOR_H_
> #define _WAYLAND_SYSTEM_COMPOSITOR_H_
>
> +#include <config.h>
> #include <pixman.h>
> #include <xkbcommon/xkbcommon.h>
> #include <wayland-server.h>
> diff --git a/src/gl-internal.h b/src/gl-internal.h
> index 994c139..83b351f 100644
> --- a/src/gl-internal.h
> +++ b/src/gl-internal.h
> @@ -24,9 +24,6 @@
> #ifndef _GL_INTERNAL_H_
> #define _GL_INTERNAL_H_
>
> -#include <GLES2/gl2.h>
> -#include <GLES2/gl2ext.h>
> -
> #include <stdlib.h>
> #include <string.h>
> #include <ctype.h>
> @@ -106,6 +103,10 @@ struct gl_renderer {
> int32_t width, height;
> } border;
>
> + GLenum bgra_internal_format, bgra_format;
> + GLenum rgba16_internal_format;
> + GLenum short_type;
These are chosen only at compile time, right? Not runtime, so we don't
really need these as variables. Maybe WESTON_GL_* macros?
> +
> PFNGLEGLIMAGETARGETTEXTURE2DOESPROC image_target_texture_2d;
> PFNEGLCREATEIMAGEKHRPROC create_image;
> PFNEGLDESTROYIMAGEKHRPROC destroy_image;
> diff --git a/src/gl-renderer.c b/src/gl-renderer.c
> index 1a9f782..1956759 100644
> --- a/src/gl-renderer.c
> +++ b/src/gl-renderer.c
> @@ -878,11 +878,12 @@ gl_renderer_read_pixels(struct weston_output *output,
> uint32_t x, uint32_t y,
> uint32_t width, uint32_t height)
> {
> + struct gl_renderer *gr = get_renderer(output->compositor);
> GLenum gl_format;
>
> switch (format) {
> case PIXMAN_a8r8g8b8:
> - gl_format = GL_BGRA_EXT;
> + gl_format = gr->bgra_format;
> break;
> case PIXMAN_a8b8g8r8:
> gl_format = GL_RGBA;
> @@ -928,9 +929,9 @@ gl_renderer_flush_damage(struct weston_surface *surface)
> glBindTexture(GL_TEXTURE_2D, gs->textures[0]);
>
> if (!gr->has_unpack_subimage) {
> - glTexImage2D(GL_TEXTURE_2D, 0, GL_BGRA_EXT,
> + glTexImage2D(GL_TEXTURE_2D, 0, gr->bgra_internal_format,
> surface->pitch, surface->buffer->height, 0,
> - GL_BGRA_EXT, GL_UNSIGNED_BYTE,
> + gr->bgra_format, GL_UNSIGNED_BYTE,
> wl_shm_buffer_get_data(surface->buffer));
>
> goto done;
> @@ -948,7 +949,7 @@ gl_renderer_flush_damage(struct weston_surface *surface)
> rectangles[i].x1, rectangles[i].y1,
> rectangles[i].x2 - rectangles[i].x1,
> rectangles[i].y2 - rectangles[i].y1,
> - GL_BGRA_EXT, GL_UNSIGNED_BYTE, data);
> + gr->bgra_format, GL_UNSIGNED_BYTE, data);
> }
> #endif
>
> @@ -1003,9 +1004,9 @@ gl_renderer_attach(struct weston_surface *es, struct wl_buffer *buffer)
>
> ensure_textures(gs, 1);
> glBindTexture(GL_TEXTURE_2D, gs->textures[0]);
> - glTexImage2D(GL_TEXTURE_2D, 0, GL_BGRA_EXT,
> + glTexImage2D(GL_TEXTURE_2D, 0, gr->bgra_internal_format,
> es->pitch, buffer->height, 0,
> - GL_BGRA_EXT, GL_UNSIGNED_BYTE, NULL);
> + gr->bgra_format, GL_UNSIGNED_BYTE, NULL);
> if (wl_shm_buffer_get_format(buffer) == WL_SHM_FORMAT_XRGB8888)
> gs->input = INPUT_RGBX;
> else
> @@ -1229,10 +1230,10 @@ gl_renderer_set_border(struct weston_compositor *ec, int32_t width, int32_t heig
> glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
> glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
>
> - glTexImage2D(GL_TEXTURE_2D, 0, GL_BGRA_EXT,
> + glTexImage2D(GL_TEXTURE_2D, 0, gr->bgra_internal_format,
> width,
> height,
> - 0, GL_BGRA_EXT, GL_UNSIGNED_BYTE,
> + 0, gr->bgra_format, GL_UNSIGNED_BYTE,
> data);
>
> wl_list_for_each(output, &ec->output_list, link)
> @@ -1366,7 +1367,7 @@ WL_EXPORT const EGLint gl_renderer_opaque_attribs[] = {
> EGL_GREEN_SIZE, 1,
> EGL_BLUE_SIZE, 1,
> EGL_ALPHA_SIZE, 0,
> - EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
> + EGL_RENDERABLE_TYPE, GL_EGL_OPENGL_BIT,
> EGL_NONE
> };
>
> @@ -1376,7 +1377,7 @@ WL_EXPORT const EGLint gl_renderer_alpha_attribs[] = {
> EGL_GREEN_SIZE, 1,
> EGL_BLUE_SIZE, 1,
> EGL_ALPHA_SIZE, 1,
> - EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
> + EGL_RENDERABLE_TYPE, GL_EGL_OPENGL_BIT,
> EGL_NONE
> };
>
> @@ -1452,19 +1453,49 @@ gl_renderer_setup(struct weston_compositor *ec, EGLSurface egl_surface)
> const char *extensions;
> EGLBoolean ret;
>
> +#ifdef BUILD_OPENGL
> + static const EGLint context_attribs[] = {
> + EGL_CONTEXT_MAJOR_VERSION_KHR, 2,
> + EGL_CONTEXT_MINOR_VERSION_KHR, 0,
> + EGL_NONE
> + };
> +
> + gr->bgra_internal_format = GL_RGBA;
> + gr->bgra_format = GL_BGRA;
> + gr->short_type = GL_UNSIGNED_SHORT;
> + gr->rgba16_internal_format = GL_RGBA16;
> +#else
> static const EGLint context_attribs[] = {
> EGL_CONTEXT_CLIENT_VERSION, 2,
> EGL_NONE
> };
>
> - if (!eglBindAPI(EGL_OPENGL_ES_API)) {
> - weston_log("failed to bind EGL_OPENGL_ES_API\n");
> + gr->bgra_internal_format = GL_BGRA_EXT;
> + gr->bgra_format = GL_BGRA_EXT;
> + gr->short_type = GL_UNSIGNED_BYTE;
> + gr->rgba16_internal_format = GL_RGBA;
> +#endif
> +
> + if (!eglBindAPI(OPENGL_ES_VER ? EGL_OPENGL_ES_API : EGL_OPENGL_API)) {
> + weston_log("failed to bind OpenGL API");
> print_egl_error_state();
> return -1;
> }
>
> log_egl_config_info(gr->egl_display, gr->egl_config);
>
> + extensions =
> + (const char *) eglQueryString(gr->egl_display, EGL_EXTENSIONS);
> + if (!extensions) {
> + weston_log("Retrieving EGL extension string failed.\n");
> + return -1;
> + }
> +
> + if (!OPENGL_ES_VER && !strstr(extensions, "EGL_KHR_create_context")) {
> + weston_log("EGL_KHR_create_context required to create OpenGL context\n");
> + return -1;
> + }
> +
> gr->egl_context = eglCreateContext(gr->egl_display, gr->egl_config,
> EGL_NO_CONTEXT, context_attribs);
> if (gr->egl_context == NULL) {
> @@ -1494,13 +1525,16 @@ gl_renderer_setup(struct weston_compositor *ec, EGLSurface egl_surface)
> gr->query_buffer =
> (void *) eglGetProcAddress("eglQueryWaylandBufferWL");
>
> + if (strstr(extensions, "EGL_WL_bind_wayland_display"))
> + gr->has_bind_display = 1;
> +
> extensions = (const char *) glGetString(GL_EXTENSIONS);
> if (!extensions) {
> weston_log("Retrieving GL extension string failed.\n");
> return -1;
> }
>
> - if (!strstr(extensions, "GL_EXT_texture_format_BGRA8888")) {
> + if (OPENGL_ES_VER && !strstr(extensions, "GL_EXT_texture_format_BGRA8888")) {
> weston_log("GL_EXT_texture_format_BGRA8888 not available\n");
> return -1;
> }
> @@ -1516,15 +1550,6 @@ gl_renderer_setup(struct weston_compositor *ec, EGLSurface egl_surface)
> if (strstr(extensions, "GL_OES_EGL_image_external"))
> gr->has_egl_image_external = 1;
>
> - extensions =
> - (const char *) eglQueryString(gr->egl_display, EGL_EXTENSIONS);
> - if (!extensions) {
> - weston_log("Retrieving EGL extension string failed.\n");
> - return -1;
> - }
> -
> - if (strstr(extensions, "EGL_WL_bind_wayland_display"))
> - gr->has_bind_display = 1;
> if (gr->has_bind_display) {
> ret = gr->bind_display(gr->egl_display, ec->wl_display);
> if (!ret)
>
> diff --git a/src/gl-renderer.h b/src/gl-renderer.h
> index 7494cff..10bc542 100644
> --- a/src/gl-renderer.h
> +++ b/src/gl-renderer.h
> @@ -22,6 +22,25 @@
>
> #include "compositor.h"
>
> +#ifdef BUILD_OPENGL
> +
> +#define OPENGL_ES_VER 0
> +#define GL_GLEXT_PROTOTYPES
> +#define GL_EGL_OPENGL_BIT EGL_OPENGL_BIT
> +
> +#include <GL/gl.h>
> +#include <GL/glext.h>
> +#include <GLES2/gl2platform.h>
I believe this is wrong, you should never mix desktop GL and GLES
headers, nor libraries.
> +
> +#else
> +
> +#define OPENGL_ES_VER 2
> +#define GL_EGL_OPENGL_BIT EGL_OPENGL_ES2_BIT
> +#include <GLES2/gl2.h>
> +
> +#endif
> +
> +#include <GLES2/gl2ext.h>
And this especially.
Or does something say that it's ok to mix them?
How do you know gl2ext.h even exists on a desktop-GL system?
> #include <EGL/egl.h>
>
> extern const EGLint gl_renderer_opaque_attribs[];
> diff --git a/src/gl-shaders.c b/src/gl-shaders.c
> index 060a87d..32bb70d 100644
> --- a/src/gl-shaders.c
> +++ b/src/gl-shaders.c
> @@ -423,8 +423,9 @@ create_shader_permutation(struct gl_renderer *renderer,
>
> shader_builder_init(&sb);
>
> - append(&sb.global, "precision mediump float;\n" \
> - "varying vec2 texture_coord;\n" \
> + if (OPENGL_ES_VER)
> + append(&sb.global, "precision mediump float;\n");
> + append(&sb.global, "varying vec2 texture_coord;\n" \
> "uniform float alpha;\n");
>
> append(&sb.body, "void main()\n{\n");
Thanks,
pq
More information about the wayland-devel
mailing list