<div class="gmail_quote">2013/1/5 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"> On 12/05/2012 04:17 AM, Tomasz Lis wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 2012/12/4 Jon TURNEY <<a href="mailto:jon.turney@dronecode.org.uk" target="_blank">jon.turney@dronecode.org.uk</a> <mailto:<a href="mailto:jon.turney@dronecode.org.uk" target="_blank">jon.turney@dronecode.org.uk</a>>><div class="im"> On 04/12/2012 15:28, Tomasz Lis wrote: > These are not all the changes, sorry for that. > > It seems I don't know how to correctly re-send the patch. > > 2012/12/4 <<a href="mailto:listom-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org" target="_blank">listom-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org</a> </div> <mailto:<a href="mailto:listom-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org" target="_blank">listom-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org</a>>> > >> From: Tomasz Lis <<a href="mailto:tomasz.lis-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org" target="_blank">tomasz.lis-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org</a> <mailto:<a href="mailto:tomasz.lis-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org" target="_blank">tomasz.lis-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org</a>>><div class="im"> >> >> 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. Your updated patch doesn't seem to address idr's comment that you must not send the GLX_FRAMEBUFFER_SRGB_CAPABLE_EXT token to clients that don't understand it. I am not sure of the mechanism for a client to indicate this understanding. This is true, the issue isn't closed yet. Please refer to the discussion over previous version of the patch. It ended with me asking: Do you still think we need a change here? Do you think we should filter whole configs, or only attributes within configs? Do you have a concept on how to implement it? This seem to be a big issue, maybe we could commit the changes as they are, and then think about a solution? </div></blockquote> I looked into this a bit more. The problem is that without GLX_ARB_create_context, the client doesn't tell the server what extensions it supports. That effectively makes it impossible to do this checking without that extension. Since Mesa already supports that extension, I think it's okay to rely on it. However, we currently just drop the client's GLX extension list after receiving it. We'll have to keep track of it (probably by converting the strings to flags, as is done with GL extensions). I believe we should take the following steps. 1. For configs that have the default value, do not send the extension attribute to the client. This allows the client to just drop configs that contain unknown attributes. This also means that some "padding" attributes will have to be inserted so that every config has the same number of attributes. 2. Modify Mesa's libGL to drop configs with unknown attributes (and back-port the patch to at least the 9.0 release tree). 3. For now, send extension attributes for configs that have non-default values to the client. 4. Add xserver infrastructure to track the client-supported GLX extensions. 5. Only send configs with non-default valued extension attributes if the client supports the required extension. Tomasz, can you handle steps 1 and 3 in your patch series? </blockquote><div>I don't really think it's a good idea to keep the constant amount of attributes while not sending some of them - but it's up to you, I can agree to your solution. The question is - what should be the padding value? We could use GL_FALSE or GLX_DONT_CARE as attribute ID, or define our own value. I recommend null-terminating the list, with either GL_FALSE or our own new define - ie. GLX_END_LIST. Bringing our own definition requires to add it to a header, glx.h or glxext.h - so using GL_FALSE is simpler. Also, I think it would be best if the list was always null-terminated - even if no attributes are skipped. Remember that the padding attribute ID must be ignored by MESA (so it influences step 2 on your list). Let me know which solution you prefer. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org" target="_blank">tomasz.lis-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org</a> <mailto:<a href="mailto:tomasz.lis-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org" target="_blank">tomasz.lis-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org</a>>><div><div class="h5"> >> --- >> glx/extension_string.c | 2 ++ >> glx/extension_string.h | 2 ++ >> 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, 29 insertions(+), 3 deletions(-) >> >> diff --git a/glx/extension_string.c b/glx/extension_string.c >> index 544ca1f..70dc915 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(1,1), N, }, >> { 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, }, According to the comments above, writing VER(1,1) here means that this extension is required in GLX 1.1 (although this data is not used in the X server). I don't think that's true, and you should be writing VER(0,0). I'm not entirely sure that this should be reported as a GLX extension at all. The ARB_framebuffer_sRGB spec says it is GLX extension. But you are completely right with the versions - OGL 1.3 has no relation to this extension, so GLX specs up to 1.4 definitely don't require it. Agreed to change required GLX version to (0,0). >> --- 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 " This is the static list of extensions reported for swrast. I don't think this extension belongs here. You're right. Agreed to remove. _______________________________________________ <a href="mailto:xorg-devel@lists.x.org" target="_blank">xorg-devel@lists.x.org</a>: X.Org development Archives: <a href="http://lists.x.org/archives/xorg-devel" target="_blank">http://lists.x.org/archives/xorg-devel</a> Info: <a href="http://lists.x.org/mailman/listinfo/xorg-devel" target="_blank">http://lists.x.org/mailman/listinfo/xorg-devel</a> </div></div></blockquote> </blockquote></div>