Looks good overall. Just a bunch of nit-picks below...
On Mon, Aug 24, 2015 at 5:19 PM, Juliet Fru <[email protected]> wrote: > --- > tests/all.py | 1 + > tests/spec/gl-1.0/CMakeLists.gl.txt | 1 + > tests/spec/gl-1.0/no-op-paths.c | 370 > ++++++++++++++++++++++++++++++++++++ > 3 files changed, 372 insertions(+) > create mode 100644 tests/spec/gl-1.0/no-op-paths.c > > diff --git a/tests/all.py b/tests/all.py > index fe088f5..a9822d3 100644 > --- a/tests/all.py > +++ b/tests/all.py > @@ -1000,6 +1000,7 @@ with profile.group_manager( > g(['gl-1.0-ortho-pos']) > g(['gl-1.0-readpixsanity']) > g(['gl-1.0-logicop']) > + g(['gl-1.0-no-op-paths']) > > with profile.group_manager( > PiglitGLTest, > diff --git a/tests/spec/gl-1.0/CMakeLists.gl.txt > b/tests/spec/gl-1.0/CMakeLists.gl.txt > index d04b835..7a7f508 100644 > --- a/tests/spec/gl-1.0/CMakeLists.gl.txt > +++ b/tests/spec/gl-1.0/CMakeLists.gl.txt > @@ -22,6 +22,7 @@ piglit_add_executable (gl-1.0-front-invalidate-back > front-invalidate-back.c) > piglit_add_executable (gl-1.0-logicop logicop.c) > piglit_add_executable (gl-1.0-long-dlist long-dlist.c) > piglit_add_executable (gl-1.0-ortho-pos orthpos.c) > +piglit_add_executable (gl-1.0-no-op-paths no-op-paths.c) > piglit_add_executable (gl-1.0-polygon-line-aa polygon-line-aa.c) > piglit_add_executable (gl-1.0-push-no-attribs push-no-attribs.c) > piglit_add_executable (gl-1.0-readpixsanity readpix.c) > diff --git a/tests/spec/gl-1.0/no-op-paths.c > b/tests/spec/gl-1.0/no-op-paths.c > new file mode 100644 > index 0000000..9a76baf > --- /dev/null > +++ b/tests/spec/gl-1.0/no-op-paths.c > @@ -0,0 +1,370 @@ > +/* BEGIN_COPYRIGHT -*- glean -*- > + * > + * Copyright (C) 1999 Allen Akin All Rights Reserved. > + * Copyright (C) 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. > + * > + * END_COPYRIGHT > + */ > +/** @file paths.c > + * > + * Test basic GL rendering paths. > + * > + * > + * This test verifies that basic, trival OpenGL paths work as > expected. > + * For example, glAlphaFunc(GL_GEQUAL, 0.0) should always pass and > + * glAlphaFunc(GL_LESS, 0.0) should always fail. We setup trivial > + * pass and fail conditions for each of alpha test, blending, color > mask, > + * depth test, logic ops, scissor, stencil, stipple, and texture and > + * make sure they work as expected. We also setup trival-pass for > all > + * these paths simultaneously and test that as well. > + * > + * To test for pass/fail we examine the color buffer for white or > black, > + * respectively. > + * > Here, I'd add an additional comment saying that this Piglit test is based on the original Glean tpaths.cpp test. > + */ > + > +#include "piglit-util-gl.h" > + > +#include <stdlib.h> > Is stdlib.h really needed? > + > + > +PIGLIT_GL_TEST_CONFIG_BEGIN > + > + config.supports_gl_compat_version = 10; > + > + config.window_visual = PIGLIT_GL_VISUAL_RGBA | > + PIGLIT_GL_VISUAL_DOUBLE | PIGLIT_GL_VISUAL_DEPTH; > + > +PIGLIT_GL_TEST_CONFIG_END > + > + > +typedef enum { > + ALPHA, > + BLEND, > + COLOR_MASK, > + DEPTH, > + LOGIC, > + SCISSOR, > + STENCIL, > + STIPPLE, > + TEXTURE, > + ZZZ /* end-of-list token */ > +} Path; > + > +typedef enum { > + DISABLE, > + ALWAYS_PASS, > + ALWAYS_FAIL > + > Unneeded empty line. > +} State; > + > + > +const char * > Could be static. > +path_name(Path path) { > Opening brace goes on next line. > + > + switch (path) { > + case ALPHA: > + return "Alpha Test"; > We use 8-column tabs for indenting in Piglit, not spaces. > + case BLEND: > + return "Blending"; > + case COLOR_MASK: > + return "Color Mask"; > + case DEPTH: > + return "Depth Test"; > + case LOGIC: > + return "LogicOp"; > + case SCISSOR: > + return "Scissor Test"; > + case STENCIL: > + return "Stencil Test"; > + case STIPPLE: > + return "Polygon Stipple"; > + case TEXTURE: > + return "Modulated Texture"; > + case ZZZ: > + return "paths"; > + default: > + return "???"; > + } > +} > + > + > +void > +set_path_state(Path path, State state) { > Same comments as above. > + > + int i; > + switch (path) { > + case ALPHA: > + if (state == ALWAYS_PASS) { > + glAlphaFunc(GL_GEQUAL, 0.0); > + glEnable(GL_ALPHA_TEST); > + } > + else if (state == ALWAYS_FAIL) { > + glAlphaFunc(GL_GREATER, 1.0); > + glEnable(GL_ALPHA_TEST); > + } > + else { > + glDisable(GL_ALPHA_TEST); > + } > + break; > + case BLEND: > + if (state == ALWAYS_PASS) { > + glBlendFunc(GL_ONE, GL_ZERO); > + glEnable(GL_BLEND); > + } > + else if (state == ALWAYS_FAIL) { > + glBlendFunc(GL_ZERO, GL_ONE); > + glEnable(GL_BLEND); > + } > + else { > + glDisable(GL_BLEND); > + } > + break; > + case COLOR_MASK: > + if (state == ALWAYS_PASS) { > + glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE); > + } > + else if (state == ALWAYS_FAIL) { > + glColorMask(GL_FALSE, GL_FALSE, GL_FALSE, > GL_FALSE); > + } > + else { > + glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE); > + } > + break; > + case DEPTH: > + if (state == ALWAYS_PASS) { > + glDepthFunc(GL_ALWAYS); > + glEnable(GL_DEPTH_TEST); > + } > + else if (state == ALWAYS_FAIL) { > + glDepthFunc(GL_NEVER); > + glEnable(GL_DEPTH_TEST); > + } > + else { > + glDisable(GL_DEPTH_TEST); > + } > + break; > + case LOGIC: > + if (state == ALWAYS_PASS) { > + glLogicOp(GL_OR); > + glEnable(GL_COLOR_LOGIC_OP); > + } > + else if (state == ALWAYS_FAIL) { > + glLogicOp(GL_AND); > + glEnable(GL_COLOR_LOGIC_OP); > + } > + else { > + glDisable(GL_COLOR_LOGIC_OP); > + } > + break; > + case SCISSOR: > + if (state == ALWAYS_PASS) { > + glScissor(0, 0, 10, 10); > + glEnable(GL_SCISSOR_TEST); > + } > + else if (state == ALWAYS_FAIL) { > + glScissor(0, 0, 0, 0); > + glEnable(GL_SCISSOR_TEST); > + } > + else { > + glDisable(GL_SCISSOR_TEST); > + } > + break; > + case STENCIL: > + if (state == ALWAYS_PASS) { > + /* pass if reference <= stencil value (ref = 0) */ > + glStencilFunc(GL_LEQUAL, 0, ~0); > + glEnable(GL_STENCIL_TEST); > + } > + else if (state == ALWAYS_FAIL) { > + /* pass if reference > stencil value (ref = 0) */ > + glStencilFunc(GL_GREATER, 0, ~0); > + glEnable(GL_STENCIL_TEST); > + } > + else { > + glDisable(GL_STENCIL_TEST); > + } > + break; > + case STIPPLE: > + if (state == ALWAYS_PASS) { > + GLubyte stipple[4*32]; > + for (i = 0; i < 4*32; i++) > + stipple[i] = 0xff; > + glPolygonStipple(stipple); > + glEnable(GL_POLYGON_STIPPLE); > + } > + else if (state == ALWAYS_FAIL) { > + GLubyte stipple[4*32]; > + for (i = 0; i < 4*32; i++) > + stipple[i] = 0x0; > + glPolygonStipple(stipple); > + glEnable(GL_POLYGON_STIPPLE); > + } > + else { > + glDisable(GL_POLYGON_STIPPLE); > + } > + break; > + case TEXTURE: > + if (state == DISABLE) { > + glDisable(GL_TEXTURE_2D); > + } > + else { > + GLubyte val = (state == ALWAYS_PASS) ? 0xff : 0x00; > + GLubyte texImage[4*4*4]; > + for (i = 0; i < 4*4*4; i++) > + texImage[i] = val; > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, 4, 4, 0, > + GL_RGBA, GL_UNSIGNED_BYTE, texImage); > + glTexParameteri(GL_TEXTURE_2D, > GL_TEXTURE_MIN_FILTER, > + GL_NEAREST); > + glTexParameteri(GL_TEXTURE_2D, > GL_TEXTURE_MAG_FILTER, > + GL_NEAREST); > + glTexEnvi(GL_TEXTURE_ENV, GL_TEXTURE_ENV_MODE, > + GL_MODULATE); > + glEnable(GL_TEXTURE_2D); > + } > + break; > + default: > + abort(); /* error */ > + } > +} > + > + > +void > +piglit_init(int argc, char **argv) > +{ > + /* No init */ > +} /* piglit_init */ > You can drop the closing /* piglit_init */ comment. > + > + > +enum piglit_result > +piglit_display(void) > +{ > + bool pass = true; > + Path p, paths[1000]; > + int i, numPaths = 0; > + > + /* draw 10x10 pixel quads */ > + glViewport(0, 0, 10, 10); > + > + glDisable(GL_DITHER); > + > + /* Build the list of paths to exercise. > + * Skip depth test if we have no depth buffer. */ > + /* Skip stencil test if we have no stencil buffer. */ > + for (p = ALPHA; p != ZZZ; p = (Path) (p + 1)) { > + paths[numPaths++] = p; > + } > + > + /* test always-pass paths */ > + for (i = 0; i < numPaths; i++) { > + glClear(GL_COLOR_BUFFER_BIT); > + > + set_path_state(paths[i], ALWAYS_PASS); > + > + /* draw polygon */ > + glColor4f(1, 1, 1, 1); > + glBegin(GL_POLYGON); > + glVertex2f(-1, -1); > + glVertex2f( 1, -1); > + glVertex2f( 1, 1); > + glVertex2f(-1, 1); > + glEnd(); > I think there's a piglit_draw_rect() function that could be used there. > + > + set_path_state(paths[i], DISABLE); > + > + /* test buffer */ > + GLfloat pixel[3]; > + glReadPixels(4, 4, 1, 1, GL_RGB, GL_FLOAT, pixel); > + if (pixel[0] != 1.0 || pixel[1] != 1.0 || pixel[2] != 1.0) > { > I think you can replace that code with piglit_probe_rect_rgb(). > + printf("\nFAIL: %s, should have had no effect" > + " (1, 1, 1) but actually modified the > fragment" > + " ( %f, %f, %f)\n", path_name(paths[i]), > pixel[0], pixel[1], pixel[2]); > + return PIGLIT_FAIL; > + } > + } > + > + /* enable all always-pass paths */ > + { > + glClear(GL_COLOR_BUFFER_BIT); > + > + for (i = 0; i < numPaths; i++) { > + set_path_state(paths[i], ALWAYS_PASS); > + } > + > + /* draw polygon */ > + glColor4f(1, 1, 1, 1); > + glBegin(GL_POLYGON); > + glVertex2f(-1, -1); > + glVertex2f( 1, -1); > + glVertex2f( 1, 1); > + glVertex2f(-1, 1); > + glEnd(); > piglit_draw_rect() > + > + for (i = 0; i < numPaths; i++) { > + set_path_state(paths[i], DISABLE); > + } > + > + /* test buffer */ > + GLfloat pixel[3]; > + glReadPixels(4, 4, 1, 1, GL_RGB, GL_FLOAT, pixel); > + if (pixel[0] != 1.0 || pixel[1] != 1.0 || pixel[2] != 1.0) > { > piglit_probe_rect_rgb() > + printf("\nFAIL: %s, should have had no effect" > + " (1, 1, 1) but actually modified the > fragment" > + " ( %f, %f, %f)\n", path_name(paths[i]), > pixel[0], pixel[1], pixel[2]); > + return PIGLIT_FAIL; > + } > + } > + > + /* test never-pass paths */ > + for (i = 0; i < numPaths; i++) { > + glClear(GL_COLOR_BUFFER_BIT); > + > + set_path_state(paths[i], ALWAYS_FAIL); > + > + /* draw polygon */ > + glColor4f(1, 1, 1, 1); > + glBegin(GL_POLYGON); > + glVertex2f(-1, -1); > + glVertex2f( 1, -1); > + glVertex2f( 1, 1); > + glVertex2f(-1, 1); > + glEnd(); > + > + set_path_state(paths[i], DISABLE); > + > + /* test buffer */ > + GLfloat pixel[3]; > + glReadPixels(4, 4, 1, 1, GL_RGB, GL_FLOAT, pixel); > + if (pixel[0] != 0.0 || pixel[1] != 0.0 || pixel[2] != 0.0) > { > + printf("\nFAIL: %s, should have had no effect" > + " (1, 1, 1) but actually modified the > fragment" > + " ( %f, %f, %f)\n", path_name(paths[i]), > pixel[0], pixel[1], pixel[2]); > + return PIGLIT_FAIL; > + } > + } > + > + /* success */ > + pass = true; > + > + return pass ? PIGLIT_PASS : PIGLIT_FAIL; > Remove the 'pass' var and just return PIGLIT_PASS here. > +} > -- > 2.4.3 > >
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
