[Piglit] [PATCH 1/3] sso: Test mixing separable and non-separable programs in various ways
Paul Berry
stereotype441 at gmail.com
Mon Oct 14 10:17:55 PDT 2013
On 1 October 2013 18:22, Ian Romanick <idr at freedesktop.org> wrote:
> From: Gregory Hainaut <gregory.hainaut at gmail.com>
>
> the goal is to test the state mix of glUseProgram /
> glBindProgramPipeline / glActiveProgram.
>
> Ian R. quote:
> "In this case, either the UseProgram state or the
> BindProgramPipeline state. If UseProgram sets a non-zero program,
> that state is used. Otherwise the BindProgramPipeline state is
> used.....In this case, I think AMD's behavior is incorrect."
>
Last time I reviewed this patch, I asked for additional context on this
quote. Can you put the additional context into the commit message so that
in the future people don't have to go to the mailing list archives to
understand the quote? Thanks.
>
> Note: Nvidia seems to be fine.
>
> V4:
> * split into a separate commit
> * Merge the 2 vertex shaders with the help of the __VERSION__ macro
> * Properly set shader version with asprintf
> * Use standard bool
> * Split test into subtest
> * Fix expected of GL_PROGRAM_PIPELINE_BINDING (based on nvidia behavior)
>
> V5 (idr):
>
> * Expect GL_PROGRAM_PIPELINE_BINDING to still be pipe when a
> non-separable program is also bound (via glUseProgram). After
> discussions in Khronos, that was determined to be the correct
> behavior. Fix NVIDIA drivers should be available soon.
> * Don't test glUseProgramStages with a non-separable program. There is
> a separate test for that now.
> * Use 'pass = foo() && pass;' idiom instead of 'pass &= foo();'. C's
> short-circuit evaulation rules cause these to be different.
> * Use piglit_build_simple_program for non-separable shader program.
> * #extension is only necessary to use layout qualifiers, so remove it.
> * s/GLboolean/bool/g
> * Produce no output with -auto.
> * Use descriptive names for subtests.
> * Add test to CMakeLists.gl.txt.
> * Trivial reformatting.
>
> V6 (idr): Major refactor / rewrite based on feedback from Paul.
> * Each subtest is moved to its own function with its own "pass" tracking.
> * Each subtest restored program, pipeline, and uniform state before
> ending. Tests also do not depend on state left over from previous
> tests.
> * The bind_program and bind_pipeline functions are eliminated.
> * s/BindPipeline/BindProgramPipeline/g
> * Paul also suggested moving piglit_init earlier in the file. This
> has not been done yet because I wasn't sure where to put it. First?
> After the utility functions? After the subtest functions?
>
> Note: vertex shader doesn't work with FGLRX. It would require GLSL150
> but that mean you can't use fixed pipeline anymore...
>
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
> ---
> tests/all.tests | 1 +
> .../arb_separate_shader_objects/CMakeLists.gl.txt | 1 +
> .../mix_pipeline_useprogram.c | 415
> +++++++++++++++++++++
> 3 files changed, 417 insertions(+)
> create mode 100644
> tests/spec/arb_separate_shader_objects/mix_pipeline_useprogram.c
>
> diff --git a/tests/all.tests b/tests/all.tests
> index ee6219c..e0cdc8b 100644
> --- a/tests/all.tests
> +++ b/tests/all.tests
> @@ -1275,6 +1275,7 @@ spec['ARB_separate_shader_objects'] =
> arb_separate_shader_objects
> arb_separate_shader_objects['GetProgramPipelineiv'] =
> concurrent_test('arb_separate_shader_object-GetProgramPipelineiv')
> arb_separate_shader_objects['IsProgramPipeline'] =
> concurrent_test('arb_separate_shader_object-IsProgramPipeline')
> arb_separate_shader_objects['UseProgramStages - non-separable program'] =
> concurrent_test('arb_separate_shader_object-UseProgramStages-non-separable')
> +arb_separate_shader_objects['Mix BindProgramPipeline and UseProgram'] =
> concurrent_test('arb_separate_shader_object-mix_pipeline_useprogram')
> arb_separate_shader_objects['ProgramUniform coverage'] =
> concurrent_test('arb_separate_shader_object-ProgramUniform-coverage')
> arb_separate_shader_objects['ValidateProgramPipeline'] =
> concurrent_test('arb_separate_shader_object-ValidateProgramPipeline')
>
> diff --git a/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt
> b/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt
> index d78f270..0ed3e2f 100644
> --- a/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt
> +++ b/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt
> @@ -11,6 +11,7 @@ link_libraries (
>
> piglit_add_executable (arb_separate_shader_object-GetProgramPipelineiv
> GetProgramPipelineiv.c)
> piglit_add_executable (arb_separate_shader_object-IsProgramPipeline
> IsProgramPipeline.c)
> +piglit_add_executable (arb_separate_shader_object-mix_pipeline_useprogram
> mix_pipeline_useprogram.c)
> piglit_add_executable (arb_separate_shader_object-ProgramUniform-coverage
> ProgramUniform-coverage.c)
> piglit_add_executable
> (arb_separate_shader_object-UseProgramStages-non-separable
> UseProgramStages-non-separable.c)
> piglit_add_executable (arb_separate_shader_object-ValidateProgramPipeline
> ValidateProgramPipeline.c)
> diff --git
> a/tests/spec/arb_separate_shader_objects/mix_pipeline_useprogram.c
> b/tests/spec/arb_separate_shader_objects/mix_pipeline_useprogram.c
> new file mode 100644
> index 0000000..7c7fb63
> --- /dev/null
> +++ b/tests/spec/arb_separate_shader_objects/mix_pipeline_useprogram.c
> @@ -0,0 +1,415 @@
> +/*
> + * Copyright © 2013 Gregory Hainaut <gregory.hainaut at gmail.com>
> + *
> + * 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 mix_pipeline_useprogram.
> + * Test mixing separable and non-separable programs in various ways.
> + *
> + * Section 2.11.3 (Program Objects) of the OpenGL 4.1 spec says:
> + *
> + * "The executable code for an individual shader stage is taken from
> the
> + * current program for that stage. If there is a current program
> object
> + * established by UseProgram, that program is considered current for
> all
> + * stages. Otherwise, if there is a bound program pipeline object (see
> + * section 2.11.4), the program bound to the appropriate stage of the
> + * pipeline object is considered current. If there is no current
> program
> + * object or bound program pipeline object, no program is current for
> any
> + * stage."
> +
> + * Section 2.11.4 (Program Pipeline Objects) of the OpenGL 4.1 spec says:
> + *
> + * "If no current program object has been established by UseProgram,
> the
> + * program objects used for each shader stage and for uniform updates
> are
> + * taken from the bound program pipeline object, if any. If there is a
> + * current program object established by UseProgram, the bound program
> + * pipeline object has no effect on rendering or uniform updates.
> When a
> + * bound program pipeline object is used for rendering, individual
> shader
> + * executables are taken from its program objects as described in the
> + * discussion of UseProgram in section 2.11.3)."
> + *
> + * Section 2.11.7 (Uniform Variables) of the OpenGL 4.1 spec says:
> + *
> + * "If a non-zero program object is bound by UseProgram, it is the
> active
> + * program object whose uniforms are updated by these commands. If no
> + * program object is bound using UseProgram, the active program
> object of
> + * the current program pipeline object set by ActiveShaderProgram is
> the
> + * active program object. If the current program pipeline object has
> no
> + * active program or there is no current program pipeline object, then
> + * there is no active program."
> + *
> + * Note that with these rules, it's not possible to mix program objects
> bound
> + * to the context with program objects bound to a program pipeline
> object; if
> + * any program is bound to the context, the current pipeline object is
> + * ignored.
> + */
> +#include "piglit-util-gl-common.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> + /* Require OpenGL 2.0+, but do *not* allow a core profile. The
> test
> + * uses GLSL 1.10, and GLSL less than 1.30 is deprecated. In
> + * addition, the test tries to use fixed-function drawing.
> + */
> + config.supports_gl_compat_version = 20;
> +
> + config.window_width = 32;
> + config.window_height = 32;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +static GLuint prog;
> +static GLuint pipe;
> +
> +static const float red[4] = {1.0, 0.0, 0.0, 1.0};
> +static const float violet[4] = {1.0, 0.0, 1.0, 1.0};
> +static const float green[4] = {0.0, 1.0, 0.0, 1.0};
> +static const float blue[4] = {0.0, 0.0, 1.0, 1.0};
> +static const float gb[4] = {0.0, 1.0, 1.0, 1.0};
> +
> +static bool
> +check_GetInteger_value(GLenum pname, GLint expected)
> +{
> + bool pass = true;
> + GLint value = 0;
> + glGetIntegerv(pname, &value);
> +
> + pass = piglit_check_gl_error(GL_NO_ERROR) && pass;
> + if (value != expected) {
> + fprintf(stderr, "Failed to get %s expected %d got %d\n",
> + piglit_get_gl_enum_name(pname), expected, value);
> + return false;
> + } else {
> + return true;
> + }
> +}
> +
> +static bool
> +restore_default_program_state(void)
> +{
> + GLint active_shader_pipe;
> +
> + glUseProgram(0);
> + glBindProgramPipeline(0);
> +
> + /* Reset uniform values to known state.
> + */
> + glProgramUniform1f(prog, 0, 0.0);
> + glGetProgramPipelineiv(pipe, GL_ACTIVE_PROGRAM,
> &active_shader_pipe);
> +
> + glProgramUniform1f(active_shader_pipe, 0, 0.0);
>
Is ACTIVE_PROGRAM known to be nonzero at this point? If not, the above
call to glProgramUniform1f() will cause an INVALID_VALUE error.
It would be nice to have either a comment here explaining why
ACTIVE_PROGRAM is known not to be zero, or a test:
if (active_shader_pipe != 0)
glProgramUniform1f(...);
> +
> + return piglit_check_gl_error(GL_NO_ERROR);
> +}
> +
> +static bool
> +draw(const float *expected)
> +{
> + piglit_draw_rect(-1, -1, 2, 2);
> + return piglit_probe_rect_rgba(0, 0, piglit_width, piglit_height,
> + expected);
> +}
> +
> +static bool
> +set_and_check_uniform(GLint program, float expected)
> +{
> + float value;
> +
> + /* glUniform* uses either the currently bound program or the active
> + * program of the currently bound pipeline. glGetUniform* uses the
> + * program expclitly passed to it. THESE MAY BE DIFFERENT!
> + */
> + glUniform1f(0, expected);
> + glGetUniformfv(program, 0, &value);
> + if (value != expected) {
> + fprintf(stderr,
> + "Failed to get uniform value of %d, expected %f, "
> + "got %f\n",
> + program, expected, value);
> + return false;
> + }
> + return true;
> +}
> +
> +static bool
> +test_UseProgram_then_BindProgramPipeline(void)
> +{
> + static const char subtest_name[] =
> + "glUseProgram, then glBindProgramPipeline";
> + bool subtest = true;
> +
> + if (!piglit_automatic)
> + printf("%s...\n", subtest_name);
> +
> + glUseProgram(prog);
> + subtest = set_and_check_uniform(prog, 1.0) && subtest;
> +
> + /* Program rendering */
> + subtest = draw(violet) && subtest;
> +
> + glBindProgramPipeline(pipe);
> +
> + /* It must ignore the pipeline */
> + subtest = set_and_check_uniform(prog, 0.0) && subtest;
> +
> + /* Still program rendering */
> + subtest = draw(red) && subtest;
> +
> + subtest = restore_default_program_state() && subtest;
> + piglit_report_subtest_result(subtest ? PIGLIT_PASS : PIGLIT_FAIL,
> + subtest_name);
> + return subtest;
> +}
> +
> +static bool
> +test_BindProgramPipeline_without_UseProgram(void)
> +{
> + static const char subtest_name[] =
> + "glBindProgramPipeline without glUseProgram";
> + bool subtest = true;
> + GLint active_shader_pipe;
> +
> + if (!piglit_automatic)
> + printf("%s...\n", subtest_name);
> +
> + glBindProgramPipeline(pipe);
> + glGetProgramPipelineiv(pipe, GL_ACTIVE_PROGRAM,
> &active_shader_pipe);
> + subtest = set_and_check_uniform(active_shader_pipe, 1.0) &&
> subtest;
> +
> + /* Pipeline rendering */
> + subtest = draw(green) && subtest;
> +
> + subtest = restore_default_program_state() && subtest;
> + piglit_report_subtest_result(subtest ? PIGLIT_PASS : PIGLIT_FAIL,
> + subtest_name);
> + return subtest;
> +}
> +
> +static bool
> +test_BindProgramPipeline_then_UseProgram(void)
> +{
> + static const char subtest_name[] =
> + "glBindProgramPipeline, then glUseProgram";
> + bool subtest = true;
> +
> + if (!piglit_automatic)
> + printf("%s...\n", subtest_name);
> +
> + glBindProgramPipeline(pipe);
> + glUseProgram(prog);
> +
> + /* UseProgram rendering */
> + subtest = draw(red) && subtest;
> +
> + subtest = restore_default_program_state() && subtest;
> + piglit_report_subtest_result(subtest ? PIGLIT_PASS : PIGLIT_FAIL,
> + subtest_name);
> + return subtest;
> +}
> +
> +static bool
> +test_BindProgramPipeline_then_UseProgram_0(void)
> +{
> + static const char subtest_name[] =
> + "glBindProgramPipeline, then glUseProgram(0)";
> + bool subtest = true;
> +
> + if (!piglit_automatic)
> + printf("%s...\n", subtest_name);
> +
> + glBindProgramPipeline(pipe);
> +
> + /* Pipeline rendering */
> + subtest = draw(gb) && subtest;
> +
> + glUseProgram(prog);
> + subtest = set_and_check_uniform(prog, 1.0) && subtest;
> + glUseProgram(0);
> +
> + /* Still pipeline rendering with the same value for the uniform.
> + */
> + subtest = draw(gb) && subtest;
> +
> + subtest = restore_default_program_state() && subtest;
> + piglit_report_subtest_result(subtest ? PIGLIT_PASS : PIGLIT_FAIL,
> + subtest_name);
> + return subtest;
> +}
> +
> +static bool
> +test_UseProgram_then_BindProgramPipeline_then_UseProgram_0(void)
> +{
> + static const char subtest_name[] =
> + "glUseProgram, then glBindProgramPipeline, then "
> + "glUseProgram(0)";
> + bool subtest = true;
> + GLint active_shader_pipe;
> +
> + if (!piglit_automatic)
> + printf("%s...\n", subtest_name);
> +
> + glGetProgramPipelineiv(pipe, GL_ACTIVE_PROGRAM,
> &active_shader_pipe);
> +
> + glUseProgram(prog);
> +
> + /* Program rendering */
> + subtest = draw(red) && subtest;
> +
> + glBindProgramPipeline(pipe);
> + subtest = set_and_check_uniform(prog, 1.0) && subtest;
> +
> + /* Still program rendering */
> + subtest = draw(violet) && subtest;
> +
> + glUseProgram(0);
> + subtest = set_and_check_uniform(active_shader_pipe, 0.0) &&
> subtest;
> +
> + /* Pipeline rendering */
> + subtest = draw(gb) && subtest;
> +
> + subtest = restore_default_program_state() && subtest;
> + piglit_report_subtest_result(subtest ? PIGLIT_PASS : PIGLIT_FAIL,
> + subtest_name);
> + return subtest;
> +}
> +
> +static bool
> +test_final_sanity(void)
> +{
> + static const char subtest_name[] = "Final sanity test";
> + bool subtest = true;
> +
> + if (!piglit_automatic)
> + printf("%s...\n", subtest_name);
> +
> + /* Fixed function rendering */
> + subtest = draw(blue) && subtest;
> +
> + piglit_report_subtest_result(subtest ? PIGLIT_PASS : PIGLIT_FAIL,
> + subtest_name);
> + return subtest;
> +}
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> + GLint active_shader_pipe;
> + GLint uniform_loc_pipe;
> + GLint uniform_loc_prog;
> + bool pass = true;
> +
> + glGetProgramPipelineiv(pipe, GL_ACTIVE_PROGRAM,
> &active_shader_pipe);
> + uniform_loc_pipe = glGetUniformLocation(active_shader_pipe,
> "blue");
> + uniform_loc_prog = glGetUniformLocation(prog, "blue");
> + /* otherwise it is difficult which program is really updated */
> + assert(uniform_loc_prog == 0);
> + assert(uniform_loc_pipe == 0);
>
It looks like this comment, from my previous review of this patch, never
got addressed:
"This seems really dodgy to me. My understanding is that OpenGL makes no
guarantees as to where any particular uniform is located, even in the
circumstance we have here where only one uniform exists.
IMHO we should write the test so that (a) if the "blue" uniform gets
assigned the same nonzero location in both prog and active_shader_pipe, the
test is still just as effective, and (b) if the "blue" uniform gets
assigned different locations in prog and active_shader_pipe, the test still
passes (but it's ok if it's a less mean test in this case). I think all
that would be required is to add a "location" argument to
set_and_check_uniform, and pass either uniform_loc_prog or uniform_loc_pipe
to it at each call site, as appropriate."
> +
> + /* Set up fixed function to draw blue if we lose our shader. This
> + * color is chosen because neither of the programs used elsewhere
> in
> + * the test can generate blue as a color.
> + */
> + glColor4f(0.0, 0.0, 1.0, 1.0);
> +
> + pass = test_UseProgram_then_BindProgramPipeline() && pass;
> + pass = test_BindProgramPipeline_without_UseProgram() && pass;
> + pass = test_BindProgramPipeline_then_UseProgram() && pass;
> + pass = test_BindProgramPipeline_then_UseProgram_0() && pass;
> + pass = test_UseProgram_then_BindProgramPipeline_then_UseProgram_0()
> + && pass;
> + pass = test_final_sanity() && pass;
> +
> + piglit_present_results();
> +
> + return pass ? PIGLIT_PASS : PIGLIT_FAIL;
> +}
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
>
Also, this comment, from my previous review, doesn't seem to have been
addressed:
"Would you have any objection to moving piglit_init before piglit_display
in this test? A long time ago I started doing that in my piglit tests
because I usually find it easier to follow the logic of piglit_dispaly once
I've seen piglit_init. That seems particularly true of this test, since
the shader source code is contained in piglit_init."
> + GLint vs_prog;
> + GLint fs_prog;
> + GLint vs, fs;
> + const char vs_source[] =
> + "#version 110\n"
> + "void main()\n"
> + "{\n"
> + " gl_Position = gl_Vertex;\n"
> + "}\n";
> + const char fs_source_r[] =
> + "#version 110\n"
> + "uniform float blue;\n"
> + "void main()\n"
> + "{\n"
> + " gl_FragColor = vec4(1.0, 0.0, blue, 1.0);\n"
> + "}\n";
> + const char fs_source_g[] =
> + "#version 110\n"
> + "uniform float blue;\n"
> + "void main()\n"
> + "{\n"
> + " gl_FragColor = vec4(0.0, 1.0, 1.0 - blue, 1.0);\n"
> + "}\n";
> + bool pass = true;
> +
> + piglit_require_extension("GL_ARB_separate_shader_objects");
> +
> + /* Standard program (ie not separate) */
> + prog = piglit_build_simple_program(vs_source, fs_source_r);
> +
> + /* Now create program for the pipeline program */
> + vs = piglit_compile_shader_text(GL_VERTEX_SHADER, vs_source);
> + fs = piglit_compile_shader_text(GL_FRAGMENT_SHADER, fs_source_g);
> +
> + vs_prog = glCreateProgram();
> + glProgramParameteri(vs_prog, GL_PROGRAM_SEPARABLE, GL_TRUE);
> + glAttachShader(vs_prog, vs);
> + glLinkProgram(vs_prog);
> + pass = piglit_link_check_status(vs_prog) && pass;
> + glDeleteShader(vs);
> +
> + fs_prog = glCreateProgram();
> + glProgramParameteri(fs_prog, GL_PROGRAM_SEPARABLE, GL_TRUE);
> + glAttachShader(fs_prog, fs);
> + glLinkProgram(fs_prog);
> + pass = piglit_link_check_status(fs_prog) && pass;
> + glDeleteShader(fs);
> +
> + glGenProgramPipelines(1, &pipe);
> + glUseProgramStages(pipe, GL_VERTEX_SHADER_BIT, vs_prog);
> + glUseProgramStages(pipe, GL_FRAGMENT_SHADER_BIT, fs_prog);
> + glActiveShaderProgram(pipe, fs_prog);
> + pass = piglit_program_pipeline_check_status(pipe) && pass;
> +
> + /* Sanity check initial state.
> + */
> + pass = check_GetInteger_value(GL_PROGRAM_PIPELINE_BINDING, 0) &&
> pass;
> + pass = check_GetInteger_value(GL_CURRENT_PROGRAM, 0) && pass;
> +
> + pass = piglit_check_gl_error(GL_NO_ERROR) && pass;
> +
> + /* If the set up went bad, none of the subtests will work. Bail
> out
> + * now.
> + */
> + if (!pass)
> + piglit_report_result(PIGLIT_FAIL);
> +}
>
Finally, this comment, from my previous review, doesn't seem to have been
addressed:
"The only parts of this test that rely on compatibility behaviour are the
VS's use of gl_Vertex, and this final sanity test. It seems to me that the
final sanity test is tangential to the primary purpose of the test; I'd
rather see this subtest removed, and gl_Vertex changed to a user-defined
vertex attribute, so that we can add supports_gl_core_version to the config
block."
With those issues addressed, this patch is:
Reviewed-by: Paul Berry <stereotype441 at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20131014/8f0528d3/attachment-0001.html>
More information about the Piglit
mailing list