<div class="gmail_quote">2013/1/29 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 01/09/2013 05:46 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). 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 <<a href="mailto:tomasz.lis@intel.com" target="_blank">tomasz.lis@intel.com</a>> </blockquote> </div> 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. :(<div><div class="h5"> </div></div></blockquote><div>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. </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/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, }, </blockquote> </div></div> 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.<div class="im"> </div> </blockquote><div>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. </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"> 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, </blockquote> </div> 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<div class="HOEnZb"><div class="h5"> </div></div></blockquote><div> You're right, this can be handled together. Agreed to change EXT bit into #define. </div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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++; </blockquote> </div></div></blockquote></div>