Bug 38842 - Various valid GLX attributes are rejected by MESA glxChooseFBConfig
Various valid GLX attributes are rejected by MESA glxChooseFBConfig
Status: RESOLVED FIXED
Product: Mesa
Classification: Unclassified
Component: Drivers/X11
7.10
x86 (IA32) Linux (All)
: medium normal
Assigned To: mesa-dev
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-30 09:35 UTC by Jonathan Kirkham
Modified: 2011-07-11 07:04 UTC (History)
0 users

See Also:


Attachments
add switch cases for missing GLX parameters in choose_visual() (1.70 KB, patch)
2011-07-06 16:20 UTC, Brian Paul
Details | Splinter Review
updated patch (1.76 KB, patch)
2011-07-08 06:55 UTC, Brian Paul
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Kirkham 2011-06-30 09:35:40 UTC
The following configuration attributes which are listed as valid in the glx specification are rejected by the the MESA implementation of glxChooseFBConfig:

GLX_MAX_PBUFFER_WIDTH
GLX_MAX_PBUFFER_HEIGHT
GLX_MAX_PBUFFER_PIXELS
GLX_VISUAL_ID
GLX_X_VISUAL_TYPE

The rejection occurs within the choose_visual method within fakeglx.c (having been called from glxChooseFBConfig.
Comment 1 Brian Paul 2011-07-06 16:20:14 UTC
Created attachment 48839 [details] [review]
add switch cases for missing GLX parameters in choose_visual()

Can you try this patch?
Comment 2 Jonathan Kirkham 2011-07-07 09:21:33 UTC
This solves the immediate problem that the attributes are rejected, however, I do not believe this change is solving the real problem.


In this patch if the attribute GLX_MAX_PBUFFER_WIDTH is passed in, the value of  desiredVisualID is set to be the value of this attribute which looks incorrect to me. The individual case statements for the attributes need to do something, not drop through to the GLX_VISUAL_ID case where desiredVisualID is set.

Is it possible that the other cases should be:

         case GLX_MAX_PBUFFER_WIDTH:
           parselist +=2; //skip

         case GLX_MAX_PBUFFER_HEIGHT:
         case GLX_MAX_PBUFFER_PIXELS:
         case GLX_VISUAL_ID:
            if (!fbConfig)
               return NULL; /* invalid config option */
            parselist++;
            desiredVisualID = *parselist++;
            break;

The result of testing this patch is that no configs are returned by glxGetFBConfig, presumably because desiredVisualID is set incorrectly.



Thanks for taking the time to look into this problem.
Comment 3 Jonathan Kirkham 2011-07-07 09:22:52 UTC
Apologies the change should have read :

         case GLX_MAX_PBUFFER_WIDTH:
           parselist +=2; //skip
           break;
         case GLX_MAX_PBUFFER_HEIGHT:
           parselist +=2; //skip
           break;
         case GLX_MAX_PBUFFER_PIXELS:
           parselist +=2; //skip
           break;
         case GLX_VISUAL_ID:
            if (!fbConfig)
               return NULL; /* invalid config option */
            parselist++;
            desiredVisualID = *parselist++;
            break;
Comment 4 Jonathan Kirkham 2011-07-07 09:30:59 UTC
The change I have submitted above works, i.e. configs are returned when these attributes.

Unfortunately the values of the attributes has been effectively thrown away.

I do not know enough about the way MESA handles configurations to know if that is important or not.
Comment 5 Jonathan Kirkham 2011-07-08 01:04:12 UTC
Having just re-read the EGL spec:

"If EGL MAX PBUFFER WIDTH, EGL MAX PBUFFER HEIGHT, EGL MAX PBUFFER PIXELS, or EGL NATIVE VISUAL ID are specified in attrib list, then they are ignored (however, if present, these attributes must still be followed by an attribute value in attrib list)."


It would seem as if my above change is acceptable. The attributes are not rejected any more they are simply ignored.
Comment 6 Brian Paul 2011-07-08 06:55:40 UTC
Created attachment 48896 [details] [review]
updated patch

Here's an updated patch.  We want to return null for the attributes in question if choose_visual() is called from glXChooseVisual() since those attributes are not legal for that call.

The actual values of the attributes will be ignored, but that's OK.
Comment 7 Jonathan Kirkham 2011-07-08 08:07:47 UTC
The attributes are valid for glxChooseFBConfig, the attributes and their values are just ignore by it.

Returning NULL in choose_visual causes glxConfigFBConfig to also return NULL which is incorrect, it should return the list of valid configs. Passing in these ignored attributes should not alter the configs returned by glxChooseFBConfig.
Comment 8 Brian Paul 2011-07-08 08:51:13 UTC
(In reply to comment #7)
> The attributes are valid for glxChooseFBConfig, the attributes and their values
> are just ignore by it.
> 
> Returning NULL in choose_visual causes glxConfigFBConfig to also return NULL
> which is incorrect, it should return the list of valid configs. Passing in
> these ignored attributes should not alter the configs returned by
> glxChooseFBConfig.

Note that the 'fbConfig' var indicates if the caller is glXChooseFBConfig or glXChooseVisual.  The attributes in question are only valid for the former, not the later.
Comment 9 Jonathan Kirkham 2011-07-11 00:55:03 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > The attributes are valid for glxChooseFBConfig, the attributes and their values
> > are just ignore by it.
> > 
> > Returning NULL in choose_visual causes glxConfigFBConfig to also return NULL
> > which is incorrect, it should return the list of valid configs. Passing in
> > these ignored attributes should not alter the configs returned by
> > glxChooseFBConfig.
> 
> Note that the 'fbConfig' var indicates if the caller is glXChooseFBConfig or
> glXChooseVisual.  The attributes in question are only valid for the former, not
> the later.

Ahh, my mistake, apologies.

This looks good to me then.

Thanks

Jon
Comment 10 Brian Paul 2011-07-11 07:04:24 UTC
Patch committed (d60880db35fd11d9348ce4b2bfbcc9325d2ebf91).  I'll put this into 7.11 too.