[Mesa-dev] [PATCH 2/2] util/draw: replace assertions with conditionals in util_draw_max_index()

[Jose Fonseca]

Looks good, but some warnings on util_draw_max_index would be useful, as this is not something apps normally do, so are much more often a symptom of a state tracker bug than apps bugs.

Jose

----- Original Message -----
> Don't assert/die if a VBO is too small.  Return zero instead.
> 
> Note that util_draw_max_index() now returns max_index+1 instead of
> max_index.  This lets us return zero to indicate that one of the VBOs
> is too small to draw anything.
> 
> Fixes a failure with the new piglit vbo-too-small test.
> ---
>  src/gallium/auxiliary/draw/draw_pt.c         |   19 ++++++++++++----
>  src/gallium/auxiliary/util/u_draw.c          |   30
>  +++++++++++++++++++------
>  src/mesa/state_tracker/st_cb_bufferobjects.c |    1 +
>  3 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/src/gallium/auxiliary/draw/draw_pt.c
> b/src/gallium/auxiliary/draw/draw_pt.c
> index e0eda67..737dd79 100644
> --- a/src/gallium/auxiliary/draw/draw_pt.c
> +++ b/src/gallium/auxiliary/draw/draw_pt.c
> @@ -422,6 +422,7 @@ draw_vbo(struct draw_context *draw,
>  {
>     unsigned reduced_prim = u_reduced_prim(info->mode);
>     unsigned instance;
> +   unsigned index_limit;
>  
>     assert(info->instance_count > 0);
>     if (info->indexed)
> @@ -470,11 +471,19 @@ draw_vbo(struct draw_context *draw,
>     if (0)
>        draw_print_arrays(draw, info->mode, info->start,
>        MIN2(info->count, 20));
>  
> -   draw->pt.max_index = util_draw_max_index(draw->pt.vertex_buffer,
> -
>                                            draw->pt.nr_vertex_buffers,
> -                                            draw->pt.vertex_element,
> -
>                                            draw->pt.nr_vertex_elements,
> -                                            info);
> +   index_limit = util_draw_max_index(draw->pt.vertex_buffer,
> +                                     draw->pt.nr_vertex_buffers,
> +                                     draw->pt.vertex_element,
> +                                     draw->pt.nr_vertex_elements,
> +                                     info);
> +
> +   if (index_limit == 0) {
> +      /* one of the buffers is too small to do any valid drawing */
> +      return;
> +   }
> +
> +   draw->pt.max_index = index_limit - 1;
> +
>  
>     /*
>      * TODO: We could use draw->pt.max_index to further narrow
> diff --git a/src/gallium/auxiliary/util/u_draw.c
> b/src/gallium/auxiliary/util/u_draw.c
> index 20837be..2da1adb 100644
> --- a/src/gallium/auxiliary/util/u_draw.c
> +++ b/src/gallium/auxiliary/util/u_draw.c
> @@ -33,9 +33,13 @@
>  
>  
>  /**
> - * Returns the largest legal index value for the current set of
> bound vertex
> - * buffers.  Regardless of any other consideration, all vertex
> lookups need to
> - * be clamped to 0..max_index to prevent an out-of-bound access.
> + * Returns the largest legal index value plus one for the current
> set
> + * of bound vertex buffers.  Regardless of any other consideration,
> + * all vertex lookups need to be clamped to 0..max_index-1 to
> prevent
> + * an out-of-bound access.
> + *
> + * Note that if zero is returned it means that one or more buffers
> is
> + * too small to contain any valid vertex data.
>   */
>  unsigned
>  util_draw_max_index(
> @@ -68,13 +72,25 @@ util_draw_max_index(
>        assert(format_desc->block.bits % 8 == 0);
>        format_size = format_desc->block.bits/8;
>  
> -      assert(buffer_size - buffer->buffer_offset <= buffer_size);
> +      if (buffer->buffer_offset >= buffer_size) {
> +         /* buffer is too small */
> +         return 0;
> +      }
> +
>        buffer_size -= buffer->buffer_offset;
>  
> -      assert(buffer_size - element->src_offset <= buffer_size);
> +      if (element->src_offset >= buffer_size) {
> +         /* buffer is too small */
> +         return 0;
> +      }
> +
>        buffer_size -= element->src_offset;
>  
> -      assert(buffer_size - format_size <= buffer_size);
> +      if (format_size > buffer_size) {
> +         /* buffer is too small */
> +         return 0;
> +      }
> +
>        buffer_size -= format_size;
>  
>        if (buffer->stride != 0) {
> @@ -95,5 +111,5 @@ util_draw_max_index(
>        }
>     }
>  
> -   return max_index;
> +   return max_index + 1;
>  }
> diff --git a/src/mesa/state_tracker/st_cb_bufferobjects.c
> b/src/mesa/state_tracker/st_cb_bufferobjects.c
> index 3abf029..286fa02 100644
> --- a/src/mesa/state_tracker/st_cb_bufferobjects.c
> +++ b/src/mesa/state_tracker/st_cb_bufferobjects.c
> @@ -221,6 +221,7 @@ st_bufferobj_data(struct gl_context *ctx,
>  
>     pipe_resource_reference( &st_obj->buffer, NULL );
>  
> +   /* note that size=0 is OK and we should get a non-null value */
>     st_obj->buffer = pipe_buffer_create(pipe->screen, bind,
>     pipe_usage, size);
>  
>     if (!st_obj->buffer) {
> --
> 1.7.3.4
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>