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

[Fredrik Höglund]

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