Bug 55503 - Constant vertex attributes broken
Constant vertex attributes broken
Status: RESOLVED FIXED
Product: Mesa
Classification: Unclassified
Component: Mesa core
git
Other All
: medium normal
Assigned To: Ian Romanick
:
Depends on:
Blocks: 67224
  Show dependency treegraph
 
Reported: 2012-10-01 15:48 UTC by Robert Bragg
Modified: 2013-08-15 22:00 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Standalone gles2 test using const attributes (9.60 KB, text/plain)
2012-10-01 15:48 UTC, Robert Bragg
Details
vbo: generic[0] only aliases pos if API=GL (1.81 KB, patch)
2012-10-01 15:52 UTC, Robert Bragg
Details | Splinter Review
test-primitive-broken.log (51.65 KB, text/plain)
2013-08-14 15:21 UTC, Robert Bragg
Details
test-primitive-working.log (54.39 KB, text/plain)
2013-08-14 15:29 UTC, Robert Bragg
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Bragg 2012-10-01 15:48:04 UTC
Created attachment 67932 [details]
Standalone gles2 test using const attributes

We recently added support in Cogl for using constant attributes which we want to use as part of a UI runtime + editor we are currently working on but I'm finding that they don't work with Mesa, even when using LIBGL_ALWAYS_SOFTWARE=1.

I was able to reproduce the issue in a standalone gles2 test case which I've attached and hopefully that will help clarify the problem.

I spent a bit of time trying to debug the issue myself yesterday, and although I can't say I quite got my head around how the attribute state moves around I did  managed to cobble together a patch that works for me. The thing I found that looked amiss was in recalculate_input_bindings in the VP_ARB case:

      if (vertexAttrib[VERT_ATTRIB_GENERIC0].Enabled)
         inputs[0] = &vertexAttrib[VERT_ATTRIB_GENERIC0];
      else if (vertexAttrib[VERT_ATTRIB_POS].Enabled)
         inputs[0] = &vertexAttrib[VERT_ATTRIB_POS];
      else {
         inputs[0] = &vbo->currval[VBO_ATTRIB_POS];
         const_inputs |= VERT_BIT_POS;
      }

      <snip/>

      for (i = 1; i < VERT_ATTRIB_GENERIC_MAX; i++) {
         if (vertexAttrib[VERT_ATTRIB_GENERIC(i)].Enabled)
            inputs[VERT_ATTRIB_GENERIC(i)] = &vertexAttrib[VERT_ATTRIB_GENERIC(i)];
         else {
            inputs[VERT_ATTRIB_GENERIC(i)] = &vbo->currval[VBO_ATTRIB_GENERIC0+i];
            const_inputs |= VERT_BIT_GENERIC(i);
         }
      }

Given that the test is a gles2 test I wouldn't expect that we should be special casing inputs[0] and in this case it is also legitimate for vertexAttrib[VERT_ATTRIB_GENERIC0] to be disabled which I guess shouldn't lead us to refer to the legacy _POS attribute which is what it looks like is happening. Also the loop that comes later to iterate the generic attributes then starts at index 1 assuming that we've already dealt with 0 as a special case which doesn't seem right for gles2.

I hacked the code like this to do what I guessed made more sense for gles2:

--- a/src/mesa/vbo/vbo_exec_array.c
+++ b/src/mesa/vbo/vbo_exec_array.c
@@ -473,11 +473,18 @@ recalculate_input_bindings(struct gl_context *ctx)
        * Otherwise, legacy attributes available in the legacy slots,
        * generic attributes in the generic slots and materials are not
        * available as per-vertex attributes.
+       *
+       * If we aren't running with the legacy GL profile, with deprecated
+       * features, (GLES2 or GL_CORE) then generic[0] has its own current value
+       * instead of being an alias for the legacy position array.
        */
       if (vertexAttrib[VERT_ATTRIB_GENERIC0].Enabled)
         inputs[0] = &vertexAttrib[VERT_ATTRIB_GENERIC0];
-      else if (vertexAttrib[VERT_ATTRIB_POS].Enabled)
-        inputs[0] = &vertexAttrib[VERT_ATTRIB_POS];
+      else if (ctx->API != API_OPENGL) {
+        inputs[0] = &vbo->currval[VBO_ATTRIB_GENERIC0];
+         const_inputs |= VERT_BIT_GENERIC(0);
+      } else if (vertexAttrib[VERT_ATTRIB_POS].Enabled)
+         inputs[0] = &vertexAttrib[VERT_ATTRIB_POS];
       else {
         inputs[0] = &vbo->currval[VBO_ATTRIB_POS];
          const_inputs |= VERT_BIT_POS;

and that seems to fix the test app for me. I had also tried skipping out the whole inputs[0] special case if API==API_OPENGL, and basing the following for loops at i = 0 but for some reason that doesn't work so I guess other things are assuming that inputs[0] is a bit special.

If you look at the attached test there are a few #ifdefs I left in there to affect the test. In create_shaders() there is an #if 0 to simply swap the order of the two constant attributes in the glsl and for me this results in the output color changing. There is an #ifdef to use glBindAttribLocation which makes the test work for me. Finally there are #ifdefs to switch to using glVertexAttribPointer instead of glVertexAttrib[24]f that also make the test work.

When working, the test should output a constant pinky-purple (0xff00ffff) across the whole window, whereas I see red or blue depending on the order of the attribute declarations in the glsl.

The color output by the fragment shader takes the red and green and alpha components from the "test" attribute and the blue component comes from the .s component of the "tex_coord" attribute. Although it looks awkward, I just did that to make sure the v_tex_coord varying and tex_coord attribute wouldn't somehow be optimized out due to not being referenced.

I'll also attach my initial Mesa patch that appears to work for me.
Comment 1 Robert Bragg 2012-10-01 15:52:47 UTC
Created attachment 67934 [details] [review]
vbo: generic[0] only aliases pos if API=GL
Comment 2 Robert Bragg 2013-07-31 09:32:51 UTC
Ping?
Comment 3 Tapani Pälli 2013-07-31 09:43:38 UTC
seems related to bug #66292
Comment 4 Ian Romanick 2013-08-07 19:28:19 UTC
I have posted a somewhat different patch to the mesa-dev mailing list for review:

http://lists.freedesktop.org/archives/mesa-dev/2013-August/042881.html
Comment 5 U. Artie Eoff 2013-08-09 18:18:45 UTC
(In reply to comment #4)
> I have posted a somewhat different patch to the mesa-dev mailing list for
> review:
> 
> http://lists.freedesktop.org/archives/mesa-dev/2013-August/042881.html

This patch fixes a few of the cogl conform regressions in https://bugzilla.gnome.org/show_bug.cgi?id=703500... but not all of them.
Comment 6 Ian Romanick 2013-08-09 19:14:19 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > I have posted a somewhat different patch to the mesa-dev mailing list for
> > review:
> > 
> > http://lists.freedesktop.org/archives/mesa-dev/2013-August/042881.html
> 
> This patch fixes a few of the cogl conform regressions in
> https://bugzilla.gnome.org/show_bug.cgi?id=703500... but not all of them.

Can you provide an apitrace of one of the failing cases?  I want to see exactly what's going on in there.  Based bug #67548, I'm suspicious that things have been working on desktop OpenGL by pure luck.
Comment 7 U. Artie Eoff 2013-08-14 15:16:52 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > I have posted a somewhat different patch to the mesa-dev mailing list for
> > > review:
> > > 
> > > http://lists.freedesktop.org/archives/mesa-dev/2013-August/042881.html
> > 
> > This patch fixes a few of the cogl conform regressions in
> > https://bugzilla.gnome.org/show_bug.cgi?id=703500... but not all of them.
> 
> Can you provide an apitrace of one of the failing cases?  I want to see
> exactly what's going on in there.  Based bug #67548, I'm suspicious that
> things have been working on desktop OpenGL by pure luck.

I will defer this task to Robert Bragg or Neil Roberts.
Comment 8 Robert Bragg 2013-08-14 15:21:59 UTC
Created attachment 84057 [details]
test-primitive-broken.log

Here's a trace of test-primitive that fails (after mesa commit c170c901d0f5384e5ab8b79b827663fa28439b0b) and I'll attach a working trace too that can be compared.
Comment 9 Robert Bragg 2013-08-14 15:29:57 UTC
Created attachment 84060 [details]
test-primitive-working.log

Here's a trace of test-primitive that passed (before mesa commit c170c901d0f5384e5ab8b79b827663fa28439b0b)

Note: I'm suspicious of these traces since they show glBindAttribLocation being called just before linking the program but Cogl doesn't actually ever explicitly call glBindAttribLocation, it relies on the compiler to assign locations and we instead use glGetAttribLocation. There actually isn't any code in Cogl that calls glBindAttribLocation and if I gdb the test and break on _mesa_BindAttribLocation I also only hit a couple of internal uses and don't see calls like the ones in the trace.
Comment 10 Robert Bragg 2013-08-14 18:01:21 UTC
(In reply to comment #4)
> I have posted a somewhat different patch to the mesa-dev mailing list for
> review:
> 
> http://lists.freedesktop.org/archives/mesa-dev/2013-August/042881.html

For reference; this patch doesn't seem to fix the test case I attached to this bug
Comment 11 Ian Romanick 2013-08-15 17:31:45 UTC
(In reply to comment #9)
> Created attachment 84060 [details]
> test-primitive-working.log
> 
> Here's a trace of test-primitive that passed (before mesa commit
> c170c901d0f5384e5ab8b79b827663fa28439b0b)
> 
> Note: I'm suspicious of these traces since they show glBindAttribLocation
> being called just before linking the program but Cogl doesn't actually ever
> explicitly call glBindAttribLocation, it relies on the compiler to assign
> locations and we instead use glGetAttribLocation. There actually isn't any
> code in Cogl that calls glBindAttribLocation and if I gdb the test and break
> on _mesa_BindAttribLocation I also only hit a couple of internal uses and
> don't see calls like the ones in the trace.

Dang.  apitrace may not be useful for this.  I'm sure that it captures the locations of attributes so that it can be sure the locations match when run on a different driver.  The calls to glVertexAttribPointer are hard-coded, so I suspect that's the only way it can make the traces work.
Comment 12 Ian Romanick 2013-08-15 18:01:15 UTC
(In reply to comment #10)
> (In reply to comment #4)
> > I have posted a somewhat different patch to the mesa-dev mailing list for
> > review:
> > 
> > http://lists.freedesktop.org/archives/mesa-dev/2013-August/042881.html
> 
> For reference; this patch doesn't seem to fix the test case I attached to
> this bug

Hmm... how did you compile the test?  No matter what combination of -DCONST_TEX_COORD, -DCONST_TEST_COLOR, or value of the #if at line 112 I pick, I get the same output in an OpenGL ES context.  What color should it produce?  I'm getting (1.0, 0.0, 1.0) in all configurations.
Comment 13 Robert Bragg 2013-08-15 18:53:41 UTC
(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #4)
> > > I have posted a somewhat different patch to the mesa-dev mailing list for
> > > review:
> > > 
> > > http://lists.freedesktop.org/archives/mesa-dev/2013-August/042881.html
> > 
> > For reference; this patch doesn't seem to fix the test case I attached to
> > this bug
> 
> Hmm... how did you compile the test?  No matter what combination of
> -DCONST_TEX_COORD, -DCONST_TEST_COLOR, or value of the #if at line 112 I
> pick, I get the same output in an OpenGL ES context.  What color should it
> produce?  I'm getting (1.0, 0.0, 1.0) in all configurations.

Oh, my bad actually your patch does fix my test case sorry, I made a mistake when checking this yesterday.

If I reset to 15436adab0ae2dea5d62567326f1f3969939781b I can see that the test case fails and then I can either apply your patch or my patch and in both cases the test outputs (1, 0, 1) as it should, as opposed to red or blue as it used to.

At least fixing this means we can hopefully close this bug if we land your patch but it also now means we don't have a simple case to try and figure out why we still have lots of cogl conformance test breakages from c170c901d0f5384e5ab8b79b827663fa28439b0b

I'd be happy for you to add a Reviewed-by: Robert Bragg <robert@sixbynine.org> for your patch and land that to close this bug.
Comment 14 Ian Romanick 2013-08-15 22:00:35 UTC
commit 41eef83cc030e7087b79b0070d00fbc56538fb81
Author: Ian Romanick <ian.d.romanick@intel.com>
Date:   Wed Aug 7 11:15:41 2013 -0700

    mesa/vbo: Fix handling of attribute 0 in non-compatibilty contexts
    
    It is only in OpenGL compatibility-style contexts where generic
    attribute 0 and GL_VERTEX_ARRAY have a bizzare, aliasing relationship.
    Moreover, it is only in OpenGL compatibility-style contexts and OpenGL
    ES 1.x where one of these attributes provokes the vertex.  In all other
    APIs each implicit call to glArrayElement provokes a vertex regardless
    of which attributes are enabled.
    
    Signed-off-by: Ian Romanick <ian.d.romanick@intel.com>
    Reviewed-by: Robert Bragg <robert@sixbynine.org>
    Cc: "9.0 9.1 9.2" <mesa-stable@lists.freedesktop.org>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=55503
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=66292
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67548