*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
--- libcanberra-0.10/src/gstreamer.c-orig 2008-11-04 19:21:24.456864000
-0600
+++ libcanberra-0.10/src/gstreamer.c 2008-11-04 19:42:00.028682000 -0600
@@ -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);
}
@@ -242,16 +245,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 +306,60 @@ 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;
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 +378,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 +407,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 +422,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 +429,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 +453,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;
}
_______________________________________________
libcanberra-discuss mailing list
[email protected]
https://tango.0pointer.de/mailman/listinfo/libcanberra-discuss
