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

2013-03-18 Thread Keith Packard 
Tomasz Lis lis...@gmail.com writes:

 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.
 Both ARB and EXT versions share the same GLX extension enabling bit.

Merged.
   5047810..cf89aa5  master - master

-- 
keith.pack...@intel.com


pgpNYiD4f0uc1.pgp
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

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

2013-03-08 Thread Ian Romanick 

On 01/09/2013 05:46 AM, Tomasz Lis wrote:

From: Tomasz Lis tomasz@intel.com

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 tomasz@intel.com


Reviewed-by: Ian Romanick ian.d.roman...@intel.com


---
  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, },
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,
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

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

2013-01-30 Thread Ian Romanick 

On 01/30/2013 03:39 AM, Tomasz Lis wrote:



2013/1/29 Ian Romanick i...@freedesktop.org mailto:i...@freedesktop.org

On 01/09/2013 05:46 AM, Tomasz Lis wrote:

From: Tomasz Lis tomasz@intel.com
mailto:tomasz@intel.com

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 tomasz@intel.com
mailto:tomasz@intel.com


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. :(

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.


---
   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, },


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.

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.


These are all differences in the GL part of the extension, not the GLX 
part.  This is GLX code.



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,


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

  You're right, this can be handled together. Agreed to change EXT bit
into #define.


diff --git a/glx/glxcmds.c b/glx/glxcmds.c
index c1f4e22..5b7a628 100644
--- a/glx/glxcmds.c
+++ b/glx/glxcmds.c

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

2013-01-29 Thread Ian Romanick 

On 01/09/2013 05:46 AM, Tomasz Lis wrote:

From: Tomasz Lis tomasz@intel.com

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 tomasz@intel.com


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. :(



---
  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, },


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.



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,


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


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);
+}
+

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

2013-01-18 Thread Tomasz Lis 
Hello,

Are there any other issues with this patch?
Is there anything more needed for it to be commited?

Regards,
Tomasz Lis

2013/1/9 Tomasz Lis lis...@gmail.com

 From: Tomasz Lis tomasz@intel.com

 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 tomasz@intel.com
 ---
  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, },
 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,
 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,

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

2013-01-07 Thread Ian Romanick 

On 01/07/2013 08:37 AM, Tomasz Lis wrote:



2013/1/5 Ian Romanick i...@freedesktop.org mailto:i...@freedesktop.org

On 12/05/2012 04:17 AM, Tomasz Lis wrote:

2012/12/4 Jon TURNEY jon.tur...@dronecode.org.uk
mailto:jon.tur...@dronecode.org.uk
mailto:jon.turney@dronecode.__org.uk
mailto:jon.tur...@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@public.__gmane.org
mailto:listom-re5jqeeqqe8avxtiumw...@public.gmane.org
 mailto:listom-__Re5JQEeQqe8AvxtiuMwx3w@public.__gmane.org
mailto:listom-re5jqeeqqe8avxtiumw...@public.gmane.org
  
   From: Tomasz Lis
 tomasz.lis-__ral2JQCrhuEAvxtiuMwx3w@public.__gmane.org
mailto:tomasz.lis-ral2jqcrhueavxtiumw...@public.gmane.org

mailto:tomasz.lis-__ral2JQCrhuEAvxtiuMwx3w@public.__gmane.org
mailto:tomasz.lis-ral2jqcrhueavxtiumw...@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

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

2013-01-04 Thread Ian Romanick 

On 12/05/2012 04:17 AM, Tomasz Lis wrote:

2012/12/4 Jon TURNEY jon.tur...@dronecode.org.uk
mailto:jon.tur...@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-re5jqeeqqe8avxtiumw...@public.gmane.org
mailto:listom-re5jqeeqqe8avxtiumw...@public.gmane.org
 
  From: Tomasz Lis
tomasz.lis-ral2jqcrhueavxtiumw...@public.gmane.org
mailto:tomasz.lis-ral2jqcrhueavxtiumw...@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-ral2jqcrhueavxtiumw...@public.gmane.org
mailto:tomasz.lis-ral2jqcrhueavxtiumw...@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).

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

2012-12-05 Thread Tomasz Lis 
2012/12/4 Jon TURNEY jon.tur...@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-re5jqeeqqe8avxtiumw...@public.gmane.org
 
  From: Tomasz Lis tomasz.lis-ral2jqcrhueavxtiumw...@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?


 
  Signed-off-by: Tomasz Lis 
 tomasz.lis-ral2jqcrhueavxtiumw...@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@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

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

2012-12-04 Thread Tomasz Lis 
Sorry again, the patch is OK.
I got confused by gmail.

2012/12/4 Tomasz Lis lis...@gmail.com

 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 lis...@gmail.com

 From: Tomasz Lis tomasz@intel.com

 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.

 Signed-off-by: Tomasz Lis tomasz@intel.com
 ---
  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, },
  { 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, },
 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,
 diff --git a/glx/glxcmds.c b/glx/glxcmds.c
 index c1f4e22..720a1a8 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,6 +1005,8 @@ __glXDisp_GetVisualConfigs(__GLXclientState * cl,
 GLbyte * pc)
  buf[p++] = modes-samples;
  buf[p++] = GLX_SAMPLE_BUFFERS_SGIS;
  buf[p++] = modes-sampleBuffers;
 +buf[p++] = GLX_FRAMEBUFFER_SRGB_CAPABLE_EXT;
 +buf[p++] = modes-sRGBCapable;
  buf[p++] = 0;   /* copy over visualSelectGroup
 (GLX_VISUAL_SELECT_GROUP_SGIX)? */
  buf[p++] = 0;

 @@ -1017,7 +1019,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 +,7 @@ 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);
 +WRITE_PAIR(GLX_FRAMEBUFFER_SRGB_CAPABLE_EXT, modes-sRGBCapable);

  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),

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

2012-12-04 Thread Jon TURNEY 
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-re5jqeeqqe8avxtiumw...@public.gmane.org
 
 From: Tomasz Lis tomasz.lis-ral2jqcrhueavxtiumw...@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.


 Signed-off-by: Tomasz Lis 
 tomasz.lis-ral2jqcrhueavxtiumw...@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.

 --- 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.

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

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

2012-11-30 Thread Tomasz Lis 
2012/11/26 Ian Romanick i...@freedesktop.org

 On 11/22/2012 07:08 AM, Tomasz Lis wrote:

 From: Tomasz Lis tomasz@intel.com

 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).


 Please word-wrap commit messages to 72 characters.


Tomasz:
Ok. will do that.


 Signed-off-by: Tomasz Lis tomasz@intel.com
 ---
   glx/extension_string.c|1 +
   glx/extension_string.h|1 +
   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, 27 insertions(+), 3 deletions(-)

 diff --git a/glx/extension_string.c b/glx/extension_string.c
 index 544ca1f..47a9746 100644
 --- a/glx/extension_string.c
 +++ b/glx/extension_string.c
 @@ -74,6 +74,7 @@ static const struct extension_info
 known_glx_extensions[] = {
   { 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, },


 Also advertise the ARB string.

 Tomasz:
Agreed. Will change.


{ 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, },
 diff --git a/glx/extension_string.h b/glx/extension_string.h
 index 7a4a8b1..a91385f 100644
 --- a/glx/extension_string.h
 +++ b/glx/extension_string.h
 @@ -41,6 +41,7 @@ enum {
   ARB_create_context_robustness_**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,
 diff --git a/glx/glxcmds.c b/glx/glxcmds.c
 index c1f4e22..720a1a8 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,6 +1005,8 @@ __glXDisp_GetVisualConfigs(__**GLXclientState *
 cl, GLbyte * pc)
   buf[p++] = modes-samples;
   buf[p++] = GLX_SAMPLE_BUFFERS_SGIS;
   buf[p++] = modes-sampleBuffers;
 +buf[p++] = GLX_FRAMEBUFFER_SRGB_CAPABLE_**EXT;
 +buf[p++] = modes-sRGBCapable;
   buf[p++] = 0;   /* copy over visualSelectGroup
 (GLX_VISUAL_SELECT_GROUP_SGIX)**? */
   buf[p++] = 0;

 @@ -1017,7 +1019,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 +,7 @@ 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);
 +WRITE_PAIR(GLX_FRAMEBUFFER_**SRGB_CAPABLE_EXT,
 modes-sRGBCapable);


 I believe that sRGB visuals and fbconfigs should only be sent to the
 client if the client has said that it supports one of the sRGB extensions.
  Otherwise the client doesn't know what GLX_FRAMEBUFFER_SRGB_CAPABLE_**ARB
 means.  It will either drop the visuals or treat the sRGB and non-sRGB
 visuals the same.

 Tomasz:
The most popular client - MESA - ignores unknown attributes (in
__glXInitializeVisualConfigFromTags). This well suits the nature of this
specific attribute - GLX_FRAMEBUFFER_SRGB_CAPABLE_ARB/EXT informs of a
capability, and it's up to client to either use this capability, or ignore
it completely.
This capability can be safely ignored by the client who does not care about
it. To use this capability, GLX client would have to call glEnable().

XServer interface documentation should contain information regarding how
clients ought to react to unknown attributes. Also, fbconfigs and specific
attributes shouldn't be filtered by the server - it is up to client to
filter them. But this issue does not concern the sRGB capability, and
should be discussed separately.
Please let me know how you'd like this patch to be changed.


 Also, please use the _ARB enums instead.

 Tomasz:
Please note that GLX_FRAMEBUFFER_SRGB_CAPABLE_ARB is not defined in
glxtokens.h, only in glxext.h which requires glx.h - which is quite hard to
include due to name conflicts with xserver.
Also,

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

2012-11-30 Thread Ian Romanick 

On 11/30/2012 07:18 AM, Tomasz Lis wrote:


2012/11/26 Ian Romanick i...@freedesktop.org mailto:i...@freedesktop.org

On 11/22/2012 07:08 AM, Tomasz Lis wrote:

From: Tomasz Lis tomasz@intel.com
mailto:tomasz@intel.com

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).


Please word-wrap commit messages to 72 characters.

Tomasz:
Ok. will do that.

Signed-off-by: Tomasz Lis tomasz@intel.com
mailto:tomasz@intel.com
---
   glx/extension_string.c|1 +
   glx/extension_string.h|1 +
   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, 27 insertions(+), 3 deletions(-)

diff --git a/glx/extension_string.c b/glx/extension_string.c
index 544ca1f..47a9746 100644
--- a/glx/extension_string.c
+++ b/glx/extension_string.c
@@ -74,6 +74,7 @@ static const struct extension_info
known_glx_extensions[] = {
   { 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, },


Also advertise the ARB string.

Tomasz:
Agreed. Will change.


   { 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, },
diff --git a/glx/extension_string.h b/glx/extension_string.h
index 7a4a8b1..a91385f 100644
--- a/glx/extension_string.h
+++ b/glx/extension_string.h
@@ -41,6 +41,7 @@ enum {
   ARB_create_context_robustness___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,
diff --git a/glx/glxcmds.c b/glx/glxcmds.c
index c1f4e22..720a1a8 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,6 +1005,8 @@
__glXDisp_GetVisualConfigs(GLXclientState * cl, GLbyte * pc)
   buf[p++] = modes-samples;
   buf[p++] = GLX_SAMPLE_BUFFERS_SGIS;
   buf[p++] = modes-sampleBuffers;
+buf[p++] = GLX_FRAMEBUFFER_SRGB_CAPABLE___EXT;
+buf[p++] = modes-sRGBCapable;
   buf[p++] = 0;   /* copy over
visualSelectGroup (GLX_VISUAL_SELECT_GROUP_SGIX)__? */
   buf[p++] = 0;

@@ -1017,7 +1019,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 +,7 @@ 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);
+WRITE_PAIR(GLX_FRAMEBUFFER___SRGB_CAPABLE_EXT,
modes-sRGBCapable);


I believe that sRGB visuals and fbconfigs should only be sent to the
client if the client has said that it supports one of the sRGB
extensions.  Otherwise the client doesn't know what
GLX_FRAMEBUFFER_SRGB_CAPABLE___ARB means.  It will either drop the
visuals or treat the sRGB and non-sRGB visuals the same.

Tomasz:
The most popular client - MESA - ignores unknown attributes (in
__glXInitializeVisualConfigFromTags). This well suits the nature of this
specific attribute - GLX_FRAMEBUFFER_SRGB_CAPABLE_ARB/EXT informs of a
capability, and it's up to client to either use this capability, or
ignore

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

2012-11-26 Thread Ian Romanick 

On 11/22/2012 07:08 AM, Tomasz Lis wrote:

From: Tomasz Lis tomasz@intel.com

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).


Please word-wrap commit messages to 72 characters.


Signed-off-by: Tomasz Lis tomasz@intel.com
---
  glx/extension_string.c|1 +
  glx/extension_string.h|1 +
  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, 27 insertions(+), 3 deletions(-)

diff --git a/glx/extension_string.c b/glx/extension_string.c
index 544ca1f..47a9746 100644
--- a/glx/extension_string.c
+++ b/glx/extension_string.c
@@ -74,6 +74,7 @@ static const struct extension_info known_glx_extensions[] = {
  { 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, },


Also advertise the ARB string.


  { 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, },
diff --git a/glx/extension_string.h b/glx/extension_string.h
index 7a4a8b1..a91385f 100644
--- a/glx/extension_string.h
+++ b/glx/extension_string.h
@@ -41,6 +41,7 @@ enum {
  ARB_create_context_robustness_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,
diff --git a/glx/glxcmds.c b/glx/glxcmds.c
index c1f4e22..720a1a8 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,6 +1005,8 @@ __glXDisp_GetVisualConfigs(__GLXclientState * cl, GLbyte 
* pc)
  buf[p++] = modes-samples;
  buf[p++] = GLX_SAMPLE_BUFFERS_SGIS;
  buf[p++] = modes-sampleBuffers;
+buf[p++] = GLX_FRAMEBUFFER_SRGB_CAPABLE_EXT;
+buf[p++] = modes-sRGBCapable;
  buf[p++] = 0;   /* copy over visualSelectGroup 
(GLX_VISUAL_SELECT_GROUP_SGIX)? */
  buf[p++] = 0;

@@ -1017,7 +1019,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 +,7 @@ 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);
+WRITE_PAIR(GLX_FRAMEBUFFER_SRGB_CAPABLE_EXT, modes-sRGBCapable);


I believe that sRGB visuals and fbconfigs should only be sent to the 
client if the client has said that it supports one of the sRGB 
extensions.  Otherwise the client doesn't know what 
GLX_FRAMEBUFFER_SRGB_CAPABLE_ARB means.  It will either drop the visuals 
or treat the sRGB and non-sRGB visuals the same.


Also, please use the _ARB enums instead.



  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),
-