[Mesa-dev] [PATCH 3/8] Support of RENDER_TYPE property [3/6] Changes to visual configs initialization.

[Ian Romanick]

On 07/15/2013 07:28 AM, Tomasz Lis wrote:
> The change is to correctly handle the value of renderType and
> drawableType in fbconfig. This part modifies glXInitializeVisualConfigFromTags
> to read the parameter value, or detect it if it's not there.
> ---
>   src/glx/glxext.c               |   35 +++++++++++++++++++++++++++++++----
>   src/mesa/drivers/x11/fakeglx.c |    7 ++++++-
>   2 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/src/glx/glxext.c b/src/glx/glxext.c
> index ef1e7ad..83580d4 100644
> --- a/src/glx/glxext.c
> +++ b/src/glx/glxext.c
> @@ -337,6 +337,7 @@ __glXInitializeVisualConfigFromTags(struct glx_config * config, int count,
>                                       Bool fbconfig_style_tags)
>   {
>      int i;
> +   GLint renderType = 0;
>
>      if (!tagged_only) {
>         /* Copy in the first set of properties */
> @@ -471,8 +472,8 @@ __glXInitializeVisualConfigFromTags(struct glx_config * config, int count,
>            config->drawableType |= GLX_WINDOW_BIT | GLX_PIXMAP_BIT | GLX_PBUFFER_BIT;
>   #endif
>            break;
> -      case GLX_RENDER_TYPE:
> -         config->renderType = *bp++;
> +      case GLX_RENDER_TYPE: /* fbconfig render type bits */
> +         renderType = *bp++;
>            break;
>         case GLX_X_RENDERABLE:
>            config->xRenderable = *bp++;
> @@ -555,8 +556,34 @@ __glXInitializeVisualConfigFromTags(struct glx_config * config, int count,
>         }
>      }
>
> -   config->renderType =
> -      (config->rgbMode) ? GLX_RGBA_BIT : GLX_COLOR_INDEX_BIT;
> +   if (renderType) {
> +       config->renderType = renderType;
> +       config->floatMode = (renderType &
> +           (GLX_RGBA_FLOAT_BIT_ARB|GLX_RGBA_UNSIGNED_FLOAT_BIT_EXT)) ?
> +           GL_TRUE : GL_FALSE;

This should be (ignoring the horrible line-wrapping that Thunderbird 
forces on me):

     config->floatMode = (renderType &
         (GLX_RGBA_FLOAT_BIT_ARB|GLX_RGBA_UNSIGNED_FLOAT_BIT_EXT)) != 0;

> +   } else if (!tagged_only) {
> +       /* If there wasn't GLX_RENDER_TYPE property, set it based on config->*Mode
> +        * It is not always explicit, but we have no other choice. We will not know
> +        * if the float mode should be signed or unsigned, and we won't know if
> +        * the mode supports both integer and float RGBA. */
> +       config->renderType =
> +          ((config->floatMode==GL_TRUE) ? GLX_RGBA_FLOAT_BIT_ARB : 0) |
> +          ((config->rgbMode==GL_TRUE && config->floatMode!=GL_TRUE) ? GLX_RGBA_BIT : 0) |
> +          ((config->rgbMode!=GL_TRUE) ? GLX_COLOR_INDEX_BIT : 0);

Ugh.  Don't compare with logic values either.

    config->renderType =
       (config->floatMode ? GLX_RGBA_FLOAT_BIT_ARB : 0) |
       ((config->rgbMode && !config->floatMode) ? GLX_RGBA_BIT : 0) |
       (!config->rgbMode ? GLX_COLOR_INDEX_BIT : 0);

Since these are all mutually exclusive values, this would probably be 
better as a series of if-else statements anyway.  I think the following 
is much more clear:

     if (config->floatMode) {
         config->renderType = GLX_RGBA_FLOAT_BIT_ARB;
     } else if (config->rgbMode) {
         config->renderType = GLX_RGBA_BIT;
     } else {
         config->renderType = GLX_COLOR_INDEX_BIT;
     }

> +   } else {
> +       /* If there wasn't such property and we should return fbconfig with only part of
> +        * properties set, then don't change (allow zero) renderType. This will allow matching
> +        * the generated fbconfig with fbconfigs_compatible(), no matter which flags are set
> +        * in the fbconfig being compared. */

I think clang (or maybe some static analysis tools) may generate a 
warning for an empty block like this.  Can someone else confirm that for me?

> +   }
> +   /* cannot create window out of float fbconfigs, these are offscreen only */
> +   if (config->floatMode == GL_TRUE) {
> +       /* GLX_ARB_fbconfig_float specs:
> +        * Note that floating point rendering is only supported for
> +        * GLXPbuffer drawables.
> +        */
> +       config->drawableType &= ~(GLX_WINDOW_BIT|GLX_PIXMAP_BIT);
> +   }
>   }
>
>   static struct glx_config *
> diff --git a/src/mesa/drivers/x11/fakeglx.c b/src/mesa/drivers/x11/fakeglx.c
> index 969ee7d..7a2cfbe 100644
> --- a/src/mesa/drivers/x11/fakeglx.c
> +++ b/src/mesa/drivers/x11/fakeglx.c

I'm not sure these last two hunks are necessary.  I don't think the 
fakeglx patch can ever see float configs.  Brian knows that code much 
better than I do.

> @@ -1090,6 +1090,9 @@ choose_visual( Display *dpy, int screen, const int *list, GLboolean fbConfig )
>               else if (*parselist & GLX_COLOR_INDEX_BIT) {
>                  rgb_flag = GL_FALSE;
>               }
> +            else if (*parselist & (GLX_RGBA_FLOAT_BIT_ARB|GLX_RGBA_UNSIGNED_FLOAT_BIT_EXT)) {
> +               rgb_flag = GL_TRUE;
> +            }
>               else if (*parselist == 0) {
>                  rgb_flag = GL_TRUE;
>               }
> @@ -1761,7 +1764,9 @@ get_config( XMesaVisual xmvis, int attrib, int *value, GLboolean fbconfig )
>         case GLX_RENDER_TYPE_SGIX:
>            if (!fbconfig)
>               return GLX_BAD_ATTRIBUTE;
> -         if (xmvis->mesa_visual.rgbMode)
> +         if (xmvis->mesa_visual.floatMode)
> +            *value = GLX_RGBA_FLOAT_BIT_ARB;
> +         else if (xmvis->mesa_visual.rgbMode)
>               *value = GLX_RGBA_BIT;
>            else
>               *value = GLX_COLOR_INDEX_BIT;
>