[PATCH] [xorg-dev][xserver] Full support of sRGB capable fbconfigs.
Ian Romanick
idr at freedesktop.org
Wed Jan 30 11:58:13 PST 2013
On 01/30/2013 03:39 AM, Tomasz Lis wrote:
>
>
> 2013/1/29 Ian Romanick <idr at freedesktop.org <mailto:idr at freedesktop.org>>
>
> On 01/09/2013 05:46 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).
> 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
> <mailto: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.
These are all differences in the GL part of the extension, not the GLX
part. This is GLX code.
> 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++;
>
>
>
>
More information about the xorg-devel
mailing list