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

Tomasz Lis listom at gmail.com
Fri Nov 30 07:18:25 PST 2012


2012/11/26 Ian Romanick <idr at freedesktop.org>

> On 11/22/2012 07:08 AM, Tomasz Lis wrote:
>
>> From: Tomasz Lis <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>
>> ---
>>   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.
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.
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.
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++;
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20121130/fb545e75/attachment-0001.html>


More information about the xorg-devel mailing list