I think I have fixed all the problems I found with the GStreamer
backend. See attached libcanberra-02-gstreamer.diff patch. This patch
does the following:
1) No longer free variables in the bus_cb function. This isn't safe
since GStreamer isn't finished tearing things down. Before this
change, I noticed that when driver_play failed to set state to
GST_STATE_PLAYING it would free the pipeline before GStreamer
had finished tearing things down.
Instead I create a new thread in driver_play and when we get
EOS/ERROR in the bus_cb, I signal the other thread to start.
This other thread sets the state to GST_STATE_NULL which
properly closes the GStreamer backend, solving the problem of leaving
the file descriptor open. Then it frees things. This seems to
work fine with no warnings or errors seen before this patch.
2) The way variables were being freed in driver_play seemed pretty
broken. You shouldn't free plugins by themselves. Instead just
free the pipeline and this will free the plugins as well. Now
I set a boolean just before we set state to GST_STATE_PLAYING.
If that boolean is set, then we know that the thread_func will
call outstanding_free to free everything. If it isn't set then
driver_play will free things at the end by calling outstanding_free.
In this case we know some error occurred before we called
gst_element_set_state and the variables may need to be freed to
avoid leaks.
3) A little cleanup in the driver_play function.
The second patch (libcanberra-01-solaris.diff) is needed for libcanberra
to compile on Solaris. I don't think this is needed on Linux, so I
surrounded the needed "#include" with "#ifdef __sun".
Can these patches go upstream?
Thanks,
Brian
Using libcanberra the GStreamer backend, I notice a bug. When you run
gnome-sound-properties, and then enable "Play alerts and sound effects"
on the "Sounds" tab, then click on one of the little arrow buttons to
preview the sound, it plays okay. However, the second time you try
clicking on any little arrow preview buttons it crashes. It does this
even if you wait for the first sound to stop playing before playing
the second sound.
After spending several hours digging into this, it seems there are
several problems here:
1) It is now more clear to me that libcanberra assumes that multiple
programs could play sounds at the same time, so it probably does
not make sense to use an audiosink which blocks when playing, like
the sunaudiosink I am currently using.
However, libcanberra probably should behave better when using
a blocking sink and not crash. It probably should just skip
sounds it can't play.
Perhaps on Solaris we need to use the ESD GStreamer sink or the
PulseAudio plugin instead.
2) The libcanberra GStreamer backend does not ever close the open file
descriptor on the sound device. So, after playing one sound, it
will fail to play future sounds because subsequent opens fail with
EBUSY. It seems the libcanberra code needs to listen for EOS
somehow and call gst_element_set_state on the pipeline to NULL.
I see other GStreamer programs do this by calling
g_signal_connect on the "message::eos" signal, but I think it
might not be appropriate to do this from libcanberra since you
may not always be sure that a glib event loop is running.
3) When the driver_play function calls gst_element_set_state to
set the state to GST_STATE_PLAYING, and this fails (like due to
the file descriptor being left open), it crashes. The crash is on
line 393 of src/gstreamer.c. This line looks as follows:
CA_LLIST_REMOVE(struct outstanding, p->outstanding, out);
Note that this line only gets called if the call to
gst_element_set_state to GST_STATE_PLAYING returns
GST_STATE_CHANGED_FAILURE.
On the error, it displays these messages:
---
(gnome-sound-properties:15054): GStreamer-CRITICAL **: file
gstelement.c: line 2030: assertion `GST_IS_ELEMENT (element)' failed
(gnome-sound-properties:15054): GLib-CRITICAL **: file gthread.c:
line 347: assertion `mutex' failed
Assertion '*_head == _item' failed at gstreamer.c:393, function ().
Aborting.
[EMAIL PROTECTED] ([EMAIL PROTECTED]) signal ABRT (Abort) in _lwp_kill at
0xfc4617c5
0xfc4617c5: _lwp_kill+0x0015: jae _lwp_kill+0x23 [ 0xfc4617d3,
.+0xe ]
---
The problem seems to be related to how the bus_cb function works.
When you play a successful sound, it calls bus_cb after the sound
is done playing and since the message iS GST_MESSAGE_EOS, and then
breaks out of the case, and calls CA_LLIST_REMOVE and
outstanding_free(). This doesn't seem to cause any problems.
However, in the failure case, the message is GST_MESSAGE_ERR. In
this case it also breaks out of the case and calls CA_LLIST_REMOVE
and outstanding_free(). Since outstanding_free unrefs the pipeline
before the gst_element_set_state function has completed, this
causes the "GST_IS_ELEMENT (element)" assertion above. On my
system, the gst_element_set_state is in the process of returning
from the error when the pipeline is freed, and the assertion is
from the gst_element_abort_state function when the structure
suddenly turns to garbage.
Even though calling CA_LLIST_REMOVE and outstanding_free seems to
work okay in the "success" case, I am not sure it is a good idea
to be unrefing the pipeline before GStreamer is done processing
the gst_element_set_state function.
The fact that we call CA_LLIST_REMOVE in the bus_cb function causes
the entry to be removed already, so when we try to remove it again
on line 393, it fails and this causes the crash.
I notice if I comment out the calls to CA_LLIST_REMOVE and
outstanding_free in the bus_cb function that it no longer crashes.
Instead it just plays the first sound okay and fails to play future
sounds without a crash (due to the file descriptor being left open).
However, I suspect this isn't the right fix and instead just makes
libcanberra leak memory.
At this point I'm not sure how to fix these problems, but it seems the
GStreamer code needs to be made more robust when the
gst_element_set_state function fails. It also should set the state to
NULL when the file is done playing so any open file handles can be
closed. I suspect the approach taken with the bus_cb function is not
the correct way to be doing memory cleanup.
Any ideas of how to improve the code would be appreciated.
Brian
_______________________________________________
libcanberra-discuss mailing list
[email protected]
https://tango.0pointer.de/mailman/listinfo/libcanberra-discuss
--- 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-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
@@ -50,6 +50,8 @@ struct outstanding {
void *userdata;
GstElement *pipeline;
struct ca_context *context;
+ pthread_cond_t cond;
+ pthread_mutex_t mutex;
};
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);
return GST_BUS_DROP;
}
@@ -310,35 +305,75 @@ 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 */
+ 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;
+}
+
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;
+ gboolean set_state;
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);
- 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;
- 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;
if (!(out = ca_new0(struct outstanding, 1)))
return CA_ERROR_OOM;
- out->id = id;
+ out->id = id;
out->callback = cb;
out->userdata = userdata;
- out->context = c;
+ out->context = c;
if (!(out->pipeline = gst_pipeline_new(NULL))
|| !(decodebin = gst_element_factory_make("decodebin2", NULL))
@@ -351,7 +386,8 @@ int driver_play(ca_context *c, uint32_t
bin = gst_bin_new("audiobin");
- 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));
gst_bus_set_sync_handler(bus, bus_cb, out);
@@ -361,11 +397,6 @@ 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;
goto fail;
}
@@ -374,13 +405,10 @@ int driver_play(ca_context *c, uint32_t
audiopad = gst_element_get_pad(audioconvert, "sink");
gst_element_add_pad(bin, gst_ghost_pad_new("sink", audiopad));
-
gst_object_unref(audiopad);
gst_bin_add(GST_BIN (out->pipeline), bin);
- decodebin = NULL;
- sink = NULL;
ca_free(f);
f = NULL;
@@ -388,11 +416,18 @@ 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);
+ 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);
+ }
- 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);
+ }
return ret;
}
_______________________________________________
libcanberra-discuss mailing list
[email protected]
https://tango.0pointer.de/mailman/listinfo/libcanberra-discuss
