[PATCH v2] gles2-renderer: Provide an API for backends to use.
Pekka Paalanen
ppaalanen at gmail.com
Thu Oct 25 23:24:01 PDT 2012
Hi John!
since this claims to provide a new API for gles2 renderer, I will
mainly comment on the API; its naming and semantics.
On Thu, 25 Oct 2012 22:53:20 +0200
John Kåre Alsaker <john.kare.alsaker at gmail.com> wrote:
> This provides an new API for gles2-renderer. It allows to move most EGL initialization code and EGL surface creation into gles2-renderer, which makes compositor backends less dependent on gles2-renderer. As a side effect weston_output gets a per-renderer state which gles2-renderer uses to store the EGL surface.
>
Message lines should be cut well below 80 chars.
Weston_output getting a renderer private member (not per-renderer,
really) should be the main goal, not just a side-effect.
> ---
> src/compositor-android.c | 72 +++++++----------
> src/compositor-drm.c | 87 ++++++---------------
> src/compositor-wayland.c | 58 +++-----------
> src/compositor-x11.c | 71 +++--------------
> src/compositor.h | 23 +++++-
> src/gles2-renderer.c | 197 ++++++++++++++++++++++++++++++++++++++++++-----
> 6 files changed, 265 insertions(+), 243 deletions(-)
>
> diff --git a/src/compositor-android.c b/src/compositor-android.c
> index 3c0273a..bf14910 100644
> --- a/src/compositor-android.c
> +++ b/src/compositor-android.c
> @@ -145,6 +145,8 @@ android_output_destroy(struct weston_output *base)
> wl_list_remove(&output->base.link);
> weston_output_destroy(&output->base);
>
> + gles2_renderer_output_unbind(base);
> +
> android_framebuffer_destroy(output->fb);
>
> free(output);
> @@ -319,6 +321,8 @@ android_seat_create(struct android_compositor *compositor)
> static int
> android_egl_choose_config(struct android_compositor *compositor,
> struct android_framebuffer *fb,
> + EGLDisplay egl_display,
> + EGLConfig *egl_config,
> const EGLint *attribs)
> {
> EGLBoolean ret;
> @@ -332,9 +336,9 @@ android_egl_choose_config(struct android_compositor *compositor,
> * surfaceflinger/DisplayHardware/DisplayHardware.cpp
> */
>
> - compositor->base.egl_config = NULL;
> + *egl_config = NULL;
>
> - ret = eglGetConfigs(compositor->base.egl_display, NULL, 0, &count);
> + ret = eglGetConfigs(egl_display, NULL, 0, &count);
> if (ret == EGL_FALSE || count < 1)
> return -1;
>
> @@ -342,27 +346,27 @@ android_egl_choose_config(struct android_compositor *compositor,
> if (!configs)
> return -1;
>
> - ret = eglChooseConfig(compositor->base.egl_display, attribs, configs,
> + ret = eglChooseConfig(egl_display, attribs, configs,
> count, &matched);
> if (ret == EGL_FALSE || matched < 1)
> goto out;
>
> for (i = 0; i < matched; ++i) {
> EGLint id;
> - ret = eglGetConfigAttrib(compositor->base.egl_display,
> + ret = eglGetConfigAttrib(egl_display,
> configs[i], EGL_NATIVE_VISUAL_ID,
> &id);
> if (ret == EGL_FALSE)
> continue;
> if (id > 0 && fb->format == id) {
> - compositor->base.egl_config = configs[i];
> + *egl_config = configs[i];
> break;
> }
> }
>
> out:
> free(configs);
> - if (!compositor->base.egl_config)
> + if (!*egl_config)
> return -1;
>
> return 0;
> @@ -372,7 +376,8 @@ static int
> android_init_egl(struct android_compositor *compositor,
> struct android_output *output)
> {
> - EGLint eglmajor, eglminor;
> + EGLDisplay egl_display;
> + EGLConfig egl_config;
> int ret;
>
> static const EGLint config_attrs[] = {
> @@ -385,66 +390,41 @@ android_init_egl(struct android_compositor *compositor,
> EGL_NONE
> };
>
> - compositor->base.egl_display = eglGetDisplay(EGL_DEFAULT_DISPLAY);
> - if (compositor->base.egl_display == EGL_NO_DISPLAY) {
> - weston_log("Failed to create EGL display.\n");
> - print_egl_error_state();
> - return -1;
> - }
> + egl_display = gles2_renderer_init_without_config(&compositor->base, EGL_DEFAULT_DISPLAY);
>
> - ret = eglInitialize(compositor->base.egl_display, &eglmajor, &eglminor);
> - if (!ret) {
> - weston_log("Failed to initialise EGL.\n");
> + if (egl_display == EGL_NO_DISPLAY) {
> + weston_log("Failed to create EGL display.\n");
> print_egl_error_state();
> return -1;
> }
>
> - ret = android_egl_choose_config(compositor, output->fb, config_attrs);
> + ret = android_egl_choose_config(compositor, output->fb, egl_display, &egl_config, config_attrs);
Long lines should be cut to below 80 chars.
> if (ret < 0) {
> weston_log("Failed to find an EGL config.\n");
> print_egl_error_state();
> return -1;
> }
>
> - output->base.egl_surface =
> - eglCreateWindowSurface(compositor->base.egl_display,
> - compositor->base.egl_config,
> - output->fb->native_window,
> - NULL);
> - if (output->base.egl_surface == EGL_NO_SURFACE) {
> - weston_log("Failed to create FB EGLSurface.\n");
> - print_egl_error_state();
> + gles2_renderer_select_config(&compositor->base, egl_config);
> +
> + if (gles2_renderer_output_state_create_and_bind(&output->base, output->fb->native_window) < 0)
> return -1;
> - }
>
> return 0;
> }
>
> static void
> -android_fini_egl(struct android_compositor *compositor)
> -{
> - gles2_renderer_destroy(&compositor->base);
> -
> - eglMakeCurrent(compositor->base.egl_display,
> - EGL_NO_SURFACE, EGL_NO_SURFACE,
> - EGL_NO_CONTEXT);
> -
> - eglTerminate(compositor->base.egl_display);
> - eglReleaseThread();
> -}
> -
> -static void
> android_compositor_destroy(struct weston_compositor *base)
> {
> struct android_compositor *compositor = to_android_compositor(base);
>
> android_seat_destroy(compositor->seat);
>
> + gles2_renderer_destroy(base);
> +
> /* destroys outputs, too */
> weston_compositor_shutdown(&compositor->base);
>
> - android_fini_egl(compositor);
> -
> free(compositor);
> }
>
> @@ -478,17 +458,17 @@ android_compositor_create(struct wl_display *display, int argc, char *argv[],
>
> android_compositor_add_output(compositor, output);
>
> - if (gles2_renderer_init(&compositor->base) < 0)
> - goto err_egl;
> + if (gles2_renderer_outputs_ready(&compositor->base) < 0)
> + goto err_gles2;
>
> compositor->seat = android_seat_create(compositor);
> if (!compositor->seat)
> - goto err_egl;
> + goto err_gles2;
>
> return &compositor->base;
>
> -err_egl:
> - android_fini_egl(compositor);
> +err_gles2:
> + gles2_renderer_destroy(&compositor->base);
> err_output:
> android_output_destroy(&output->base);
> err_compositor:
snip...
> diff --git a/src/compositor.h b/src/compositor.h
> index 3176bfd..59eb4bf 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -154,7 +154,8 @@ enum dpms_enum {
> struct weston_output {
> uint32_t id;
>
> - EGLSurface egl_surface;
> + void *renderer_state;
> +
> struct wl_list link;
> struct wl_list resource_list;
> struct wl_global *global;
> @@ -822,8 +823,26 @@ weston_surface_destroy(struct weston_surface *surface);
> int
> weston_output_switch_mode(struct weston_output *output, struct weston_mode *mode);
>
> +struct gles2_output_state;
> +
> +int
> +gles2_renderer_init(struct weston_compositor *ec, EGLNativeDisplayType display, int alpha);
> +EGLDisplay
> +gles2_renderer_init_without_config(struct weston_compositor *ec, EGLNativeDisplayType display);
> +void
> +gles2_renderer_select_config(struct weston_compositor *ec, EGLConfig config);
gles2_renderer_select_config is more like gles2_renderer_set_config,
since it does not "select" anything.
Instead of:
gles2_renderer_init OR
(gles2_renderer_init_without_config AND gles2_renderer_select_config)
Would it not be more intuitive and explicit to have:
gles2_renderer_init AND
(config = gles2_renderer_choose_config OR config = my_funny_config) AND
gles2_renderer_set_config(config)
Here gles2_renderer_choose_config is a helper function, and the other
two must always be called. Would be a bit clearer API, IMHO, since then
you don't have two alternate functions for the same thing (init).
> int
> -gles2_renderer_init(struct weston_compositor *ec);
> +gles2_renderer_outputs_ready(struct weston_compositor *ec);
outputs_ready sounds more like a predicate than an action.
So the point here is that this function must be called when there is at
least one output properly hooked up in the gles2 renderer, right?
It's a hard one to come up with a good name.
gles2_renderer_make_current()? gles2_renderer_start()? If it is start,
then what is stop...
How about this:
- rename your gles2_renderer_init to gles2_renderer_open_display
- have gles2_renderer_init as it was, not gles2_renderer_outputs_ready
Then the calling sequence in a backend would look like:
gles2_renderer_open_display(weston_compositor, ...)
create outputs somehow
gles2_renderer_init(weston_compositor)
In the future, we should just perhaps automatically init the renderer
when the first output appears (is registered with the renderer) rather
than relying on having an output at start, and it never going away.
But that's not necessarily for this patch.
> +int
> +gles2_renderer_output_state_create_and_bind(struct weston_output *output, EGLNativeWindowType window);
> +struct gles2_output_state *
> +gles2_renderer_output_state_create(struct weston_compositor *ec, EGLNativeWindowType window);
> +void
> +gles2_renderer_output_state_destroy(struct weston_compositor *ec, struct gles2_output_state *state);
> +void
> +gles2_renderer_output_bind(struct weston_output *output, struct gles2_output_state *state);
> +void
> +gles2_renderer_output_unbind(struct weston_output *output);
This output_state create/destroy/bind/unbind is complex. Why does the
backend been to be in control of when the renderer creates its private
state for an output?
bind/unbind is asymmetric: bind simply uses a given output_state, but
unbind destroys it. That's not the usual meaning of bind and unbind, as
ownership of the object is not expected to be transferred. Especially
when remotely related to GL.
output_state_create vs. output_state_create_and_bind has the same issue
as with gles2_renderer_init vs. gles2_renderer_init_without_config.
Creating and binding outputs does not occur in so many places that it
could use a helper that does both. Remove output_state_create_and_bind
to simplify the API.
> void
> gles2_renderer_destroy(struct weston_compositor *ec);
It would be even better to make the whole API like this, if possible:
init:
gles2_renderer_create(weston_compositor, nativedisplay, ...)
get a config with gles2_renderer_choose_config or some other way like
the android backend
gles2_renderer_set_config
clean-up:
gles2_renderer_destroy
- is the counterpart to create
output handling:
gles2_renderer_output_add(weston_compositor, weston_output)
- This would do the gles2_renderer_outputs_ready thing automatically on
the first output added.
gles2_renderer_output_remove(weston_output)
- Needs to cope with the last output being removed.
- is the counterpart to add
And you never have to have even an opaque struct
gles2_output_state exposed.
I didn't carefully check if such API is possible, but that IMO should
be the goal. I believe we can fix any issues that might prevent
designing it like this.
This API would have only 5 entry points one has to use always, plus
one helper. None of the entry points are alternatives.
HTH, thanks,
pq
>
> diff --git a/src/gles2-renderer.c b/src/gles2-renderer.c
> index 544cc15..a03a7dd 100644
> --- a/src/gles2-renderer.c
> +++ b/src/gles2-renderer.c
> @@ -30,6 +30,26 @@
>
> #include "compositor.h"
>
> +struct gles2_output_state {
> + EGLSurface egl_surface;
> +};
> +
> +struct gles2_renderer {
> + struct weston_renderer base;
> +};
> +
> +static inline struct gles2_output_state *
> +get_output_state(struct weston_output *output)
> +{
> + return (struct gles2_output_state *)output->renderer_state;
> +}
> +
> +static inline struct gles2_renderer *
> +get_renderer(struct weston_compositor *ec)
> +{
> + return (struct gles2_renderer *)ec->renderer;
> +}
> +
> static const char *
> egl_error_string(EGLint code)
> {
> @@ -700,6 +720,7 @@ static void
> gles2_renderer_repaint_output(struct weston_output *output,
> pixman_region32_t *output_damage)
> {
> + struct gles2_output_state *output_state = get_output_state(output);
> struct weston_compositor *compositor = output->compositor;
> EGLBoolean ret;
> static int errored;
> @@ -712,8 +733,8 @@ gles2_renderer_repaint_output(struct weston_output *output,
>
> glViewport(0, 0, width, height);
>
> - ret = eglMakeCurrent(compositor->egl_display, output->egl_surface,
> - output->egl_surface, compositor->egl_context);
> + ret = eglMakeCurrent(compositor->egl_display, output_state->egl_surface,
> + output_state->egl_surface, compositor->egl_context);
> if (ret == EGL_FALSE) {
> if (errored)
> return;
> @@ -749,7 +770,7 @@ gles2_renderer_repaint_output(struct weston_output *output,
>
> wl_signal_emit(&output->frame_signal, output);
>
> - ret = eglSwapBuffers(compositor->egl_display, output->egl_surface);
> + ret = eglSwapBuffers(compositor->egl_display, output_state->egl_surface);
> if (ret == EGL_FALSE && !errored) {
> errored = 1;
> weston_log("Failed in eglSwapBuffers.\n");
> @@ -1157,24 +1178,167 @@ log_egl_config_info(EGLDisplay egldpy, EGLConfig eglconfig)
> weston_log_continue(" unknown\n");
> }
>
> -struct gles2_renderer {
> - struct weston_renderer base;
> -};
> +static int
> +create_renderer(struct weston_compositor *ec)
> +{
> + struct gles2_renderer *renderer = malloc(sizeof *renderer);
> +
> + if (renderer == NULL)
> + return -1;
> +
> + renderer->base.repaint_output = gles2_renderer_repaint_output;
> + renderer->base.flush_damage = gles2_renderer_flush_damage;
> + renderer->base.attach = gles2_renderer_attach;
> + renderer->base.destroy_surface = gles2_renderer_destroy_surface;
> +
> + ec->renderer = &renderer->base;
> +
> + return 0;
> +}
> +
> +static int
> +init_display(struct weston_compositor *ec, EGLNativeDisplayType display)
> +{
> + EGLint major, minor;
> +
> + if (create_renderer(ec) < 0)
> + return -1;
> +
> + ec->egl_display = eglGetDisplay(display);
> + if (ec->egl_display == NULL) {
> + weston_log("failed to create display\n");
> + print_egl_error_state();
> + return -1;
> + }
> +
> + if (!eglInitialize(ec->egl_display, &major, &minor)) {
> + weston_log("failed to initialize display\n");
> + print_egl_error_state();
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +WL_EXPORT int
> +gles2_renderer_output_state_create_and_bind(struct weston_output *output, EGLNativeWindowType window)
> +{
> + struct gles2_output_state *state = gles2_renderer_output_state_create(output->compositor, window);
> +
> + if (!state)
> + return -1;
> +
> + gles2_renderer_output_bind(output, state);
> +
> + return 0;
> +}
> +
> +WL_EXPORT struct gles2_output_state *
> +gles2_renderer_output_state_create(struct weston_compositor *ec, EGLNativeWindowType window)
> +{
> + struct gles2_output_state *state = calloc(1, sizeof *state);
> +
> + if (!state)
> + return NULL;
> +
> + state->egl_surface =
> + eglCreateWindowSurface(ec->egl_display,
> + ec->egl_config,
> + window, NULL);
> +
> + if (state->egl_surface == EGL_NO_SURFACE) {
> + weston_log("failed to create egl surface\n");
> + free(state);
> + return NULL;
> + }
> +
> + return state;
> +}
> +
> +WL_EXPORT void
> +gles2_renderer_output_state_destroy(struct weston_compositor *ec, struct gles2_output_state *state)
> +{
> + eglDestroySurface(ec->egl_display, state->egl_surface);
> +
> + free(state);
> +}
> +
> +WL_EXPORT void
> +gles2_renderer_output_bind(struct weston_output *output, struct gles2_output_state *state)
> +{
> + output->renderer_state = state;
> +}
> +
> +WL_EXPORT void
> +gles2_renderer_output_unbind(struct weston_output *output)
> +{
> + gles2_renderer_output_state_destroy(output->compositor, get_output_state(output));
> +}
>
> WL_EXPORT void
> gles2_renderer_destroy(struct weston_compositor *ec)
> {
> if (ec->has_bind_display)
> ec->unbind_display(ec->egl_display, ec->wl_display);
> +
> + /* Work around crash in egl_dri2.c's dri2_make_current() - when does this apply? */
> + eglMakeCurrent(ec->egl_display,
> + EGL_NO_SURFACE, EGL_NO_SURFACE,
> + EGL_NO_CONTEXT);
> +
> + eglTerminate(ec->egl_display);
> + eglReleaseThread();
> +}
> +
> +WL_EXPORT int
> +gles2_renderer_init(struct weston_compositor *ec, EGLNativeDisplayType display, int alpha)
> +{
> + EGLint n;
> +
> + const EGLint config_attribs[] = {
> + EGL_SURFACE_TYPE, EGL_WINDOW_BIT,
> + EGL_RED_SIZE, 1,
> + EGL_GREEN_SIZE, 1,
> + EGL_BLUE_SIZE, 1,
> + EGL_ALPHA_SIZE, alpha ? 1 : 0,
> + EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
> + EGL_NONE
> + };
> +
> + if (init_display(ec, display) < 0)
> + return -1;
> +
> + if (!eglChooseConfig(ec->egl_display, config_attribs,
> + &ec->egl_config, 1, &n) || n != 1) {
> + weston_log("failed to choose config: %d\n", n);
> + print_egl_error_state();
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +WL_EXPORT EGLDisplay
> +gles2_renderer_init_without_config(struct weston_compositor *ec, EGLNativeDisplayType display)
> +{
> + if (init_display(ec, display) < 0)
> + return EGL_NO_DISPLAY;
> + else
> + return ec->egl_display;
> +}
> +
> +void
> +gles2_renderer_select_config(struct weston_compositor *ec, EGLConfig config)
> +{
> + ec->egl_config = config;
> }
>
> WL_EXPORT int
> -gles2_renderer_init(struct weston_compositor *ec)
> +gles2_renderer_outputs_ready(struct weston_compositor *ec)
> {
> - struct gles2_renderer *renderer;
> const char *extensions;
> int has_egl_image_external = 0;
> - struct weston_output *output;
> + struct gles2_output_state *output;
> EGLBoolean ret;
>
> static const EGLint context_attribs[] = {
> @@ -1182,10 +1346,6 @@ gles2_renderer_init(struct weston_compositor *ec)
> EGL_NONE
> };
>
> - renderer = malloc(sizeof *renderer);
> - if (renderer == NULL)
> - return -1;
> -
> if (!eglBindAPI(EGL_OPENGL_ES_API)) {
> weston_log("failed to bind EGL_OPENGL_ES_API\n");
> print_egl_error_state();
> @@ -1202,8 +1362,8 @@ gles2_renderer_init(struct weston_compositor *ec)
> return -1;
> }
>
> - output = container_of(ec->output_list.next,
> - struct weston_output, link);
> + output = get_output_state(container_of(ec->output_list.next,
> + struct weston_output, link));
> ret = eglMakeCurrent(ec->egl_display, output->egl_surface,
> output->egl_surface, ec->egl_context);
> if (ret == EGL_FALSE) {
> @@ -1289,12 +1449,6 @@ gles2_renderer_init(struct weston_compositor *ec)
> vertex_shader, solid_fragment_shader) < 0)
> return -1;
>
> - renderer->base.repaint_output = gles2_renderer_repaint_output;
> - renderer->base.flush_damage = gles2_renderer_flush_damage;
> - renderer->base.attach = gles2_renderer_attach;
> - renderer->base.destroy_surface = gles2_renderer_destroy_surface;
> - ec->renderer = &renderer->base;
> -
> weston_log("GL ES 2 renderer features:\n");
> weston_log_continue(STAMP_SPACE "read-back format: %s\n",
> ec->read_format == GL_BGRA_EXT ? "BGRA" : "RGBA");
> @@ -1303,5 +1457,6 @@ gles2_renderer_init(struct weston_compositor *ec)
> weston_log_continue(STAMP_SPACE "EGL Wayland extension: %s\n",
> ec->has_bind_display ? "yes" : "no");
>
> +
> return 0;
> }
More information about the wayland-devel
mailing list