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

[Ian Romanick]

On 08/23/2012 12:00 AM, Kenneth Graunke wrote:
> 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)) {

Right.  Most of the patches in this series are pretty old, so they 
predate the existence of that function.  I converted most, but I 
obviously missed some.

>> +      /* 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.

Yeah.  When I wrote that assertion there was no API_OPENGL_CORE.

>> +
>>         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.)

Not exactly.  FIXED_ES_BIT and FIXED_GL_BIT are generated depending on 
the context API.

    case GL_FIXED:
       return _mesa_is_desktop_gl(ctx) ? FIXED_GL_BIT : FIXED_ES_BIT;

In a non-ES context, FIXED_ES_BIT should be impossible, and likewise for 
FIXED_GL_BIT in an ES context.  It's weird, and I don't understand why 
it is the way it is.  I didn't want to change it in this series, and I 
forgot about it after writing these patches.

> 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...)