Re: [Dri-devel] glXChooseVisual behaviour

[Ian R. <id...@us...>]

Leif Delgass wrote:
> On Fri, 2 May 2003, Ian Romanick wrote:
> 
> 
>>Leif Delgass wrote:
>>
>>
>>>It was determined that this is a bug in the server glx code
>>>(xc/programs/Xserver/GL/mesa/src/X/xf86glx.c).  The bufferSize should be
>>>32, but the root depth is always used.  At first glance, it appears 
>>>to be a one or two line fix, but there seem to be some assumptions 
>>>in the code regarding this.  I recall running into other problems when 
>>>trying to fix this.  I'm sure it can be fixed, but we need to track down 
>>>the dependencies/assumptions.
>>
>>It looks like there are a few places where it tries to match 
>>pVis->nplanes against pGLXVis->bufferSize.  If we set bufferSize to the 
>>correct value, that test will fail.  Can we change it to something like:
>>
>>     if ( pVis->nplanes ==
>>          (pGLXVis->redSize + pGLXVis->blueSize + pGLXVis->greenSize) )
>>
>>There also seem to be a couple places in glx/glxutil.c that assume 
>>bufferSize is only the RGB bits, but we should be able to fix that the 
>>same way.
> 
> 
> Attached is the patch I had tried before.  Applied to the current trunk, 
> it seems to work.  I wish I could remember what the problem I ran into 
> before was.  Was there a change from the texmem branch that would have 
> fixed this?  I'm wondering if there would be any compatiblity issues with 
> this change.
>  
> 
>>>On a related note, I noticed something else that looks inconsistent in 
>>>some of the drivers.  Should visuals with an accumulation buffer only 
>>>advertise an alpha channel in the accum buffer if there is one in the 
>>>color buffer?
>>
>>Probably, but I don't know that it matters.
> 
> 
> Well, at any rate, I fixed it so that all the drivers are consistent with
> each other.
> 
> 
> 
> ------------------------------------------------------------------------
> 
> Index: xf86glx.c
> ===================================================================
> RCS file: /cvsroot/dri/xc/xc/programs/Xserver/GL/mesa/src/X/xf86glx.c,v
> retrieving revision 1.19
> diff -u -r1.19 xf86glx.c
> --- xf86glx.c	25 Mar 2003 12:04:26 -0000	1.19
> +++ xf86glx.c	2 May 2003 20:00:50 -0000
> @@ -405,7 +405,10 @@
>  		glXVisualPtr[j].greenMask  = pVisual[i].greenMask;
>  		glXVisualPtr[j].blueMask   = pVisual[i].blueMask;
>  		glXVisualPtr[j].alphaMask  = glXVisualPtr[j].alphaMask;
> -		glXVisualPtr[j].bufferSize = rootDepth;
> +		glXVisualPtr[j].bufferSize = glXVisualPtr[j].redSize +
> +		                             glXVisualPtr[j].greenSize +
> +		                             glXVisualPtr[j].blueSize +
> +		                             glXVisualPtr[j].alphaSize; /* rootDepth; */
>  	    }
>  
>  	    /* Save the device-dependent private for this visual */

I guess my question is why do we need to "fix up" bufferSize?  Isn't it 
supplied by the DDX driver?  Why can't the X-server trust the driver to 
supply the correct values for all of these items?

> @@ -505,7 +508,7 @@
>  	/* Find a visual that matches the GLX visual's class and size */
>  	for (j = 0; j < pScreen->numVisuals; j++, pVis++) {
>  	    if (pVis->class == pGLXVis->class &&
> -		pVis->nplanes == pGLXVis->bufferSize) {
> +		pVis->nplanes == (pGLXVis->bufferSize - pGLXVis->alphaSize)) {
>  
>  		/* Fixup the masks */
>  		pGLXVis->redMask   = pVis->redMask;

Why is fixup_visuals even needed?  Can't we trust the driver to provide 
the correct values?  I suppose matching nplanes in this way works, but 
it just "feels" error prone to me.  The comment on line 73 of 
programs/Xserver/include/scrnintstr.h doesn't inspire much confidence:

     short	nplanes;/* = log2 (ColormapEntries). This does not
			 * imply that the screen has this many planes.
			 * it may have more or fewer */

> @@ -545,7 +548,7 @@
>  	for (j = 0; j < pScreen->numVisuals; j++, pVis++) {
>  
>  	    if (pVis->class == pGLXVis->class &&
> -		pVis->nplanes == pGLXVis->bufferSize &&
> +		pVis->nplanes == (pGLXVis->bufferSize - pGLXVis->alphaSize) &&
>  		!used[j]) {
>  
>  		if (pVis->redMask   == pGLXVis->redMask &&

Ditto.

bufferSize seems to be used in the same odd ways in aqua/aquaGlx.c and 
glx/glxscreens.c.