On 08/22/2012 07:26 PM, Ian Romanick wrote:
> From: Ian Romanick <ian.d.roman...@intel.com>
>
> Signed-off-by: Ian Romanick <ian.d.roman...@intel.com>
> ---
> src/mesa/main/varray.c | 28 +++++++++++++++++++---------
> 1 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/src/mesa/main/varray.c b/src/mesa/main/varray.c
> index 327fabb..36d0eb8 100644
> --- a/src/mesa/main/varray.c
> +++ b/src/mesa/main/varray.c
> @@ -133,16 +133,26 @@ update_array(struct gl_context *ctx,
> GLsizei elementSize;
> GLenum format = GL_RGBA;
>
> - if (ctx->API != API_OPENGLES && ctx->API != API_OPENGLES2) {
> - /* fixed point arrays / data is only allowed with OpenGL ES 1.x/2.0 */
> + if (ctx->API == API_OPENGLES || ctx->API == API_OPENGLES2) {
if (_mesa_is_gles(ctx)) {
> + /* Once Mesa gets support for GL_OES_vertex_half_float this mask will
> + * change. Adding support for this extension isn't quite as trivial as
> + * we'd like because ES uses a different enum value for GL_HALF_FLOAT.
> + */
> + legalTypesMask &= ~(FIXED_GL_BIT
> + | UNSIGNED_INT_2_10_10_10_REV_BIT
> + | INT_2_10_10_10_REV_BIT
> + | HALF_BIT | DOUBLE_BIT);
> + } else {
> + assert(ctx->API == API_OPENGL);
I believe this assertion is wrong. Both VertexAttribPointer and
VertexAttribIPointer call update_arrays, and neither of those functions
are deprecated. So it's possible to get here from API_OPENGL_CORE.
I would just remove it.
> +
> legalTypesMask &= ~FIXED_ES_BIT;
> - }
> - if (!ctx->Extensions.ARB_ES2_compatibility) {
> - legalTypesMask &= ~FIXED_GL_BIT;
> - }
> - if (!ctx->Extensions.ARB_vertex_type_2_10_10_10_rev) {
> - legalTypesMask &= ~(UNSIGNED_INT_2_10_10_10_REV_BIT |
> - INT_2_10_10_10_REV_BIT);
> +
> + if (!ctx->Extensions.ARB_ES2_compatibility)
> + legalTypesMask &= ~FIXED_GL_BIT;
> +
> + if (!ctx->Extensions.ARB_vertex_type_2_10_10_10_rev)
> + legalTypesMask &= ~(UNSIGNED_INT_2_10_10_10_REV_BIT |
> + INT_2_10_10_10_REV_BIT);
> }
>
> typeBit = type_to_bit(ctx, type);
>
This is also not a mere refactor---unless I'm reading something wrong,
it changes behavior rather substantially.
Consider the old code:
if (ctx->API != API_OPENGLES && ctx->API != API_OPENGLES2) {
/* fixed point arrays / data is only allowed with OpenGL ES 1.x/2.0 */
legalTypesMask &= ~FIXED_ES_BIT;
}
if (!ctx->Extensions.ARB_ES2_compatibility) {
legalTypesMask &= ~FIXED_GL_BIT;
}
In the API_OPENGL case, the first conditional would mask out
FIXED_ES_BIT, disallowing it even if ARB_ES2_compatibility was
supported. Your new code allows it, which is a bug fix. (The
ARB_ES2_compatibility spec explicitly allows GL_FIXED with
VertexAttribPointer.)
In the API_OPENGLES (1 or 2) case, if the driver supports
ARB_ES2_compatibility, the second conditional would /still/ mask out
FIXED_ES_BIT, disallowing it even on ES! Your new code correctly allows
that (another serious bug fix).
So unless I'm misreading it, the old code was just completely broken
regarding GL_FIXED data, and your new code is reasonable and fixes it.
I believe at least a comment in the commit message is in order; maybe
even a mark for stable? (Unless GL_FIXED is just totally broken
elsewhere and allowing it through here causes worse bugs...)
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
