It was almost a month ago that Hotti provided an updated GStreamer patch for
libcanberra. Has this gone upstream yet? I think the issues that were
raised have all been addressed, and it would be good to get this upstream, I
think.
Also it would be nice if the libcanberra-01-solaris.diff patch could go
upstream. It is needed for libcanberaa to compile on Solaris. I don't
think this is needed on other platforms, so I surrounded the needed
"#include" with "#ifdef __sun".
Thanks,
Brian
Hotti Timo wrote:
Hi,
I fixed a small bug from canberra and I used this patch as a base. When a sound
is playing and you are trying to destroy the canberra context, it just freezes
because the pthread_cond_signal was called only when EOS was received. I added
this pthread_cond_signal-call to driver_cancel and driver_destroy to fix the
problem. But a better fix could be that the NULL-state change is handled
differently in bus_cb, so we don't need to destroy the pipeline in thread_func
and we can just set the pipeline state to NULL instead of using this
pthread_cond_signal. There was also a small error in configure.ac, it did set
GSTREAMER_CFLAGS and GSTREAMER_LIBS and in makefile.am GST_CFLAGS and GST_LIBS
was used. The attached patch includes the Brian's patch and the fixes.
Timo Hotti
-----Original Message-----
From: [EMAIL PROTECTED]
[mailto:[EMAIL PROTECTED] On
Behalf Of Brian Cameron
Sent: Wednesday, November 12, 2008 6:51 PM
To: Brian Cameron
Cc: Jan Schmidt; [email protected]
Subject: Re: [libcanberra-discuss] Bugs with libcanberra
*ping*
Can this patch go upstream? I believe I addressed all issues that
Marc-Andre brought up with my original patch.
Thanks,
Brian
Marc-André:
I think I have fixed all the problems I found with the GStreamer
backend. See attached libcanberra-02-gstreamer.diff
patch. This patch
Great! Sorry for causing extra work. Thank you for your patch fix.
Attached find an updated patch. This patch removes the
C-style changes
I made in the previous patch.
I also improved the cleanup code so that the code calls
outstanding_free
in the various error conditions and avoid using the
"set_state" boolean.
Can this patch go upstream? Hopefully it addresses the issues in the
previous patch.
Brian
--- libcanberra-0.10/src/gstreamer.c-orig 2008-10-30
02:01:23.980831000 -0500
+++ libcanberra-0.10/src/gstreamer.c 2008-10-30
02:34:00.030254000
-0500
struct private {
@@ -242,16 +244,9 @@ static GstBusSyncReply bus_cb(GstBus *bu
if (!out->dead && out->callback)
out->callback(out->context, out->id, err, out->userdata);
- ca_mutex_lock(p->outstanding_mutex);
-
- CA_LLIST_REMOVE(struct outstanding, p->outstanding, out);
-
- if (!p->outstanding && p->signal_semaphore)
- sem_post(&p->semaphore);
-
- outstanding_free(out);
-
- ca_mutex_unlock(p->outstanding_mutex);
+ pthread_mutex_lock(&out->mutex);
+ pthread_cond_signal(&out->cond);
+ pthread_mutex_unlock(&out->mutex);
That make sense according to your comments.
+static void* thread_func(void *userdata) {
+ struct outstanding *out = userdata;
+ struct private *p;
+
+ p = PRIVATE(out->context);
+
+ pthread_detach(pthread_self());
+
+ /* Wait until we receive EOS */
+ pthread_mutex_lock(&out->mutex);
+ pthread_cond_wait (&out->cond, &out->mutex);
+
+ ca_mutex_lock(p->outstanding_mutex);
+
+ /* Set pipeline back to NULL to close things */
+ if (gst_element_set_state(out->pipeline,
+ GST_STATE_NULL) ==
GST_STATE_CHANGE_FAILURE) {
+ goto fail;
+ }
+
+ CA_LLIST_REMOVE(struct outstanding, p->outstanding, out);
+
+ if (!p->outstanding && p->signal_semaphore)
+ sem_post(&p->semaphore);
+
+ outstanding_free(out);
+
+fail:
+ ca_mutex_unlock(p->outstanding_mutex);
+ pthread_mutex_unlock(&out->mutex);
+ return NULL;
+}
Looks good yes.
- f = NULL;
- sink = NULL;
- decodebin = NULL;
-
- p = PRIVATE(c);
+ out = NULL;
+ f = NULL;
+ decodebin = NULL;
+ sink = NULL;
+ audioconvert = NULL;
+ audioresample = NULL;
+ bin = NULL;
+ p = PRIVATE(c);
+ set_state = FALSE;
I am not sure if it's lennart-c-mode compliant ;)
- if ((ret = ca_lookup_sound_with_callback(&f,
ca_gst_sound_file_open, NULL, &p->theme, c->props, proplist)) < 0)
+ if ((ret = ca_lookup_sound_with_callback(&f,
ca_gst_sound_file_open,
+ NULL, &p->theme, c->props, proplist)) < 0)
goto fail;
- out->id = id;
+ out->id = id;
out->callback = cb;
out->userdata = userdata;
- out->context = c;
+ out->context = c;
- g_signal_connect(decodebin, "new-decoded-pad", G_CALLBACK
(on_pad_added), bin);
+ g_signal_connect(decodebin, "new-decoded-pad",
+ G_CALLBACK (on_pad_added), bin);
bus = gst_pipeline_get_bus(GST_PIPELINE (out->pipeline));
Not really necessary, imho.
- if (gst_element_set_state(out->pipeline,
GST_STATE_PLAYING) ==
GST_STATE_CHANGE_FAILURE) {
+ if (pthread_create(&thread, NULL, thread_func, out) < 0) {
ca_mutex_lock(p->outstanding_mutex);
CA_LLIST_REMOVE(struct outstanding, p->outstanding, out);
ca_mutex_unlock(p->outstanding_mutex);
+ ret=CA_ERROR_OOM;
+ goto fail;
+ }
+ + set_state = TRUE;
+ if (gst_element_set_state(out->pipeline,
+ GST_STATE_PLAYING) ==
GST_STATE_CHANGE_FAILURE) {
ret = CA_ERROR_NOTAVAILABLE;
goto fail;
}
@@ -400,22 +435,21 @@ int driver_play(ca_context *c, uint32_t
return CA_SUCCESS;
fail:
- if (f && f->fdsrc)
- gst_object_unref(f->fdsrc);
-
- if (f)
+ if (f) {
ca_free(f);
- if (sink)
- gst_object_unref(sink);
-
- if (decodebin)
- gst_object_unref(decodebin);
-
- if (out && out->pipeline)
- gst_object_unref(out->pipeline);
+ if (f->fdsrc)
+ gst_object_unref(f->fdsrc);
+ }
I prefer a branch-less condition for the unref/free "fail"
section. But ok for me!
- ca_free(out);
+ /*
+ * If we did not start playing, then we need to free
any allocated
+ * variables. If the pipeline did start, the free
will be done by
+ * thread_func() .
+ */
+ if (set_state == FALSE) {
+ outstanding_free(out);
+ }
You could outstanding_free(out) under the gst_element_set_state
condition and avoid the set_state variable?
I still need to run/test it now, but that was for the patch.
Are you ok to resend an improved version of the patch?
/me learning to do code review with git/mutt/emacsclient (& emacs
-daemon). :-P
thanks again
cheers,
---------------------------------------------------------------
---------
_______________________________________________
libcanberra-discuss mailing list
[email protected]
https://tango.0pointer.de/mailman/listinfo/libcanberra-discuss
diff -pruN libcanberra-0.10/configure.ac libcanberra-0.10-mod/configure.ac
--- libcanberra-0.10/configure.ac 2008-10-06 17:10:29.000000000 +0300
+++ libcanberra-0.10-mod/configure.ac 2008-11-14 15:17:12.000000000 +0200
@@ -303,8 +303,8 @@ else
HAVE_GSTREAMER=0
fi
-AC_SUBST(GSTREAMER_CFLAGS)
-AC_SUBST(GSTREAMER_LIBS)
+AC_SUBST(GST_CFLAGS)
+AC_SUBST(GST_LIBS)
### Null output (optional) ####
diff -pruN libcanberra-0.10/src/gstreamer.c libcanberra-0.10-mod/src/gstreamer.c
--- libcanberra-0.10/src/gstreamer.c 2008-10-06 17:10:29.000000000 +0300
+++ libcanberra-0.10-mod/src/gstreamer.c 2008-11-14 15:15:52.000000000
+0200
@@ -50,6 +50,8 @@ struct outstanding {
void *userdata;
GstElement *pipeline;
struct ca_context *context;
+ pthread_cond_t cond;
+ pthread_mutex_t mutex;
};
struct private {
@@ -68,12 +70,13 @@ static void outstanding_free(struct outs
ca_assert(o);
- bus = gst_pipeline_get_bus(GST_PIPELINE (o->pipeline));
- gst_bus_set_sync_handler(bus, NULL, NULL);
- gst_object_unref(bus);
+ if (o->pipeline) {
+ bus = gst_pipeline_get_bus(GST_PIPELINE (o->pipeline));
+ gst_bus_set_sync_handler(bus, NULL, NULL);
+ gst_object_unref(bus);
- if (o->pipeline)
gst_object_unref(GST_OBJECT(o->pipeline));
+ }
ca_free(o);
}
@@ -141,14 +144,13 @@ int driver_destroy(ca_context *c) {
if (out->callback)
out->callback(c, out->id, CA_ERROR_DESTROYED, out->userdata);
- out = out->next;
-
ca_mutex_unlock(p->outstanding_mutex);
-
- gst_element_set_state(pipeline, GST_STATE_NULL);
- gst_object_unref(GST_OBJECT(pipeline));
-
+ pthread_mutex_lock(&out->mutex);
+ pthread_cond_signal(&out->cond);
+ pthread_mutex_unlock(&out->mutex);
ca_mutex_lock(p->outstanding_mutex);
+
+ out = out->next;
}
if (p->semaphore_allocated) {
@@ -242,16 +244,9 @@ static GstBusSyncReply bus_cb(GstBus *bu
if (!out->dead && out->callback)
out->callback(out->context, out->id, err, out->userdata);
- ca_mutex_lock(p->outstanding_mutex);
-
- CA_LLIST_REMOVE(struct outstanding, p->outstanding, out);
-
- if (!p->outstanding && p->signal_semaphore)
- sem_post(&p->semaphore);
-
- outstanding_free(out);
-
- ca_mutex_unlock(p->outstanding_mutex);
+ pthread_mutex_lock(&out->mutex);
+ pthread_cond_signal(&out->cond);
+ pthread_mutex_unlock(&out->mutex);
return GST_BUS_DROP;
}
@@ -310,23 +305,62 @@ static void on_pad_added(GstElement *ele
gst_caps_unref(caps);
}
+static void* thread_func(void *userdata) {
+ struct outstanding *out = userdata;
+ struct private *p;
+
+ p = PRIVATE(out->context);
+
+ pthread_detach(pthread_self());
+
+ /* Wait until we receive EOS -> also close and destroy needs to get here.
*/
+ pthread_mutex_lock(&out->mutex);
+ pthread_cond_wait (&out->cond, &out->mutex);
+
+ ca_mutex_lock(p->outstanding_mutex);
+
+ /* Set pipeline back to NULL to close things */
+ if (gst_element_set_state(out->pipeline,
+ GST_STATE_NULL) == GST_STATE_CHANGE_FAILURE) {
+ goto fail;
+ }
+
+ gst_object_unref(GST_OBJECT(out->pipeline));
+
+ CA_LLIST_REMOVE(struct outstanding, p->outstanding, out);
+
+ if (!p->outstanding && p->signal_semaphore)
+ sem_post(&p->semaphore);
+
+ outstanding_free(out);
+
+fail:
+ ca_mutex_unlock(p->outstanding_mutex);
+ pthread_mutex_unlock(&out->mutex);
+ return NULL;
+}
+
int driver_play(ca_context *c, uint32_t id, ca_proplist *proplist,
ca_finish_callback_t cb, void *userdata) {
struct private *p;
- struct outstanding *out = NULL;
+ struct outstanding *out;
ca_sound_file *f;
GstElement *decodebin, *sink, *audioconvert, *audioresample, *bin;
GstBus *bus;
GstPad *audiopad;
+ pthread_t thread;
int ret;
ca_return_val_if_fail(c, CA_ERROR_INVALID);
ca_return_val_if_fail(proplist, CA_ERROR_INVALID);
ca_return_val_if_fail(!userdata || cb, CA_ERROR_INVALID);
+ out = NULL;
f = NULL;
sink = NULL;
decodebin = NULL;
-
+ audioconvert = NULL;
+ audioresample = NULL;
+ bin = NULL;
p = PRIVATE(c);
if ((ret = ca_lookup_sound_with_callback(&f, ca_gst_sound_file_open, NULL,
&p->theme, c->props, proplist)) < 0)
@@ -345,6 +379,19 @@ int driver_play(ca_context *c, uint32_t
|| !(audioconvert = gst_element_factory_make("audioconvert", NULL))
|| !(audioresample = gst_element_factory_make("audioresample", NULL))
|| !(sink = gst_element_factory_make("autoaudiosink", NULL))) {
+
+ /* At this point, if there is a failure, free each plugin separately.
*/
+ if (out->pipeline != NULL)
+ g_object_unref (out->pipeline);
+ if (decodebin != NULL)
+ g_object_unref(decodebin);
+ if (audioconvert != NULL)
+ g_object_unref(audioconvert);
+ if (audioresample != NULL)
+ g_object_unref(audioresample);
+ if (sink != NULL)
+ g_object_unref(sink);
+
ret = CA_ERROR_OOM;
goto fail;
}
@@ -361,11 +408,8 @@ int driver_play(ca_context *c, uint32_t
f->fdsrc, decodebin, NULL);
if (!gst_element_link(f->fdsrc, decodebin)) {
- f->fdsrc = NULL;
- decodebin = NULL;
- audioconvert = NULL;
- audioresample = NULL;
- sink = NULL;
+ outstanding_free(out);
+ ret = CA_ERROR_OOM;
goto fail;
}
@@ -379,8 +423,6 @@ int driver_play(ca_context *c, uint32_t
gst_bin_add(GST_BIN (out->pipeline), bin);
- decodebin = NULL;
- sink = NULL;
ca_free(f);
f = NULL;
@@ -388,11 +430,17 @@ int driver_play(ca_context *c, uint32_t
CA_LLIST_PREPEND(struct outstanding, p->outstanding, out);
ca_mutex_unlock(p->outstanding_mutex);
- if (gst_element_set_state(out->pipeline, GST_STATE_PLAYING) ==
GST_STATE_CHANGE_FAILURE) {
+ if (pthread_create(&thread, NULL, thread_func, out) < 0) {
ca_mutex_lock(p->outstanding_mutex);
CA_LLIST_REMOVE(struct outstanding, p->outstanding, out);
ca_mutex_unlock(p->outstanding_mutex);
+ outstanding_free(out);
+ ret = CA_ERROR_OOM;
+ goto fail;
+ }
+ if (gst_element_set_state(out->pipeline,
+ GST_STATE_PLAYING) == GST_STATE_CHANGE_FAILURE) {
ret = CA_ERROR_NOTAVAILABLE;
goto fail;
}
@@ -406,17 +454,6 @@ fail:
if (f)
ca_free(f);
- if (sink)
- gst_object_unref(sink);
-
- if (decodebin)
- gst_object_unref(decodebin);
-
- if (out && out->pipeline)
- gst_object_unref(out->pipeline);
-
- ca_free(out);
-
return ret;
}
@@ -431,15 +468,15 @@ int driver_cancel(ca_context *c, uint32_
ca_mutex_lock(p->outstanding_mutex);
- for (out = p->outstanding; out; out = out->next) {
+ for (out = p->outstanding; out;/* out = out->next*/) {
GstElement *pipeline;
- if (out->id != id)
+ if (out->id != id || out->pipeline == NULL)
+ {
+ out = out->next;
continue;
-
- if (out->pipeline == NULL)
- break;
-
+ }
+
if (out->callback)
out->callback(c, out->id, CA_ERROR_CANCELED, out->userdata);
@@ -447,10 +484,11 @@ int driver_cancel(ca_context *c, uint32_
out->dead = TRUE;
ca_mutex_unlock(p->outstanding_mutex);
- gst_element_set_state(out->pipeline, GST_STATE_NULL);
+ pthread_mutex_lock(&out->mutex);
+ pthread_cond_signal(&out->cond);
+ pthread_mutex_unlock(&out->mutex);
+ out = out->next;
ca_mutex_lock(p->outstanding_mutex);
-
- gst_object_unref(pipeline);
}
ca_mutex_unlock(p->outstanding_mutex);
--- libcanberra-0.8/src/common.h-orig 2008-08-21 00:21:50.594788000 -0500
+++ libcanberra-0.8/src/common.h 2008-08-21 00:22:25.218467000 -0500
@@ -21,6 +21,10 @@
<http://www.gnu.org/licenses/>.
***/
+#ifdef __sun
+#include <sys/varargs.h>
+#endif
+
#include "canberra.h"
#include "macro.h"
#include "mutex.h"
_______________________________________________
libcanberra-discuss mailing list
[email protected]
https://tango.0pointer.de/mailman/listinfo/libcanberra-discuss
