On 15 December 2012 14:56, Ian Romanick <i...@freedesktop.org> wrote:
> On 12/15/2012 01:27 PM, Paul Berry wrote: > >> On 14 December 2012 13:01, Paul Berry <stereotype...@gmail.com >> <mailto:stereotype441@gmail.**com <stereotype...@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@lists.freedesktop.org >> http://lists.freedesktop.org/**mailman/listinfo/mesa-dev<http://lists.freedesktop.org/mailman/listinfo/mesa-dev> >> > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
