[xserver] glx: Fix attribute handling for GLX_TEXTURE_FORMAT_EXT

Submitted by Adam Jackson on April 2, 2018, 6:52 p.m.

Details

Message ID 20180402185228.22508-1-ajax@redhat.com
State New
Headers show
Series "glx: Fix attribute handling for GLX_TEXTURE_FORMAT_EXT" ( rev: 1 ) in X.org

Not browsing as part of any series.

Commit Message

Adam Jackson April 2, 2018, 6:52 p.m.
Initialize it correctly to GLX_TEXTURE_FORMAT_NONE_EXT, and report it in
GLXGetDrawableAttributes. Note that it and TEXTURE_TARGET are only
meaningful attributes for GLXPixmaps.

Signed-off-by: Adam Jackson <ajax@redhat.com>
---
 glx/glxcmds.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/glx/glxcmds.c b/glx/glxcmds.c
index 6785e9db38..847271e9e3 100644
--- a/glx/glxcmds.c
+++ b/glx/glxcmds.c
@@ -1173,6 +1173,9 @@  DoCreateGLXDrawable(ClientPtr client, __GLXscreen * pGlxScreen,
         !AddResource(pDraw->id, __glXDrawableRes, pGlxDraw))
         return BadAlloc;
 
+    if (type == GLX_DRAWABLE_PIXMAP)
+        pGlxDraw->format = GLX_TEXTURE_FORMAT_NONE_EXT;
+
     return Success;
 }
 
@@ -1737,6 +1740,9 @@  __glXDisp_BindTexImageEXT(__GLXclientState * cl, GLbyte * pc)
                           DixReadAccess, &pGlxDraw, &error))
         return error;
 
+    if (pGlxDraw->format == GLX_TEXTURE_FORMAT_NONE_EXT)
+        return BadMatch;
+
     if (!context->bindTexImage)
         return __glXError(GLXUnsupportedPrivateRequest);
 
@@ -1851,7 +1857,7 @@  DoGetDrawableAttributes(__GLXclientState * cl, XID drawId)
     xGLXGetDrawableAttributesReply reply;
     __GLXdrawable *pGlxDraw = NULL;
     DrawablePtr pDraw;
-    CARD32 attributes[18];
+    CARD32 attributes[19];
     int num = 0, error;
 
     if (!validGlxDrawable(client, drawId, GLX_DRAWABLE_ANY,
@@ -1876,11 +1882,14 @@  DoGetDrawableAttributes(__GLXclientState * cl, XID drawId)
     ATTRIB(GLX_HEIGHT, pDraw->height);
     ATTRIB(GLX_SCREEN, pDraw->pScreen->myNum);
     if (pGlxDraw) {
-        ATTRIB(GLX_TEXTURE_TARGET_EXT,
-               pGlxDraw->target == GL_TEXTURE_2D ?
-                GLX_TEXTURE_2D_EXT : GLX_TEXTURE_RECTANGLE_EXT);
         ATTRIB(GLX_EVENT_MASK, pGlxDraw->eventMask);
         ATTRIB(GLX_FBCONFIG_ID, pGlxDraw->config->fbconfigID);
+        if (pGlxDraw->type == GLX_DRAWABLE_PIXMAP) {
+            ATTRIB(GLX_TEXTURE_TARGET_EXT,
+                   pGlxDraw->target == GL_TEXTURE_2D ?
+                    GLX_TEXTURE_2D_EXT : GLX_TEXTURE_RECTANGLE_EXT);
+            ATTRIB(GLX_TEXTURE_FORMAT_EXT, pGlxDraw->format);
+        }
         if (pGlxDraw->type == GLX_DRAWABLE_PBUFFER) {
             ATTRIB(GLX_PRESERVED_CONTENTS, GL_TRUE);
         }

Comments

On Mon, 2018-04-02 at 14:52 -0400, Adam Jackson wrote:

> @@ -1737,6 +1740,9 @@ __glXDisp_BindTexImageEXT(__GLXclientState * cl, GLbyte * pc)
>                            DixReadAccess, &pGlxDraw, &error))
>          return error;
>  
> +    if (pGlxDraw->format == GLX_TEXTURE_FORMAT_NONE_EXT)
> +        return BadMatch;
> +
>      if (!context->bindTexImage)
>          return __glXError(GLXUnsupportedPrivateRequest);
> 

The conditional added here is correct in a specification sense, but the
behavior change would be subtle. As it stands you can do
glXBindTexImage on _any_ GLX pixmap, even one created without a
texture-target attribute; due to accident of how various conditionals
are written it looks like such pixmaps would implicitly be treated as
RGBA. I'm not aware of any apps that do that and I don't think they
would work on non-Mesa drivers, but we could instead set a default
texture target derived from (the number of alpha bits of) the requested
fbconfig and it would probably do the right thing.

- ajax
Adam Jackson <ajax@nwnk.net> writes:

> On Mon, 2018-04-02 at 14:52 -0400, Adam Jackson wrote:
>
>> @@ -1737,6 +1740,9 @@ __glXDisp_BindTexImageEXT(__GLXclientState * cl, GLbyte * pc)
>>                            DixReadAccess, &pGlxDraw, &error))
>>          return error;
>>  
>> +    if (pGlxDraw->format == GLX_TEXTURE_FORMAT_NONE_EXT)
>> +        return BadMatch;
>> +
>>      if (!context->bindTexImage)
>>          return __glXError(GLXUnsupportedPrivateRequest);
>> 
>
> The conditional added here is correct in a specification sense, but the
> behavior change would be subtle. As it stands you can do
> glXBindTexImage on _any_ GLX pixmap, even one created without a
> texture-target attribute; due to accident of how various conditionals
> are written it looks like such pixmaps would implicitly be treated as
> RGBA. I'm not aware of any apps that do that and I don't think they
> would work on non-Mesa drivers, but we could instead set a default
> texture target derived from (the number of alpha bits of) the requested
> fbconfig and it would probably do the right thing.

Hmm, I don't see any required default value for GLX_TEXTURE_FORMAT in
the spec.  I'm also concerned that we're going to break some backwards
compat here, so your alpha bits plan sounds pretty decent.

Also, you need to bump the attributes by 2, not 1, when adding a new
attribute to output. :)