[PATCH v2] gles2-renderer: Provide an API for backends to use.
John Kåre Alsaker
john.kare.alsaker at gmail.com
Fri Oct 26 05:31:40 PDT 2012
On Fri, Oct 26, 2012 at 8:24 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 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.
It was more of a separate goal, but ended up being required for this.
>
>> ---
>> 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).
The gles2_renderer_init_without_config and
gles2_renderer_select_config combo is only there to make the Android
backend work, since it does some weird things. I'd rather have those
two functions go away. I don't like the explicit path because it
exposes and makes the backend work with EGL types directly. It also
adds more error logic to the already long backend initialization
functions.
>
>> 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)
I'd say that's more confusing as none of the gles2_renderer functions
(exposed in this API) depend on gles2_renderer_outputs_ready being
called.
>
> 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.
Yeah, that could be moved into gl_renderer_output_bind. I'd say we
just keep gles2_renderer_init/gles2_renderer_outputs_ready and get rid
of the latter in a later 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.
This is so it can encapsulate EGLSurfaces and do mode switching
atomically. It has to try to create the renderer state (which can
fail) and only then try do the mode switch which calls
gles2_renderer_output_bind if the succeeds or
gles2_renderer_output_state_destroy if it fails.
>
> 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.
It's used once in each backend, only the drm backend handles
gles2_output_state directly because it does mode switches.
>
>> 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, ...)
We could rename init to create, although the code base is quite
inconsistent about this.
>
> 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.
That won't fly for mode switching.
>
> 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