[PATCH weston v2] clients: Require EGL_MIN_SWAP_INTERVAL to be 0 for subsurfaces

[Jonas Ådahl]

On Fri, Aug 21, 2015 at 04:05:34PM +0300, Pekka Paalanen wrote:
> On Tue, 27 Jan 2015 10:47:29 +0800
> Jonas Ådahl <jadahl at gmail.com> wrote:
> 
> > Warn and fail when assert to create sub surfaces when swap interval 0 is
> > not supported by the EGL platform.
> > 
> > Programs wanting to add subsurfaces need to check whether it is
> > subsurfaces are supported by the display with the new
> > display_is_subsurfaces_supported() function.
> > 
> > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > ---
> > 
> > Changed since v1:
> > 
> > Coding style fix.
> > 
> > Require clients to check whether subsurfaces is supported or not prior to
> > adding them.
> > 
> > 
> >  clients/nested.c      |  6 ++++++
> >  clients/subsurfaces.c |  6 ++++++
> >  clients/window.c      | 30 ++++++++++++++++++++++++++++++
> >  clients/window.h      |  4 ++++
> >  4 files changed, 46 insertions(+)
> > 
> > diff --git a/clients/nested.c b/clients/nested.c
> > index f094237..f52173e 100644
> > --- a/clients/nested.c
> > +++ b/clients/nested.c
> > @@ -1127,6 +1127,12 @@ main(int argc, char *argv[])
> >  		return -1;
> >  	}
> >  
> > +	if (!display_is_subsurfaces_supported(display)) {
> > +		fprintf(stderr, "subsurface support required\n");
> > +		display_destroy(display);
> > +		return -1;
> > +	}
> > +
> >  	nested = nested_create(display);
> >  
> >  	launch_client(nested, "weston-nested-client");
> > diff --git a/clients/subsurfaces.c b/clients/subsurfaces.c
> > index fcbe496..8376399 100644
> > --- a/clients/subsurfaces.c
> > +++ b/clients/subsurfaces.c
> > @@ -792,6 +792,12 @@ main(int argc, char *argv[])
> >  		return -1;
> >  	}
> >  
> > +	if (!display_is_subsurfaces_supported(display)) {
> > +		fprintf(stderr, "subsurface support required\n");
> > +		display_destroy(display);
> > +		return -1;
> > +	}
> > +
> >  	app = demoapp_create(display);
> >  
> >  	display_run(display);
> > diff --git a/clients/window.c b/clients/window.c
> > index b45b499..3b95896 100644
> > --- a/clients/window.c
> > +++ b/clients/window.c
> > @@ -2023,6 +2023,9 @@ window_create_tooltip(struct tooltip *tooltip)
> >  
> >  	tooltip->widget = window_add_subsurface(parent->window, tooltip, SUBSURFACE_DESYNCHRONIZED);
> >  
> > +	if (!tooltip->widget)
> > +		return -1;
> > +
> >  	extents = get_text_extents(display, tooltip);
> >  	widget_set_redraw_handler(tooltip->widget, tooltip_redraw_handler);
> >  	widget_set_allocation(tooltip->widget,
> > @@ -4845,6 +4848,8 @@ window_add_subsurface(struct window *window, void *data,
> >  	struct wl_surface *parent;
> >  	struct wl_subcompositor *subcompo = window->display->subcompositor;
> >  
> > +	assert(display_is_subsurfaces_supported(window->display));
> > +
> >  	surface = surface_create(window);
> >  	surface->buffer_type = window_get_buffer_type(window);
> >  	widget = widget_create(window, surface, data);
> > @@ -5620,6 +5625,31 @@ display_has_subcompositor(struct display *display)
> >  	return display->subcompositor != NULL;
> >  }
> >  
> > +bool
> > +display_is_subsurfaces_supported(struct display *display)
> 
> Hi,
> 
> this name is quite misleading. It's more like "is EGL supported for
> sub-surfaces?"

True.

> 
> > +{
> > +	GLint min_swap_interval;
> > +
> > +	if (!display_has_subcompositor(display)) {
> > +		return false;
> > +	}
> > +
> > +	if (!eglGetConfigAttrib(display->dpy,
> > +				display->argb_config,
> > +				EGL_MIN_SWAP_INTERVAL,
> > +				&min_swap_interval)) {
> > +		fprintf(stderr, "failed to retrieve EGL_MIN_SWAP_INTERVAL\n");
> > +		return false;
> > +	}
> > +
> > +	if (min_swap_interval > 0) {
> > +		fprintf(stderr, "warning: EGLSwapInterval(0) not supported\n");
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> >  cairo_device_t *
> >  display_get_cairo_device(struct display *display)
> >  {
> > diff --git a/clients/window.h b/clients/window.h
> > index 5247f19..b47e0b2 100644
> > --- a/clients/window.h
> > +++ b/clients/window.h
> > @@ -28,6 +28,7 @@
> >  #include <xkbcommon/xkbcommon.h>
> >  #include <wayland-client.h>
> >  #include <cairo.h>
> > +#include <stdbool.h>
> >  #include "../shared/config-parser.h"
> >  #include "../shared/zalloc.h"
> >  
> > @@ -84,6 +85,9 @@ display_get_display(struct display *display);
> >  int
> >  display_has_subcompositor(struct display *display);
> >  
> > +bool
> > +display_is_subsurfaces_supported(struct display *display);
> > +
> >  cairo_device_t *
> >  display_get_cairo_device(struct display *display);
> >  
> 
> clients/subsurfaces.c already has egl_make_swapbuffers_nonblock(), and
> I'm not sure nested.c even needs this. Nested.c does not attempt to set
> zero swap interval either.
> 
> Therefore I don't think this patch really adds anything worthwhile.

Right. The purpose was to not knowingly let the subsurface demo dead
lock. Now it will warn it might dead lock, and then dead lock. The point
was to make it clear that one cannot use an EGL surface as a subsurface
reliably if eglSwapInterval(0) is not supported. But since it's a demo
client, I guess it doesn't really matter if we dead lock, at least when
we warn about it first.


Jonas

> 
> 
> Thanks,
> pq