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

[Chad Versace]

On 02/25/2013 04:22 PM, Chad Versace wrote:
> 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>


Well... I shouldn't have said the code is *correct*, because I don't know
what correct really means here given the constraints of EGLConfig suckage.
The code *correctly* does what you want it do, which trades an unwanted bug
to a preferable one. Yeah, that's what I originally meant to say.