[PATCH] [xorg-dev][xserver] Full support of sRGB capable fbconfigs.

Ian Romanick idr at freedesktop.org
Fri Nov 30 09:20:24 PST 2012


On 11/30/2012 07:18 AM, Tomasz Lis wrote:
>
> 2012/11/26 Ian Romanick <idr at freedesktop.org <mailto:idr at freedesktop.org>>
>
>     On 11/22/2012 07:08 AM, Tomasz Lis wrote:
>
>         From: Tomasz Lis <tomasz.lis at intel.com
>         <mailto:tomasz.lis at intel.com>>
>
>         Changes to correctly initialize the sRGB capability attribute
>         and transfer it between XServer and the client.
>         Modifications include extension string, transferring visual
>         config attribs and fbconfig attribs.
>         Also, attribute is initialized in the modules which do not
>         really use it (xquartz and xwin).
>
>
>     Please word-wrap commit messages to 72 characters.
>
> Tomasz:
> Ok. will do that.
>
>         Signed-off-by: Tomasz Lis <tomasz.lis at intel.com
>         <mailto:tomasz.lis at intel.com>>
>         ---
>            glx/extension_string.c        |    1 +
>            glx/extension_string.h        |    1 +
>            glx/glxcmds.c                 |    7 +++++--
>            glx/glxdri2.c                 |    7 +++++++
>            glx/glxdricommon.c            |    4 +++-
>            glx/glxscreens.c              |    2 ++
>            glx/glxscreens.h              |    3 +++
>            hw/xquartz/GL/visualConfigs.c |    3 +++
>            hw/xwin/glx/indirect.c        |    2 ++
>            9 files changed, 27 insertions(+), 3 deletions(-)
>
>         diff --git a/glx/extension_string.c b/glx/extension_string.c
>         index 544ca1f..47a9746 100644
>         --- a/glx/extension_string.c
>         +++ b/glx/extension_string.c
>         @@ -74,6 +74,7 @@ static const struct extension_info
>         known_glx_extensions[] = {
>                { GLX(ARB_multisample),             VER(1,4), Y, },
>
>                { GLX(EXT_create_context_es2___profile), VER(0,0), N, },
>         +    { GLX(EXT_framebuffer_sRGB),        VER(1,1), N, },
>
>
>     Also advertise the ARB string.
>
> Tomasz:
> Agreed. Will change.
>
>
>                { GLX(EXT_import_context),          VER(0,0), Y, },
>                { GLX(EXT_texture_from_pixmap),     VER(0,0), Y, },
>                { GLX(EXT_visual_info),             VER(0,0), Y, },
>         diff --git a/glx/extension_string.h b/glx/extension_string.h
>         index 7a4a8b1..a91385f 100644
>         --- a/glx/extension_string.h
>         +++ b/glx/extension_string.h
>         @@ -41,6 +41,7 @@ enum {
>                ARB_create_context_robustness___bit,
>                ARB_multisample_bit,
>                EXT_create_context_es2___profile_bit,
>         +    EXT_framebuffer_sRGB_bit,
>                EXT_import_context_bit,
>                EXT_texture_from_pixmap_bit,
>                EXT_visual_info_bit,
>         diff --git a/glx/glxcmds.c b/glx/glxcmds.c
>         index c1f4e22..720a1a8 100644
>         --- a/glx/glxcmds.c
>         +++ b/glx/glxcmds.c
>         @@ -913,7 +913,7 @@ __glXDisp_CopyContext(____GLXclientState *
>         cl, GLbyte * pc)
>
>            enum {
>                GLX_VIS_CONFIG_UNPAIRED = 18,
>         -    GLX_VIS_CONFIG_PAIRED = 20
>         +    GLX_VIS_CONFIG_PAIRED = 22
>            };
>
>            enum {
>         @@ -1005,6 +1005,8 @@
>         __glXDisp_GetVisualConfigs(____GLXclientState * cl, GLbyte * pc)
>                    buf[p++] = modes->samples;
>                    buf[p++] = GLX_SAMPLE_BUFFERS_SGIS;
>                    buf[p++] = modes->sampleBuffers;
>         +        buf[p++] = GLX_FRAMEBUFFER_SRGB_CAPABLE___EXT;
>         +        buf[p++] = modes->sRGBCapable;
>                    buf[p++] = 0;           /* copy over
>         visualSelectGroup (GLX_VISUAL_SELECT_GROUP_SGIX)__? */
>                    buf[p++] = 0;
>
>         @@ -1017,7 +1019,7 @@
>         __glXDisp_GetVisualConfigs(____GLXclientState * cl, GLbyte * pc)
>                return Success;
>            }
>
>         -#define __GLX_TOTAL_FBCONFIG_ATTRIBS (36)
>         +#define __GLX_TOTAL_FBCONFIG_ATTRIBS (37)
>            #define __GLX_FBCONFIG_ATTRIBS_LENGTH
>         (__GLX_TOTAL_FBCONFIG_ATTRIBS * 2)
>            /**
>             * Send the set of GLXFBConfigs to the client.  There is not
>         currently
>         @@ -1109,6 +1111,7 @@ DoGetFBConfigs(____GLXclientState * cl,
>         unsigned screen)
>                    WRITE_PAIR(GLX_BIND_TO_MIPMAP___TEXTURE_EXT,
>         modes->bindToMipmapTexture);
>                    WRITE_PAIR(GLX_BIND_TO___TEXTURE_TARGETS_EXT,
>                               modes->bindToTextureTargets);
>         +        WRITE_PAIR(GLX_FRAMEBUFFER___SRGB_CAPABLE_EXT,
>         modes->sRGBCapable);
>
>
>     I believe that sRGB visuals and fbconfigs should only be sent to the
>     client if the client has said that it supports one of the sRGB
>     extensions.  Otherwise the client doesn't know what
>     GLX_FRAMEBUFFER_SRGB_CAPABLE___ARB means.  It will either drop the
>     visuals or treat the sRGB and non-sRGB visuals the same.
>
> Tomasz:
> The most popular client - MESA - ignores unknown attributes (in
> __glXInitializeVisualConfigFromTags). This well suits the nature of this
> specific attribute - GLX_FRAMEBUFFER_SRGB_CAPABLE_ARB/EXT informs of a
> capability, and it's up to client to either use this capability, or
> ignore it completely.
> This capability can be safely ignored by the client who does not care
> about it. To use this capability, GLX client would have to call glEnable().
>
> XServer interface documentation should contain information regarding how
> clients ought to react to unknown attributes. Also, fbconfigs and
> specific attributes shouldn't be filtered by the server - it is up to
> client to filter them. But this issue does not concern the sRGB
> capability, and should be discussed separately.
> Please let me know how you'd like this patch to be changed.
>
>     Also, please use the _ARB enums instead.
>
> Tomasz:
> Please note that GLX_FRAMEBUFFER_SRGB_CAPABLE_ARB is not defined in
> glxtokens.h, only in glxext.h which requires glx.h - which is quite hard
> to include due to name conflicts with xserver.

Usually this would mean that the missing names would be added to 
glxtokens.h.  If these tokens were part of a core version (i.e., no 
suffix at all), I'd suggest updating glxtokens.h and this patch. 
However, it's probably not worth the effort in this case.

> Also, ARB and EXT defines are identical and will always be identical. I
> can't see any superiority of ARB define over EXT. Most of the other
> defines used in that file are not ARBs - but EXTs.
> Do you really think it is necessary to do such change? If you do, we
> need to discuss the correct way to #include a header with the definition
> of ARB.
> Maybe we should define GLX_FRAMEBUFFER_SRGB_CAPABLE_ARB in another
> xserver header, or patch MESA before making such change to XServer?
>
>
>
>                    if (client->swapped) {
>                        __GLX_SWAP_INT_ARRAY(buf,
>         __GLX_FBCONFIG_ATTRIBS_LENGTH)__;
>         diff --git a/glx/glxdri2.c b/glx/glxdri2.c
>         index bce1bfa..ef7afb4 100644
>         --- a/glx/glxdri2.c
>         +++ b/glx/glxdri2.c
>         @@ -882,6 +882,13 @@ initializeExtensions(____GLXDRIscreen * screen)
>                               "AIGLX: enabled GLX_SGI_swap_control and
>         GLX_MESA_swap_control\n");
>                }
>
>         +    /* enable EXT_framebuffer_sRGB extension (even if there are
>         no sRGB capable fbconfigs) */
>         +    {
>         +        __glXEnableExtension(screen->__glx_enable_bits,
>         +                 "GLX_EXT_framebuffer_sRGB");
>         +        LogMessage(X_INFO, "AIGLX: enabled
>         GLX_EXT_framebuffer_sRGB\n");
>         +    }
>         +
>                for (i = 0; extensions[i]; i++) {
>            #ifdef __DRI_READ_DRAWABLE
>                    if (strcmp(extensions[i]->name, __DRI_READ_DRAWABLE)
>         == 0) {
>         diff --git a/glx/glxdricommon.c b/glx/glxdricommon.c
>         index c90f380..b027f24 100644
>         --- a/glx/glxdricommon.c
>         +++ b/glx/glxdricommon.c
>         @@ -105,7 +105,9 @@ __ATTRIB(__DRI_ATTRIB_BUFFER___SIZE, rgbBits),
>                    __ATTRIB(__DRI_ATTRIB_BIND_TO___TEXTURE_RGB,
>         bindToTextureRgb),
>                    __ATTRIB(__DRI_ATTRIB_BIND_TO___TEXTURE_RGBA,
>         bindToTextureRgba),
>                    __ATTRIB(__DRI_ATTRIB_BIND_TO___MIPMAP_TEXTURE,
>         bindToMipmapTexture),
>         -        __ATTRIB(__DRI_ATTRIB___YINVERTED, yInverted),};
>         +        __ATTRIB(__DRI_ATTRIB___YINVERTED, yInverted),
>         +        __ATTRIB(__DRI_ATTRIB___FRAMEBUFFER_SRGB_CAPABLE,
>         sRGBCapable),
>         +        };
>
>            static void
>            setScalar(__GLXconfig * config, unsigned int attrib, unsigned
>         int value)
>         diff --git a/glx/glxscreens.c b/glx/glxscreens.c
>         index 61d590c..e87256d 100644
>         --- a/glx/glxscreens.c
>         +++ b/glx/glxscreens.c
>         @@ -164,7 +164,9 @@ static char GLXServerVendorName[] = "SGI";
>            unsigned glxMajorVersion = SERVER_GLX_MAJOR_VERSION;
>            unsigned glxMinorVersion = SERVER_GLX_MINOR_VERSION;
>            static char GLXServerExtensions[] =
>         +    "GLX_ARB_framebuffer_sRGB "
>                "GLX_ARB_multisample "
>         +    "GLX_EXT_framebuffer_sRGB "
>                "GLX_EXT_visual_info "
>                "GLX_EXT_visual_rating "
>                "GLX_EXT_import_context "
>         diff --git a/glx/glxscreens.h b/glx/glxscreens.h
>         index b29df58..0a7b604 100644
>         --- a/glx/glxscreens.h
>         +++ b/glx/glxscreens.h
>         @@ -102,6 +102,9 @@ struct __GLXconfig {
>                GLint bindToMipmapTexture;
>                GLint bindToTextureTargets;
>                GLint yInverted;
>         +
>         +    /* ARB_framebuffer_sRGB */
>         +    GLint sRGBCapable;
>            };
>
>            GLint glxConvertToXVisualType(int visualType);
>         diff --git a/hw/xquartz/GL/visualConfigs.__c
>         b/hw/xquartz/GL/visualConfigs.__c
>         index e37eefb..18518af 100644
>         --- a/hw/xquartz/GL/visualConfigs.__c
>         +++ b/hw/xquartz/GL/visualConfigs.__c
>         @@ -326,6 +326,9 @@ __glXAquaCreateVisualConfigs(__int
>         *numConfigsPtr, int screenNumber)
>
>         c->bindToTextureTargets = 0;
>                                                    c->yInverted = 0;
>
>         +                                        /* EXT_framebuffer_sRGB */
>         +                                        c->sRGBCapable =
>         GL_DONT_CARE;
>
>
>     This is wrong.  sRGBCapable should be either True or False.
>     GL_DONT_CARE (~0) is not a valid value.
>
> I can do that change, the xquartz and xwin are not really my area of
> expertise.
> Still, please consider the following scenario:
> The client asks xserver for configs, then compares them with its own
> configs. Only configs which match on client and server are accepted.
> Attribute comparison function checks if the attributes are exact match,
> or if one of them is GLX_DONT_CARE.

GLX_DONT_CARE filtering only happens in glXChooseVisual and friends. 
The client / server config handshake for direct rendering is different. 
  Mesa has at least one or two bugs in this area.  I have a patch for 
one of them related to sRGB, and I could have sworn that I sent it to 
you.  I'll try to get it posted to the public list soon.

> This means that if we'll advertise sRGBCapable = false, then all
> sRGB-capable configs will be dropped.
> But if we'll advertise sRGBCapable = GLX_DONT_CARE, then both sRGB
> capable and non-capable configs will be accepted.

GLX_DONT_CARE violates the spec, so the rest of the argument is void. 
This also argues against sending the GLX_FRAMEBUFFER_SRGB_CAPABLE 
attribute to clients that don't understand it. :)  If the client never 
sees the attribute, it can't do the wrong thing with it.

> As we don't really support sRGB in the xquartz and xwin, this may be
> desired effect.
>
> Now, MESA works in the exact way described above - it compares the two
> sets of attributes and select matches using such comparison.
> I'm not sure if the situation I described is related to xquartz and
> xwin, so please confirm if the attributes should be set to GL_FALSE.
>
>     +
>                                                c = c->next;
>                                            }
>                                        }
>     diff --git a/hw/xwin/glx/indirect.c b/hw/xwin/glx/indirect.c
>     index c0069a2..5b339af 100644
>     --- a/hw/xwin/glx/indirect.c
>     +++ b/hw/xwin/glx/indirect.c
>     @@ -2017,6 +2017,7 @@ glxWinCreateConfigs(HDC hdc, glxWinScreen *
>     screen)
>                c->base.bindToMipmapTexture = -1;
>                c->base.bindToTextureTargets = -1;
>                c->base.yInverted = -1;
>     +        c->base.sRGBCapable = -1;
>
>                n++;
>
>     @@ -2411,6 +2412,7 @@ glxWinCreateConfigsExt(HDC hdc, glxWinScreen *
>     screen)
>                    GLX_TEXTURE_1D_BIT_EXT | GLX_TEXTURE_2D_BIT_EXT |
>                    GLX_TEXTURE_RECTANGLE_BIT_EXT;
>                c->base.yInverted = -1;
>     +        c->base.sRGBCapable = -1;
>
>                n++;



More information about the xorg-devel mailing list