[Mesa-dev] [PATCH 5/5] mesa: Fix GL_TRANSFORM_FEEDBACK_BUFFER_SIZE query when glBindBufferBase used.
Paul Berry
stereotype441 at gmail.com
Sat Dec 15 19:53:03 PST 2012
On 15 December 2012 14:56, Ian Romanick <idr at freedesktop.org> wrote:
> 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 <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...
>
Yeah, that should be easy enough. We ought to be able to use a
PRIMITIVES_WRITTEN query to detect if the driver tried to write past the
end of the buffer. I'll add it to my list of things to do Monday.
>
> 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<http://lists.freedesktop.org/mailman/listinfo/mesa-dev>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20121215/ecdfdd79/attachment.html>
More information about the mesa-dev
mailing list