On 04/26/2016 10:08 PM, Jamey Sharp wrote: > OpenGL 4.5 Core Profile section 7.1, in the documentation for > CompileShader, says: "Changing the source code of a shader object with > ShaderSource does not change its compile status or the compiled shader > code." (I haven't checked older versions of the spec.) > > This test creates a shader, compiles it, changes its source, and links > it. The spec requires rendering done with this shader to be consistent > with the old source, not the new source, since the shader isn't compiled > again after the source is changed. > > According to Karol Herbst, the game "Divinity: Original Sin - Enhanced > Edition" depends on this odd quirk of the spec. See: > https://lists.freedesktop.org/archives/mesa-dev/2016-March/109789.html > > This test fails against current Mesa master, but passes with a one-line > patch to src/mesa/main/shaderapi.c. That patch, together with > MESA_GL_VERSION_OVERRIDE=4.2, also allowed "Divinity" to start up > successfully on i965, though rendering bugs remain. > > Based on Karol's report, I expect this test should pass on Catalyst, but > I only have Intel hardware to test on. > > Signed-off-by: Jamey Sharp <ja...@minilop.net> > Cc: Ian Romanick <i...@freedesktop.org> > --- > > "But Ian, I learned it from watching you!" Most of the things you > complained about, I copied verbatim from your sso-simple.c. ;-)
Coding style, especially in piglit, is always a moving target because we keep adding more infrastructure. A lot of the infrastructure and idioms didn't exist when I wrote sso-simple.c. The "best practice" is generally to find the test most recently added by someone you trust and copy that. sso-simple.c is pretty old. :) > I did notice I apparently introduced the first use of C99 array literals Piglit used to have a C99 allergy, but I think recent changes in MSVC make that less a problem. > anywhere in Piglit, and I didn't really need to, so I re-wrote that. And > after your suggestions, piglit_init was embarrasingly tiny, so I moved > more setup code into it. I think it's a little more clear this way. > > I think this test can perhaps be simplified. Can all the work be done in > one shader, either just a vertex shader or just a fragment shader? Or > does OpenGL require both shaders to be present? I didn't spot the answer > while skimming the spec. In OpenGL 2.0, you don't need both shaders. In OpenGL 3.1+ (core profile) or OpenGL ES 2.0+ you do need both. In this case you could just have two different fragment shaders that write a constant color and omit the vertex shader altogether. I think that was Ken's suggestion. > > tests/all.py | 1 + > tests/shaders/CMakeLists.gl.txt | 1 + > tests/shaders/shadersource-no-compile.c | 102 > ++++++++++++++++++++++++++++++++ > 3 files changed, 104 insertions(+) > create mode 100644 tests/shaders/shadersource-no-compile.c > > diff --git a/tests/all.py b/tests/all.py > index 93d64e6..9f5c019 100644 > --- a/tests/all.py > +++ b/tests/all.py > @@ -572,6 +572,7 @@ with profile.group_manager(PiglitGLTest, 'shaders') as g: > g(['glsl-kwin-blur-2']) > g(['gpu_shader4_attribs']) > g(['link-unresolved-function']) > + g(['shadersource-no-compile']) > g(['sso-simple']) > g(['sso-uniforms-01']) > g(['sso-uniforms-02']) > diff --git a/tests/shaders/CMakeLists.gl.txt b/tests/shaders/CMakeLists.gl.txt > index afbcc4b..2db2ded 100644 > --- a/tests/shaders/CMakeLists.gl.txt > +++ b/tests/shaders/CMakeLists.gl.txt > @@ -150,6 +150,7 @@ ENDIF (UNIX) > piglit_add_executable (glsl-kwin-blur-1 glsl-kwin-blur-1.c) > piglit_add_executable (glsl-kwin-blur-2 glsl-kwin-blur-2.c) > piglit_add_executable (link-unresolved-function link-unresolved-function.c) > +piglit_add_executable (shadersource-no-compile shadersource-no-compile.c) > piglit_add_executable (sso-simple sso-simple.c) > piglit_add_executable (sso-uniforms-01 sso-uniforms-01.c) > piglit_add_executable (sso-uniforms-02 sso-uniforms-02.c) > diff --git a/tests/shaders/shadersource-no-compile.c > b/tests/shaders/shadersource-no-compile.c > new file mode 100644 > index 0000000..b5db55d > --- /dev/null > +++ b/tests/shaders/shadersource-no-compile.c > @@ -0,0 +1,102 @@ > +/* > + * Copyright © 2016 Jamey Sharp > + * > + * 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 shadersource-no-compile.c > + * OpenGL 4.5 Core Profile section 7.1, in the documentation for > CompileShader, > + * says: "Changing the source code of a shader object with ShaderSource does > not > + * change its compile status or the compiled shader code." > + * > + * This test creates a shader, compiles it, changes its source, and links it. > + * The spec requires rendering done with this shader to be consistent with > the > + * old source, not the new source, since the shader isn't compiled again > after > + * the source is changed. > + * > + * According to Karol Herbst, the game "Divinity: Original Sin - Enhanced > + * Edition" depends on this odd quirk of the spec. See: > + * https://lists.freedesktop.org/archives/mesa-dev/2016-March/109789.html > + */ > +#include "piglit-util-gl.h" > + > +PIGLIT_GL_TEST_CONFIG_BEGIN > + > + config.supports_gl_compat_version = 20; > + > + config.window_visual = PIGLIT_GL_VISUAL_RGB | PIGLIT_GL_VISUAL_DOUBLE; > + > +PIGLIT_GL_TEST_CONFIG_END > + > +static const char vs_text[] = > + "void main() { gl_Position = gl_Vertex; " > + "gl_FrontColor = vec4(0.0, 1.0, 0.0, 1.0); }"; > + > +static const char good_fs_text[] = > + "void main() { gl_FragColor = gl_Color; }"; > + > +/* It is important that this shader *not* use gl_Color. > + */ > +static const char bad_fs_text[] = > + "void main() { gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0); }"; > + > +enum piglit_result > +piglit_display(void) > +{ > + static const float green[3] = { 0.0, 1.0, 0.0 }; > + static const float blue[3] = { 0.0, 0.0, 1.0 }; > + > + glClear(GL_COLOR_BUFFER_BIT); > + > + /* good_fs_text uses the color from the vertex shader, which is green. > + * bad_fs_text uses a constant red color. Both are distinct from the > + * color set here, and from the clear-color. > + */ > + glColor3fv(blue); > + > + piglit_draw_rect(-1, -1, 2, 2); > + if (!piglit_probe_pixel_rgb(15, 15, green)) > + return PIGLIT_FAIL; > + > + return PIGLIT_PASS; > +} > + > +void > +piglit_init(int argc, char **argv) > +{ > + GLuint vs = piglit_compile_shader_text(GL_VERTEX_SHADER, vs_text); > + GLuint fs = piglit_compile_shader_text(GL_FRAGMENT_SHADER, > good_fs_text); > + const GLchar *bad_fs_texts[] = { bad_fs_text }; > + GLuint prog; > + > + /* Change the shader source, but don't recompile it before linking. */ > + glShaderSource(fs, 1, bad_fs_texts, NULL); > + prog = piglit_link_simple_program(vs, fs); > + if (!prog) > + piglit_report_result(PIGLIT_FAIL); > + > + glDeleteShader(vs); > + glDeleteShader(fs); > + > + glUseProgram(prog); > + > + glClearColor(0.3, 0.3, 0.3, 0.0); > +} > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
