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

[Fredrik Höglund]

On Tuesday 03 October 2017, 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.

GlxBackend::initFbConfig() selects the fbconfig that kwin will use
for its own GLXWindow, so that function should (preferably) not end
up with the ARGB visual.

The function that selects fbconfigs for composited windows is
GlxBackend::infoForVisual(xcb_visualid_t).

> 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.

GlxBackend::infoForVisual() already does that in a sense, since it filters
out all fbconfigs where the depth of the associated visual doesn't match
the depth of the window visual.

> 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
> 
> 
> 
> 
>