[PATCH] [xorg-dev][xserver] Full support of sRGB capable fbconfigs.
Ian Romanick
idr at freedesktop.org
Mon Jan 7 13:13:36 PST 2013
On 01/07/2013 08:37 AM, Tomasz Lis wrote:
>
>
> 2013/1/5 Ian Romanick <idr at freedesktop.org <mailto:idr at freedesktop.org>>
>
> 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>
> <mailto: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>
> <mailto: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>
>
> <mailto: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?
>
> 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.
As far as I can tell, there's no way to have a different number of
attributes for each config. Looking at how the protocol is decoded, I
think we can pad with zeros at the end. That should make the
server-side implementation a lot easier.
> 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.
>
> >>
> >> Signed-off-by: Tomasz Lis
> <tomasz.lis-__ral2JQCrhuEAvxtiuMwx3w at public.__gmane.org
> <mailto:tomasz.lis-ral2JQCrhuEAvxtiuMwx3w at public.gmane.org>
>
> <mailto: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 <mailto:xorg-devel at lists.x.org>: X.Org
> development
> Archives: http://lists.x.org/archives/__xorg-devel
> <http://lists.x.org/archives/xorg-devel>
> Info: http://lists.x.org/mailman/__listinfo/xorg-devel
> <http://lists.x.org/mailman/listinfo/xorg-devel>
More information about the xorg-devel
mailing list