[Mesa-dev] [PATCH 3/4] vbo: Rework checking of 'end' against _MaxElement.

[Roland Scheidegger]

Am 08.02.2012 14:08, schrieb Kenneth Graunke:
> This failed to take basevertex into account:
> 
> If basevertex < 0:
>    (end + basevertex) might actually be in-bounds while 'end' is not.
>    We would have clamped in this case when we probably shouldn't.
>    This could break application drawing.
> 
> If basevertex > 0:
>    'end' might be in-bounds while (end + basevertex) might not.
>    We would have failed to clamp in this place.  There's a comment
>    indicating the TNL module depends on max_index being in-bounds;
>    if so, it would likely break horribly.
> 
> Rather than trying to clamp correctly in the face of basevertex, simply
> delete the clamping code and indicate that we don't have a valid range.
> This causes _tnl_vbo_draw_prims to use vbo_get_minmax_indices() to
> compute the actual bounds, which is much safer.
> 
> NOTE: This is a candidate for release branches.
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/vbo/vbo_exec_array.c |   17 ++++++-----------
>  1 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/src/mesa/vbo/vbo_exec_array.c b/src/mesa/vbo/vbo_exec_array.c
> index c26a8cd..67073df 100644
> --- a/src/mesa/vbo/vbo_exec_array.c
> +++ b/src/mesa/vbo/vbo_exec_array.c
> @@ -859,6 +859,7 @@ vbo_exec_DrawRangeElementsBaseVertex(GLenum mode,
>  				     GLint basevertex)
>  {
>     static GLuint warnCount = 0;
> +   GLboolean index_bounds_valid = GL_TRUE;
>     GET_CURRENT_CONTEXT(ctx);
>  
>     if (MESA_VERBOSE & VERBOSE_DRAW)
> @@ -911,16 +912,6 @@ vbo_exec_DrawRangeElementsBaseVertex(GLenum mode,
>        end = MIN2(end, 0xffff);
>     }
>  
> -   if (end >= ctx->Array.ArrayObj->_MaxElement) {
> -      /* Set 'end' to the max possible legal value */
> -      assert(ctx->Array.ArrayObj->_MaxElement >= 1);
> -      end = ctx->Array.ArrayObj->_MaxElement - 1;
> -
> -      if (end < start) {
> -         return;
> -      }
> -   }
> -
>     if (0) {
>        printf("glDraw[Range]Elements{,BaseVertex}"
>  	     "(start %u, end %u, type 0x%x, count %d) ElemBuf %u, "
> @@ -930,13 +921,17 @@ vbo_exec_DrawRangeElementsBaseVertex(GLenum mode,
>  	     basevertex);
>     }
>  
> +   if ((int)(end + basevertex) < 0 ||
> +       end + basevertex >= ctx->Array.ArrayObj->_MaxElement)
> +      index_bounds_valid = GL_FALSE;
by the same reasoning as in 2/4, this now needs to be start + basevertex
< 0 here.
I'm only a little bit worried that we get overflow handling wrong if the
indices are legitimately more than 2^31 (and similar size basevertex).
Probably not an immediate concern though.
Otherwise looks good (though it doesn't fix the likely confusing
warnings in case it was really the BaseVertex variant being called).
I hope though it's ok to pass in not ~0 for the start/end values in case
they aren't valid (which we seemed to have always done before). Can't
see why not but I didn't check the code.
Rest of the series also looks good, though personally I don't think it
really is worth 4 patches but I certainly don't oppose that...

Roland


> +
>  #if 0
>     check_draw_elements_data(ctx, count, type, indices);
>  #else
>     (void) check_draw_elements_data;
>  #endif
>  
> -   vbo_validated_drawrangeelements(ctx, mode, GL_TRUE, start, end,
> +   vbo_validated_drawrangeelements(ctx, mode, index_bounds_valid, start, end,
>  				   count, type, indices, basevertex, 1);
>  }
>