[Mesa-dev] [Mesa-stable] [PATCH] gallium: improve selection of pixel format

[Andres Gomez]

Great!

Thanks for the feedback! ☺

On Mon, 2017-07-10 at 12:18 +0200, Olivier Lauffenburger wrote:
> Technically, a correctly written application should not rely on
> ChoosePixelFormat but should enumerate by itself through all the
> pixel formats and select the best one with its own algorithm.
> 
> However, for the (numerous) applications that use ChoosePixelFormat,
> color depth is more a question of quality of rendering, whereas depth
> and stencil buffers are a question a correct/incorrect rendering.
> 
> The current method gives a higher priority to color depth than to
> depth and stencil buffers depth to the point that a different color
> depth disables the stencil buffer.
> 
> To make things even worse, many applications (including GLUT)
> incorrectly set ppfd->cColorBits to 32 instead of 24, although the
> documentation clearly states that "For RGBA pixel types, it is the
> size of the color buffer, EXCLUDING THE ALPHA BITPLANES" (emphasis is
> mine).
> 
> As a result, those applications never get a stencil buffer, which
> leads to incorrect rendering. I stumbled on this problem while giving
> a try to OpenCSG and it took me a while to discover that the wrong
> results were caused by the absence of a stencil buffer requested by
> GLUT.
> 
> Although there is no universal selection algorithm, the one I suggest
> tries to enforce the following policy:
> 
> - Most important is to enable all the buffers (depth, stencil,
> accumulation...) that are requested (correction).
> - Then, try to allocate at least as many bits as requested (quality +
> performance).
> - Least important, try not to allocate more bits than requested
> (economy).
> 
> This algorithm seems to be in line with the behavior of most Windows
> drivers (Microsoft, NVIDIA, AMD) and, more important, I can't imagine
> a sensible scenario where this change would break an existing
> application.
> 
> -Olivier
> 
> -----Message d'origine-----
> De : Andres Gomez [mailto:agomez at igalia.com] 
> Envoyé : samedi 8 juillet 2017 22:08
> À : Olivier Lauffenburger <o.lauffenburger at topsolid.com>; mesa-dev at li
> sts.freedesktop.org
> Cc : mesa-stable at lists.freedesktop.org; Brian Paul <brianp at vmware.com
> >
> Objet : Re: [Mesa-dev] [PATCH] gallium: improve selection of pixel
> format
> 
> Olivier, Brian, do we want this into -stable?
> 
> On Thu, 2017-07-06 at 17:08 +0200, Olivier Lauffenburger wrote:
> > Current selection of pixel format does not enforce the request of 
> > stencil or depth buffer if the color depth is not the same as 
> > requested.
> > For instance, GLUT requests a 32-bit color buffer with an 8-bit 
> > stencil buffer, but because color buffers are only 24-bit, no
> > priority 
> > is given to creating a stencil buffer.
> > 
> > This patch gives more priority to the creation of requested
> > buffers 
> > and less priority to the difference in bit depth.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101703
> > 
> > Signed-off-by: Olivier Lauffenburger <o.lauffenburger at topsolid.com>
> > ---
> >  src/gallium/state_trackers/wgl/stw_pixelformat.c | 36 
> > +++++++++++++++++++-----
> >  1 file changed, 29 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/gallium/state_trackers/wgl/stw_pixelformat.c 
> > b/src/gallium/state_trackers/wgl/stw_pixelformat.c
> > index 7763f71cbc..833308d964 100644
> > --- a/src/gallium/state_trackers/wgl/stw_pixelformat.c
> > +++ b/src/gallium/state_trackers/wgl/stw_pixelformat.c
> > @@ -432,17 +432,39 @@ stw_pixelformat_choose(HDC hdc, CONST
> > PIXELFORMATDESCRIPTOR *ppfd)
> >            !!(pfi->pfd.dwFlags & PFD_DOUBLEBUFFER))
> >           continue;
> >  
> > -      /* FIXME: Take in account individual channel bits */
> > -      if (ppfd->cColorBits != pfi->pfd.cColorBits)
> > -         delta += 8;
> > +      /* Selection logic:
> > +      * - Enabling a feature (depth, stencil...) is given highest
> > priority.
> > +      * - Giving as many bits as requested is given medium
> > priority.
> > +      * - Giving no more bits than requested is given lowest
> > priority.
> > +      */
> >  
> > -      if (ppfd->cDepthBits != pfi->pfd.cDepthBits)
> > -         delta += 4;
> > +      /* FIXME: Take in account individual channel bits */
> > +      if (ppfd->cColorBits && !pfi->pfd.cColorBits)
> > +         delta += 10000;
> > +      else if (ppfd->cColorBits > pfi->pfd.cColorBits)
> > +         delta += 100;
> > +      else if (ppfd->cColorBits < pfi->pfd.cColorBits)
> > +         delta++;
> >  
> > -      if (ppfd->cStencilBits != pfi->pfd.cStencilBits)
> > +      if (ppfd->cDepthBits && !pfi->pfd.cDepthBits)
> > +         delta += 10000;
> > +      else if (ppfd->cDepthBits > pfi->pfd.cDepthBits)
> > +         delta += 200;
> > +      else if (ppfd->cDepthBits < pfi->pfd.cDepthBits)
> >           delta += 2;
> >  
> > -      if (ppfd->cAlphaBits != pfi->pfd.cAlphaBits)
> > +      if (ppfd->cStencilBits && !pfi->pfd.cStencilBits)
> > +         delta += 10000;
> > +      else if (ppfd->cStencilBits > pfi->pfd.cStencilBits)
> > +         delta += 400;
> > +      else if (ppfd->cStencilBits < pfi->pfd.cStencilBits)
> > +         delta++;
> > +
> > +      if (ppfd->cAlphaBits && !pfi->pfd.cAlphaBits)
> > +         delta += 10000;
> > +      else if (ppfd->cAlphaBits > pfi->pfd.cAlphaBits)
> > +         delta += 100;
> > +      else if (ppfd->cAlphaBits < pfi->pfd.cAlphaBits)
> >           delta++;
> >  
> >        if (delta < bestdelta) {
> 
> --
> Br,
> 
> Andres
> 
> 
> _______________________________________________
> mesa-stable mailing list
> mesa-stable at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
-- 
Br,

Andres