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

[Olivier Lauffenburger]

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