[PATCH weston v2] gl-renderer: If an XRGB format is requested but unavailable, try ARGB

[Pekka Paalanen]

On Tue,  5 May 2015 16:22:22 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

> Recent versions of Mesa have stopped exposing XRGB visuals for gl on
> some Intel GPUs.  While this may be changed in Mesa eventually, it's
> not impossible that some other hardware in the future won't provide
> XRGB visuals either.
> 
> Let's try again with an ARGB visual if XRGB is unavailable.  Since
> we're not changing the scanout buffer format, and our current
> rendering loop always results in saturated alpha in the frame buffer,
> it should be Just Fine(tm) - and probably better than just exiting.
> 
> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> ---
> 
> Dima Ryazanov called me out for fallback_format() not working on all endians,
> and I've reworked the loop to be a single loop instead of two, similar to
> Daniel's suggestion (but still keeping the NULL visual_id means "give me the
> first match" semantic of the original)
> 
> I've also replaced calloc with aloca so I don't have to worry about freeing
> the array on return, which simplifies things a little bit.  I didn't bother
> zeroing out the array, since it's filled in by eglChooseConfig.
> 
> It's still an ugly function. :)
> 
>  src/gl-renderer.c | 57 +++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 43 insertions(+), 14 deletions(-)
> 
> diff --git a/src/gl-renderer.c b/src/gl-renderer.c
> index ae3122f..a4fd3a0 100644
> --- a/src/gl-renderer.c
> +++ b/src/gl-renderer.c
> @@ -934,6 +934,14 @@ output_rotate_damage(struct weston_output *output,
>  	go->border_damage[go->buffer_damage_index] = border_status;
>  }
>  
> +/* NOTE: We now allow falling back to ARGB  gl visual ids when XRGB is
> + * unavailable, so we're assuming the background has no transparency
> + * and that everything with a blend, like drop shadows, will have something
> + * opaque (like the background) drawn underneath it.
> + *
> + * Depending on the underlying hardware, violating that assumption could
> + * result in seeing through to another display plane.
> + */
>  static void
>  gl_renderer_repaint_output(struct weston_output *output,
>  			      pixman_region32_t *output_damage)
> @@ -1897,6 +1905,15 @@ log_egl_config_info(EGLDisplay egldpy, EGLConfig eglconfig)
>  		weston_log_continue(" unknown\n");
>  }
>  
> +static EGLint
> +fallback_format(EGLint format)
> +{
> +	if ((format & 0xFF) != 'X')
> +		return 0;
> +
> +	return (format & 0xFFFFFF00) | 'A';
> +}

I read the function name, I read the code, and I repeat that 6 times
before I figure out the right interpretation. That's pretty awesome. In
a not fun way. :-P

How about calling it get_fallback_format_for()?

Are you sure this works for X11 Visuals too? Oh, but x11-backend
doesn't pass any visual in, so that saves you.

What about GBM visuals (drmfourccs) like RA24 or BA24? Oh, but this
actually a GBM format code, which is hand-picked and cannot just
whatever, so this works.

There's so much fun going on, that I'd like to have big comment
explaining it all, please. :-)

With all these assumptions around, you might as well just hardcode a
switch-statement and say in comments where the funny numbers come from.

Or, even better, extend the gl_renderer_create() API to have the
fallback format passed in explicitly from the backend, which can then
use the proper #defined names. Or go even further, if there is a list
of acceptable visuals.

It's a question of how far you want to go...

> +
>  static int
>  egl_choose_config(struct gl_renderer *gr, const EGLint *attribs,
>  		  const EGLint *visual_id,
> @@ -1904,41 +1921,53 @@ egl_choose_config(struct gl_renderer *gr, const EGLint *attribs,
>  {
>  	EGLint count = 0;
>  	EGLint matched = 0;
> +	EGLint fallback_id;
>  	EGLConfig *configs;
> +	EGLConfig fallback_config = 0;
>  	int i;
>  
>  	if (!eglGetConfigs(gr->egl_display, NULL, 0, &count) || count < 1)
>  		return -1;
>  
> -	configs = calloc(count, sizeof *configs);
> +	configs = alloca(count * sizeof *configs);

The number of eglconfigs could be large, so maybe alloca isn't quite
right here...

>  	if (!configs)
>  		return -1;

Can't test this with alloca.

>  
>  	if (!eglChooseConfig(gr->egl_display, attribs, configs,
>  			      count, &matched))
> -		goto out;
> +		return -1;
> +
> +	if (!matched)
> +		return -1;
>  
> +	if (!visual_id) {
> +		*config_out = configs[0];
> +		return 0;
> +	}
> +
> +	fallback_id = fallback_format(*visual_id);
>  	for (i = 0; i < matched; ++i) {
>  		EGLint id;
>  
> -		if (visual_id) {
> -			if (!eglGetConfigAttrib(gr->egl_display,
> -					configs[i], EGL_NATIVE_VISUAL_ID,
> -					&id))
> -				continue;
> +		if (!eglGetConfigAttrib(gr->egl_display,
> +				configs[i], EGL_NATIVE_VISUAL_ID,
> +				&id))
> +			continue;
>  
> -			if (id != 0 && id != *visual_id)
> -				continue;
> +		if (id == *visual_id) {
> +			*config_out = configs[i];
> +			return 0;
>  		}
> +		if (id == fallback_id)
> +			fallback_config = configs[i];
> +	}

Hmm, but if we end up with the fallback_config, we will get the last
config that matches fallback. Usually we prefer the first config that
matches. Would it make sense to use the first fallback-matching config
in that case?

>  
> -		*config_out = configs[i];
> -
> -		free(configs);
> +	if (fallback_config) {
> +		weston_log("Falling back to ARGB visual\n");
> +		*config_out = fallback_config;
>  		return 0;
>  	}
>  
> -out:
> -	free(configs);
>  	return -1;
>  }
>  

Thanks,
pq