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 > >
libcanberra-freeze-fix.patch
Description: libcanberra-freeze-fix.patch
_______________________________________________ libcanberra-discuss mailing list [email protected] https://tango.0pointer.de/mailman/listinfo/libcanberra-discuss
