| Summary: | Constant vertex attributes broken | ||
|---|---|---|---|
| Product: | Mesa | Reporter: | Robert Bragg <robert> |
| Component: | Mesa core | Assignee: | Ian Romanick <idr> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | medium | CC: | nroberts, ullysses.a.eoff |
| Version: | git | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Bug Depends on: | |||
| Bug Blocks: | 67224 | ||
| Attachments: |
Standalone gles2 test using const attributes
vbo: generic[0] only aliases pos if API=GL test-primitive-broken.log test-primitive-working.log |
||
Created attachment 67934 [details] [review] vbo: generic[0] only aliases pos if API=GL Ping? seems related to bug #66292 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 (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. (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. (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. 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.
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.
(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 (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. (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. (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. 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 |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
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.