[RFCv2 3/9] gl-renderer: Add optional support for desktop OpenGL.
John Kåre Alsaker
john.kare.alsaker at gmail.com
Mon Nov 19 06:33:16 PST 2012
On Mon, Nov 19, 2012 at 9:21 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 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.
I'll rename that to BUILD_DESKTOP_OPENGL.
>
>> 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?
We should probably just use GL_RENDERER_*, although that could be confusing too.
>
>> 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?
I heard Kristian didn't like 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?
We still need the EGL OpenGL ES extension which is provided by Mesa on
desktop OpenGL.
>
>> #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