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

[Thomas Hellstrom]

On 10/03/2017 12:44 PM, Thomas Hellstrom wrote:
> 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
>
>
>
>
Hi,

Never mind the above. My kwin debug printout made me draw premature 
conclusions that weren't correct.

Still need to figure out why and if the Intel driver fails...

/Thomas