Bug 38842

Created attachment 48839 [details] [review]
add switch cases for missing GLX parameters in choose_visual()

Can you try this patch?

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.

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;

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.

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.

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.

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.

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

(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

Patch committed (d60880db35fd11d9348ce4b2bfbcc9325d2ebf91).  I'll put this into 7.11 too.