[Piglit] [PATCH 1/3] sso: Test mixing separable and non-separable programs in various ways

Paul Berry stereotype441 at gmail.com
Mon Oct 14 13:45:11 PDT 2013


On 14 October 2013 12:36, Ian Romanick <idr at freedesktop.org> wrote:

> On 10/14/2013 10:17 AM, Paul Berry wrote:
> > On 1 October 2013 18:22, Ian Romanick <idr at freedesktop.org
> > <mailto:idr at freedesktop.org>> wrote:
> >
> >     From: Gregory Hainaut <gregory.hainaut at gmail.com
> >     <mailto: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.
>
> Several of the missed things were due to lag between you sending out
> comments and me getting back to these tests... and lag between me
> getting back to these tests and me sending the tests back out for
> review.  Sorry about that. :(
>

I'm just as much to blame.  I've been about 2 weeks behind in patch review
since XDC.  I'm very slowly catching up.


>
> After the quote, how about if I add:
>
>     I believe  that Gregory observed AMD using whichever state
>     (glUseProgram or glBindProgramPipeline) was most recently set.
>     This was quite some time ago, and the tests have not recently
>     been run on AMD's driver.
>

That would be great, 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
> >     <mailto: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
> >     <mailto: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(...);
>
> I believe it is because every test is trying to exercise glActiveProgram
> functionality, so it always happens to be set.  If it's 0, then the
> function can't do its just (reset the uniform state), and other tests
> could fail because a stale value is left over.  Hmm...  I'm not sure
> what the right answer is.
>

I'd also be ok with doing this:

/* All tests leave a nonzero pipe active */
assert(active_shader_pipe != 0);
gl_ProgramUniform1f(...);


>
> >     +
> >     +       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."
>
> I really thought that I either replied to this, or that we talked about
> it face-to-face later.
>
> It's a little bit dodgy, but this test really only works if the
> locations are the same.  If they're not, the test won't have any teeth.
>  At that point, why bother running the test at all?
>

Actually, on further reflection, I think the test would still be just as
valuable even if the locations are not the same (assuming it's modified
according to my suggestion), because of the fact that glUniform() generates
an INVALID_OPERATION error if location is != -1 and is not a valid uniform
location.  So the test will still fail if the implementation tries to apply
a glUniform() call to the wrong program--it will just fail with a GL error
rather than with incorrect rendering.


>
> In the case that they are the same, I'd bet $1 that every real
> implementation puts the uniform at location 0.  Given the assumptions
> that we've seen shipping applications make, any implementation that
> doesn't, while technically correct, isn't likely to be useful to anyone.
>
> The value / effort equation really starts to wane for me at that point.
>

The effort to fix this seems really minimal to me: just add a location
parameter to set_and_check_uniform(), query the locations in piglit_init(),
and pass in the correct location at each call site.


>
> What might be useful is to report PIGLIT_SKIP (instead of assert) if the
> locations aren't zero.  I have mixed feelings about that too.  If there
> is an implementation that doesn't put both uniforms at location zero, I
> would sure like to know about it!  An assertion failure is more likely
> to be reported than a PIGLIT_SKIP.
>
> >     +
> >     +       /* 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."
>
> I can do that.  I moved a big pile of the test before piglit_display in
> the other refactoring, and I think I missed this in that bit of shuffle.
>

Thanks.


>
> >     +       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."
>
> We have two competing goals, and this test can't satisfy both of them.
> One if for tests to only fail because the thing being tested is broken,
> and the other is for the test to run in both core / compat.  If I make
> the test run with core,
>
> - I have to do a bunch of work to dynamically generate the #version line.
>
> - The test will fail on AMD because of reasons pointed out in other
> tests in the original series (having to redeclare gl_PerVertex).
>
> - I then have to work around the other AMD bugs, and that add a big pile
> of clutter (that needs explicit documenting).  The signal to noise ratio
> in the test gets fairly poor at that point, and it starts to feel like
> the old woman that swallowed a fly.
>
> Suggestion?
>

Ok, I didn't realize those AMD bugs would make things really different.  In
that case, would you mind adding a small comment to the test that says
something "Don't bother testing with core contexts because of AMD bugs
having to do with redeclaring gl_PerVertex"?


>
> On a related topic... We need a better way of dealing with versioning
> shaders in tests.  All this asprintf crap, in every test, is for the
> birds. :(
>

Yeah, the asprintf() thing is a pain.  I don't have a good idea of what to
do instead.  I'll try to think about it in the background.


>
> > With those issues addressed, this patch is:
> >
> > Reviewed-by: Paul Berry <stereotype441 at gmail.com
> > <mailto:stereotype441 at gmail.com>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20131014/bc376d69/attachment-0001.html>


More information about the Piglit mailing list