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 4da5690551bdffb5a860316fadc4c0effc61512c..642c1fc422225141a1fa1a555113746a69b8a93e 100644 (file)
--- 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 730653c1b06844ed0822cc784c0c61b6a0303269..63d18afab387288831e45e50cd33ecfd42801805 100644 (file)
--- 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 301e2ca5e1ae36a045a044d8f69c072e82c62a88..ba8409dfff4e42375629ba50f8a99281ac1aa574 100644 (file)
--- 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 \