[Mesa-dev] [PATCH v2 5/6] mesa: Fix corner cases of BindBufferBase with transform feedback.

[Ian Romanick]

On 12/15/2012 10:09 PM, Paul Berry wrote:
> This patch implements the following behaviours, which are mandated by
> the GL 4.3 and GLES3 specs.
>
> 1. Regarding the GL_TRANSFORM_FEEDBACK_BUFFER_SIZE query: "If the
>     ... size was not specified when the buffer object was bound
>     (e.g. if it was bound with BindBufferBase), ... zero is returned."
>     (GL 4.3 section 6.7.1 "Indexed Buffer Object Limits and Binding
>     Queries").
>
> 2. "BindBufferBase binds the entire buffer, even when the size of the
>     buffer is changed after the binding is established. It is
>     equivalent to calling BindBufferRange with offset zero, while size
>     is determined by the size of the bound buffer at the time the
>     binding is used."  (GL 4.3 section 6.1.1 "Binding Buffer Objects to
>     Indexed Targets").  I interpret "at the time the binding is used"
>     to mean "at the time of the call to glBeginTransformFeedback".
>
> 3. "Regardless of the size specified with BindBufferRange, or
>     indirectly with BindBufferBase, the GL will never read or write
>     beyond the end of a bound buffer. In some cases this constraint may
>     result in visibly different behavior when a buffer overflow would
>     otherwise result, such as described for transform feedback
>     operations in section 13.2.2."  (GL 4.3 section 6.1.1 "Binding
>     Buffer Objects to Indexed Targets").
>
> Item 1 has been part of the spec all the way back to the inception of
> the EXT_transform_feedback extension.  Items 2 and 3 were added in GL
> 4.2 and GLES 3.
>
> Prior to GL 4.2, in place of items 2 and 3, the spec simply said
> "BindBufferBase is equivalent to calling BindBufferRange with offset
> zero and size equal to the size of buffer."  For transform feedback,
> Mesa behaved as though this meant "...equal to the size of buffer at
> the time of the call to BindBufferBase".  However, this was
> problematic because it left it ambiguous what to do if the buffer is
> shrunk between the call to BindBuffer{Base,Range} and the call to
> BeginTransformFeedback.  Prior to this patch, Mesa's behaviour was to
> try to write beyond the end of the buffer, likely resulting in memory
> corruption.  In light of this, I'm interpreting the spec change as a
> clarification, not an intended behavioural change, so I'm making the
> change apply regardless of API version.
>
> Fixes GLES3 conformance test transform_feedback2_pause_resume.test.
>
> Cc: Eric Anholt <eric at anholt.net>

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

> ---
>   src/mesa/main/get.c               |  3 +-
>   src/mesa/main/mtypes.h            | 13 +++++++-
>   src/mesa/main/transformfeedback.c | 69 ++++++++++++++++++++++++++++++---------
>   3 files changed, 68 insertions(+), 17 deletions(-)
>
> diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c
> index f3dbda2..273a79f 100644
> --- a/src/mesa/main/get.c
> +++ b/src/mesa/main/get.c
> @@ -1574,7 +1574,8 @@ find_value_indexed(const char *func, GLenum pname, GLuint index, union value *v)
>   	 goto invalid_value;
>         if (!ctx->Extensions.EXT_transform_feedback)
>   	 goto invalid_enum;
> -      v->value_int64 = ctx->TransformFeedback.CurrentObject->Size[index];
> +      v->value_int64
> +         = ctx->TransformFeedback.CurrentObject->RequestedSize[index];
>         return TYPE_INT64;
>
>      case GL_TRANSFORM_FEEDBACK_BUFFER_BINDING:
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index 18ab2db..96b84d8 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -1825,8 +1825,19 @@ struct gl_transform_feedback_object
>
>      /** Start of feedback data in dest buffer */
>      GLintptr Offset[MAX_FEEDBACK_BUFFERS];
> -   /** Max data to put into dest buffer (in bytes) */
> +
> +   /**
> +    * Max data to put into dest buffer (in bytes).  Computed based on
> +    * RequestedSize and the actual size of the buffer.
> +    */
>      GLsizeiptr Size[MAX_FEEDBACK_BUFFERS];
> +
> +   /**
> +    * Size that was specified when the buffer was bound.  If the buffer was
> +    * bound with glBindBufferBase() or glBindBufferOffsetEXT(), this value is
> +    * zero.
> +    */
> +   GLsizeiptr RequestedSize[MAX_FEEDBACK_BUFFERS];
>   };
>
>
> diff --git a/src/mesa/main/transformfeedback.c b/src/mesa/main/transformfeedback.c
> index 4782a6f..dd03038 100644
> --- a/src/mesa/main/transformfeedback.c
> +++ b/src/mesa/main/transformfeedback.c
> @@ -247,6 +247,55 @@ _mesa_init_transform_feedback_functions(struct dd_function_table *driver)
>
>
>   /**
> + * Fill in the correct Size value for each buffer in \c obj.
> + *
> + * From the GL 4.3 spec, section 6.1.1 ("Binding Buffer Objects to Indexed
> + * Targets"):
> + *
> + *   BindBufferBase binds the entire buffer, even when the size of the buffer
> + *   is changed after the binding is established. It is equivalent to calling
> + *   BindBufferRange with offset zero, while size is determined by the size of
> + *   the bound buffer at the time the binding is used.
> + *
> + *   Regardless of the size specified with BindBufferRange, or indirectly with
> + *   BindBufferBase, the GL will never read or write beyond the end of a bound
> + *   buffer. In some cases this constraint may result in visibly different
> + *   behavior when a buffer overflow would otherwise result, such as described
> + *   for transform feedback operations in section 13.2.2.
> + */
> +static void
> +compute_transform_feedback_buffer_sizes(
> +      struct gl_transform_feedback_object *obj)
> +{
> +   unsigned i = 0;
> +   for (i = 0; i < MAX_FEEDBACK_BUFFERS; ++i) {
> +      GLintptr offset = obj->Offset[i];
> +      GLsizeiptr buffer_size
> +         = obj->Buffers[i] == NULL ? 0 : obj->Buffers[i]->Size;
> +      GLsizeiptr available_space
> +         = buffer_size <= offset ? 0 : buffer_size - offset;
> +      GLsizeiptr computed_size;
> +      if (obj->RequestedSize[i] == 0) {
> +         /* No size was specified at the time the buffer was bound, so allow
> +          * writing to all available space in the buffer.
> +          */
> +         computed_size = available_space;
> +      } else {
> +         /* A size was specified at the time the buffer was bound, however
> +          * it's possible that the buffer has shrunk since then.  So only
> +          * allow writing to the minimum of the specified size and the space
> +          * available.
> +          */
> +         computed_size = MIN2(available_space, obj->RequestedSize[i]);
> +      }
> +
> +      /* Legal sizes must be multiples of four, so round down if necessary. */
> +      obj->Size[i] = computed_size & ~0x3;
> +   }
> +}
> +
> +
> +/**
>    * Compute the maximum number of vertices that can be written to the currently
>    * enabled transform feedback buffers without overflowing any of them.
>    */
> @@ -337,6 +386,8 @@ _mesa_BeginTransformFeedback(GLenum mode)
>      obj->Active = GL_TRUE;
>      ctx->TransformFeedback.Mode = mode;
>
> +   compute_transform_feedback_buffer_sizes(obj);
> +
>      if (_mesa_is_gles3(ctx)) {
>         /* In GLES3, we are required to track the usage of the transform
>          * feedback buffer and report INVALID_OPERATION if a draw call tries to
> @@ -407,7 +458,7 @@ bind_buffer_range(struct gl_context *ctx, GLuint index,
>      obj->BufferNames[index] = bufObj->Name;
>
>      obj->Offset[index] = offset;
> -   obj->Size[index] = size;
> +   obj->RequestedSize[index] = size;
>   }
>
>
> @@ -466,7 +517,6 @@ _mesa_bind_buffer_base_transform_feedback(struct gl_context *ctx,
>   					  struct gl_buffer_object *bufObj)
>   {
>      struct gl_transform_feedback_object *obj;
> -   GLsizeiptr size;
>
>      obj = ctx->TransformFeedback.CurrentObject;
>
> @@ -481,12 +531,7 @@ _mesa_bind_buffer_base_transform_feedback(struct gl_context *ctx,
>         return;
>      }
>
> -   /* default size is the buffer size rounded down to nearest
> -    * multiple of four.
> -    */
> -   size = bufObj->Size & ~0x3;
> -
> -   bind_buffer_range(ctx, index, bufObj, 0, size);
> +   bind_buffer_range(ctx, index, bufObj, 0, 0);
>   }
>
>
> @@ -502,7 +547,6 @@ _mesa_BindBufferOffsetEXT(GLenum target, GLuint index, GLuint buffer,
>      struct gl_transform_feedback_object *obj;
>      struct gl_buffer_object *bufObj;
>      GET_CURRENT_CONTEXT(ctx);
> -   GLsizeiptr size;
>
>      if (target != GL_TRANSFORM_FEEDBACK_BUFFER) {
>         _mesa_error(ctx, GL_INVALID_ENUM, "glBindBufferOffsetEXT(target)");
> @@ -542,12 +586,7 @@ _mesa_BindBufferOffsetEXT(GLenum target, GLuint index, GLuint buffer,
>         return;
>      }
>
> -   /* default size is the buffer size rounded down to nearest
> -    * multiple of four.
> -    */
> -   size = (bufObj->Size - offset) & ~0x3;
> -
> -   bind_buffer_range(ctx, index, bufObj, offset, size);
> +   bind_buffer_range(ctx, index, bufObj, offset, 0);
>   }
>
>
>