[Mesa-dev] [PATCH 02/12] mesa: Rearrange array type checking, filter more types in ES

[Kenneth Graunke]

On 08/22/2012 07:26 PM, Ian Romanick wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
> 
> Signed-off-by: Ian Romanick <ian.d.romanick at 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...)