On Thu, Oct 30, 2008 at 02:49:30AM -0500, Brian Cameron wrote:
> 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.
> 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".
> --- 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
> +#ifdef __sun
> +#include <sys/varargs.h>
> +#endif
I have never seen that before. I checked in glib, it seems they don't
use this include neither. I'll let Lennart review.
> --- 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,
--
Marc-Andre Lureau
_______________________________________________
libcanberra-discuss mailing list
[email protected]
https://tango.0pointer.de/mailman/listinfo/libcanberra-discuss
