On 10/28/2011 10:42 AM, Ian Romanick wrote: > From: Ian Romanick <ian.d.roman...@intel.com> > > Signed-off-by: Ian Romanick <ian.d.roman...@intel.com> > --- > src/mesa/main/uniforms.c | 142 > +++++++++++++++++++++++++++++----------------- > 1 files changed, 90 insertions(+), 52 deletions(-) > > diff --git a/src/mesa/main/uniforms.c b/src/mesa/main/uniforms.c > index f968d9b..2a5318d 100644 > --- a/src/mesa/main/uniforms.c > +++ b/src/mesa/main/uniforms.c > @@ -35,7 +35,7 @@ > * 2. Insert FLUSH_VERTICES calls in various places > */ > > - > +#include <stdbool.h> > #include "main/glheader.h" > #include "main/context.h" > #include "main/dispatch.h" > @@ -317,6 +317,81 @@ get_uniform_rows_cols(const struct gl_program_parameter > *p, > } > } > > +static bool > +validate_uniform_parameters(struct gl_context *ctx, > + struct gl_shader_program *shProg, > + GLint location, GLsizei count, > + unsigned *loc, > + unsigned *array_index, > + const char *caller, > + bool negative_one_is_not_valid) > +{ > + if (!shProg || !shProg->LinkStatus) { > + _mesa_error(ctx, GL_INVALID_OPERATION, "%s(program not linked)", > caller); > + return false; > + } > + > + if (location == -1) { > + /* Page 264 (page 278 of the PDF) of the OpenGL 2.1 spec says: > + * > + * "The error INVALID_OPERATION is generated if program has not > been > + * linked successfully, or if location is not a valid location for > + * program." > + * > + * However, page 82 (page 96 of the PDF) of the OpenGL 2.1 spec says: > + * > + * "If the value of location is -1, the Uniform* commands will > + * silently ignore the data passed in, and the current uniform > + * values will not be changed." > + * > + * The negative_one_is_not_valid flag selects between the two > behaviors.
Your wording here makes it sound like the GL spec contradicts itself, when it's actually quite clear. The first behavior is for GetUniform, while the second is for setting Uniforms. Perhaps mention that for clarity? Otherwise, looks like a nice cleanup. > + */ > + if (negative_one_is_not_valid) { > + _mesa_error(ctx, GL_INVALID_OPERATION, "%s(location=%d)", > + caller, location); > + } > + > + return false; > + } > + > + /* From page 12 (page 26 of the PDF) of the OpenGL 2.1 spec: > + * > + * "If a negative number is provided where an argument of type sizei > or > + * sizeiptr is specified, the error INVALID_VALUE is generated." > + */ > + if (count < 0) { > + _mesa_error(ctx, GL_INVALID_VALUE, "%s(count < 0)", caller); > + return false; > + } > + > + /* Page 82 (page 96 of the PDF) of the OpenGL 2.1 spec says: > + * > + * "If any of the following conditions occur, an INVALID_OPERATION > + * error is generated by the Uniform* commands, and no uniform values > + * are changed: > + * > + * ... > + * > + * - if no variable with a location of location exists in the > + * program object currently in use and location is not -1, > + */ > + if (location < -1) { > + _mesa_error(ctx, GL_INVALID_OPERATION, "%s(location=%d)", > + caller, location); > + return false; > + } > + > + _mesa_uniform_split_location_offset(location, loc, array_index); > + > + if (*loc >= shProg->Uniforms->NumUniforms) { > + _mesa_error(ctx, GL_INVALID_OPERATION, "%s(location=%d)", > + caller, location); > + return false; > + } > + > + return true; > +} > + > /** > * Called via glGetUniform[fiui]v() to get the current value of a uniform. > */ > @@ -327,14 +402,14 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint > program, GLint location, > struct gl_shader_program *shProg = > _mesa_lookup_shader_program_err(ctx, program, "glGetUniformfv"); > struct gl_program *prog; > - GLint paramPos, offset; > + GLint paramPos; > + unsigned loc, offset; > > - if (!shProg) > + if (!validate_uniform_parameters(ctx, shProg, location, 1, > + &loc, &offset, "glGetUniform", true)) > return; > > - _mesa_uniform_split_location_offset(location, &location, &offset); > - > - if (!find_uniform_parameter_pos(shProg, location, &prog, ¶mPos)) { > + if (!find_uniform_parameter_pos(shProg, loc, &prog, ¶mPos)) { > _mesa_error(ctx, GL_INVALID_OPERATION, "glGetUniformfv(location)"); > } > else { > @@ -743,41 +818,20 @@ _mesa_uniform(struct gl_context *ctx, struct > gl_shader_program *shProg, > const GLvoid *values, GLenum type) > { > struct gl_uniform *uniform; > - GLint elems, offset; > + GLint elems; > + unsigned loc, offset; > > ASSERT_OUTSIDE_BEGIN_END(ctx); > > - if (!shProg || !shProg->LinkStatus) { > - _mesa_error(ctx, GL_INVALID_OPERATION, "glUniform(program not > linked)"); > - return; > - } > - > - if (location == -1) > - return; /* The standard specifies this as a no-op */ > - > - if (location < -1) { > - _mesa_error(ctx, GL_INVALID_OPERATION, "glUniform(location=%d)", > - location); > - return; > - } > - > - _mesa_uniform_split_location_offset(location, &location, &offset); > - > - if (location < 0 || location >= (GLint) shProg->Uniforms->NumUniforms) { > - _mesa_error(ctx, GL_INVALID_VALUE, "glUniform(location=%d)", location); > + if (!validate_uniform_parameters(ctx, shProg, location, count, > + &loc, &offset, "glUniform", false)) > return; > - } > - > - if (count < 0) { > - _mesa_error(ctx, GL_INVALID_VALUE, "glUniform(count < 0)"); > - return; > - } > > elems = _mesa_sizeof_glsl_type(type); > > FLUSH_VERTICES(ctx, _NEW_PROGRAM_CONSTANTS); > > - uniform = &shProg->Uniforms->Uniforms[location]; > + uniform = &shProg->Uniforms->Uniforms[loc]; > > if (ctx->Shader.Flags & GLSL_UNIFORMS) { > const GLenum basicType = base_uniform_type(type); > @@ -924,30 +978,14 @@ _mesa_uniform_matrix(struct gl_context *ctx, struct > gl_shader_program *shProg, > GLboolean transpose, const GLfloat *values) > { > struct gl_uniform *uniform; > - GLint offset; > + unsigned loc, offset; > > ASSERT_OUTSIDE_BEGIN_END(ctx); > > - if (!shProg || !shProg->LinkStatus) { > - _mesa_error(ctx, GL_INVALID_OPERATION, > - "glUniformMatrix(program not linked)"); > + if (!validate_uniform_parameters(ctx, shProg, location, count, > + &loc, &offset, "glUniformMatrix", false)) > return; > - } > - > - if (location == -1) > - return; /* The standard specifies this as a no-op */ > > - if (location < -1) { > - _mesa_error(ctx, GL_INVALID_OPERATION, "glUniformMatrix(location)"); > - return; > - } > - > - _mesa_uniform_split_location_offset(location, &location, &offset); > - > - if (location < 0 || location >= (GLint) shProg->Uniforms->NumUniforms) { > - _mesa_error(ctx, GL_INVALID_VALUE, "glUniformMatrix(location)"); > - return; > - } > if (values == NULL) { > _mesa_error(ctx, GL_INVALID_VALUE, "glUniformMatrix"); > return; > @@ -955,7 +993,7 @@ _mesa_uniform_matrix(struct gl_context *ctx, struct > gl_shader_program *shProg, > > FLUSH_VERTICES(ctx, _NEW_PROGRAM_CONSTANTS); > > - uniform = &shProg->Uniforms->Uniforms[location]; > + uniform = &shProg->Uniforms->Uniforms[loc]; > > if (shProg->_LinkedShaders[MESA_SHADER_VERTEX]) { > /* convert uniform location to program parameter index */ _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev