[PATCH weston 1/2] gl-renderer: Take a list of acceptable formats in create functions
Bryce Harrington
bryce at osg.samsung.com
Thu May 14 20:14:53 PDT 2015
On Mon, May 11, 2015 at 02:19:02PM -0500, Derek Foreman wrote:
> Currently we pass either a single format or no formats to the gl renderer
> create and output_create functions. We extend this to any number of
> formats so we can allow fallback formats if we don't get our first pick.
>
> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
This all looks fine, but I have some bikesheddy comments. I'll give a
R-B and you can decide whether or not my suggestions have any utility.
Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>
> ---
> src/compositor-drm.c | 5 ++--
> src/compositor-fbdev.c | 4 +--
> src/compositor-wayland.c | 12 +++++----
> src/compositor-x11.c | 5 ++--
> src/gl-renderer.c | 68 +++++++++++++++++++++++++++++++-----------------
> src/gl-renderer.h | 6 +++--
> 6 files changed, 63 insertions(+), 37 deletions(-)
>
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index 0cdb8f4..69bdcfd 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -1398,7 +1398,7 @@ drm_compositor_create_gl_renderer(struct drm_compositor *ec)
>
> format = ec->format;
> if (gl_renderer->create(&ec->base, EGL_PLATFORM_GBM_KHR, (void *) ec->gbm,
> - gl_renderer->opaque_attribs, &format) < 0) {
> + gl_renderer->opaque_attribs, &format, 1) < 0) {
> return -1;
> }
>
> @@ -1620,7 +1620,8 @@ drm_output_init_egl(struct drm_output *output, struct drm_compositor *ec)
> (EGLNativeDisplayType)output->surface,
> output->surface,
> gl_renderer->opaque_attribs,
> - &format) < 0) {
> + &format,
> + 1) < 0) {
> weston_log("failed to create gl renderer output state\n");
> gbm_surface_destroy(output->surface);
> return -1;
> diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
> index 7c505ce..3f3394f 100644
> --- a/src/compositor-fbdev.c
> +++ b/src/compositor-fbdev.c
> @@ -570,7 +570,7 @@ fbdev_output_create(struct fbdev_compositor *compositor,
> if (gl_renderer->output_create(&output->base,
> (EGLNativeWindowType)NULL, NULL,
> gl_renderer->opaque_attribs,
> - NULL) < 0) {
> + NULL, 0) < 0) {
> weston_log("gl_renderer_output_create failed.\n");
> goto out_shadow_surface;
> }
> @@ -871,7 +871,7 @@ fbdev_compositor_create(struct wl_display *display, int *argc, char *argv[],
> if (gl_renderer->create(&compositor->base, NO_EGL_PLATFORM,
> EGL_DEFAULT_DISPLAY,
> gl_renderer->opaque_attribs,
> - NULL) < 0) {
> + NULL, 0) < 0) {
> weston_log("gl_renderer_create failed.\n");
> goto out_launcher;
> }
> diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
> index 303151c..c9983e0 100644
> --- a/src/compositor-wayland.c
> +++ b/src/compositor-wayland.c
> @@ -648,7 +648,8 @@ wayland_output_init_gl_renderer(struct wayland_output *output)
> output->gl.egl_window,
> output->gl.egl_window,
> gl_renderer->alpha_attribs,
> - NULL) < 0)
> + NULL,
> + 0) < 0)
> goto cleanup_window;
>
> return 0;
> @@ -1970,10 +1971,11 @@ wayland_compositor_create(struct wl_display *display, int use_pixman,
>
> if (!c->use_pixman) {
> if (gl_renderer->create(&c->base,
> - EGL_PLATFORM_WAYLAND_KHR,
> - c->parent.wl_display,
> - gl_renderer->alpha_attribs,
> - NULL) < 0) {
> + EGL_PLATFORM_WAYLAND_KHR,
> + c->parent.wl_display,
> + gl_renderer->alpha_attribs,
> + NULL,
> + 0) < 0) {
> weston_log("Failed to initialize the GL renderer; "
> "falling back to pixman.\n");
> c->use_pixman = 1;
> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> index 5129e85..3565677 100644
> --- a/src/compositor-x11.c
> +++ b/src/compositor-x11.c
> @@ -906,7 +906,8 @@ x11_compositor_create_output(struct x11_compositor *c, int x, int y,
> (EGLNativeWindowType) output->window,
> &xid,
> gl_renderer->opaque_attribs,
> - NULL);
> + NULL,
> + 0);
> if (ret < 0)
> return NULL;
> }
> @@ -1493,7 +1494,7 @@ init_gl_renderer(struct x11_compositor *c)
> return -1;
>
> ret = gl_renderer->create(&c->base, EGL_PLATFORM_X11_KHR, (void *) c->dpy,
> - gl_renderer->opaque_attribs, NULL);
> + gl_renderer->opaque_attribs, NULL, 0);
>
> return ret;
> }
> diff --git a/src/gl-renderer.c b/src/gl-renderer.c
> index ae3122f..6746a9b 100644
> --- a/src/gl-renderer.c
> +++ b/src/gl-renderer.c
> @@ -1898,14 +1898,37 @@ log_egl_config_info(EGLDisplay egldpy, EGLConfig eglconfig)
> }
>
> static int
> +match_config(EGLDisplay egl_display,
This function name seems a bit non-descript.
match_config_to_visual() maybe? Or match_visual_config()?
> + EGLint visual_id,
> + EGLConfig *configs,
> + int count)
> +{
> + int i;
> +
> + for (i = 0; i < count; ++i) {
> + EGLint id;
> +
> + if (!eglGetConfigAttrib(egl_display,
> + configs[i], EGL_NATIVE_VISUAL_ID,
> + &id))
> + continue;
I gather eglGetConfigAttrib logs an error code somewhere when it fails?
> + if (id == visual_id)
> + return i;
> + }
> +
For future debugging purposes, might be friendly to log an error here.
> + return -1;
> +}
> +
> +static int
> egl_choose_config(struct gl_renderer *gr, const EGLint *attribs,
> - const EGLint *visual_id,
> + const EGLint *visual_id, const int n_ids,
> EGLConfig *config_out)
Since this appears to just return 0 or -1, would it be clearer to change
it to returning just a boolean?
> {
> EGLint count = 0;
> EGLint matched = 0;
> EGLConfig *configs;
> - int i;
> + int i, config_index = -1;
>
> if (!eglGetConfigs(gr->egl_display, NULL, 0, &count) || count < 1)
> return -1;
> @@ -1915,31 +1938,25 @@ egl_choose_config(struct gl_renderer *gr, const EGLint *attribs,
> return -1;
>
> if (!eglChooseConfig(gr->egl_display, attribs, configs,
> - count, &matched))
> + count, &matched) || !matched)
> goto out;
>
> - for (i = 0; i < matched; ++i) {
> - EGLint id;
> + if (!visual_id)
> + config_index = 0;
>
> - if (visual_id) {
> - if (!eglGetConfigAttrib(gr->egl_display,
> - configs[i], EGL_NATIVE_VISUAL_ID,
> - &id))
> - continue;
> + for (i = 0; config_index == -1 && i < n_ids; i++)
> + config_index = match_config(gr->egl_display, visual_id[i],
> + configs, matched);
>
> - if (id != 0 && id != *visual_id)
> - continue;
> - }
> -
> - *config_out = configs[i];
> -
> - free(configs);
> - return 0;
> - }
> + if (config_index != -1)
> + *config_out = configs[config_index];
>
> out:
> free(configs);
> - return -1;
> + if (config_index == -1)
> + return -1;
> +
> + return 0;
> }
The logic above with the config_index seems a bit clunky to me, but I
can't seem to work out something less clunky. My sense is that
overloading the return value of match_config() to both return indexes
and error codes makes handling the result more difficult.
Perhaps instead of an index, what if you had match_config() returned a
pointer to the appropriate configs[] element (or NULL if none were
found)?
> static void
> @@ -1976,7 +1993,8 @@ gl_renderer_output_create(struct weston_output *output,
> EGLNativeWindowType window_for_legacy,
> void *window_for_platform,
> const EGLint *attribs,
> - const EGLint *visual_id)
> + const EGLint *visual_id,
> + int n_ids)
> {
> struct weston_compositor *ec = output->compositor;
> struct gl_renderer *gr = get_renderer(ec);
> @@ -1984,7 +2002,8 @@ gl_renderer_output_create(struct weston_output *output,
> EGLConfig egl_config;
> int i;
>
> - if (egl_choose_config(gr, attribs, visual_id, &egl_config) == -1) {
> + if (egl_choose_config(gr, attribs, visual_id,
> + n_ids, &egl_config) == -1) {
> weston_log("failed to choose EGL config for output\n");
> return -1;
> }
> @@ -2260,7 +2279,7 @@ platform_to_extension(EGLenum platform)
> static int
> gl_renderer_create(struct weston_compositor *ec, EGLenum platform,
> void *native_window, const EGLint *attribs,
> - const EGLint *visual_id)
> + const EGLint *visual_id, int n_formats)
> {
> struct gl_renderer *gr;
> EGLint major, minor;
> @@ -2323,7 +2342,8 @@ gl_renderer_create(struct weston_compositor *ec, EGLenum platform,
> goto err_egl;
> }
>
> - if (egl_choose_config(gr, attribs, visual_id, &gr->egl_config) < 0) {
> + if (egl_choose_config(gr, attribs, visual_id,
> + n_formats, &gr->egl_config) < 0) {
> weston_log("failed to choose EGL config\n");
> goto err_egl;
> }
> diff --git a/src/gl-renderer.h b/src/gl-renderer.h
> index ebc139f..ceb4206 100644
> --- a/src/gl-renderer.h
> +++ b/src/gl-renderer.h
> @@ -76,7 +76,8 @@ struct gl_renderer_interface {
> EGLenum platform,
> void *native_window,
> const EGLint *attribs,
> - const EGLint *visual_id);
> + const EGLint *visual_id,
> + const int n_ids);
>
> EGLDisplay (*display)(struct weston_compositor *ec);
>
> @@ -84,7 +85,8 @@ struct gl_renderer_interface {
> EGLNativeWindowType window_for_legacy,
> void *window_for_platform,
> const EGLint *attribs,
> - const EGLint *visual_id);
> + const EGLint *visual_id,
> + const int n_ids);
>
> void (*output_destroy)(struct weston_output *output);
Bryce
More information about the wayland-devel
mailing list