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

Tomasz Lis listom at gmail.com
Wed Jan 30 03:39:04 PST 2013


2013/1/29 Ian Romanick <idr at freedesktop.org>

> On 01/09/2013 05:46 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).
>> This version advertises both ARB and EXT strings, and initializes
>> the capability to default value of FALSE. It has corrected required
>> GLX version and does not influence swrast. The sRGB capable attribute
>> is attached only to those configs which do have this capability.
>>
>> Signed-off-by: Tomasz Lis <tomasz.lis at intel.com>
>>
>
> Other than the one thing below, this patch looks good.  Sorry for the
> massive lag.  I've been overwhelmed by OpenGL ES 3.0 work all year so far.
> :(
>
> I think we should work on improving the communication. I hope that
together we will be able to prepare a path which would allow to close
reviews of future patches within 2 weeks max.
Long review process forces us to spend resources on temporary workarounds,
and we'd very much like to avoid that in the future.

>
>  ---
>>   glx/extension_string.c        |    2 ++
>>   glx/extension_string.h        |    2 ++
>>   glx/glxcmds.c                 |   26 ++++++++++++++++++++++----
>>   glx/glxdri2.c                 |    7 +++++++
>>   glx/glxdricommon.c            |    4 +++-
>>   glx/glxscreens.h              |    3 +++
>>   hw/xquartz/GL/visualConfigs.c |    3 +++
>>   hw/xwin/glx/indirect.c        |    2 ++
>>   8 files changed, 44 insertions(+), 5 deletions(-)
>>
>> diff --git a/glx/extension_string.c b/glx/extension_string.c
>> index 544ca1f..58f930f 100644
>> --- a/glx/extension_string.c
>> +++ b/glx/extension_string.c
>> @@ -71,9 +71,11 @@ static const struct extension_info
>> known_glx_extensions[] = {
>>       { GLX(ARB_create_context),          VER(0,0), N, },
>>       { GLX(ARB_create_context_**profile),  VER(0,0), N, },
>>       { GLX(ARB_create_context_**robustness), VER(0,0), N, },
>> +    { GLX(ARB_framebuffer_sRGB),        VER(0,0), N, },
>>       { GLX(ARB_multisample),             VER(1,4), Y, },
>>
>>       { GLX(EXT_create_context_es2_**profile), VER(0,0), N, },
>> +    { GLX(EXT_framebuffer_sRGB),        VER(0,0), N, },
>>       { 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, },
>>
>
> Do we want to expose both extensions together always?  As far as I can
> tell, these are the same other than the name.  Is that your understanding
> as well?  This is how I treat them on the client side.
>
> Actually, there are simple differences in the specs - it's quite easy to
use 'diff' on them. Still, they are non-exclusive - one code can be used to
support both EXT and ARB.

The differences are in attributes returned by GetBooleanv and similar
functions. There is a <pname> value in EXT which was removed from ARB,
because it was replaced by FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING (which is
defined in different extension).
The pname code is different, and also values returned aren't matching (EXT
one gives true/false, and the one from ARB gives LINEAR/SRGB which are
defined as some hex values).

Do you think we should sometimes expose only one of the extension names?
Please provide some details.


>  diff --git a/glx/extension_string.h b/glx/extension_string.h
>> index 7a4a8b1..3ce5593 100644
>> --- a/glx/extension_string.h
>> +++ b/glx/extension_string.h
>> @@ -39,8 +39,10 @@ enum {
>>       ARB_create_context_bit = 0,
>>       ARB_create_context_profile_**bit,
>>       ARB_create_context_robustness_**bit,
>> +    ARB_framebuffer_sRGB_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,
>>
>
> If we decide yes to the above, we can handle this the same way I did on
> the client:
>
> #define EXT_framebuffer_sRGB_bit ARB_framebuffer_sRGB_bit
>
>  You're right, this can be handled together. Agreed to change EXT bit into
#define.

>
>  diff --git a/glx/glxcmds.c b/glx/glxcmds.c
>> index c1f4e22..5b7a628 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,8 +1005,17 @@ __glXDisp_GetVisualConfigs(__**GLXclientState *
>> cl, GLbyte * pc)
>>           buf[p++] = modes->samples;
>>           buf[p++] = GLX_SAMPLE_BUFFERS_SGIS;
>>           buf[p++] = modes->sampleBuffers;
>> -        buf[p++] = 0;           /* copy over visualSelectGroup
>> (GLX_VISUAL_SELECT_GROUP_SGIX)**? */
>> -        buf[p++] = 0;
>> +        /* Add attribute only if its value is not default. */
>> +        if (modes->sRGBCapable != GL_FALSE) {
>> +            buf[p++] = GLX_FRAMEBUFFER_SRGB_CAPABLE_**EXT;
>> +            buf[p++] = modes->sRGBCapable;
>> +        }
>> +        /* Don't add visualSelectGroup (GLX_VISUAL_SELECT_GROUP_SGIX)**?
>> +         * Pad the remaining place with zeroes, so that attributes count
>> is constant. */
>> +        while (p < GLX_VIS_CONFIG_TOTAL) {
>> +            buf[p++] = 0;
>> +            buf[p++] = 0;
>> +        }
>>
>>           assert(p == GLX_VIS_CONFIG_TOTAL);
>>           if (client->swapped) {
>> @@ -1017,7 +1026,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 +1118,15 @@ 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);
>> +        /* Add attribute only if its value is not default. */
>> +        if (modes->sRGBCapable != GL_FALSE) {
>> +            WRITE_PAIR(GLX_FRAMEBUFFER_**SRGB_CAPABLE_EXT,
>> modes->sRGBCapable);
>> +        }
>> +        /* Pad the remaining place with zeroes, so that attributes count
>> is constant. */
>> +        while (p < __GLX_FBCONFIG_ATTRIBS_LENGTH) {
>> +            WRITE_PAIR(0, 0);
>> +        }
>> +        assert(p == __GLX_FBCONFIG_ATTRIBS_LENGTH)**;
>>
>>           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.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..5efd04f 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_FALSE;
>> +
>>                                           c = c->next;
>>                                       }
>>                                   }
>> diff --git a/hw/xwin/glx/indirect.c b/hw/xwin/glx/indirect.c
>> index c0069a2..e6faf19 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 = 0;
>>
>>           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 = 0;
>>
>>           n++;
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20130130/ff0f7117/attachment-0001.html>


More information about the xorg-devel mailing list