[Piglit] [PATCH 3/3] vertex-program-two-side: Make the argument handling processing more obvious.
Ian Romanick
idr at freedesktop.org
Thu Sep 22 15:12:48 PDT 2011
On 09/21/2011 03:35 PM, Paul Berry wrote:
I agree with Paul's suggestions. However, that can be done in future
patches.
Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
> On 20 September 2011 14:51, Eric Anholt <eric at anholt.net
> <mailto:eric at anholt.net>> wrote:
>
> As fallout, test a couple more combinations.
> ---
> tests/all.tests | 29 ++++++-----
> tests/spec/gl-2.0/vertex-program-two-side.c | 73
> +++++++++++++--------------
> 2 files changed, 50 insertions(+), 52 deletions(-)
>
> diff --git a/tests/all.tests b/tests/all.tests
> index 94b3184..5404880 100644
> --- a/tests/all.tests
> +++ b/tests/all.tests
> @@ -732,21 +732,22 @@ add_texwrap_test3(gl20, '2D', 'npot', 'proj')
> add_texwrap_test2(gl20, '3D', 'npot')
> add_texwrap_test3(gl20, '3D', 'npot', 'proj')
> add_plain_test(gl20, 'getattriblocation-conventional')
> -add_concurrent_test(gl20, 'vertex-program-two-side')
> +add_concurrent_test(gl20, 'vertex-program-two-side enabled front
> back front2 back2')
> +add_concurrent_test(gl20, 'vertex-program-two-side enabled front back')
> +add_concurrent_test(gl20, 'vertex-program-two-side enabled front2
> back2')
> +add_concurrent_test(gl20, 'vertex-program-two-side enabled front
> front2')
> +add_concurrent_test(gl20, 'vertex-program-two-side enabled back back2')
> +add_concurrent_test(gl20, 'vertex-program-two-side enabled front')
> +add_concurrent_test(gl20, 'vertex-program-two-side enabled back')
> +add_concurrent_test(gl20, 'vertex-program-two-side enabled front2')
> +add_concurrent_test(gl20, 'vertex-program-two-side enabled back2')
> +
> +add_concurrent_test(gl20, 'vertex-program-two-side front back
> front2 back2')
> +add_concurrent_test(gl20, 'vertex-program-two-side front back')
> +add_concurrent_test(gl20, 'vertex-program-two-side front2 back2')
> +add_concurrent_test(gl20, 'vertex-program-two-side front front2')
> add_concurrent_test(gl20, 'vertex-program-two-side front')
> -add_concurrent_test(gl20, 'vertex-program-two-side back')
> -add_concurrent_test(gl20, 'vertex-program-two-side primary')
> -add_concurrent_test(gl20, 'vertex-program-two-side primary front')
> -add_concurrent_test(gl20, 'vertex-program-two-side primary back')
> -add_concurrent_test(gl20, 'vertex-program-two-side secondary')
> -add_concurrent_test(gl20, 'vertex-program-two-side secondary front')
> -add_concurrent_test(gl20, 'vertex-program-two-side secondary back')
> -add_concurrent_test(gl20, 'vertex-program-two-side disabled')
> -add_concurrent_test(gl20, 'vertex-program-two-side disabled front')
> -add_concurrent_test(gl20, 'vertex-program-two-side disabled primary')
> -add_concurrent_test(gl20, 'vertex-program-two-side disabled primary
> front')
> -add_concurrent_test(gl20, 'vertex-program-two-side disabled secondary')
> -add_concurrent_test(gl20, 'vertex-program-two-side disabled
> secondary front')
> +add_concurrent_test(gl20, 'vertex-program-two-side front2')
>
>
> There are a few nonsensical use cases that won't produce well-defined
> colors, but are probably worth testing just to make sure they don't
> cause a GPU hang or assertion failure, such as "enabled front back
> back2", or "back back2". In fact, I would argue that it's worth testing
> all 32 combinations of enable, front, back, front2, and back2 just to be
> on the safe side.
>
>
> # Group spec/glsl-1.00
> spec['glsl-1.00'] = Group()
> diff --git a/tests/spec/gl-2.0/vertex-program-two-side.c
> b/tests/spec/gl-2.0/vertex-program-two-side.c
> index e57e508..611e1a9 100644
> --- a/tests/spec/gl-2.0/vertex-program-two-side.c
> +++ b/tests/spec/gl-2.0/vertex-program-two-side.c
> @@ -50,11 +50,12 @@ int piglit_window_mode = GLUT_RGB | GLUT_ALPHA |
> GLUT_DOUBLE;
>
> static GLint prog;
>
> -static bool primary = true;
> -static bool secondary = true;
> -static bool enabled = true;
> -static bool front = true;
> -static bool back = true;
> +static bool enabled = false;
> +static bool front = false;
> +static bool back = false;
> +static bool front2 = false;
> +static bool back2 = false;
> +
> static float frontcolor[4] = {0.0, 0.5, 0.0, 0.0};
> static float backcolor[4] = {0.0, 0.0, 0.5, 0.0};
> static float secondary_frontcolor[4] = {0.0, 0.25, 0.0, 0.0};
> @@ -78,51 +79,49 @@ piglit_display(void)
> int w = piglit_width / 2, h = piglit_height / 2;
> int x2 = piglit_width - w, y2 = piglit_height - h;
> bool pass = true;
> - bool draw_back = back || !enabled;
>
> glClearColor(0.5, 0.5, 0.5, 0.5);
> glClear(GL_COLOR_BUFFER_BIT);
>
> glUniform1i(draw_secondary_loc, false);
> - if (front && primary)
> + if (front)
> piglit_draw_rect(-1, 0, 1, 1); /* top left */
> - if (draw_back && primary)
> + if (back || (front && !enabled))
> piglit_draw_rect( 1, 0, -1, 1); /* top right */
>
> glUniform1i(draw_secondary_loc, true);
> - if (front && secondary)
> + if (front2)
> piglit_draw_rect(-1, -1, 1, 1); /* bot left */
> - if (draw_back && secondary)
> + if (back2 || (front2 && !enabled))
> piglit_draw_rect( 1, -1, -1, 1); /* bot right */
>
>
> Along the same lines of what I said above, I think it would be worth
> drawing all four rects regardless of the command-line arguments, just to
> make sure there isn't an assertion failure or GPU hang when drawing a
> rect with an undefined color. We can rely on the if tests below to
> ensure that we only *check* the colors in the cases where they are
> well-defined.
>
>
> if (front) {
> - if (primary) {
> - pass = pass && piglit_probe_rect_rgba(x1,
> y2, w, h,
> -
> frontcolor);
> - }
> - if (secondary) {
> - pass = pass && piglit_probe_rect_rgba(x1,
> y1, w, h,
> -
> secondary_frontcolor);
> - }
> + pass = pass && piglit_probe_rect_rgba(x1, y2, w, h,
> + frontcolor);
> + }
> +
> + if (front2) {
> + pass = pass && piglit_probe_rect_rgba(x1, y1, w, h,
> +
> secondary_frontcolor);
> }
>
> - if (back && enabled) {
> + if (enabled) {
> /* Two-sided: Get the back color/secondarycolor. */
> - if (primary) {
> + if (back) {
> pass = pass && piglit_probe_rect_rgba(x2,
> y2, w, h,
>
> backcolor);
> }
> - if (secondary) {
> + if (back2) {
> pass = pass && piglit_probe_rect_rgba(x2,
> y1, w, h,
>
> secondary_backcolor);
> }
> - } else if (back) {
> + } else {
> /* Non-two-sided: Get the front color/secondarycolor. */
> - if (primary) {
> + if (front) {
> pass = pass && piglit_probe_rect_rgba(x2,
> y2, w, h,
>
> frontcolor);
> }
> - if (secondary) {
> + if (front2) {
> pass = pass && piglit_probe_rect_rgba(x2,
> y1, w, h,
>
> secondary_frontcolor);
> }
> @@ -168,30 +167,28 @@ piglit_init(int argc, char **argv)
>
> printf("+-------------------------+------------------------+\n");
>
> for (i = 1; i < argc; i++) {
> - if (strcmp(argv[i], "disabled") == 0) {
> - enabled = false;
> + if (strcmp(argv[i], "enabled") == 0) {
> + enabled = true;
> } else if (strcmp(argv[i], "front") == 0) {
> - back = false;
> + front = true;
> } else if (strcmp(argv[i], "back") == 0) {
> - front = false;
> - } else if (strcmp(argv[i], "primary") == 0) {
> - secondary = false;
> - } else if (strcmp(argv[i], "secondary") == 0) {
> - primary = false;
> + back = true;
> + } else if (strcmp(argv[i], "front2") == 0) {
> + front2 = true;
> + } else if (strcmp(argv[i], "back2") == 0) {
> + back2 = true;
> } else {
> fprintf(stderr, "unknown argument %s\n",
> argv[i]);
> }
> }
>
> - assert(enabled || front);
> -
> - if (front && primary)
> + if (front)
> setup_output(&vs_outputs[0], "gl_FrontColor",
> frontcolor);
> - if (back && primary)
> + if (back)
> setup_output(&vs_outputs[1], "gl_BackColor", backcolor);
> - if (front && secondary)
> + if (front2)
> setup_output(&vs_outputs[2],
> "gl_FrontSecondaryColor", secondary_frontcolor);
> - if (back && secondary)
> + if (back2)
> setup_output(&vs_outputs[3],
> "gl_BackSecondaryColor", secondary_backcolor);
>
> asprintf(&vs_source,
> --
> 1.7.5.4
>
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org <mailto:Piglit at lists.freedesktop.org>
> http://lists.freedesktop.org/mailman/listinfo/piglit
>
>
> Other than the two comments above, this series is
>
> Reviewed-by: Paul Berry <stereotype441 at gmail.com
> <mailto:stereotype441 at gmail.com>>
More information about the Piglit
mailing list