fix a race condition in test_buffer.py

Original commit message from CVS:
fix a race condition in test_buffer.py


* gst/gst.override:
* gst/gstmodule.c: (init_gst):
add a pygst debug category for bindings themselves to use
* gst/gstbuffer.override:
add a repr method; add some assertions
* gst/pygstminiobject.c: (pygst_miniobject_init),
(pygstminiobject_register_wrapper), (pygstminiobject_new),
(pygstminiobject_new_noref), (pygstminiobject_dealloc),
(pygstminiobject_clear):
make the miniobjs hash private with an underscore
add debugging for inserting/removal in hash
fix pygstminiobject_clear - it also needs to remove
from the global hash.  Fixes a nasty race problem in
test_buffer
* testsuite/test_buffer.py:
expand on the subbuffer test

6 files changed, 172 insertions, 79 deletions

diff --git a/ChangeLog b/ChangeLog
index b9b1ddb..d8822ae 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,22 @@
+2005-09-01  Thomas Vander Stichele  <thomas at apestaart dot org>
+
+	* gst/gst.override:
+	* gst/gstmodule.c: (init_gst):
+	  add a pygst debug category for bindings themselves to use
+	* gst/gstbuffer.override:
+	  add a repr method; add some assertions
+	* gst/pygstminiobject.c: (pygst_miniobject_init),
+	(pygstminiobject_register_wrapper), (pygstminiobject_new),
+	(pygstminiobject_new_noref), (pygstminiobject_dealloc),
+	(pygstminiobject_clear):
+	  make the miniobjs hash private with an underscore
+	  add debugging for inserting/removal in hash
+	  fix pygstminiobject_clear - it also needs to remove
+	  from the global hash.  Fixes a nasty race problem in
+	  test_buffer
+	* testsuite/test_buffer.py:
+	  expand on the subbuffer test
+
 2005-09-01  Andy Wingo  <wingo@pobox.com>
 
 	* examples/Makefile.am (examples_DATA): Dist fixer.
diff --git a/gst/gst.override b/gst/gst.override
index 8d09fd0..5f57c51 100644
--- a/gst/gst.override
+++ b/gst/gst.override
@@ -53,8 +53,8 @@ headers
 
 PyObject *PyGstExc_LinkError = NULL;
 
-GST_DEBUG_CATEGORY_EXTERN (gst_python);
-#define GST_CAT_DEFAULT gst_python
+GST_DEBUG_CATEGORY_EXTERN (python_debug);
+#define GST_CAT_DEFAULT python_debug
 
 GSList *mainloops = NULL;
 void
diff --git a/gst/gstbuffer.override b/gst/gstbuffer.override
index e452520..f58797e 100644
--- a/gst/gstbuffer.override
+++ b/gst/gstbuffer.override
@@ -131,16 +131,56 @@ _wrap_gst_buffer_set_data(PyObject *self, PyObject *args, PyObject *kwargs)
 	Py_INCREF(Py_None);
 	return Py_None;
 }
+
 %%
 override-slot GstBuffer.tp_str
 static PyObject *
-_wrap_gst_buffer_tp_str(PyGstMiniObject *self)
+_wrap_gst_buffer_tp_str (PyGstMiniObject *self)
 {
-	GstBuffer *buf = pyg_boxed_get(self, GstBuffer);
+	GstBuffer *buf;
+
+	g_assert (self);
+	buf = pyg_boxed_get (self, GstBuffer);
+	g_assert (buf);
 
 	return PyString_FromStringAndSize((const gchar*) GST_BUFFER_DATA(buf),
 					  (gint) GST_BUFFER_SIZE(buf));
 }
+
+%%
+override-slot GstBuffer.tp_repr
+static PyObject *
+_wrap_gst_buffer_tp_repr (PyGstMiniObject *self)
+{
+    GstBuffer *buf;
+    guchar *data;
+    gchar *repr;
+    gint size = 0;
+    PyObject *ret;
+
+    g_assert (self);
+    buf = pyg_boxed_get (self, GstBuffer);
+    g_assert (buf);
+
+    size = GST_BUFFER_SIZE (buf);
+    
+    if (size == 0) {
+        repr = g_strdup_printf ("<gst.Buffer %p of size %d>", buf, size);
+    } else {
+        data = GST_BUFFER_DATA (buf);
+        repr = g_strdup_printf (
+            "<gst.Buffer %p of size %d and data 0x%02x%02x%02x%02x>", buf, size,
+            *data,
+            size > 0 ? *(data + 1) : 0,
+            size > 1 ? *(data + 2) : 0,
+            size > 2 ? *(data + 3) : 0
+            );
+    }
+    ret = PyString_FromStringAndSize(repr, strlen (repr));
+    g_free (repr);
+    return ret;
+}
+
 %%
 override-slot GstBuffer.tp_as_buffer
 static PyBufferProcs _wrap_gst_buffer_tp_as_buffer = {
@@ -367,6 +407,7 @@ _wrap_gst_buffer_flag_unset(PyObject *self, PyObject *args)
 	Py_INCREF(Py_None);
 	return Py_None;
 }
+
 %%
 override-attr GstBuffer.timestamp
 static PyObject *
@@ -417,57 +458,73 @@ _wrap_gst_buffer__set_duration(PyGstMiniObject *self, PyObject *value, void *clo
     GST_BUFFER(self->obj)->duration = val;
     return 0;
 }
+
 %%
 override-attr GstBuffer.offset
 static PyObject *
-_wrap_gst_buffer__get_offset(PyObject *self, void *closure)
+_wrap_gst_buffer__get_offset (PyObject *self, void *closure)
 {
     guint64 ret;
+    GstMiniObject *miniobject;
+    g_assert (self);
 
-    ret = GST_BUFFER(pygstminiobject_get(self))->offset;
-    return PyLong_FromUnsignedLongLong(ret);
+    miniobject = pygstminiobject_get (self);
+    g_assert (miniobject);
+
+    ret = GST_BUFFER_OFFSET (GST_BUFFER (miniobject));
+    return PyLong_FromUnsignedLongLong (ret);
 }
+
 static int
-_wrap_gst_buffer__set_offset(PyGstMiniObject *self, PyObject *value, void *closure)
+_wrap_gst_buffer__set_offset (PyGstMiniObject *self, PyObject *value, void *closure)
 {
     guint64 val;
+    g_assert (self);
 
-    if (PyInt_CheckExact(value))
-      val = PyInt_AsUnsignedLongLongMask(value);
+    if (PyInt_CheckExact (value))
+      val = PyInt_AsUnsignedLongLongMask (value);
     else
-      val = PyLong_AsUnsignedLongLong(value);
+      val = PyLong_AsUnsignedLongLong (value);
     if (PyErr_Occurred())
         return -1;
     
-    GST_BUFFER(self->obj)->offset = val;
+    GST_BUFFER_OFFSET (GST_BUFFER (self->obj)) = val;
     return 0;
 }
+
 %%
 override-attr GstBuffer.offset_end
 static PyObject *
-_wrap_gst_buffer__get_offset_end(PyObject *self, void *closure)
+_wrap_gst_buffer__get_offset_end (PyObject *self, void *closure)
 {
     guint64 ret;
+    GstMiniObject *miniobject;
+    g_assert (self);
+
+    miniobject = pygstminiobject_get (self);
+    g_assert (miniobject);
     
-    ret = GST_BUFFER(pygstminiobject_get(self))->offset_end;
-    return PyLong_FromUnsignedLongLong(ret);
+    ret = GST_BUFFER_OFFSET_END (GST_BUFFER (miniobject));
+    return PyLong_FromUnsignedLongLong (ret);
 }
 
 static int
-_wrap_gst_buffer__set_offset_end(PyGstMiniObject *self, PyObject *value, void *closure)
+_wrap_gst_buffer__set_offset_end (PyGstMiniObject *self, PyObject *value, void *closure)
 {
     guint64 val;
+    g_assert (self);
 
-    if (PyInt_CheckExact(value))
-      val = PyInt_AsUnsignedLongLongMask(value);
+    if (PyInt_CheckExact (value))
+      val = PyInt_AsUnsignedLongLongMask (value);
     else
-      val = PyLong_AsUnsignedLongLong(value);
-    if (PyErr_Occurred())
+      val = PyLong_AsUnsignedLongLong (value);
+    if (PyErr_Occurred ())
         return -1;
     
-    GST_BUFFER(self->obj)->offset_end = val;
+    GST_BUFFER_OFFSET_END (GST_BUFFER (self->obj)) = val;
     return 0;
 }
+
 %%
 override gst_buffer_stamp kwargs
 static PyObject *
diff --git a/gst/gstmodule.c b/gst/gstmodule.c
index 37a35d8..ee51502 100644
--- a/gst/gstmodule.c
+++ b/gst/gstmodule.c
@@ -42,7 +42,8 @@ extern GSList *mainloops;
 extern void _pygst_main_quit(void);
 extern PyObject *PyGstExc_LinkError;
 
-GST_DEBUG_CATEGORY (gst_python);
+GST_DEBUG_CATEGORY (pygst_debug);  /* for bindings code */
+GST_DEBUG_CATEGORY (python_debug); /* for python code */
 
 /* This is a timeout that gets added to the mainloop to handle SIGINT (Ctrl-C)
  * Other signals get handled at some other point where transition from
@@ -170,7 +171,8 @@ init_gst (void)
      pygst_add_constants (m, "GST_");
 
      /* Initialize debugging category */
-     GST_DEBUG_CATEGORY_INIT (gst_python, "python", 0, "GStreamer python bindings");
+     GST_DEBUG_CATEGORY_INIT (pygst_debug, "pygst", 0, "GStreamer python bindings");
+     GST_DEBUG_CATEGORY_INIT (python_debug, "python", 0, "python code using gst-python");
 
      g_timeout_add_full (0, 100, python_do_pending_calls, NULL, NULL);
      
diff --git a/gst/pygstminiobject.c b/gst/pygstminiobject.c
index 15c5433..b6bbbc0 100644
--- a/gst/pygstminiobject.c
+++ b/gst/pygstminiobject.c
@@ -31,12 +31,15 @@ static void pygstminiobject_dealloc(PyGstMiniObject *self);
 static int  pygstminiobject_traverse(PyGstMiniObject *self, visitproc visit, void *arg);
 static int  pygstminiobject_clear(PyGstMiniObject *self);
 
-static GHashTable *miniobjs;
+GST_DEBUG_CATEGORY_EXTERN (pygst_debug);
+#define GST_CAT_DEFAULT pygst_debug
+
+static GHashTable *_miniobjs;
 
 void
-pygst_miniobject_init()
+pygst_miniobject_init ()
 {
-    miniobjs = g_hash_table_new (NULL, NULL);
+    _miniobjs = g_hash_table_new (NULL, NULL);
 }
 
 /**
@@ -133,16 +136,17 @@ pygstminiobject_register_class(PyObject *dict, const gchar *type_name,
  * floating references on the Gstminiobject.
  */
 void
-pygstminiobject_register_wrapper(PyObject *self)
+pygstminiobject_register_wrapper (PyObject *self)
 {
-    GstMiniObject *obj = ((PyGstMiniObject *)self)->obj;
+    GstMiniObject *obj = ((PyGstMiniObject *) self)->obj;
     PyGILState_STATE state;
 
-    Py_INCREF(self);
-    state = pyg_gil_state_ensure();
-    g_hash_table_insert (miniobjs, (gpointer) obj, (gpointer) self);
-
-    pyg_gil_state_release(state);
+    g_assert (obj);
+    Py_INCREF (self);
+    GST_DEBUG ("inserting self %p in the table for object %p", self, obj);
+    state = pyg_gil_state_ensure ();
+    g_hash_table_insert (_miniobjs, (gpointer) obj, (gpointer) self);
+    pyg_gil_state_release (state);
 }
 
 
@@ -158,50 +162,56 @@ pygstminiobject_register_wrapper(PyObject *self)
  * Returns: a reference to the wrapper for the GstMiniObject.
  */
 PyObject *
-pygstminiobject_new(GstMiniObject *obj)
+pygstminiobject_new (GstMiniObject *obj)
 {
     PyGILState_STATE state;
-    PyGstMiniObject *self;
+    PyGstMiniObject *self = NULL;
 
     if (obj == NULL) {
-	Py_INCREF(Py_None);
+	Py_INCREF (Py_None);
 	return Py_None;
     }
-    
-    /* we already have a wrapper for this object -- return it. */
-    state = pyg_gil_state_ensure();
-    self = (PyGstMiniObject *)g_hash_table_lookup (miniobjs, (gpointer) obj);
-    pyg_gil_state_release(state);
+
+    /* see if we already have a wrapper for this object */
+    state = pyg_gil_state_ensure ();
+    self = (PyGstMiniObject *) g_hash_table_lookup (_miniobjs, (gpointer) obj);
+    pyg_gil_state_release (state);
 
     if (self != NULL) {
-	Py_INCREF(self);
+        GST_DEBUG ("had self %p in the table for object %p", self, obj);
+	/* make sure the lookup returned our object */
+        g_assert (self->obj);
+        g_assert (self->obj == obj);
+	Py_INCREF (self);
     } else {
-	/* create wrapper */
-	PyTypeObject *tp = pygstminiobject_lookup_class(G_OBJECT_TYPE(obj));
+        GST_DEBUG ("have to create wrapper for object %p", obj);
+	/* we don't, so create one */
+	PyTypeObject *tp = pygstminiobject_lookup_class (G_OBJECT_TYPE (obj));
 	if (!tp)
 	    g_warning ("Couldn't get class for type object : %p", obj);
 	/* need to bump type refcount if created with
 	   pygstminiobject_new_with_interfaces(). fixes bug #141042 */
 	if (tp->tp_flags & Py_TPFLAGS_HEAPTYPE)
-	    Py_INCREF(tp);
-	self = PyObject_GC_New(PyGstMiniObject, tp);
+	    Py_INCREF (tp);
+	self = PyObject_GC_New (PyGstMiniObject, tp);
 	if (self == NULL)
 	    return NULL;
- 	self->obj = gst_mini_object_ref(obj);
-	
+	self->obj = gst_mini_object_ref (obj);
+
 	self->inst_dict = NULL;
 	self->weakreflist = NULL;
 
-	Py_INCREF(self);
-	state = pyg_gil_state_ensure();
+	Py_INCREF (self);
 
 	/* save wrapper pointer so we can access it later */
-	g_hash_table_insert (miniobjs, (gpointer) obj, (gpointer) self);
-	pyg_gil_state_release(state);
-	
-	PyObject_GC_Track((PyObject *)self);
+        GST_DEBUG ("inserting self %p in the table for object %p", self, obj);
+	state = pyg_gil_state_ensure ();
+	g_hash_table_insert (_miniobjs, (gpointer) obj, (gpointer) self);
+	pyg_gil_state_release (state);
+
+	PyObject_GC_Track ((PyObject *)self);
     }
-    return (PyObject *)self;
+    return (PyObject *) self;
 }
 
 /**
@@ -223,7 +233,7 @@ pygstminiobject_new_noref(GstMiniObject *obj)
 	Py_INCREF(Py_None);
 	return Py_None;
     }
-    
+
     /* create wrapper */
     PyTypeObject *tp = pygstminiobject_lookup_class(G_OBJECT_TYPE(obj));
     if (!tp)
@@ -238,31 +248,28 @@ pygstminiobject_new_noref(GstMiniObject *obj)
     /* DO NOT REF !! */
     self->obj = obj;
     /*self->obj = gst_mini_object_ref(obj);*/
-    
+
     self->inst_dict = NULL;
     self->weakreflist = NULL;
     /* save wrapper pointer so we can access it later */
     Py_INCREF(self);
+
+    GST_DEBUG ("inserting self %p in the table for object %p", self, obj);
     state = pyg_gil_state_ensure();
-    
-    g_hash_table_insert (miniobjs, (gpointer) obj, (gpointer) self);
+    g_hash_table_insert (_miniobjs, (gpointer) obj, (gpointer) self);
     pyg_gil_state_release(state);
-    
+
     PyObject_GC_Track((PyObject *)self);
     return (PyObject *)self;
 }
 
-
-
 static void
 pygstminiobject_dealloc(PyGstMiniObject *self)
 {
-    GstMiniObject	*obj = NULL;
-
     g_return_if_fail (self != NULL);
 
     PyGILState_STATE state;
-    
+
     state = pyg_gil_state_ensure();
 
     PyObject_ClearWeakRefs((PyObject *)self);
@@ -270,9 +277,14 @@ pygstminiobject_dealloc(PyGstMiniObject *self)
     PyObject_GC_UnTrack((PyObject *)self);
 
     if (self->obj) {
- 	gst_mini_object_unref(self->obj);
-	obj = self->obj;
+        /* the following causes problems with subclassed types */
+        /* self->ob_type->tp_free((PyObject *)self); */
+        GST_DEBUG ("removing self %p from the table for object %p", self,
+             self->obj);
+        g_assert (g_hash_table_remove (_miniobjs, (gpointer) self->obj));
+	gst_mini_object_unref(self->obj);
     }
+    GST_DEBUG ("setting self %p -> obj to NULL", self);
     self->obj = NULL;
 
     if (self->inst_dict) {
@@ -280,10 +292,6 @@ pygstminiobject_dealloc(PyGstMiniObject *self)
     }
     self->inst_dict = NULL;
 
-    /* the following causes problems with subclassed types */
-    /* self->ob_type->tp_free((PyObject *)self); */
-    g_hash_table_remove (miniobjs, (gpointer) obj);
-
     PyObject_GC_Del(self);
     pyg_gil_state_release(state);
 }
@@ -333,15 +341,20 @@ pygstminiobject_traverse(PyGstMiniObject *self, visitproc visit, void *arg)
 static int
 pygstminiobject_clear(PyGstMiniObject *self)
 {
-
     if (self->inst_dict) {
 	Py_DECREF(self->inst_dict);
     }
     self->inst_dict = NULL;
 
     if (self->obj) {
-	gst_mini_object_unref(self->obj);
+        /* the following causes problems with subclassed types */
+        /* self->ob_type->tp_free((PyObject *)self); */
+        GST_DEBUG ("removing self %p from the table for object %p", self,
+             self->obj);
+        g_assert (g_hash_table_remove (_miniobjs, (gpointer) self->obj));
+	gst_mini_object_unref (self->obj);
     }
+    GST_DEBUG ("setting self %p -> obj to NULL", self);
     self->obj = NULL;
 
     return 0;
diff --git a/testsuite/test_buffer.py b/testsuite/test_buffer.py
index 26e91d6..66269c4 100644
--- a/testsuite/test_buffer.py
+++ b/testsuite/test_buffer.py
@@ -34,10 +34,10 @@ class BufferTest(unittest.TestCase):
         assert str(buffer) == 'test'
 
     def testBufferAlloc(self):
-	bla = 'mooooooo'
-	buffer = gst.Buffer(bla + '12345')
-	gc.collect ()
-	assert str(buffer) == 'mooooooo12345'
+        bla = 'mooooooo'
+        buffer = gst.Buffer(bla + '12345')
+        gc.collect ()
+        assert str(buffer) == 'mooooooo12345'
 		
     def testBufferBadConstructor(self):
         self.assertRaises(TypeError, gst.Buffer, 'test', 0)
@@ -60,10 +60,12 @@ class BufferTest(unittest.TestCase):
             s += '%02d' % i
             
         buffer = gst.Buffer(s)
-        assert len(buffer) == 128
+        self.assertEquals(len(buffer), 128)
 
         sub = buffer.create_sub(16, 16)
-        assert sub.offset == gst.CLOCK_TIME_NONE, sub.offset
+        self.assertEquals(sub.size, 16)
+        #self.assertEquals(sub.data, buffer.data[16:32])
+        self.assertEquals(sub.offset, gst.CLOCK_TIME_NONE)
 
     def testBufferMerge(self):
         buffer1 = gst.Buffer('foo')