[PATCH] [xorg-dev][xserver] Full support of sRGB capable fbconfigs.
Ian Romanick
idr at freedesktop.org
Fri Jan 4 16:47:25 PST 2013
On 12/05/2012 04:17 AM, Tomasz Lis wrote:
> 2012/12/4 Jon TURNEY <jon.turney at dronecode.org.uk
> <mailto:jon.turney at dronecode.org.uk>>
>
> 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 <listom-Re5JQEeQqe8AvxtiuMwx3w at public.gmane.org
> <mailto:listom-Re5JQEeQqe8AvxtiuMwx3w at public.gmane.org>>
> >
> >> From: Tomasz Lis
> <tomasz.lis-ral2JQCrhuEAvxtiuMwx3w at public.gmane.org
> <mailto:tomasz.lis-ral2JQCrhuEAvxtiuMwx3w at public.gmane.org>>
> >>
> >> 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?
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?
> >>
> >> Signed-off-by: Tomasz Lis
> <tomasz.lis-ral2JQCrhuEAvxtiuMwx3w at public.gmane.org
> <mailto:tomasz.lis-ral2JQCrhuEAvxtiuMwx3w at public.gmane.org>>
> >> ---
> >> 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.
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
More information about the xorg-devel
mailing list