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

[Fredrik Höglund]

On Thursday 05 October 2017, Thomas Hellstrom wrote:
> 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...

Qt also uses a similar algorithm to pick fbconfigs, so it's possible that
that's the code path that is still broken:

http://code.qt.io/cgit/qt/qtbase.git/tree/src/platformsupport/glxconvenience/qglxconvenience.cpp#n179

That being said, looking at the glxinfo output in the bug report I don't see
any obvious reason why either code path would be broken on Intel.

Fredrik