[Mesa-dev] [PATCH] egl: Allow 24-bit visuals for 32-bit RGBA8888 configs

[Chad Versace]

Two nits.

On 02/25/2013 10:51 AM, Ian Romanick wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
> 
> Previously only the 32-bit X visual would match the 32-bit RGBA8888
> configs.  This resulted in every config with alpha getting the "magic"
> visual whose alpha is used by the compositor.  This also resulted in no
> multisample visuals being advertised.  How many ways could we lose?
> 
> This patch inverts the problem... now you can't get the visual with
> alpha used by the compositor even if you want it.  I think we need to
> invent a new value for EGL_TRANSPARENT_TYPE that apps can use to get
> this.  I'm surprised that there isn't already a choice for
> EGL_TRANSPARENT_ALPHA.
> 
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> Tested-by: Tian Ye <yex.tian at intel.com>
> Cc: Kristian Høgsberg <krh at bitplanet.net>
> Cc: Chad Versace <chad.versace at linux.intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=59783
> ---
>  src/egl/drivers/dri2/egl_dri2.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index ae842d7..5d00e4f 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -195,7 +195,14 @@ dri2_add_config(_EGLDisplay *disp, const __DRIconfig *dri_config, int id,
>        for (i = 0; attr_list[i] != EGL_NONE; i += 2)
>           _eglSetConfigKey(&base, attr_list[i], attr_list[i+1]);
>  
> -   if (depth > 0 && depth != base.BufferSize)
> +   /* Allow a 24-bit RGB visual to match a 32-bit RGBA fbconfig.  Otherwise it

fbconfig occurs nowhere in this code, and encountering it here confused me. The
code is dealing with an EGLConfig, so let's call it what it is.

> +    * will only match a 32-bit RGBA visual.  On a composited window manager on
> +    * X11, this will make all of the fbconfigs with destination alpha get
> +    * blended by the compositor.  This is probably not what the application
> +    * wants... especially on drivers that only have 32-bit RGBA fbconfigs!
> +    */
> +   if (depth > 0 &&
> +       !(depth == base.BufferSize || (depth == 24 && base.BufferSize == 32)))
>        return NULL;

DeMorgan could simplify this to:

  depth > 0 &&
  depth != base.BufferSize &&
  !(depth == 24 && base.BufferSize == 32)

>  
>     if (rgba_masks && memcmp(rgba_masks, dri_masks, sizeof(dri_masks)))
> 

The code's correct, so feel free to ignore my nits.
And THANK YOU for fixing this bug. It's been bothering me for months.

Reviewed-by: Chad Versace <chad.versace at linux.intel.com>