[Mesa-dev] [PATCH 5/5] mesa: Fix GL_TRANSFORM_FEEDBACK_BUFFER_SIZE query when glBindBufferBase used.

[Paul Berry]

On 14 December 2012 13:01, Paul Berry <stereotype441 at gmail.com> wrote:

> From the GL 4.3 spec, section 6.7.1 (Indexed Buffer Object Limits and
> Binding Queries):
>
>     To query the starting offset or size of the range of a buffer
>     object binding in an indexed array, call GetInteger64i v with
>     target set to respectively the starting offset or binding size
>     name from table 6.5 for that array. index must be in the range
>     zero to the number of bind points supported minus one. If the
>     starting offset or size was not specified when the buffer object
>     was bound (e.g. if it was bound with BindBufferBase), or if no
>     buffer object is bound to the target array at index, zero is
>     returned.
>
> Similar language exists in the EXT_transform_feedback spec and in the
> GLES3 spec.
>
> Previously, if a buffer object was bound by glBindBufferBase(), the
> GL_TRANSFORM_FEEDBACK_BUFFER_SIZE query returned the size of the
> buffer.
>
> Fixes GLES3 conformance tests
> transform_feedback_{builtin_type,state_variables}.test.
>

NAK this patch.  After some more careful re-reading of the GL and GLES
specs, I believe it implements the wrong behaviour.  I'm thinking in
particular of this text (the last two paragraphs of section 2.9.1 of the
GLES 3 spec):

"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 2.14.2."

In the context of transform feedback, I interpret the phrase "at the time
the binding is used" to mean "at the time BeginTransformFeedback is
called."  Mesa's current behaviour isn't consistent with the above two
paragraphs: it records the size of the buffer at the time BindBufferBase is
called, which means that if the buffer is grown after it's bound (but
before it's used), the entire buffer won't be used.  Worse yet, in the
absence of extra checking by the driver, if the buffer is shrunk after it's
bound, there's a danger of transform feedback results overflowing past the
end of the buffer.

I'm working on a replacement patch.


> ---
>  src/mesa/main/get.c               |  2 +-
>  src/mesa/main/mtypes.h            |  7 +++++++
>  src/mesa/main/transformfeedback.c | 10 ++++++----
>  3 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c
> index f3dbda2..2348486 100644
> --- a/src/mesa/main/get.c
> +++ b/src/mesa/main/get.c
> @@ -1574,7 +1574,7 @@ 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->QuerySize[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..333b2c9 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -1827,6 +1827,13 @@ struct gl_transform_feedback_object
>     GLintptr Offset[MAX_FEEDBACK_BUFFERS];
>     /** Max data to put into dest buffer (in bytes) */
>     GLsizeiptr Size[MAX_FEEDBACK_BUFFERS];
> +   /**
> +    * Size that should be returned from GL_TRANSFORM_FEEDBACK_BUFFER_SIZE
> +    * queries.  If the transform feedback buffer was set up with
> +    * glBindBufferBase() or glBindBufferOffsetEXT().  If it was set up
> with
> +    * glBindBufferRange() it is equal to ActualSize.
> +    */
> +   GLsizeiptr QuerySize[MAX_FEEDBACK_BUFFERS];
>  };
>
>
> diff --git a/src/mesa/main/transformfeedback.c
> b/src/mesa/main/transformfeedback.c
> index 7d500bc..092c343 100644
> --- a/src/mesa/main/transformfeedback.c
> +++ b/src/mesa/main/transformfeedback.c
> @@ -383,7 +383,8 @@ _mesa_EndTransformFeedback(void)
>  static void
>  bind_buffer_range(struct gl_context *ctx, GLuint index,
>                    struct gl_buffer_object *bufObj,
> -                  GLintptr offset, GLsizeiptr size)
> +                  GLintptr offset, GLsizeiptr size,
> +                  GLsizeiptr query_size)
>  {
>     struct gl_transform_feedback_object *obj =
>        ctx->TransformFeedback.CurrentObject;
> @@ -407,6 +408,7 @@ bind_buffer_range(struct gl_context *ctx, GLuint index,
>
>     obj->Offset[index] = offset;
>     obj->Size[index] = size;
> +   obj->QuerySize[index] = query_size;
>  }
>
>
> @@ -450,7 +452,7 @@ _mesa_bind_buffer_range_transform_feedback(struct
> gl_context *ctx,
>        return;
>     }
>
> -   bind_buffer_range(ctx, index, bufObj, offset, size);
> +   bind_buffer_range(ctx, index, bufObj, offset, size, size);
>  }
>
>
> @@ -485,7 +487,7 @@ _mesa_bind_buffer_base_transform_feedback(struct
> gl_context *ctx,
>      */
>     size = bufObj->Size & ~0x3;
>
> -   bind_buffer_range(ctx, index, bufObj, 0, size);
> +   bind_buffer_range(ctx, index, bufObj, 0, size, 0);
>  }
>
>
> @@ -546,7 +548,7 @@ _mesa_BindBufferOffsetEXT(GLenum target, GLuint index,
> GLuint buffer,
>      */
>     size = (bufObj->Size - offset) & ~0x3;
>
> -   bind_buffer_range(ctx, index, bufObj, offset, size);
> +   bind_buffer_range(ctx, index, bufObj, offset, size, 0);
>  }
>
>
> --
> 1.8.0.2
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20121215/07369cc2/attachment.html>