[Mesa-dev] [RFC][PATCH] vbo: do not restore all the arrayobj stuff if the bufobj is deleted
Yuanhan Liu
yuanhan.liu at linux.intel.com
Thu Sep 29 22:41:46 PDT 2011
On Thu, Sep 29, 2011 at 09:58:18AM -0600, Brian Paul wrote:
> On 09/29/2011 03:43 AM, Yuanhan Liu wrote:
> >I hope I can find something from OpenGL spec to support this. Badly, I
> >didn't make it with a simply searching.
> >
> >Basically, it's an issue that should we restore all the arrayobj stuff
> >if the bufobj is deleted? Say, in a following case:
> >
> > glGenBuffersARB(2, bufname);
> >
> > glBindBufferARB(GL_ARRAY_BUFFER_ARB, bufname[1]);
> >
> > glBindBufferARB(GL_ARRAY_BUFFER_ARB, bufname[0]);
> > glVertexPointer(2, GL_INT, 0, NULL);
> >
> > glPushClientAttrib(GL_CLIENT_VERTEX_ARRAY_BIT);
> > glDeleteBuffersARB(1, bufname); /* This would delete bufname[0] */
> > glPopClientAttrib();
> >
> >Then what's the expected value of the following two queries:
> > glGetIntegerv(GL_ARRAY_BUFFER_BINDING_ARB,&val1);
> > glGetIntegerv(GL_VERTEX_ARRAY_BUFFER_BINDING_ARB,&val2);
> >
> >The old code would make val1 and val2 both to be bufname[0]. Well, this
> >patch keep the val1 but let the val2 to be 0.
> >
> >I know this patch is ugly, but I'd heard about your comments?
>
> Buffer objects are reference counted so the glDeleteBuffersARB()
> call wouldn't really delete the buffer since it should be referenced
> by something saved by glPushClientAttrib().
>
> Looking at attrib.c, we're fudging the reference counts but it's not
> handling the case of deleting the buffer object between
> glPushClientAttrib() and the Pop() because while we're decrementing
> the refcounts in Pop(), we're not freeing the arrays if the refcount
> hits zero.
Sorry, I didn't get it. The Push() would reference the BufferObj, while
the Pop() would also reference the BufferObj. Then how could the RefCount
of the BufferObj be 0?
>
> I'm attaching the beginnings of a patch to do the reference counting
> properly. Maybe you can take this and do the rest?
I still didn't see the code to dereference the BufferObj at Pop() in
your attached patch. I guess I misunderstood your points somewhere.
Anyway, I'm attaching the patch based on yours. And, as I said above,
it didn't fix this issues.
>
> I'm also attaching a simple piglit test. It passes with NVIDIA's
> driver but fails with Mesa.
About the pass, intel oglc would prefer val1 to be bufname[0] while val2
to be 0. So, which one is the right one?
Thanks,
Yuanhan Liu
>
> -Brian
> diff --git a/src/mesa/main/attrib.c b/src/mesa/main/attrib.c
> index 2f391c5..47190d2 100644
> --- a/src/mesa/main/attrib.c
> +++ b/src/mesa/main/attrib.c
> @@ -1395,24 +1395,31 @@ _mesa_PushClientAttrib(GLbitfield mask)
> struct gl_array_attrib *attr;
> struct gl_array_object *obj;
>
> - attr = MALLOC_STRUCT( gl_array_attrib );
> - obj = MALLOC_STRUCT( gl_array_object );
> -
> -#if FEATURE_ARB_vertex_buffer_object
> - /* increment ref counts since we're copying pointers to these objects */
> - ctx->Array.ArrayBufferObj->RefCount++;
> - ctx->Array.ElementArrayBufferObj->RefCount++;
> -#endif
> -
> - memcpy( attr, &ctx->Array, sizeof(struct gl_array_attrib) );
> - memcpy( obj, ctx->Array.ArrayObj, sizeof(struct gl_array_object) );
> -
> - attr->ArrayObj = obj;
> + attr = CALLOC_STRUCT( gl_array_attrib );
> + obj = CALLOC_STRUCT( gl_array_object );
> +
> + /* Copy gl_array_attrib fields */
> + _mesa_reference_array_object(ctx, &attr->ArrayObj, ctx->Array.ArrayObj);
> + /* skip DefaultArrayObj, Objects */
> + attr->ActiveTexture = ctx->Array.ActiveTexture;
> + attr->LockFirst = ctx->Array.LockFirst;
> + attr->LockCount = ctx->Array.LockCount;
> + attr->PrimitiveRestart = ctx->Array.PrimitiveRestart;
> + /* XXX more fields */
> + _mesa_reference_buffer_object(ctx, &attr->ArrayBufferObj,
> + ctx->Array.ArrayBufferObj);
> + _mesa_reference_buffer_object(ctx, &attr->ElementArrayBufferObj,
> + ctx->Array.ElementArrayBufferObj);
> +
> + /* Copy gl_array_object fields */
> + obj->VBOonly = ctx->Array.ArrayObj->VBOonly;
> + _mesa_copy_client_array(ctx, &obj->Vertex, &ctx->Array.ArrayObj->Vertex);
> + _mesa_copy_client_array(ctx, &obj->Normal, &ctx->Array.ArrayObj->Normal);
> + /* XXX copy more fields */
> +
> + _mesa_reference_array_object(ctx, &attr->ArrayObj, obj);
>
> save_attrib_data(&head, GL_CLIENT_VERTEX_ARRAY_BIT, attr);
> -
> - /* bump reference counts on buffer objects */
> - adjust_buffer_object_ref_counts(ctx->Array.ArrayObj, 1);
> }
>
> ctx->ClientAttribStack[ctx->ClientAttribStackDepth] = head;
> @@ -1462,6 +1469,10 @@ _mesa_PopClientAttrib(void)
> struct gl_array_attrib * data =
> (struct gl_array_attrib *) node->data;
>
> + /* XXX rewrite this code to call _mesa_reference_array_object(),
> + _mesa_reference_buffer_object(), etc.
> + */
> +
> adjust_buffer_object_ref_counts(ctx->Array.ArrayObj, -1);
>
> ctx->Array.ActiveTexture = data->ActiveTexture;
[-- Error: could not find beginning of PGP message! --]
>
> #include "piglit-util.h"
>
> int piglit_width = 600, piglit_height = 200;
> int piglit_window_mode = GLUT_RGBA | GLUT_ALPHA | GLUT_DOUBLE;
>
>
> enum piglit_result
> piglit_display(void)
> {
> GLuint bufname[2];
> GLint val1, val2;
>
> glGenBuffersARB(2, bufname);
>
> glBindBufferARB(GL_ARRAY_BUFFER_ARB, bufname[1]);
>
> glBindBufferARB(GL_ARRAY_BUFFER_ARB, bufname[0]);
> glVertexPointer(2, GL_INT, 0, NULL);
>
> glPushClientAttrib(GL_CLIENT_VERTEX_ARRAY_BIT);
> glDeleteBuffersARB(1, bufname); /* This would delete bufname[0] */
> glPopClientAttrib();
>
> glGetIntegerv(GL_ARRAY_BUFFER_BINDING_ARB, &val1);
> glGetIntegerv(GL_VERTEX_ARRAY_BUFFER_BINDING_ARB, &val2);
>
> /*printf("val1 %d val2 %d\n", val1, val2);*/
>
> if (val1 == 0 && val2 == 0)
> return PIGLIT_PASS;
> else
> return PIGLIT_FAIL;
> }
>
>
> void
> piglit_init(int argc, char **argv)
> {
> /* nop */
> }
-------------- next part --------------
diff --git a/src/mesa/main/attrib.c b/src/mesa/main/attrib.c
index 2f391c5..dd1ef54 100644
--- a/src/mesa/main/attrib.c
+++ b/src/mesa/main/attrib.c
@@ -1366,6 +1366,7 @@ _mesa_PushClientAttrib(GLbitfield mask)
{
struct gl_attrib_node *head;
+
GET_CURRENT_CONTEXT(ctx);
ASSERT_OUTSIDE_BEGIN_END(ctx);
@@ -1394,25 +1395,53 @@ _mesa_PushClientAttrib(GLbitfield mask)
if (mask & GL_CLIENT_VERTEX_ARRAY_BIT) {
struct gl_array_attrib *attr;
struct gl_array_object *obj;
+ int i;
+
+ attr = CALLOC_STRUCT( gl_array_attrib );
+ obj = CALLOC_STRUCT( gl_array_object );
+
+ /* Copy gl_array_attrib fields */
+ _mesa_reference_array_object(ctx, &attr->ArrayObj, ctx->Array.ArrayObj);
+ /* skip DefaultArrayObj, Objects */
+ attr->ActiveTexture = ctx->Array.ActiveTexture;
+ attr->LockFirst = ctx->Array.LockFirst;
+ attr->LockCount = ctx->Array.LockCount;
+ attr->PrimitiveRestart = ctx->Array.PrimitiveRestart;
+ attr->RestartIndex = ctx->Array.RestartIndex;
+ attr->NewState = ctx->Array.NewState;
+ attr->RebindArrays = ctx->Array.RebindArrays;
+
+ _mesa_reference_buffer_object(ctx, &attr->ArrayBufferObj,
+ ctx->Array.ArrayBufferObj);
+ _mesa_reference_buffer_object(ctx, &attr->ElementArrayBufferObj,
+ ctx->Array.ElementArrayBufferObj);
+
+ /* Copy gl_array_object fields */
+ obj->VBOonly = ctx->Array.ArrayObj->VBOonly;
+ obj->_Enabled = ctx->Array.ArrayObj->_Enabled;
+ obj->_MaxElement = ctx->Array.ArrayObj->_MaxElement;
+ obj->RefCount = ctx->Array.ArrayObj->RefCount;
+
+ _mesa_copy_client_array(ctx, &obj->Vertex, &ctx->Array.ArrayObj->Vertex);
+ _mesa_copy_client_array(ctx, &obj->Weight, &ctx->Array.ArrayObj->Weight);
+ _mesa_copy_client_array(ctx, &obj->Normal, &ctx->Array.ArrayObj->Normal);
+ _mesa_copy_client_array(ctx, &obj->Color, &ctx->Array.ArrayObj->Color);
+ _mesa_copy_client_array(ctx, &obj->SecondaryColor, &ctx->Array.ArrayObj->SecondaryColor);
+ _mesa_copy_client_array(ctx, &obj->FogCoord, &ctx->Array.ArrayObj->FogCoord);
+ _mesa_copy_client_array(ctx, &obj->Index, &ctx->Array.ArrayObj->Index);
+ _mesa_copy_client_array(ctx, &obj->EdgeFlag, &ctx->Array.ArrayObj->EdgeFlag);
+ _mesa_copy_client_array(ctx, &obj->PointSize, &ctx->Array.ArrayObj->PointSize);
+
+ for (i = 0; i < Elements(ctx->Array.ArrayObj->TexCoord); i++) {
+ _mesa_copy_client_array(ctx, &obj->TexCoord[i], &ctx->Array.ArrayObj->TexCoord[i]);
+ }
+ for (i = 0; i < Elements(ctx->Array.ArrayObj->VertexAttrib); i++) {
+ _mesa_copy_client_array(ctx, &obj->VertexAttrib[i], &ctx->Array.ArrayObj->VertexAttrib[i]);
+ }
- attr = MALLOC_STRUCT( gl_array_attrib );
- obj = MALLOC_STRUCT( gl_array_object );
-
-#if FEATURE_ARB_vertex_buffer_object
- /* increment ref counts since we're copying pointers to these objects */
- ctx->Array.ArrayBufferObj->RefCount++;
- ctx->Array.ElementArrayBufferObj->RefCount++;
-#endif
-
- memcpy( attr, &ctx->Array, sizeof(struct gl_array_attrib) );
- memcpy( obj, ctx->Array.ArrayObj, sizeof(struct gl_array_object) );
-
- attr->ArrayObj = obj;
+ _mesa_reference_array_object(ctx, &attr->ArrayObj, obj);
save_attrib_data(&head, GL_CLIENT_VERTEX_ARRAY_BIT, attr);
-
- /* bump reference counts on buffer objects */
- adjust_buffer_object_ref_counts(ctx->Array.ArrayObj, 1);
}
ctx->ClientAttribStack[ctx->ClientAttribStackDepth] = head;
@@ -1461,35 +1490,44 @@ _mesa_PopClientAttrib(void)
case GL_CLIENT_VERTEX_ARRAY_BIT: {
struct gl_array_attrib * data =
(struct gl_array_attrib *) node->data;
+ int i;
+
+ _mesa_reference_buffer_object(ctx, &ctx->Array.ArrayBufferObj, data->ArrayBufferObj);
+ _mesa_reference_buffer_object(ctx, &ctx->Array.ElementArrayBufferObj, data->ElementArrayBufferObj);
+
+ _mesa_copy_client_array(ctx, &ctx->Array.ArrayObj->Vertex, &data->ArrayObj->Vertex);
+ _mesa_copy_client_array(ctx, &ctx->Array.ArrayObj->Weight, &data->ArrayObj->Weight);
+ _mesa_copy_client_array(ctx, &ctx->Array.ArrayObj->Normal, &data->ArrayObj->Normal);
+ _mesa_copy_client_array(ctx, &ctx->Array.ArrayObj->Color, &data->ArrayObj->Color);
+ _mesa_copy_client_array(ctx, &ctx->Array.ArrayObj->SecondaryColor, &data->ArrayObj->SecondaryColor);
+ _mesa_copy_client_array(ctx, &ctx->Array.ArrayObj->FogCoord, &data->ArrayObj->FogCoord);
+ _mesa_copy_client_array(ctx, &ctx->Array.ArrayObj->Index, &data->ArrayObj->Index);
+ _mesa_copy_client_array(ctx, &ctx->Array.ArrayObj->EdgeFlag, &data->ArrayObj->EdgeFlag);
+ _mesa_copy_client_array(ctx, &ctx->Array.ArrayObj->PointSize, &data->ArrayObj->PointSize);
+ for (i = 0; i < Elements(ctx->Array.ArrayObj->TexCoord); i++) {
+ _mesa_copy_client_array(ctx, &ctx->Array.ArrayObj->TexCoord[i], &data->ArrayObj->TexCoord[i]);
+ }
+ for (i = 0; i < Elements(ctx->Array.ArrayObj->VertexAttrib); i++) {
+ _mesa_copy_client_array(ctx, &ctx->Array.ArrayObj->VertexAttrib[i], &data->ArrayObj->VertexAttrib[i]);
+ }
+
+ _mesa_reference_array_object(ctx, &ctx->Array.ArrayObj, data->ArrayObj);
+
- adjust_buffer_object_ref_counts(ctx->Array.ArrayObj, -1);
-
ctx->Array.ActiveTexture = data->ActiveTexture;
if (data->LockCount != 0)
_mesa_LockArraysEXT(data->LockFirst, data->LockCount);
else if (ctx->Array.LockCount)
_mesa_UnlockArraysEXT();
-
- _mesa_BindVertexArrayAPPLE( data->ArrayObj->Name );
-#if FEATURE_ARB_vertex_buffer_object
- _mesa_BindBufferARB(GL_ARRAY_BUFFER_ARB,
- data->ArrayBufferObj->Name);
- _mesa_BindBufferARB(GL_ELEMENT_ARRAY_BUFFER_ARB,
- data->ElementArrayBufferObj->Name);
-#endif
-
- memcpy( ctx->Array.ArrayObj, data->ArrayObj,
- sizeof( struct gl_array_object ) );
+ ctx->Array.PrimitiveRestart = data->PrimitiveRestart;
+ ctx->Array.NewState = data->NewState;
+ ctx->Array.RebindArrays = data->RebindArrays;
- FREE( data->ArrayObj );
-
- /* FIXME: Should some bits in ctx->Array->NewState also be set
- * FIXME: here? It seems like it should be set to inclusive-or
- * FIXME: of the old ArrayObj->_Enabled and the new _Enabled.
- */
+ ctx->Array.ArrayObj->VBOonly = data->ArrayObj->VBOonly;
+ ctx->Array.ArrayObj->_MaxElement = data->ArrayObj->_MaxElement;
+ ctx->Array.ArrayObj->_Enabled = data->ArrayObj->_Enabled;
- ctx->NewState |= _NEW_ARRAY;
break;
}
default:
More information about the mesa-dev
mailing list