[PATCH] [xorg-dev][xserver] Full support of sRGB capable fbconfigs.

Tomasz Lis listom at gmail.com
Mon Jan 7 08:37:44 PST 2013 

2013/1/5 Ian Romanick <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 <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<listom-Re5JQEeQqe8AvxtiuMwx3w at public.gmane.org>
>>     <mailto:listom-**Re5JQEeQqe8AvxtiuMwx3w at public.**gmane.org<listom-Re5JQEeQqe8AvxtiuMwx3w at public.gmane.org>
>> >>
>>      >
>>      >> From: Tomasz Lis
>>     <tomasz.lis-**ral2JQCrhuEAvxtiuMwx3w at public.**gmane.org<tomasz.lis-ral2JQCrhuEAvxtiuMwx3w at public.gmane.org>
>>     <mailto:tomasz.lis-**ral2JQCrhuEAvxtiuMwx3w at public.**gmane.org<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.

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<tomasz.lis-ral2JQCrhuEAvxtiuMwx3w at public.gmane.org>
>>     <mailto:tomasz.lis-**ral2JQCrhuEAvxtiuMwx3w at public.**gmane.org<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<http://lists.x.org/archives/xorg-devel>
>> Info: http://lists.x.org/mailman/**listinfo/xorg-devel<http://lists.x.org/mailman/listinfo/xorg-devel>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20130107/73b58d94/attachment.html>

More information about the xorg-devel mailing list