<div dir="ltr">On 10 September 2013 17:47, Chad Versace <<a href="mailto:chad.versace@linux.intel.com" target="_blank">chad.versace@linux.intel.com</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div>On 09/10/2013 01:47 PM, Jacob Penner wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> --- tests/all.tests | 1 + .../gl-3.2/layered-rendering/CMakeLists.gl.txt | 1 + tests/spec/gl-3.2/layered-rendering/clear-depth.c | 224 +++++++++++++++++++++ 3 files changed, 226 insertions(+) create mode 100644 tests/spec/gl-3.2/layered-rendering/clear-depth.c diff --git a/tests/all.tests b/tests/all.tests index e1366ca..2444337 100644 --- a/tests/all.tests +++ b/tests/all.tests @@ -750,6 +750,7 @@ spec['!OpenGL 3.2/get-integer-64iv'] = concurrent_test('gl-3.2-get-integer-64iv' spec['!OpenGL 3.2/get-integer-64v'] = concurrent_test('gl-3.2-get-integer-64v') spec['!OpenGL 3.2/layered-rendering/blit'] = concurrent_test('gl-3.2-layered-rendering-blit') spec['!OpenGL 3.2/layered-rendering/clear-color'] = concurrent_test('gl-3.2-layered-rendering-clear-color') +spec['!OpenGL 3.2/layered-rendering/clear-depth'] = concurrent_test('gl-3.2-layered-rendering-clear-depth') spec['!OpenGL 3.2/layered-rendering/framebuffertexture-buffer-textures'] = concurrent_test('gl-3.2-layered-rendering-framebuffertexture-buffer-textures') spec['!OpenGL 3.2/layered-rendering/readpixels'] = concurrent_test('gl-3.2-layered-rendering-readpixels') spec['!OpenGL 3.2/layered-rendering/gl-layer'] = concurrent_test('gl-3.2-layered-rendering-gl-layer') diff --git a/tests/spec/gl-3.2/layered-rendering/CMakeLists.gl.txt b/tests/spec/gl-3.2/layered-rendering/CMakeLists.gl.txt index 9a26b34..bc4415d 100644 --- a/tests/spec/gl-3.2/layered-rendering/CMakeLists.gl.txt +++ b/tests/spec/gl-3.2/layered-rendering/CMakeLists.gl.txt @@ -11,6 +11,7 @@ link_libraries ( piglit_add_executable (gl-3.2-layered-rendering-blit blit.c) piglit_add_executable (gl-3.2-layered-rendering-clear-color clear-color.c) +piglit_add_executable (gl-3.2-layered-rendering-clear-depth clear-depth.c) piglit_add_executable (gl-3.2-layered-rendering-framebuffertexture-buffer-textures framebuffertexture-buffer-textures.c) piglit_add_executable (gl-3.2-layered-rendering-readpixels readpixels.c) piglit_add_executable (gl-3.2-layered-rendering-gl-layer gl-layer.c) diff --git a/tests/spec/gl-3.2/layered-rendering/clear-depth.c b/tests/spec/gl-3.2/layered-rendering/clear-depth.c new file mode 100644 index 0000000..d680780 --- /dev/null +++ b/tests/spec/gl-3.2/layered-rendering/clear-depth.c @@ -0,0 +1,224 @@ +/* + * Copyright © 2013 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 clear-depth.c + * + * Section 4.4.7(Framebuffer Objects) From GL spec 3.2 core: + * When the Clear or ClearBuffer* commands are used to clear a layered + * framebuffer attachment, all layers of the attachment are cleared. + * + * Test Layout + * Tex1 Tex2 + * *--------*--------* + * | layer4 | layer4 | + * *--------*--------* Each Layer for both tex1 and tex2 will be + * | layer3 | layer3 | different depths. + * *--------*--------* + * | layer2 | layer2 | Tex1 will be cleared using glClear() + * *--------*--------* + * | layer1 | layer1 | Tex2 will be cleared using glClearBuffer() + * *--------*--------* + * + * Result: + * Layer 1-4 of both tex1 and tex2 should be the clearDepth + */ + +#include "piglit-util-gl-common.h" + +PIGLIT_GL_TEST_CONFIG_BEGIN + + config.supports_gl_compat_version = 32; + config.supports_gl_core_version = 32; + +PIGLIT_GL_TEST_CONFIG_END + +const char *vs_source = { + "#version 150\n" + "in vec4 piglit_vertex;\n" + "void main()\n" + "{\n" + " gl_Position = piglit_vertex;\n" + "}\n" +}; + +bool +probe_texture_layered_depth(GLuint texture, int x, int y, int z, + int w, int h, int d, float *expected) +{ + GLint prev_read_fbo; + GLint prev_draw_fbo; + + GLuint fbo; + int i, j, k; + + GLfloat *probe; + GLfloat *pixels = malloc(w*h*sizeof(float)); + + glGetIntegerv(GL_DRAW_FRAMEBUFFER_BINDING, &prev_draw_fbo); + glGetIntegerv(GL_READ_FRAMEBUFFER_BINDING, &prev_read_fbo); + + glGenFramebuffers(1, &fbo); </blockquote> </div></div> Even though this is just a test, please cleanup by deleting the temporary fbo.<div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + glBindFramebuffer(GL_FRAMEBUFFER, fbo); + + for(k = 0; k < d; k++ ) { + + glFramebufferTextureLayer(GL_FRAMEBUFFER, GL_DEPTH_ATTACHMENT, + texture, 0, k+z); + </blockquote> </div> There already exists a function for probing depth values, piglit_probe_rect_depth. Just use that instead of open-coding the below probing.<div><div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + glReadPixels(x, y, w, h, GL_DEPTH_COMPONENT, GL_FLOAT, pixels); + + for(j = 0; j < h; j++) { + for(i = 0; i < w; i++) { + probe = &pixels[j*w+i]; + if (fabs(*probe - expected[k]) >= 0.01) { + printf("Probe depth at (%i,%i,%i)\n", + x+i, y+j,z+k); + printf(" Expected: %f\n", expected[k]); + printf(" Observed: %f\n", *probe); + + free(pixels); + return false; + } + } + } + } + free(pixels); + + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, prev_draw_fbo); + glBindFramebuffer(GL_READ_FRAMEBUFFER, prev_read_fbo); + return true; +} + +void +piglit_init(int argc, char **argv) +{ + + int i, j; + GLenum fbstatus; + bool pass = true; + GLuint fbo[2], texture[2]; + GLint program; + + float depths[4] = { + 0.25, 0.5, 0.75, 1.0 + }; + + GLfloat clearDepth = 0.0; + float expected[4] = { 0.0, 0.0, 0.0, 0.0 }; + + program = piglit_build_simple_program(vs_source, NULL); + glUseProgram(program); + + glGenTextures(2, texture); + glGenFramebuffers(2, fbo); + for(i = 0; i < 2; i++) { + glBindTexture(GL_TEXTURE_2D_ARRAY, texture[i]); + glTexParameteri(GL_TEXTURE_2D_ARRAY, GL_TEXTURE_MIN_FILTER, + GL_LINEAR); + glTexParameteri(GL_TEXTURE_2D_ARRAY, GL_TEXTURE_MAG_FILTER, + GL_LINEAR); + glTexParameteri(GL_TEXTURE_2D_ARRAY, GL_TEXTURE_WRAP_S, + GL_REPEAT); + glTexParameteri(GL_TEXTURE_2D_ARRAY, GL_TEXTURE_WRAP_T, + GL_REPEAT); + glTexImage3D(GL_TEXTURE_2D_ARRAY, 0, GL_DEPTH_COMPONENT32, + 10, 10, 4, 0, GL_DEPTH_COMPONENT, GL_FLOAT, NULL); + + glEnable(GL_DEPTH_TEST); + glDepthFunc(GL_ALWAYS); + glDepthRange(0, 1); + glDrawBuffer(GL_NONE); </blockquote> </div></div> Ah... I was unaware of piglit_draw_rect_z. Since we're using that function, there is no need to call glDepthRange. I'm confused by the call to glDrawBuffer. What are you trying to accomplish with it?</blockquote><div> </div><div>Looking at the specs yesterday, I *thought* the call is necessary in order to ensure framebuffer completeness, since there's no color attachment to the framebuffer. In GL 3.2 (section 4.4.4 Framebuffer Completeness, under the "Whole Framebuffer Completeness" section), the following is listed as a criterion for framebuffer completeness: The value of FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE must not be NONE for any color attachment point(s) named by DRAW_BUFFERi. { FRAMEBUFFER_INCOMPLETE_DRAW_BUFFER } </div><div>However, maybe I'm interpreting the spec wrong, because neither Mesa nor the NVIDIA proprietary driver requires the glDrawBuffer() call in order for the framebuffer to be complete. Any insight on this, Jacob?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + glBindFramebuffer(GL_FRAMEBUFFER, fbo[i]); + + for(j = 0; j < 4; j++) { + glFramebufferTextureLayer(GL_FRAMEBUFFER, + GL_DEPTH_ATTACHMENT, + texture[i], 0, j); + + + fbstatus = glCheckFramebufferStatus(GL_FRAMEBUFFER); + if(fbstatus != GL_FRAMEBUFFER_COMPLETE){ + printf("%s\n", piglit_get_gl_enum_name(fbstatus)); + piglit_report_result(PIGLIT_FAIL); + } + + piglit_check_gl_error(GL_NO_ERROR); </blockquote> </div> If you want the test to fail, tt's not enough to call piglit_check_gl_error. That function doesn't terminate test. You need this: if (!piglit_check_gl_error(GL_NO_ERROR)) piglit_report_result(PIGLIT_FAIL);<div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + piglit_draw_rect_z(depths[j], -1, -1, 2, 2); + piglit_check_gl_error(GL_NO_ERROR); + } + + glDisable(GL_DEPTH_TEST); </blockquote> </div> The code from here...<div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + /* Once values are set, attach the texture to the fbo + * as a layered texture + */ + glFramebufferTexture(GL_FRAMEBUFFER, GL_DEPTH_ATTACHMENT, + texture[i], 0); + + + fbstatus = glCheckFramebufferStatus(GL_FRAMEBUFFER); + if(fbstatus != GL_FRAMEBUFFER_COMPLETE){ + printf("%s\n", piglit_get_gl_enum_name(fbstatus)); + piglit_report_result(PIGLIT_FAIL); + } + + if(!piglit_check_gl_error(GL_NO_ERROR)) + piglit_report_result(PIGLIT_FAIL); </blockquote> </div> ... to here. I'm unsure what it is trying to do. I don't see the need to test for framebuffer completeness if nothing is rendered to the framebuffer with those attachments.</blockquote><div> </div><div>IMHO the framebuffer completeness check is useful because if for some unexpected reason the framebuffer isn't complete, the clear calls below will likely fail. It's easier to debug a test failure caused by unexpected framebuffer incompleteness if the test explicitly checks framebuffer completeness before doing its rendering. </div><div>I'm mildly in favor of keeping the framebuffer completeness check but I don't feel terribly strongly. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + } + + + glBindFramebuffer(GL_FRAMEBUFFER, fbo[0]); + /* Clear fbo 0 with glClear() */ + glClearDepth(clearDepth); + glClear(GL_DEPTH_BUFFER_BIT); + + if(!probe_texture_layered_depth(texture[0], 0, 0, 0, 10, + 10, 4, expected)) { + printf("Incorrect depth values recieved with glClear()\n"); + pass = false; + } + + /* Clear fbo 1 with glClearBuffer() */ + glBindFramebuffer(GL_FRAMEBUFFER, fbo[1]); + glClearBufferfv(GL_DEPTH, 0, &clearDepth); + + if(!probe_texture_layered_depth(texture[1], 0, 0, 0, 10, + 10, 4, expected)) { + printf("Incorrect depth values recieved with glClearBuffer()\n"); + pass = false; + } + + pass = piglit_check_gl_error(GL_NO_ERROR) && pass; + + piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL); +} + + +enum piglit_result +piglit_display(void) +{ + /* UNREACHABLE */ + return PIGLIT_FAIL; +} </blockquote> </div></div> The rest of the test looks good to me. Of course, we should get Paul's opinion too.</blockquote><div> </div><div>I agree with Chad's comments about deleting the temporary fbo, using piglit_probe_rect_depth, removing the call to glDepthRange(), and checking the return value of piglit_check_gl_error(). With those changes, the patch is: </div><div>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> </div><div>I could go either way on the framebuffer completeness check and the use of glDrawBuffer(). </div></div></div></div>