[PATCH 1/3] Float fbconfigs exchange patch [1/3] Improved handling of renderType attribute in GLX structures.

Ian Romanick idr at freedesktop.org
Thu Oct 3 09:22:12 PDT 2013


On 07/15/2013 08:06 AM, Tomasz Lis wrote:
> From: Tomasz Lis <tomasz.lis at intel.com>
> 
> Support of GLX_RGBA*_FLOAT_BIT*, and correct setting of the
> flags. Also commented each renderType use with information
> which (fbconfig or context) RENDER_TYPE it is.

I know almost nothing about the DMX or xquartz code, so I don't feel
comfortable offering definitive review on those bits.

> Signed-off-by: Tomasz Lis <listom at gmail.com>
> ---
>  glx/createcontext.c            |    2 ++
>  glx/glxext.h                   |   15 ++++++++++++++
>  hw/dmx/dmx_glxvisuals.c        |    7 +++++--
>  hw/dmx/glxProxy/glxcmds.c      |   42 +++++++++++++++++++++++++++++++++-------
>  hw/xquartz/GL/glcontextmodes.c |   10 ++++++++--
>  5 files changed, 65 insertions(+), 11 deletions(-)
> 
> diff --git a/glx/createcontext.c b/glx/createcontext.c
> index 13d21cc..41ecd11 100644
> --- a/glx/createcontext.c
> +++ b/glx/createcontext.c
> @@ -68,6 +68,8 @@ validate_render_type(uint32_t render_type)
>      switch (render_type) {
>      case GLX_RGBA_TYPE:
>      case GLX_COLOR_INDEX_TYPE:
> +    case GLX_RGBA_FLOAT_TYPE_ARB:
> +    case GLX_RGBA_UNSIGNED_FLOAT_TYPE_EXT:
>          return True;
>      default:
>          return False;
> diff --git a/glx/glxext.h b/glx/glxext.h
> index 9b0978b..2d67af3 100644
> --- a/glx/glxext.h
> +++ b/glx/glxext.h
> @@ -35,6 +35,21 @@
>   * Silicon Graphics, Inc.
>   */
>  
> +// doing #include <GL/glx.h> & #include <GL/glxext.h> could cause problems with
> +// overlapping definitions, so let's use the easy way
> +#ifndef GLX_RGBA_FLOAT_BIT_ARB
> +#define GLX_RGBA_FLOAT_BIT_ARB             0x00000004
> +#endif
> +#ifndef GLX_RGBA_FLOAT_TYPE_ARB
> +#define GLX_RGBA_FLOAT_TYPE_ARB            0x20B9
> +#endif
> +#ifndef GLX_RGBA_UNSIGNED_FLOAT_BIT_EXT
> +#define GLX_RGBA_UNSIGNED_FLOAT_BIT_EXT    0x00000008
> +#endif
> +#ifndef GLX_RGBA_UNSIGNED_FLOAT_TYPE_EXT
> +#define GLX_RGBA_UNSIGNED_FLOAT_TYPE_EXT   0x20B1
> +#endif
> +
>  extern GLboolean __glXFreeContext(__GLXcontext * glxc);
>  extern void __glXFlushContextCache(void);
>  
> diff --git a/hw/dmx/dmx_glxvisuals.c b/hw/dmx/dmx_glxvisuals.c
> index f903b74..ba051a4 100644
> --- a/hw/dmx/dmx_glxvisuals.c
> +++ b/hw/dmx/dmx_glxvisuals.c
> @@ -439,8 +439,11 @@ GetGLXFBConfigs(Display * dpy, int glxMajorOpcode, int *nconfigs)
>          /* Fill in derived values */
>          config->screen = screen;
>  
> -        config->rgbMode = config->renderType & GLX_RGBA_BIT;
> -        config->colorIndexMode = !config->rgbMode;
> +        /* The rgbMode should be true for any mode which has distinguishible R, G and B components */
> +        config->rgbMode = (config->renderType & (GLX_RGBA_BIT |
> +               GLX_RGBA_FLOAT_BIT_ARB | GLX_RGBA_UNSIGNED_FLOAT_BIT_EXT)) != 0;

Blank line here.

> +        /* The colorIndexMode isn't really needed, as all non-RGB modes are CI modes */
> +        config->colorIndexMode = (config->renderType & GLX_COLOR_INDEX_BIT) != 0;

If this comment is correct, why change the code?  In a hunk below you
also use colorIndexMode = !rgbMode, so that seems fine.

>  
>          config->haveAccumBuffer =
>              config->accumRedBits > 0 ||
> diff --git a/hw/dmx/glxProxy/glxcmds.c b/hw/dmx/glxProxy/glxcmds.c
> index 4538274..b325f4d 100644
> --- a/hw/dmx/glxProxy/glxcmds.c
> +++ b/hw/dmx/glxProxy/glxcmds.c
> @@ -308,12 +308,12 @@ CreateContext(__GLXclientState * cl,
>          /* send the create context request to the back-end server */
>          dpy = GetBackEndDisplay(cl, screen);
>          if (glxc->pFBConfig) {
> -            /*Since for a certain visual both RGB and COLOR INDEX
> -             *can be on then the only parmeter to choose the renderType
> -             * should be the class of the colormap since all 4 first 
> -             * classes does not support RGB mode only COLOR INDEX ,
> -             * and so TrueColor and DirectColor does not support COLOR INDEX*/
> -            int renderType = glxc->pFBConfig->renderType;
> +            /* For a specific visual, multiple render types (ie. both RGB and COLOR INDEX)
> +            * can be accessible. The only parameter to choose the renderType
> +            * should be the class of the colormap, since all 4 first classes
> +            * does not support RGB mode only COLOR INDEX,
> +            * and so TrueColor and DirectColor does not support COLOR INDEX. */

This comment is formatted weird (the old one was too).  All of the *
should be in the same column, and the */ should be on its own line.

> +            int renderType = GLX_RGBA_TYPE;
>  
>              if (pVisual) {
>                  switch (pVisual->class) {
> @@ -329,7 +329,21 @@ CreateContext(__GLXclientState * cl,
>                      renderType = GLX_RGBA_TYPE;
>                      break;
>                  }
> +            } else {

Hmm... if the fbconfig has multiple bits set, does the spec give any
guidance as to which mode should actually be used?  There's an implicit
prioritization here, but is that correct?

> +                /*  Convert the render type bits from fbconfig into context render type. */
> +                if (glxc->pFBConfig->renderType & GLX_RGBA_BIT)
> +                    renderType = GLX_RGBA_TYPE;
> +                else if (glxc->pFBConfig->renderType & GLX_COLOR_INDEX_BIT)
> +                    renderType = GLX_COLOR_INDEX_TYPE;
> +                else if (glxc->pFBConfig->renderType & GLX_RGBA_FLOAT_BIT_ARB)
> +                    renderType = GLX_RGBA_FLOAT_TYPE_ARB;
> +                else if (glxc->pFBConfig->renderType & GLX_RGBA_UNSIGNED_FLOAT_BIT_EXT)
> +                    renderType = GLX_RGBA_UNSIGNED_FLOAT_TYPE_EXT;
> +                else { /* There's no recognized renderType in the config */
> +                    renderType =  GLX_RGBA_TYPE;
> +                }
>              }
> +
>              if (__GLX_IS_VERSION_SUPPORTED(1, 3)) {
>                  LockDisplay(dpy);
>                  GetReq(GLXCreateNewContext, be_new_req);
> @@ -3193,6 +3207,7 @@ __glXQueryContext(__GLXclientState * cl, GLbyte * pc)
>      __GLXcontext *ctx;
>      xGLXQueryContextReq *req;
>      xGLXQueryContextReply reply;
> +    int renderType;
>      int nProps;
>      int *sendBuf, *pSendBuf;
>      int nReplyBytes;
> @@ -3205,6 +3220,19 @@ __glXQueryContext(__GLXclientState * cl, GLbyte * pc)
>          return __glXBadContext;
>      }
>  
> +    /*  Convert the render type bits from fbconfig into context render type. */

This code appears twice, so it should be refactored to its own function.

> +    if (ctx->pFBConfig->renderType & GLX_RGBA_BIT)
> +        renderType = GLX_RGBA_TYPE;
> +    else if (ctx->pFBConfig->renderType & GLX_COLOR_INDEX_BIT)
> +        renderType = GLX_COLOR_INDEX_TYPE;
> +    else if (ctx->pFBConfig->renderType & GLX_RGBA_FLOAT_BIT_ARB)
> +        renderType = GLX_RGBA_FLOAT_TYPE_ARB;
> +    else if (ctx->pFBConfig->renderType & GLX_RGBA_UNSIGNED_FLOAT_BIT_EXT)
> +        renderType = GLX_RGBA_UNSIGNED_FLOAT_TYPE_EXT;
> +    else { /* There's no recognized renderType in the config */
> +        renderType =  GLX_RGBA_TYPE;
> +    }
> +
>      nProps = 3;
>  
>      reply = (xGLXQueryContextReply) {
> @@ -3220,7 +3248,7 @@ __glXQueryContext(__GLXclientState * cl, GLbyte * pc)
>      *pSendBuf++ = GLX_FBCONFIG_ID;
>      *pSendBuf++ = (int) (ctx->pFBConfig->id);
>      *pSendBuf++ = GLX_RENDER_TYPE;
> -    *pSendBuf++ = (int) (ctx->pFBConfig->renderType);
> +    *pSendBuf++ = (int)(renderType); /* context render type (one of GLX_*_TYPE values) */

You can drop the (int)() casting since renderType is already int.

>      *pSendBuf++ = GLX_SCREEN;
>      *pSendBuf++ = (int) (ctx->pScreen->myNum);
>  
> diff --git a/hw/xquartz/GL/glcontextmodes.c b/hw/xquartz/GL/glcontextmodes.c
> index dc97f89..1fffaa8 100644
> --- a/hw/xquartz/GL/glcontextmodes.c
> +++ b/hw/xquartz/GL/glcontextmodes.c
> @@ -141,9 +141,15 @@ _gl_copy_visual_to_context_mode(__GLcontextModes * mode,
>      mode->drawableType = GLX_WINDOW_BIT | GLX_PIXMAP_BIT;
>  
>      mode->rgbMode = (config->rgba != 0);
> -    mode->renderType = (mode->rgbMode) ? GLX_RGBA_BIT : GLX_COLOR_INDEX_BIT;
> -
>      mode->colorIndexMode = !(mode->rgbMode);

Blank line here.

> +    /* It's impossible to create float config from visual. */
> +    mode->floatMode = GL_FALSE;

Blank line here.

> +    /* The fact that we can't have float mode makes the code below unequivocal.
> +     * The rgbMode is true for both float or integer modes, but this time we're sure it's integer.
> +     * We could even skip checking colorIndexMode as all non-RGB modes are CI. */
> +    mode->renderType = (mode->rgbMode) ? GLX_RGBA_BIT : 0 |
> +        (mode->colorIndexMode) ? GLX_COLOR_INDEX_BIT : 0;
> +

I'm confused.  A few lines earlier you set mode->colorIndexMode to
!mode->rgbMode.  If that's the case, why change this line?  I can
understand grouping all of the mode->*Mode lines together, but this last
change seems spurious.

>      mode->doubleBufferMode = (config->doubleBuffer != 0);
>      mode->stereoMode = (config->stereo != 0);
>  
> 



More information about the xorg-devel mailing list