[Mesa-dev] [PATCH 5/5] mesa: Fix GL_TRANSFORM_FEEDBACK_BUFFER_SIZE query when glBindBufferBase used.
Ian Romanick
idr at freedesktop.org
Sat Dec 15 14:56:02 PST 2012
On 12/15/2012 01:27 PM, Paul Berry wrote:
> On 14 December 2012 13:01, Paul Berry <stereotype441 at gmail.com
> <mailto: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 think your new interpretation is correct. Can we come up with a
piglit test that would expose the buffer-growth-after-bind bug? I'm
curious to see what other drivers do...
> 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
>
>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list