From dc83edf73e079e267585429a439790e684fe2441 Mon Sep 17 00:00:00 2001 From: Thomas Vander Stichele Date: Thu, 1 Sep 2005 14:41:28 +0000 Subject: [PATCH] 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 --- ChangeLog | 19 ++++++++ gst/gst.override | 4 +- gst/gstbuffer.override | 95 +++++++++++++++++++++++++++++++-------- gst/gstmodule.c | 6 ++- gst/pygstminiobject.c | 113 ++++++++++++++++++++++++++--------------------- testsuite/test_buffer.py | 14 +++--- 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 + + * 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 * 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 ("", buf, size); + } else { + data = GST_BUFFER_DATA (buf); + repr = g_strdup_printf ( + "", 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') -- 2.7.4