[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