[Mesa-dev] [PATCH 4/4] mesa: tidy up left over APPLE_vertex_array_object semantics
Timothy Arceri
tarceri at itsqueeze.com
Tue Apr 25 00:13:58 UTC 2017
On 25/04/17 01:30, Nicolai Hähnle wrote:
> On 24.04.2017 12:28, Timothy Arceri wrote:
>>
>>
>> On 24/04/17 20:13, Nicolai Hähnle wrote:
>>> On 24.04.2017 07:28, Timothy Arceri wrote:
>>>> ---
>>>> src/mesa/main/arrayobj.c | 11 +----------
>>>> src/mesa/main/attrib.c | 23 +++++++----------------
>>>> src/mesa/main/mtypes.h | 16 ----------------
>>>> src/mesa/main/varray.c | 2 +-
>>>> 4 files changed, 9 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/src/mesa/main/arrayobj.c b/src/mesa/main/arrayobj.c
>>>> index c52a07b..82c00fb 100644
>>>> --- a/src/mesa/main/arrayobj.c
>>>> +++ b/src/mesa/main/arrayobj.c
>>>> @@ -443,30 +443,21 @@ _mesa_BindVertexArray( GLuint id )
>>>> }
>>>> else {
>>>> /* non-default array object */
>>>> newObj = _mesa_lookup_vao(ctx, id);
>>>> if (!newObj) {
>>>> _mesa_error(ctx, GL_INVALID_OPERATION,
>>>> "glBindVertexArray(non-gen name)");
>>>> return;
>>>> }
>>>>
>>>> - if (!newObj->EverBound) {
>>>> - /* The "Interactions with APPLE_vertex_array_object"
>>>> section of the
>>>> - * GL_ARB_vertex_array_object spec says:
>>>> - *
>>>> - * "The first bind call, either BindVertexArray or
>>>> - * BindVertexArrayAPPLE, determines the semantic of the
>>>> object."
>>>> - */
>>>> - newObj->ARBsemantics = GL_TRUE;
>>>> - newObj->EverBound = GL_TRUE;
>>>> - }
>>>> + newObj->EverBound = GL_TRUE;
>>>> }
>>>>
>>>> if (ctx->Array.DrawMethod == DRAW_ARRAYS) {
>>>> /* The _DrawArrays pointer is pointing at the VAO being
>>>> unbound and
>>>> * that VAO may be in the process of being deleted. If it's
>>>> not going
>>>> * to be deleted, this will have no effect, because the
>>>> pointer needs
>>>> * to be updated by the VBO module anyway.
>>>> *
>>>> * Before the VBO module can update the pointer, we have to
>>>> set it
>>>> * to NULL for drivers not to set up arrays which are not bound,
>>>> diff --git a/src/mesa/main/attrib.c b/src/mesa/main/attrib.c
>>>> index 87d3276..dbcfb4e 100644
>>>> --- a/src/mesa/main/attrib.c
>>>> +++ b/src/mesa/main/attrib.c
>>>> @@ -1472,23 +1472,20 @@ copy_pixelstore(struct gl_context *ctx,
>>>> static void
>>>> copy_array_object(struct gl_context *ctx,
>>>> struct gl_vertex_array_object *dest,
>>>> struct gl_vertex_array_object *src)
>>>> {
>>>> GLuint i;
>>>>
>>>> /* skip Name */
>>>> /* skip RefCount */
>>>>
>>>> - /* In theory must be the same anyway, but on recreate make sure
>>>> it matches */
>>>> - dest->ARBsemantics = src->ARBsemantics;
>>>> -
>>>> for (i = 0; i < ARRAY_SIZE(src->VertexAttrib); i++) {
>>>> _mesa_copy_client_array(ctx, &dest->_VertexAttrib[i],
>>>> &src->_VertexAttrib[i]);
>>>> _mesa_copy_vertex_attrib_array(ctx, &dest->VertexAttrib[i],
>>>> &src->VertexAttrib[i]);
>>>> _mesa_copy_vertex_buffer_binding(ctx, &dest->BufferBinding[i],
>>>> &src->BufferBinding[i]);
>>>> }
>>>>
>>>> /* _Enabled must be the same than on push */
>>>> dest->_Enabled = src->_Enabled;
>>>> /* The bitmask of bound VBOs needs to match the VertexBinding
>>>> array */
>>>> dest->VertexAttribBufferMask = src->VertexAttribBufferMask;
>>>> @@ -1550,56 +1547,50 @@ save_array_attrib(struct gl_context *ctx,
>>>> }
>>>>
>>>> /**
>>>> * Restore the content of src to dest.
>>>> */
>>>> static void
>>>> restore_array_attrib(struct gl_context *ctx,
>>>> struct gl_array_attrib *dest,
>>>> struct gl_array_attrib *src)
>>>> {
>>>> + bool is_vao_name_zero = src->VAO->Name == 0;
>>>> +
>>>> /* The ARB_vertex_array_object spec says:
>>>> *
>>>> * "BindVertexArray fails and an INVALID_OPERATION error is
>>>> generated
>>>> * if array is not a name returned from a previous call to
>>>> * GenVertexArrays, or if such a name has since been deleted
>>>> with
>>>> * DeleteVertexArrays."
>>>> *
>>>> * Therefore popping a deleted VAO cannot magically recreate it.
>>>> - *
>>>> - * The semantics of objects created using
>>>> APPLE_vertex_array_objects behave
>>>> - * differently. These objects expect to be recreated by pop.
>>>> Alas.
>>>> */
>>>> - const bool arb_vao = (src->VAO->Name != 0
>>>> - && src->VAO->ARBsemantics);
>>>> -
>>>> - if (arb_vao && !_mesa_IsVertexArray(src->VAO->Name))
>>>> + if (!is_vao_name_zero && !_mesa_IsVertexArray(src->VAO->Name))
>>>> return;
>>>>
>>>> _mesa_BindVertexArray(src->VAO->Name);
>>>>
>>>> /* Restore or recreate the buffer objects by the names ... */
>>>> - if (!arb_vao
>>>> - || src->ArrayBufferObj->Name == 0
>>>> - || _mesa_IsBuffer(src->ArrayBufferObj->Name)) {
>>>> + if (is_vao_name_zero || src->ArrayBufferObj->Name == 0 ||
>>>> + _mesa_IsBuffer(src->ArrayBufferObj->Name)) {
>>>> /* ... and restore its content */
>>>> copy_array_attrib(ctx, dest, src, false);
>>>>
>>>> _mesa_BindBuffer(GL_ARRAY_BUFFER_ARB,
>>>> src->ArrayBufferObj->Name);
>>>> } else {
>>>> copy_array_attrib(ctx, dest, src, true);
>>>> }
>>>>
>>>> - if (!arb_vao
>>>> - || src->VAO->IndexBufferObj->Name == 0
>>>> - || _mesa_IsBuffer(src->VAO->IndexBufferObj->Name))
>>>> + if (is_vao_name_zero || src->VAO->IndexBufferObj->Name == 0 ||
>>>> + _mesa_IsBuffer(src->VAO->IndexBufferObj->Name))
>>>> _mesa_BindBuffer(GL_ELEMENT_ARRAY_BUFFER_ARB,
>>>> src->VAO->IndexBufferObj->Name);
>>>> }
>>>>
>>>> /**
>>>> * init/alloc the fields of 'attrib'.
>>>> * Needs to the init part matching free_array_attrib_data below.
>>>> */
>>>> static bool
>>>> init_array_attrib_data(struct gl_context *ctx,
>>>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>>>> index 2e3a423..002e692 100644
>>>> --- a/src/mesa/main/mtypes.h
>>>> +++ b/src/mesa/main/mtypes.h
>>>> @@ -1502,36 +1502,20 @@ struct gl_vertex_buffer_binding
>>>> struct gl_vertex_array_object
>>>> {
>>>> /** Name of the VAO as received from glGenVertexArray. */
>>>> GLuint Name;
>>>>
>>>> GLint RefCount;
>>>>
>>>> GLchar *Label; /**< GL_KHR_debug */
>>>>
>>>> /**
>>>> - * Does the VAO use ARB semantics or Apple semantics?
>>>> - *
>>>> - * There are several ways in which ARB_vertex_array_object and
>>>> - * APPLE_vertex_array_object VAOs have differing semantics. At
>>>> the very
>>>> - * least,
>>>> - *
>>>> - * - ARB VAOs require that all array data be sourced from
>>>> vertex buffer
>>>> - * objects, but Apple VAOs do not.
>>>> - *
>>>> - * - ARB VAOs require that names come from GenVertexArrays.
>>>> - *
>>>> - * This flag notes which behavior governs this VAO.
>>>> - */
>>>> - GLboolean ARBsemantics;
>>>> -
>>>> - /**
>>>> * Has this array object been bound?
>>>> */
>>>> GLboolean EverBound;
>>>>
>>>> /**
>>>> * Derived vertex attribute arrays
>>>> *
>>>> * This is a legacy data structure created from
>>>> gl_vertex_attrib_array and
>>>> * gl_vertex_buffer_binding, for compatibility with existing
>>>> driver code.
>>>> */
>>>> diff --git a/src/mesa/main/varray.c b/src/mesa/main/varray.c
>>>> index 78dc004..2fb3c18 100644
>>>> --- a/src/mesa/main/varray.c
>>>> +++ b/src/mesa/main/varray.c
>>>> @@ -502,21 +502,21 @@ validate_array(struct gl_context *ctx, const
>>>> char *func,
>>>> * "An INVALID_OPERATION error is generated under any of the
>>>> following
>>>> * conditions:
>>>> *
>>>> * ...
>>>> *
>>>> * * any of the *Pointer commands specifying the location and
>>>> * organization of vertex array data are called while zero
>>>> is bound
>>>> * to the ARRAY_BUFFER buffer object binding point (see
>>>> section
>>>> * 2.9.6), and the pointer argument is not NULL."
>>>> */
>>>> - if (ptr != NULL && vao->ARBsemantics &&
>>>> + if (ptr != NULL && vao->EverBound &&
>>>
>>> The EverBound is an extraneous change and should be unnecessary. With
>>> that fixed, patches 2-4 are
>>
>> Actually many many piglit/cts tests end up hitting this error without
>> adding EverBound here.
>
> Okay, I think I understand now. The point is that those tests use a
> compatibility profile with user array pointers and no VAO.
>
> Perhaps the test could be
>
> if (ptr != NULL && vao != ctx->Array.DefaultVAO &&
> !_mesa_is_bufferobj(ctx->Array.ArrayBufferObj))
>
> I think that should work, and it's a bit more explicit about what the
> check means.
Yeah that seems to work. Thanks.
More information about the mesa-dev
mailing list