[PATCH xserver 2/2] glx: Fix visual fbconfig matching with respect to swap method

[Thomas Hellstrom]

Hi!

On 10/02/2017 11:51 PM, Fredrik Höglund wrote:
> On Monday 02 October 2017, Thomas Hellstrom wrote:
>> Hi, Fredrik,
>>
>> On 10/02/2017 07:10 PM, Fredrik Höglund wrote:
>>> On Tuesday 26 September 2017, Thomas Hellstrom wrote:
>>>> Hi, Fredrik,
>>>>
>>>> On 09/26/2017 11:53 AM, Fredrik Höglund wrote:
>>>>> On Wednesday 06 September 2017, Thomas Hellstrom wrote:
>>>>>> For the built in visuals, we'd typically select the "best" fbconfig
>>>>>> without considering the swap method. If the client then requests a
>>>>>> specific swap method, say GLX_SWAP_COPY_OML, it may well happen that the
>>>>>> first fbconfig matching requirements would have been paired with the 32-bit
>>>>>> compositing visual, and the client would render a potentially transparent
>>>>>> window.
>>>>> Hi Thomas,
>>>>>
>>>>> Unfortunately this patch breaks GL applications that actually want the
>>>>> ARGB visual. They now end up using a visual that does not have an
>>>>> alpha channel. In addition, it breaks compositing of ARGB windows,
>>>>> at least in kwin.
>>>>>
>>>>> Fredrik
>>>>>
>>>> Yeah, this path, while IMO correct, uncover existing bug(s). (which are
>>>> possible to trigger by other means as well),
>>>> There is a bug for this, (fdo #102806). I have a fix in the making.
>>>> Needs some cleanup, though.
>>> Thanks, I should have checked bugzilla first.
>>>
>>> Having read through the discussion in the bug report, I would like to
>>> add a couple of things though.
>>>
>>> Being the author of the fbconfig selection code in kwin, I very much care
>>> about the correctness of that code, so if anyone has any suggestions for
>>> how to make it more robust, I would be happy to implement those changes.
>>>
>>> But the list of fbconfigs returned by glXChooseFBConfig are filtered and
>>> sorted according to a criteria defined by the GLX specification. The point of
>>> using it is that it should be safer than attempting to sort the list manually,
>>> since clients can't know about future attributes.
>>>
>>> The default value for GLX_SWAP_METHOD_OML is GLX_DONT_CARE, so
>>> I think giving this attribute precedence over other attributes with non-
>>> default values would qualify as a bug in the implementation.
>> I agree. However, it is not given precedence in glXChooseFBConfig. It is
>> just given precedence when pairing fbconfigs with the default X visuals,
>> the idea being that all default X visuals should, if possible, have
>> fbconfigs with identical swap methods and that swap method should
>> preferrably be GLX_SWAP_UNDEFINED_OML.
>>
>> Now I looked through the kwin visual selection code briefly, but could
>> not really deduce how it selects a 32-bit ARGB visual when it wants one.
>> Could you perhaps briefly outline that and we could hopefully come up
>> with a fix for the remaining problem with Intel drivers?
> It first looks up the Xrender format associated with the window visual
> and computes the values for the GLX_RED/GREEN/BLUE/ALPHA_SIZE
> attributes from the red/green/blue/alpha masks.
> So in the case of the ARGB visual, glXChooseFBConfig should filter out
> all fbconfigs with an alpha size less than 8. An additional pass is then
> done to filter out all fbconfigs where the red, green and blue channel
> sizes don't match the render format precisely, since the list may include
> fbconfigs with larger channel sizes. This pass also filters out fbconfigs
> where the depth of the associated X visual doesn't match the depth of
> the window, and fbconfigs that don't have GLX_BIND_TO_TEXTURE_RGBA_EXT
> or GLX_BIND_TO_TEXTURE_RGB_EXT set
>
> Based on your description, I guess the problem was the depth of the
> associated visual.
>
> The list is then sorted to favor fbconfigs with a smaller depth size,
> while otherwise maintaining the order.  This is the one part of the code
> that I consider risky, but there is unfortunately no way to get
> glXChooseFBConfig to do this.
>
> Fredrik
>
But this only selects a suitable fbconfig based on a predetermined 
visual depth.

Looking a bit more closely at the code, it appears like that visual 
depth is taken from the toplevel visual, which is selected in

  GlxBackend::initFbConfig()

But there is nothing in that function to assure that a compositing 
visual is actually favoured. It just grabs the visual of the fbconfig 
that happens to appear first according to the sorting criterion, which 
is preferring (A)RGB fbconfigs without depth- or stencil buffers.

You'd probably want to modify the sorting of available fbconfigs to 
first favour 32-bit visualDepths over non-32 bit visualDepths. That 
should fix the remaining Intel Issue.

That alone won't replace my Xserver fix, though, since for gallium 
drivers the 32-bit visual ended up being srgb-capable, which excluded it 
from being returned from glxChooseFBConfig...

/Thomas