<div class="gmail_quote">2012/11/26 Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im">On 11/22/2012 07:08 AM, Tomasz Lis wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> From: Tomasz Lis <<a href="mailto:tomasz.lis@intel.com" target="_blank">tomasz.lis@intel.com</a>> 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). </blockquote> </div> Please word-wrap commit messages to 72 characters.<div></div></blockquote><div> </div><div>Tomasz: Ok. will do that. </div><div> </div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Signed-off-by: Tomasz Lis <<a href="mailto:tomasz.lis@intel.com" target="_blank">tomasz.lis@intel.com</a>> --- 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, }, </blockquote> </div> Also advertise the ARB string.<div><div class="h5"> </div></div></blockquote><div>Tomasz: Agreed. Will change. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> { 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); </blockquote> </div></div> 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. </blockquote><div>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. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Also, please use the _ARB enums instead.<div><div class="h5"> </div></div></blockquote><div>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? </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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; </blockquote> </div></div> This is wrong. sRGBCapable should be either True or False. GL_DONT_CARE (~0) is not a valid value.<div class="HOEnZb"><div class="h5"> </div></div></blockquote><div class="h5">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. <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + 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++; </blockquote> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"></div></blockquote></div>