[Piglit] [PATCH] V3. Add a case to valid gl_VertexID for bug 87611
Wang, Shuo
shuo.wang at intel.com
Tue Mar 31 18:43:23 PDT 2015
Thank you very much for the information. I have already update them and send out v4.
Thanks,
Shuo
-----Original Message-----
From: ibmirkin at gmail.com [mailto:ibmirkin at gmail.com] On Behalf Of Ilia Mirkin
Sent: Tuesday, March 24, 2015 10:28 PM
To: Wang, Shuo
Cc: piglit at lists.freedesktop.org; Jin, Gordon
Subject: Re: [Piglit] [PATCH] V3. Add a case to valid gl_VertexID for bug 87611
On Tue, Mar 24, 2015 at 10:41 AM, Wang Shuo <shuo.wang at intel.com> wrote:
> V3: This case is passed by master.
> Below is the test point of this case:
> 1. Use user-allocated vertex arrays (NOT vertex buffer objects).
> 2. Use a glDrawArrays with a non-zero 'first'.
> 3. Use primitive restart to trigger multiple _mesa_prim without an
> intervening state change.
> For primitive restart test:
> In OpenGL ES, primitive restart do not support glDrawArrays.
There's a lot of talk about GL ES for a test that tests desktop GL...
Also apparently GL 4.5 changed this de-facto behaviour into specified behaviour -- primitive restart is only supposed to work on Draw*Elements*.
> Below is the spec of OpenGLR ES Version 3.0.1(January 10, 2013)
> page26:
> When DrawElements,DrawElementsInstanced, or DrawRangeElements
> transfers a set of generic attribute array elements to the GL,
> if the index within the vertex arrays corresponding to that
> set is equal to 2^N - 1, where N is 8, 16 or 32 if the type is
> UNSIGNED_-BYTE, UNSIGNED_SHORT, or UNSIGNED_INT, respectively,
> then the GL does not process those elements as a vertex.
> In OpenGL, primitive restarting to work with all rendering
> commands, including the non-indexed commands. However,
> implementations did not implement this, and the rule itself
> makes no sense.(https://www.opengl.org/wiki/Vertex_Rendering)
> So I used glDrawElements to test primitive restart 4. Validate
> the value of gl_VertexID in the vertex shader.
> Both glDrawArrays testing and glDrawElements testing test the
> value of gl_VertexID
> 5. Using GL_TRIANGLE_FAN because GL_TRIANGLE_STRIP are supported
> by the HW path, and GL_TRIANGLE_FAN specifically to hit the
> SW primitive restart path which force the draw to be decomposed
> into multiple draws
>
> Signed-off-by: Wang Shuo <shuo.wang at intel.com>
> ---
> tests/all.py | 1 +
> tests/shaders/CMakeLists.gl.txt | 1 +
> tests/shaders/glsl-bug-87611.c | 158
> ++++++++++++++++++++++++++++++++++++++++
The current style is to avoid these bug-12345 tests. It's unclear what they do at a glance, and they're disorganized. The current style is to put this test into, say, tests/spec/gl-3.0 (or 3.1?) and call it primitive-restart-vertex-id or something.
Note that there's already a tests/general/primitive-restart.c which perhaps could be extended to also test TRI_FAN. But I also think a separate test is totally fine.
[Shuo]Yes. Thank you, I will move the case to Piglit\tests\spec\gl-3.1\ primitive-restart-vertex-id.c
> 3 files changed, 160 insertions(+)
> create mode 100644 tests/shaders/glsl-bug-87611.c
>
> diff --git a/tests/all.py b/tests/all.py index b2266db..54cecdc 100644
> --- a/tests/all.py
> +++ b/tests/all.py
> @@ -477,6 +477,7 @@ with profile.group_manager(PiglitGLTest, 'shaders') as g:
> g(['glsl-arb-fragment-coord-conventions'])
> g(['glsl-arb-fragment-coord-conventions-define'])
> g(['glsl-bug-22603'])
> + g(['glsl-bug-87611'])
> g(['glsl-bindattriblocation'])
> g(['glsl-dlist-getattriblocation'])
> g(['glsl-getactiveuniform-array-size'])
> diff --git a/tests/shaders/CMakeLists.gl.txt
> b/tests/shaders/CMakeLists.gl.txt index 3efc6bf..b707ce7 100644
> --- a/tests/shaders/CMakeLists.gl.txt
> +++ b/tests/shaders/CMakeLists.gl.txt
> @@ -47,6 +47,7 @@ piglit_add_executable
> (glsl-arb-fragment-coord-conventions glsl-arb-fragment-coo
> piglit_add_executable (glsl-arb-fragment-coord-conventions-define
> glsl-arb-fragment-coord-conventions-define.c)
> piglit_add_executable (glsl-bindattriblocation
> glsl-bindattriblocation.c) piglit_add_executable (glsl-bug-22603
> glsl-bug-22603.c)
> +piglit_add_executable (glsl-bug-87611 glsl-bug-87611.c)
> piglit_add_executable (glsl-dlist-getattriblocation
> glsl-dlist-getattriblocation.c) piglit_add_executable
> (glsl-explicit-location-01 glsl-explicit-location-01.c)
> piglit_add_executable (glsl-explicit-location-02
> glsl-explicit-location-02.c) diff --git
> a/tests/shaders/glsl-bug-87611.c b/tests/shaders/glsl-bug-87611.c new
> file mode 100644 index 0000000..13e3d72
> --- /dev/null
> +++ b/tests/shaders/glsl-bug-87611.c
> @@ -0,0 +1,158 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person
> +obtaining a
> + * copy of this software and associated documentation files (the
> +"Software"),
> + * to deal in the Software without restriction, including without
> +limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> +sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> +the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including
> +the next
> + * paragraph) shall be included in all copies or substantial portions
> +of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> +EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> +MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> +SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
> +OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> +ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> +OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +/**
> + * \file glsl-bug-87611.c
> + *
> + * Check gl_VertexID
> + * This case is added for *Bug 87611 - Exercise gl_VertexID corner
> + * cases in i965 driver*, and used to watch the status of *Bug 85529
> + * - Surfaces not drawn in Unvanquished* This case is failed by
> +master,
> + * and passed by the patch of vcelestialragev at gmail.com
> + * (https://bugs.freedesktop.org/attachment.cgi?id=110839)
> + * which is agree by Kenneth Graunke.
> + *
> + *
> + * \author Wang Shuo <shuo.wang at intel.com> */
> +
> +
> +#include "piglit-util-gl.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> + config.supports_gl_compat_version = 20;
> +
> + config.window_visual = PIGLIT_GL_VISUAL_RGB |
> + PIGLIT_GL_VISUAL_DOUBLE;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +// Validate the value of gl_VertexID in the vertex shader.
I believe the style is to use C-style comments throughout piglit's C files.
[Shuo] Done.
> +static const GLchar *vertShaderText =
> + "#version 130\n"
> + "in vec4 piglit_vertex;\n"
> + "void main()\n"
> + "{\n"
> + " gl_Position = piglit_vertex;\n"
> + " if (gl_VertexID >= 2 && gl_VertexID <= 5 ) {\n"
> + " gl_FrontColor = vec4(0.0, 1.0, 0.0, 1.0);\n"
> + " } else {\n"
> + " gl_FrontColor = vec4(1.0, 0.0, 0.0, 1.0);\n"
> + " }\n"
> + "}\n";
> +
> +
> +// Validate the value of gl_VertexID in the vertex shader.
> +static const GLchar *vertShaderTextIndex =
> + "#version 130\n"
> + "in vec4 piglit_vertex;\n"
> + "void main()\n"
> + "{\n"
> + " gl_Position = piglit_vertex;\n"
> + " if (gl_VertexID >= 2 && gl_VertexID <= 5 ) {\n"
> + " gl_FrontColor = vec4(0.0, 1.0, 0.0, 1.0);\n"
> + " } else {\n"
> + " gl_FrontColor = vec4(1.0, 0.0, 0.0, 1.0);\n"
> + " }\n"
> + "}\n";
I may be missing something... how are these 2 different from one another?
[Shuo] Yes, sorry. I just merge them together.
> +
> +static GLint prog, progIndex;
> +
> +static const GLfloat Vcoords[6][2] = { {-1, -1}, {-1, -1}, {-1, -1},
> +{1, -1}, {1, 1}, {-1, 1}};
> +
> +static const unsigned short indiceTriangleFan[] =
There is no word 'indice'. There is 'index', whose plural is 'indices'. If it were me, I'd just call this array 'indices' and not mention the tri-fan bits.
[Shuo]Yes, done, thank you.
> + {
Odd indentation... should be aligned to column 0. Or left on the previous line...
[Shuo]Yes, done.
> + 2, 0xFFFF, 2, 3, 0xFFFF, 0xFFFF, 2, 3, 4, 5, 0xFFFF,
> + 0xFFFF, 0xFFFF,5
Missing space before the last 5.
[Shuo]Yes, done.
> + };
> +
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> + static const GLfloat expColor[4] = {0, 1, 0, 1};
> + GLboolean pass = GL_TRUE;
> + enum piglit_result result;
> +
> + glUseProgram(prog);
> +
> + glClear(GL_COLOR_BUFFER_BIT);
> +
> + // Use user-allocated vertex arrays (NOT vertex buffer objects).
> + glVertexAttribPointer(PIGLIT_ATTRIB_POS, 2, GL_FLOAT, GL_FALSE, 0, Vcoords);
> + glEnableVertexAttribArray(PIGLIT_ATTRIB_POS);
> +
> + // Use a glDrawArrays with a non-zero 'first'.
> + glDrawArrays(GL_TRIANGLE_FAN, 2, 4);
> +
> + pass = piglit_probe_pixel_rgba(20, 20, expColor);
> +
> + glDisableVertexAttribArray(PIGLIT_ATTRIB_POS);
> +
> + /* Use primitive restart to trigger multiple _mesa_prim without an
> + * intervening state change.
> + * Because for OpenGL ES PRIMITIVE_RESTART testing, only support
> + * DrawElements, DrawElementsInstanced, or DrawRangeElements.
Not sure why you mention GL ES here. This test only runs on GL.
[Shuo]Removed.
> + * And for OpenGL, PRIMITIVE_RESTART work with all
> + * rendering commands, including the non-indexed commands. However,
> + * implementations did not implement this, and the rule itself makes
> + * no sense (https://www.opengl.org/wiki/Vertex_Rendering)
> + * So for testing PRIMITIVE_RESTART I used glDrawElements
> + * OpenGL ES spec:When DrawElements,DrawElementsInstanced,
> + * or DrawRangeElements transfers a set of generic attribute array
> + * elements to the GL, if the index within the vertex arrays
> + * corresponding to that set is equal to 2N 1, where N is 8, 16
Funny unicode char between 2N and 1.
[Shuo]Removed.
> + * or 32 if the type is UNSIGNED_-BYTE, UNSIGNED_SHORT, or
> + * UNSIGNED_INT, respectively, then the GL does not process those
> + * elements as a vertex.
> + */
> + glUseProgram(progIndex);
> +
> + glClear(GL_COLOR_BUFFER_BIT);
> +
> + glEnable(GL_PRIMITIVE_RESTART_FIXED_INDEX);
> +
> + glVertexAttribPointer(PIGLIT_ATTRIB_POS, 2, GL_FLOAT, GL_FALSE, 0, Vcoords);
> + glEnableVertexAttribArray(PIGLIT_ATTRIB_POS);
> +
> + glDrawElements(GL_TRIANGLE_FAN, sizeof(indiceTriangleFan) /
> + sizeof(indiceTriangleFan[0]), GL_UNSIGNED_SHORT, indiceTriangleFan);
There's an ARRAY_SIZE macro (in piglit-util.h) which does the same thing. I believe you should already be getting it via piglit-util-gl.h.
[Shuo]Yes, done.
Also, you never set the restart index. Is it specified to default to 2^N - 1?
[Shuo]Yes, make it as default.
> +
> + pass = piglit_probe_pixel_rgba(20, 20, expColor) && pass;
> +
> + glDisableVertexAttribArray(PIGLIT_ATTRIB_POS);
> +
> + glDisable(GL_PRIMITIVE_RESTART_FIXED_INDEX);
> +
> + piglit_present_results();
> +
> + return pass ? PIGLIT_PASS : PIGLIT_FAIL; }
> +
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> + piglit_require_GLSL_version(130);
You need to get your version stuff in order... you say you want a 2.0 context, but then you require glsl 1.30, and there is no restart support in unextended GL prior to GL 3.1. So... you could require NV_primitive_restart, or you could require GL3.1 (which would make it a core context).
[Shuo] Yes, thank you. GL_VertexID is supported from GLSL 1.30, so I required glsl 1.30. And I will add the judgment of check the extension.
> +
> + prog = piglit_build_simple_program(vertShaderText, NULL);
> +
> + progIndex = piglit_build_simple_program(vertShaderTextIndex,
> +NULL); }
> --
> 1.8.3.2
>
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
More information about the Piglit
mailing list