From: Andre Noll Date: Sun, 25 May 2014 13:31:07 +0000 (+0200) Subject: Merge branch 't/ao_fixes' X-Git-Tag: v0.5.3~20 X-Git-Url: http://git.tuebingen.mpg.de/?p=paraslash.git;a=commitdiff_plain;h=08ae151a088d1a1bc5442d8ee95960353a7a5782;hp=d1e6b28f66e243516d01916f9125baee75dd98d6 Merge branch 't/ao_fixes' Cooking since 2014-03-22. * t/ao_fixes: ao_write: Call ao_initialize() only once. ao_write: Join threads before returning an error from aow_post_select(). ao_write: Simplify locking. Don't unlock and lock the thread mutex unnecessarily. ao_write: Check return value of pthread functions. ao_write: Avoid segfault on exit. ao_write: Avoid pthread_join(). ao_write: Enforce a 20ms timeout. ao_write: Fix spurious segfault. --- diff --git a/NEWS b/NEWS index 4da56905..642c1fc4 100644 --- a/NEWS +++ b/NEWS @@ -8,6 +8,8 @@ NEWS - Various alsa-related fixes, mostly for the raspberry pi. - The test suite has been extended to include sanity checks for the generated man pages. + - ao_writer fixes. This writer was in a quite bad shape. Many + serious bugs have been fixed. ---------------------------------------- 0.5.2 (2014-04-11) "orthogonal interior" diff --git a/ao_write.c b/ao_write.c index 730653c1..63d18afa 100644 --- a/ao_write.c +++ b/ao_write.c @@ -28,6 +28,7 @@ struct private_aow_data { pthread_t thread; pthread_attr_t attr; + /* The mutex and the condition variable serialize access to ->btrn */ pthread_mutex_t mutex; pthread_cond_t data_available; struct btr_node *thread_btrn; @@ -39,19 +40,46 @@ static void aow_close(struct writer_node *wn) if (!pawd) return; + assert(!pawd->thread_btrn); ao_close(pawd->dev); free(pawd); wn->private_data = NULL; - ao_shutdown(); } static void aow_pre_select(struct sched *s, struct task *t) { struct writer_node *wn = container_of(t, struct writer_node, task); - int ret = btr_node_status(wn->btrn, wn->min_iqs, BTR_NT_LEAF); + struct private_aow_data *pawd = wn->private_data; + int ret; - if (ret == 0) - return; + if (!pawd) { /* not yet started */ + assert(wn->btrn); + ret = btr_node_status(wn->btrn, wn->min_iqs, BTR_NT_LEAF); + if (ret != 0) + goto min_delay; + return; /* no data available */ + } + if (!wn->btrn) { /* EOF */ + if (!pawd->thread_btrn) /* ready to exit */ + goto min_delay; + /* wait for the play thread to terminate */ + goto timeout; + } + pthread_mutex_lock(&pawd->mutex); + ret = btr_node_status(wn->btrn, wn->min_iqs, BTR_NT_LEAF); + pthread_mutex_unlock(&pawd->mutex); + if (ret != 0) + goto min_delay; + /* + * Even though the node status is zero, we might have data available, + * but the output buffer is full. If we don't set a timeout here, we + * are woken up only if new data arrives, which might be too late and + * result in a buffer underrun in the playing thread. To avoid this we + * never sleep longer than the (default) buffer time. + */ +timeout: + return sched_request_timeout_ms(20, s); +min_delay: sched_min_delay(s); } @@ -129,7 +157,6 @@ static int aow_init(struct writer_node *wn, unsigned sample_rate, struct private_aow_data *pawd = para_malloc(sizeof(*pawd)); struct ao_write_args_info *conf = wn->conf; - ao_initialize(); if (conf->driver_given) { ret = -E_AO_BAD_DRIVER; id = ao_driver_id(conf->driver_arg); @@ -192,16 +219,12 @@ __noreturn static void *aow_play(void *priv) char *data; int ret; + pthread_mutex_lock(&pawd->mutex); for (;;) { - /* - * Lock mutex and wait for signal. pthread_cond_wait() will - * automatically and atomically unlock mutex while it waits. - */ - pthread_mutex_lock(&pawd->mutex); for (;;) { ret = btr_node_status(btrn, wn->min_iqs, BTR_NT_LEAF); if (ret < 0) - goto unlock; + goto fail; if (ret > 0) { btr_merge(btrn, wn->min_iqs); bytes = btr_next_buffer(btrn, &data); @@ -210,26 +233,35 @@ __noreturn static void *aow_play(void *priv) break; /* eof and less than a single frame available */ ret = -E_WRITE_COMMON_EOF; - goto unlock; + goto fail; } - //PARA_CRIT_LOG("waiting for data\n"); - //usleep(1000); - //pthread_mutex_unlock(&pawd->mutex); - pthread_cond_wait(&pawd->data_available, &pawd->mutex); + /* + * No data available, go to sleep and wait for the main + * thread to wake us up. pthread_cond_wait() unlocks + * the mutex while it waits and locks it again upon + * return. + */ + ret = pthread_cond_wait(&pawd->data_available, + &pawd->mutex); + /* pthread_cond_wait() can never fail here */ + assert(ret == 0); } - pthread_mutex_unlock(&pawd->mutex); assert(frames > 0); bytes = frames * pawd->bytes_per_frame; - ret = -E_AO_PLAY; - if (ao_play(pawd->dev, data, bytes) == 0) /* failure */ - goto out; + pthread_mutex_unlock(&pawd->mutex); + ret = ao_play(pawd->dev, data, bytes); + pthread_mutex_lock(&pawd->mutex); + if (ret == 0) { /* failure */ + ret = -E_AO_PLAY; + goto fail; + } btr_consume(btrn, bytes); } -unlock: - pthread_mutex_unlock(&pawd->mutex); -out: +fail: + btr_remove_node(&pawd->thread_btrn); assert(ret < 0); PARA_NOTICE_LOG("%s\n", para_strerror(-ret)); + pthread_mutex_unlock(&pawd->mutex); pthread_exit(NULL); } @@ -314,21 +346,32 @@ static int aow_post_select(__a_unused struct sched *s, goto remove_thread_btrn; return 0; } + if (!wn->btrn) { + if (!pawd->thread_btrn) { + pthread_join(pawd->thread, NULL); + return -E_AO_EOF; + } + PARA_INFO_LOG("waiting for play thread to terminate\n"); + return 0; + } pthread_mutex_lock(&pawd->mutex); ret = btr_node_status(wn->btrn, wn->min_iqs, BTR_NT_LEAF); if (ret > 0) { btr_pushdown(wn->btrn); - pthread_cond_signal(&pawd->data_available); + if (pthread_cond_signal(&pawd->data_available) != 0) { + ret = -E_AO_PTHREAD; + PARA_ERROR_LOG("pthread_cond_signal() failed\n"); + goto remove_thread_btrn; + } } - pthread_mutex_unlock(&pawd->mutex); - if (ret >= 0) + if (ret >= 0) { + pthread_mutex_unlock(&pawd->mutex); goto out; - pthread_mutex_lock(&pawd->mutex); + } btr_remove_node(&wn->btrn); - PARA_INFO_LOG("waiting for thread to terminate\n"); pthread_cond_signal(&pawd->data_available); pthread_mutex_unlock(&pawd->mutex); - pthread_join(pawd->thread, NULL); + return 0; remove_thread_btrn: btr_remove_node(&pawd->thread_btrn); remove_btrn: @@ -410,6 +453,5 @@ void ao_write_init(struct writer *w) dh[num_lines] = NULL; w->help.detailed_help = (const char **)dh; ao_write_cmdline_parser_free(&dummy); - ao_shutdown(); } diff --git a/error.h b/error.h index 301e2ca5..ba8409df 100644 --- a/error.h +++ b/error.h @@ -181,6 +181,7 @@ extern const char **para_errlist[]; PARA_ERROR(AO_PLAY, "ao_play() failed"), \ PARA_ERROR(AO_BAD_SAMPLE_FORMAT, "ao: unsigned sample formats not supported"), \ PARA_ERROR(AO_PTHREAD, "pthread error"), \ + PARA_ERROR(AO_EOF, "ao: end of file"), \ #define COMPRESS_FILTER_ERRORS \