From 30b9a26f0a186e9565aa38567e4cfe26c9645a57 Mon Sep 17 00:00:00 2001 From: Timo Wischer Date: Wed, 17 Jan 2018 09:26:35 +0100 Subject: [PATCH 01/13] jack: always fill JACK buffer with silence when no audio data was copied Signed-off-by: Timo Wischer --- jack/pcm_jack.c | 53 ++++++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index 4468ede..a1a97a6 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -147,35 +147,38 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) jack->areas[channel].first = 0; jack->areas[channel].step = jack->sample_bits; } - - if (io->state != SND_PCM_STATE_RUNNING) { - if (io->stream == SND_PCM_STREAM_PLAYBACK) { - for (channel = 0; channel < io->channels; channel++) - snd_pcm_area_silence(&jack->areas[channel], 0, nframes, io->format); - return 0; - } - } - - areas = snd_pcm_ioplug_mmap_areas(io); - while (xfer < nframes) { - snd_pcm_uframes_t frames = nframes - xfer; - snd_pcm_uframes_t offset = jack->hw_ptr; - snd_pcm_uframes_t cont = io->buffer_size - offset; + if (io->state == SND_PCM_STATE_RUNNING) { + areas = snd_pcm_ioplug_mmap_areas(io); + + while (xfer < nframes) { + snd_pcm_uframes_t frames = nframes - xfer; + snd_pcm_uframes_t offset = jack->hw_ptr; + snd_pcm_uframes_t cont = io->buffer_size - offset; - if (cont < frames) - frames = cont; + if (cont < frames) + frames = cont; - for (channel = 0; channel < io->channels; channel++) { - if (io->stream == SND_PCM_STREAM_PLAYBACK) - snd_pcm_area_copy(&jack->areas[channel], xfer, &areas[channel], offset, frames, io->format); - else - snd_pcm_area_copy(&areas[channel], offset, &jack->areas[channel], xfer, frames, io->format); + for (channel = 0; channel < io->channels; channel++) { + if (io->stream == SND_PCM_STREAM_PLAYBACK) + snd_pcm_area_copy(&jack->areas[channel], xfer, &areas[channel], offset, frames, io->format); + else + snd_pcm_area_copy(&areas[channel], offset, &jack->areas[channel], xfer, frames, io->format); + } + + jack->hw_ptr += frames; + jack->hw_ptr %= io->buffer_size; + xfer += frames; + } + } + + /* check if requested frames were copied */ + if (xfer < nframes) { + if (io->stream == SND_PCM_STREAM_PLAYBACK) { + const unsigned int samples = nframes - xfer; + for (channel = 0; channel < io->channels; channel++) + snd_pcm_area_silence(&jack->areas[channel], xfer, samples, io->format); } - - jack->hw_ptr += frames; - jack->hw_ptr %= io->buffer_size; - xfer += frames; } pcm_poll_unblock_check(io); /* unblock socket for polling if needed */ From 6a55118d9abb6e33499cca7d04a78b3773481601 Mon Sep 17 00:00:00 2001 From: Timo Wischer Date: Thu, 21 Dec 2017 15:45:10 +0100 Subject: [PATCH 02/13] jack: Use boundary as hw_ptr wrap around instead of using buffer_size as wrap around. This is required to detect Xruns. It is also required to allow the JACK thread to processes the whole buffer without a snd_pcm_avail_update() call in between. Signed-off-by: Timo Wischer --- jack/pcm_jack.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index a1a97a6..3fa3794 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -40,6 +40,7 @@ typedef struct { char **port_names; unsigned int num_ports; + snd_pcm_uframes_t boundary; unsigned int hw_ptr; unsigned int sample_bits; snd_pcm_uframes_t min_avail; @@ -130,6 +131,21 @@ static int snd_pcm_jack_poll_revents(snd_pcm_ioplug_t *io, static snd_pcm_sframes_t snd_pcm_jack_pointer(snd_pcm_ioplug_t *io) { snd_pcm_jack_t *jack = io->private_data; + + /* ALSA library is calulating the delta between the last pointer and + * the current one. + * Normally it is expecting a value between 0 and buffer_size. + * The following example would result in an negative delta + * which would result in a hw_ptr which will be reduced. + * last_hw = jack->boundary - io->buffer_size + * hw = 0 + * But we cannot use + * return jack->hw_ptr % io->buffer_size; + * because in this case an update of + * hw_ptr += io->buffer_size + * would not be recognized by the ALSA library. + * Therefore we are using jack->boundary as the wrap around. + */ return jack->hw_ptr; } @@ -137,7 +153,6 @@ static int snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) { snd_pcm_jack_t *jack = io->private_data; - const snd_pcm_channel_area_t *areas; snd_pcm_uframes_t xfer = 0; unsigned int channel; @@ -149,13 +164,16 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) } if (io->state == SND_PCM_STATE_RUNNING) { - areas = snd_pcm_ioplug_mmap_areas(io); + const snd_pcm_channel_area_t *areas = snd_pcm_ioplug_mmap_areas(io); while (xfer < nframes) { snd_pcm_uframes_t frames = nframes - xfer; - snd_pcm_uframes_t offset = jack->hw_ptr; + snd_pcm_uframes_t offset = jack->hw_ptr % io->buffer_size; snd_pcm_uframes_t cont = io->buffer_size - offset; + /* split the snd_pcm_area_copy() function into two parts + * if the data to copy passes the buffer wrap around + */ if (cont < frames) frames = cont; @@ -167,7 +185,8 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) } jack->hw_ptr += frames; - jack->hw_ptr %= io->buffer_size; + if (jack->hw_ptr >= jack->boundary) + jack->hw_ptr -= jack->boundary; xfer += frames; } } @@ -200,6 +219,8 @@ static int snd_pcm_jack_prepare(snd_pcm_ioplug_t *io) err = snd_pcm_sw_params_current(io->pcm, swparams); if (err == 0) { snd_pcm_sw_params_get_avail_min(swparams, &jack->min_avail); + /* get boundary for available calulation */ + snd_pcm_sw_params_get_boundary(swparams, &jack->boundary); } /* deactivate jack connections if this is XRUN recovery */ From 3a64f385f1f70a1e0787e346b9033c86e68faef9 Mon Sep 17 00:00:00 2001 From: Timo Wischer Date: Thu, 21 Dec 2017 15:45:10 +0100 Subject: [PATCH 03/13] jack: Do not Xrun the ALSA buffer when the JACK thread is requesting to many frames In playback use case fill the JACK buffer with silence. Signed-off-by: Timo Wischer --- jack/pcm_jack.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index 3fa3794..c9ac4bb 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -54,6 +54,54 @@ typedef struct { static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io); + +static inline snd_pcm_uframes_t snd_pcm_jack_playback_avail(snd_pcm_ioplug_t *io) +{ + snd_pcm_jack_t *jack = io->private_data; + + /* cannot use io->hw_ptr without calling snd_pcm_avail_update() + * because it is not guranteed that snd_pcm_jack_pointer() was already + * called + */ + snd_pcm_sframes_t avail; + avail = jack->hw_ptr + io->buffer_size - io->appl_ptr; + if (avail < 0) + avail += jack->boundary; + else if ((snd_pcm_uframes_t) avail >= jack->boundary) + avail -= jack->boundary; + return avail; +} + +static inline snd_pcm_uframes_t snd_pcm_jack_capture_avail(snd_pcm_ioplug_t *io) +{ + snd_pcm_jack_t *jack = io->private_data; + + /* cannot use io->hw_ptr without calling snd_pcm_avail_update() + * because it is not guranteed that snd_pcm_jack_pointer() was already + * called + */ + snd_pcm_sframes_t avail; + avail = jack->hw_ptr - io->appl_ptr; + if (avail < 0) + avail += jack->boundary; + return avail; +} + +static inline snd_pcm_uframes_t snd_pcm_jack_avail(snd_pcm_ioplug_t *io) +{ + return (io->stream == SND_PCM_STREAM_PLAYBACK) ? + snd_pcm_jack_playback_avail(io) : + snd_pcm_jack_capture_avail(io); +} + +static inline snd_pcm_uframes_t snd_pcm_jack_hw_avail(snd_pcm_ioplug_t *io) +{ + /* available data/space which can be transfered by the user application */ + const snd_pcm_uframes_t user_avail = snd_pcm_jack_avail(io); + /* available data/space which can be transfered by the DMA */ + return io->buffer_size - user_avail; +} + static int pcm_poll_block_check(snd_pcm_ioplug_t *io) { static char buf[32]; @@ -170,6 +218,11 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) snd_pcm_uframes_t frames = nframes - xfer; snd_pcm_uframes_t offset = jack->hw_ptr % io->buffer_size; snd_pcm_uframes_t cont = io->buffer_size - offset; + snd_pcm_uframes_t hw_avail = snd_pcm_jack_hw_avail(io); + + /* stop copying if there is no more data available */ + if (hw_avail <= 0) + break; /* split the snd_pcm_area_copy() function into two parts * if the data to copy passes the buffer wrap around @@ -177,6 +230,9 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) if (cont < frames) frames = cont; + if (hw_avail < frames) + frames = hw_avail; + for (channel = 0; channel < io->channels; channel++) { if (io->stream == SND_PCM_STREAM_PLAYBACK) snd_pcm_area_copy(&jack->areas[channel], xfer, &areas[channel], offset, frames, io->format); From 3521977d4bbcada5ab91bc9ad04898dfe7656b5c Mon Sep 17 00:00:00 2001 From: Timo Wischer Date: Fri, 12 Jan 2018 12:43:38 +0100 Subject: [PATCH 04/13] jack: Use ALSA data type for internal hardware pointer to avoid issues with architeture which are using a different size for unsigned int Signed-off-by: Timo Wischer --- jack/pcm_jack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index c9ac4bb..3ba54fd 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -41,7 +41,7 @@ typedef struct { char **port_names; unsigned int num_ports; snd_pcm_uframes_t boundary; - unsigned int hw_ptr; + snd_pcm_uframes_t hw_ptr; unsigned int sample_bits; snd_pcm_uframes_t min_avail; From b3a5414c131cdfc408fc900b6589ee7b17e2394c Mon Sep 17 00:00:00 2001 From: Timo Wischer Date: Fri, 12 Jan 2018 15:28:46 +0100 Subject: [PATCH 05/13] jack: Ignore under run detection in prepare or draining state because the JACK process callback will be called before io->state == SND_PCM_STATE_RUNNING Signed-off-by: Timo Wischer --- jack/pcm_jack.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index 3ba54fd..de759d4 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -249,11 +249,33 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) /* check if requested frames were copied */ if (xfer < nframes) { + /* always fill the not yet written JACK buffer with silence */ if (io->stream == SND_PCM_STREAM_PLAYBACK) { const unsigned int samples = nframes - xfer; for (channel = 0; channel < io->channels; channel++) snd_pcm_area_silence(&jack->areas[channel], xfer, samples, io->format); } + + if (io->state == SND_PCM_STATE_PREPARED) { + /* After activating this JACK client with + * jack_activate() this process callback will be called. + * But the processing of snd_pcm_jack_start() would take + * a while longer due to the jack_connect() calls. + * Therefore the device was already started + * but it is not yet in RUNNING state. + * Due to this expected behaviour it is not a Xrun. + */ + } else if (io->state == SND_PCM_STATE_DRAINING) { + /* If the remaining data in the audio buffer is smaller + * than the requested amount of frames + * we want to provide silence to JACK. + * Therefore this is also not an under run. + */ + } else { + SNDERR("XRUN: JACK requests/provides %u frames but only %u frames were available in the ALSA buffer. (hw %u app %u)", + nframes, xfer, jack->hw_ptr, io->appl_ptr); + return 0; + } } pcm_poll_unblock_check(io); /* unblock socket for polling if needed */ From 53e9f9f0fac8d4c471618deb1a2ac63094e44493 Mon Sep 17 00:00:00 2001 From: Timo Wischer Date: Fri, 12 Jan 2018 16:43:21 +0100 Subject: [PATCH 06/13] jack: Report Xruns to user application Signed-off-by: Timo Wischer --- jack/pcm_jack.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index de759d4..e1bf2c2 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -20,6 +20,7 @@ * */ +#include #include #include #include @@ -37,6 +38,7 @@ typedef struct { int fd; int activated; /* jack is activated? */ + volatile bool xrun_detected; char **port_names; unsigned int num_ports; @@ -180,6 +182,17 @@ static snd_pcm_sframes_t snd_pcm_jack_pointer(snd_pcm_ioplug_t *io) { snd_pcm_jack_t *jack = io->private_data; +#ifdef TEST_SIMULATE_XRUNS + static int i=0; + if (++i > 1000) { + i = 0; + return -EPIPE; + } +#endif + + if (jack->xrun_detected) + return -EPIPE; + /* ALSA library is calulating the delta between the last pointer and * the current one. * Normally it is expecting a value between 0 and buffer_size. @@ -274,6 +287,7 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) } else { SNDERR("XRUN: JACK requests/provides %u frames but only %u frames were available in the ALSA buffer. (hw %u app %u)", nframes, xfer, jack->hw_ptr, io->appl_ptr); + jack->xrun_detected = true; return 0; } } @@ -291,6 +305,7 @@ static int snd_pcm_jack_prepare(snd_pcm_ioplug_t *io) int err; jack->hw_ptr = 0; + jack->xrun_detected = false; jack->min_avail = io->period_size; snd_pcm_sw_params_alloca(&swparams); From 6d8963fe8f334fa87baac1d4885508cef22e1840 Mon Sep 17 00:00:00 2001 From: Timo Wischer Date: Mon, 15 Jan 2018 12:47:46 +0100 Subject: [PATCH 07/13] jack: Block on drain till available samples played Signed-off-by: Timo Wischer --- jack/pcm_jack.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 79 insertions(+), 3 deletions(-) diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index e1bf2c2..7d8afa0 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -29,6 +29,17 @@ #include #include +/* ALSA supports up to 64 periods per buffer. + * Therefore at least 64 retries are valid and + * should not be handled as an error case + */ +#define MAX_DRAIN_RETRIES 100 +/* ALSA supports a a period with 8192 frames. + * This would result in ~170ms at 48kHz. + * Therefore a time out of 1 second is sufficient + */ +#define DRAIN_TIMEOUT 1000 + typedef enum _jack_format { SND_PCM_JACK_FORMAT_RAW } snd_pcm_jack_format_t; @@ -39,6 +50,7 @@ typedef struct { int fd; int activated; /* jack is activated? */ volatile bool xrun_detected; + volatile bool draining; char **port_names; unsigned int num_ports; @@ -130,7 +142,12 @@ static int pcm_poll_unblock_check(snd_pcm_ioplug_t *io) snd_pcm_jack_t *jack = io->private_data; avail = snd_pcm_avail_update(io->pcm); - if (avail < 0 || avail >= jack->min_avail) { + /* In draining state poll_fd is used to wait + * till all pending frames are played. + * Therefore it has to be guarantee that a poll event is also generated + * if the buffer contains less than min_avail frames + */ + if (avail < 0 || avail >= jack->min_avail || jack->draining) { write(jack->fd, &buf, 1); return 1; } @@ -224,7 +241,9 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) jack->areas[channel].step = jack->sample_bits; } - if (io->state == SND_PCM_STATE_RUNNING) { + if (io->state == SND_PCM_STATE_RUNNING || + io->state == SND_PCM_STATE_DRAINING || + jack->draining) { const snd_pcm_channel_area_t *areas = snd_pcm_ioplug_mmap_areas(io); while (xfer < nframes) { @@ -278,7 +297,7 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) * but it is not yet in RUNNING state. * Due to this expected behaviour it is not a Xrun. */ - } else if (io->state == SND_PCM_STATE_DRAINING) { + } else if (io->state == SND_PCM_STATE_DRAINING || jack->draining) { /* If the remaining data in the audio buffer is smaller * than the requested amount of frames * we want to provide silence to JACK. @@ -380,6 +399,60 @@ static int snd_pcm_jack_start(snd_pcm_ioplug_t *io) return 0; } +static int snd_pcm_jack_drain(snd_pcm_ioplug_t *io) +{ + snd_pcm_jack_t *jack = io->private_data; + unsigned int retries = MAX_DRAIN_RETRIES; + char buf[32]; + + /* Immediately stop on capture device. + * snd_pcm_jack_stop() will be automatically called + * by snd_pcm_ioplug_drain() + */ + if (io->stream == SND_PCM_STREAM_CAPTURE) { + return 0; + } + + if (snd_pcm_jack_hw_avail(io) <= 0) { + /* No data pending. Nothing to drain. */ + return 0; + } + + /* FIXME: io->state will not be set to SND_PCM_STATE_DRAINING by the + * ALSA library before calling this function. + * Therefore this state has to be stored internally. + */ + jack->draining = true; + + /* start device if not yet done */ + if (!jack->activated) { + snd_pcm_jack_start(io); + } + + struct pollfd pfd; + pfd.fd = io->poll_fd; + pfd.events = io->poll_events | POLLERR | POLLNVAL; + + while (snd_pcm_jack_hw_avail(io) > 0) { + if (retries <= 0) { + SNDERR("Pending frames not yet processed."); + return -ETIMEDOUT; + } + + if (poll(&pfd, 1, DRAIN_TIMEOUT) < 0) { + SNDERR("Waiting for next JACK process callback failed (err %d)", + -errno); + return -errno; + } + + /* clean pending events. */ + while (read(io->poll_fd, &buf, sizeof(buf)) == sizeof(buf)) + ; + } + + return 0; +} + static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io) { snd_pcm_jack_t *jack = io->private_data; @@ -388,6 +461,8 @@ static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io) jack_deactivate(jack->client); jack->activated = 0; } + + jack->draining = false; #if 0 unsigned i; for (i = 0; i < io->channels; i++) { @@ -406,6 +481,7 @@ static snd_pcm_ioplug_callback_t jack_pcm_callback = { .stop = snd_pcm_jack_stop, .pointer = snd_pcm_jack_pointer, .prepare = snd_pcm_jack_prepare, + .drain = snd_pcm_jack_drain, .poll_revents = snd_pcm_jack_poll_revents, }; From 1682af69e4b7b4d3c54f89af6dca71fb0fe973bc Mon Sep 17 00:00:00 2001 From: Timo Wischer Date: Tue, 16 Jan 2018 13:38:32 +0100 Subject: [PATCH 08/13] jack: Use internal avail functions to avoid dead locks with internal locking Signed-off-by: Timo Wischer --- jack/pcm_jack.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index 7d8afa0..0525f3b 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -124,7 +124,7 @@ static int pcm_poll_block_check(snd_pcm_ioplug_t *io) if (io->state == SND_PCM_STATE_RUNNING || (io->state == SND_PCM_STATE_PREPARED && io->stream == SND_PCM_STREAM_CAPTURE)) { - avail = snd_pcm_avail_update(io->pcm); + avail = snd_pcm_jack_avail(io); if (avail >= 0 && avail < jack->min_avail) { while (read(io->poll_fd, &buf, sizeof(buf)) == sizeof(buf)) ; @@ -141,7 +141,7 @@ static int pcm_poll_unblock_check(snd_pcm_ioplug_t *io) snd_pcm_sframes_t avail; snd_pcm_jack_t *jack = io->private_data; - avail = snd_pcm_avail_update(io->pcm); + avail = snd_pcm_jack_avail(io); /* In draining state poll_fd is used to wait * till all pending frames are played. * Therefore it has to be guarantee that a poll event is also generated From 9ea3b0d418cc34d4eae9efcde16f876dd2a3a81b Mon Sep 17 00:00:00 2001 From: Timo Wischer Date: Wed, 17 Jan 2018 10:56:24 +0100 Subject: [PATCH 09/13] jack: Use atomic access for hardware pointer and xrun flag volatile is not sufficient for guaranteeing the order of buffer access and hardware pointer update. In playback it could happen that the hardware pointer is updated but the audio data is not yet forwarded to the JACK daemon. Therefore the audio data could be overwritten by the ALSA application before it was copied to the JACK daemon. Signed-off-by: Timo Wischer --- jack/pcm_jack.c | 57 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 42 insertions(+), 15 deletions(-) diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index 0525f3b..eb46ae4 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -40,6 +40,24 @@ */ #define DRAIN_TIMEOUT 1000 + +/* All these atomic instructions include a full memory barrier */ +#ifndef __STDC_NO_ATOMICS__ +#include +#define ATOMIC_WRITE(VARP, VAL) atomic_store((VARP), (VAL)) +#define ATOMIC_READ(VARP) atomic_load((VARP)) +typedef _Atomic snd_pcm_uframes_t atomic_snd_pcm_uframes_t; + +#else +#define ATOMIC_WRITE(VARP, VAL) \ + while ( !__sync_bool_compare_and_swap((VARP), *(VARP), (VAL)) ); +#define ATOMIC_READ(VARP) __sync_fetch_and_or((VARP), 0) + +typedef volatile int atomic_bool; +typedef volatile snd_pcm_uframes_t atomic_snd_pcm_uframes_t; +#endif + + typedef enum _jack_format { SND_PCM_JACK_FORMAT_RAW } snd_pcm_jack_format_t; @@ -49,21 +67,28 @@ typedef struct { int fd; int activated; /* jack is activated? */ - volatile bool xrun_detected; volatile bool draining; char **port_names; unsigned int num_ports; snd_pcm_uframes_t boundary; - snd_pcm_uframes_t hw_ptr; unsigned int sample_bits; snd_pcm_uframes_t min_avail; unsigned int channels; - snd_pcm_channel_area_t *areas; + jack_client_t *client; + + /* variables used by ALSA and JACK thread */ + atomic_snd_pcm_uframes_t hw_ptr; + atomic_bool xrun_detected; + /* variables used by JACK thread + * They will be initialized before the JACK thread was started and + * not changed by the ALSA thread as long as the JACK thread is active. + * Therefore no locking is required. + */ + snd_pcm_channel_area_t *areas; jack_port_t **ports; - jack_client_t *client; } snd_pcm_jack_t; static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io); @@ -78,7 +103,7 @@ static inline snd_pcm_uframes_t snd_pcm_jack_playback_avail(snd_pcm_ioplug_t *io * called */ snd_pcm_sframes_t avail; - avail = jack->hw_ptr + io->buffer_size - io->appl_ptr; + avail = ATOMIC_READ(&jack->hw_ptr) + io->buffer_size - io->appl_ptr; if (avail < 0) avail += jack->boundary; else if ((snd_pcm_uframes_t) avail >= jack->boundary) @@ -95,7 +120,7 @@ static inline snd_pcm_uframes_t snd_pcm_jack_capture_avail(snd_pcm_ioplug_t *io) * called */ snd_pcm_sframes_t avail; - avail = jack->hw_ptr - io->appl_ptr; + avail = ATOMIC_READ(&jack->hw_ptr) - io->appl_ptr; if (avail < 0) avail += jack->boundary; return avail; @@ -207,7 +232,7 @@ static snd_pcm_sframes_t snd_pcm_jack_pointer(snd_pcm_ioplug_t *io) } #endif - if (jack->xrun_detected) + if ( ATOMIC_READ(&jack->xrun_detected) ) return -EPIPE; /* ALSA library is calulating the delta between the last pointer and @@ -224,7 +249,7 @@ static snd_pcm_sframes_t snd_pcm_jack_pointer(snd_pcm_ioplug_t *io) * would not be recognized by the ALSA library. * Therefore we are using jack->boundary as the wrap around. */ - return jack->hw_ptr; + return ATOMIC_READ(&jack->hw_ptr); } static int @@ -248,7 +273,8 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) while (xfer < nframes) { snd_pcm_uframes_t frames = nframes - xfer; - snd_pcm_uframes_t offset = jack->hw_ptr % io->buffer_size; + snd_pcm_uframes_t hw_ptr = ATOMIC_READ(&jack->hw_ptr); + snd_pcm_uframes_t offset = hw_ptr % io->buffer_size; snd_pcm_uframes_t cont = io->buffer_size - offset; snd_pcm_uframes_t hw_avail = snd_pcm_jack_hw_avail(io); @@ -272,9 +298,10 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) snd_pcm_area_copy(&areas[channel], offset, &jack->areas[channel], xfer, frames, io->format); } - jack->hw_ptr += frames; - if (jack->hw_ptr >= jack->boundary) - jack->hw_ptr -= jack->boundary; + hw_ptr += frames; + if (hw_ptr >= jack->boundary) + hw_ptr -= jack->boundary; + ATOMIC_WRITE(&jack->hw_ptr, hw_ptr); xfer += frames; } } @@ -306,7 +333,7 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) } else { SNDERR("XRUN: JACK requests/provides %u frames but only %u frames were available in the ALSA buffer. (hw %u app %u)", nframes, xfer, jack->hw_ptr, io->appl_ptr); - jack->xrun_detected = true; + ATOMIC_WRITE(&jack->xrun_detected, false); return 0; } } @@ -323,8 +350,8 @@ static int snd_pcm_jack_prepare(snd_pcm_ioplug_t *io) snd_pcm_sw_params_t *swparams; int err; - jack->hw_ptr = 0; - jack->xrun_detected = false; + ATOMIC_WRITE(&jack->hw_ptr, 0); + ATOMIC_WRITE(&jack->xrun_detected, false); jack->min_avail = io->period_size; snd_pcm_sw_params_alloca(&swparams); From 0ba616391ea70e0170ef2eed937ab4eee88d4cd2 Mon Sep 17 00:00:00 2001 From: Timo Wischer Date: Wed, 17 Jan 2018 11:11:28 +0100 Subject: [PATCH 10/13] jack: Use internal snd_pcm_state with atomic access This replaces the additional draining and activated variables. The state has to be accessed with memory barriers to guarantee the order of execution. e.g. to guarantee that snd_pcm_state not changes from DRAINING to RUNNING Signed-off-by: Timo Wischer --- jack/pcm_jack.c | 47 +++++++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index eb46ae4..87bd78e 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -46,6 +46,7 @@ #include #define ATOMIC_WRITE(VARP, VAL) atomic_store((VARP), (VAL)) #define ATOMIC_READ(VARP) atomic_load((VARP)) +typedef _Atomic snd_pcm_state_t atomic_snd_pcm_state_t; typedef _Atomic snd_pcm_uframes_t atomic_snd_pcm_uframes_t; #else @@ -54,6 +55,7 @@ typedef _Atomic snd_pcm_uframes_t atomic_snd_pcm_uframes_t; #define ATOMIC_READ(VARP) __sync_fetch_and_or((VARP), 0) typedef volatile int atomic_bool; +typedef volatile snd_pcm_state_t atomic_snd_pcm_state_t; typedef volatile snd_pcm_uframes_t atomic_snd_pcm_uframes_t; #endif @@ -66,8 +68,6 @@ typedef struct { snd_pcm_ioplug_t io; int fd; - int activated; /* jack is activated? */ - volatile bool draining; char **port_names; unsigned int num_ports; @@ -79,6 +79,7 @@ typedef struct { jack_client_t *client; /* variables used by ALSA and JACK thread */ + atomic_snd_pcm_state_t state; atomic_snd_pcm_uframes_t hw_ptr; atomic_bool xrun_detected; @@ -172,7 +173,8 @@ static int pcm_poll_unblock_check(snd_pcm_ioplug_t *io) * Therefore it has to be guarantee that a poll event is also generated * if the buffer contains less than min_avail frames */ - if (avail < 0 || avail >= jack->min_avail || jack->draining) { + if (avail < 0 || avail >= jack->min_avail || + ATOMIC_READ(&jack->state) == SND_PCM_STATE_DRAINING) { write(jack->fd, &buf, 1); return 1; } @@ -258,6 +260,7 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) snd_pcm_jack_t *jack = io->private_data; snd_pcm_uframes_t xfer = 0; unsigned int channel; + snd_pcm_state_t state; for (channel = 0; channel < io->channels; channel++) { jack->areas[channel].addr = @@ -266,9 +269,9 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) jack->areas[channel].step = jack->sample_bits; } - if (io->state == SND_PCM_STATE_RUNNING || - io->state == SND_PCM_STATE_DRAINING || - jack->draining) { + state = ATOMIC_READ(&jack->state); + if (state == SND_PCM_STATE_RUNNING || + state == SND_PCM_STATE_DRAINING) { const snd_pcm_channel_area_t *areas = snd_pcm_ioplug_mmap_areas(io); while (xfer < nframes) { @@ -315,7 +318,7 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) snd_pcm_area_silence(&jack->areas[channel], xfer, samples, io->format); } - if (io->state == SND_PCM_STATE_PREPARED) { + if (state == SND_PCM_STATE_PREPARED) { /* After activating this JACK client with * jack_activate() this process callback will be called. * But the processing of snd_pcm_jack_start() would take @@ -324,7 +327,7 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) * but it is not yet in RUNNING state. * Due to this expected behaviour it is not a Xrun. */ - } else if (io->state == SND_PCM_STATE_DRAINING || jack->draining) { + } else if (state == SND_PCM_STATE_DRAINING) { /* If the remaining data in the audio buffer is smaller * than the requested amount of frames * we want to provide silence to JACK. @@ -393,6 +396,9 @@ static int snd_pcm_jack_prepare(snd_pcm_ioplug_t *io) jack_set_process_callback(jack->client, (JackProcessCallback)snd_pcm_jack_process_cb, io); + + ATOMIC_WRITE(&jack->state, SND_PCM_STATE_PREPARED); + return 0; } @@ -404,8 +410,6 @@ static int snd_pcm_jack_start(snd_pcm_ioplug_t *io) if (jack_activate (jack->client)) return -EIO; - jack->activated = 1; - for (i = 0; i < io->channels && i < jack->num_ports; i++) { if (jack->port_names[i]) { const char *src, *dst; @@ -422,6 +426,13 @@ static int snd_pcm_jack_start(snd_pcm_ioplug_t *io) } } } + + /* Do not change back to running if we are already in draining. + * This can happen when the playback was not yet started + * but the stream will be stopped with snd_pcm_drain() + */ + if (ATOMIC_READ(&jack->state) != SND_PCM_STATE_DRAINING) + ATOMIC_WRITE(&jack->state, SND_PCM_STATE_RUNNING); return 0; } @@ -429,6 +440,7 @@ static int snd_pcm_jack_start(snd_pcm_ioplug_t *io) static int snd_pcm_jack_drain(snd_pcm_ioplug_t *io) { snd_pcm_jack_t *jack = io->private_data; + snd_pcm_state_t state; unsigned int retries = MAX_DRAIN_RETRIES; char buf[32]; @@ -449,10 +461,11 @@ static int snd_pcm_jack_drain(snd_pcm_ioplug_t *io) * ALSA library before calling this function. * Therefore this state has to be stored internally. */ - jack->draining = true; + state = ATOMIC_READ(&jack->state); + ATOMIC_WRITE(&jack->state, SND_PCM_STATE_DRAINING); /* start device if not yet done */ - if (!jack->activated) { + if (state != SND_PCM_STATE_RUNNING) { snd_pcm_jack_start(io); } @@ -483,13 +496,14 @@ static int snd_pcm_jack_drain(snd_pcm_ioplug_t *io) static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io) { snd_pcm_jack_t *jack = io->private_data; - - if (jack->activated) { + const snd_pcm_state_t state = ATOMIC_READ(&jack->state); + + if (state == SND_PCM_STATE_RUNNING || + state == SND_PCM_STATE_DRAINING) { jack_deactivate(jack->client); - jack->activated = 0; } - jack->draining = false; + ATOMIC_WRITE(&jack->state, SND_PCM_STATE_SETUP); #if 0 unsigned i; for (i = 0; i < io->channels; i++) { @@ -611,6 +625,7 @@ static int snd_pcm_jack_open(snd_pcm_t **pcmp, const char *name, jack->fd = -1; jack->io.poll_fd = -1; + ATOMIC_WRITE(&jack->state, SND_PCM_STATE_OPEN); err = parse_ports(jack, stream == SND_PCM_STREAM_PLAYBACK ? playback_conf : capture_conf); From 3dbf5bef0ec3d7c17c9e6d1e4e690ff31abd8081 Mon Sep 17 00:00:00 2001 From: Timo Wischer Date: Wed, 17 Jan 2018 11:25:21 +0100 Subject: [PATCH 11/13] jack: use internal application pointer with atomic access To guarantee that the execution order. e.g. In playback case do not updating the application pointer before the audio data was copied to the buffer Additionally guarantee that the read of the application pointer is atomic and an invalid value will not be read. Signed-off-by: Timo Wischer --- jack/pcm_jack.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index 87bd78e..9a8dd65 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -80,6 +80,7 @@ typedef struct { /* variables used by ALSA and JACK thread */ atomic_snd_pcm_state_t state; + atomic_snd_pcm_uframes_t appl_ptr; atomic_snd_pcm_uframes_t hw_ptr; atomic_bool xrun_detected; @@ -104,7 +105,8 @@ static inline snd_pcm_uframes_t snd_pcm_jack_playback_avail(snd_pcm_ioplug_t *io * called */ snd_pcm_sframes_t avail; - avail = ATOMIC_READ(&jack->hw_ptr) + io->buffer_size - io->appl_ptr; + avail = ATOMIC_READ(&jack->hw_ptr) + io->buffer_size - + ATOMIC_READ(&jack->appl_ptr); if (avail < 0) avail += jack->boundary; else if ((snd_pcm_uframes_t) avail >= jack->boundary) @@ -121,7 +123,7 @@ static inline snd_pcm_uframes_t snd_pcm_jack_capture_avail(snd_pcm_ioplug_t *io) * called */ snd_pcm_sframes_t avail; - avail = ATOMIC_READ(&jack->hw_ptr) - io->appl_ptr; + avail = ATOMIC_READ(&jack->hw_ptr) - ATOMIC_READ(&jack->appl_ptr); if (avail < 0) avail += jack->boundary; return avail; @@ -254,6 +256,22 @@ static snd_pcm_sframes_t snd_pcm_jack_pointer(snd_pcm_ioplug_t *io) return ATOMIC_READ(&jack->hw_ptr); } +static snd_pcm_sframes_t snd_pcm_jack_transfer(snd_pcm_ioplug_t *io, + const snd_pcm_channel_area_t *areas, + snd_pcm_uframes_t offset, + snd_pcm_uframes_t size) +{ + snd_pcm_jack_t *jack = io->private_data; + + /* The application pointer will be updated after calling the transfer + * function therefore we have to add the size here + */ + const snd_pcm_uframes_t forwarded_appl_ptr = io->appl_ptr + size; + ATOMIC_WRITE(&jack->appl_ptr, forwarded_appl_ptr); + + return size; +} + static int snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) { @@ -335,7 +353,7 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) */ } else { SNDERR("XRUN: JACK requests/provides %u frames but only %u frames were available in the ALSA buffer. (hw %u app %u)", - nframes, xfer, jack->hw_ptr, io->appl_ptr); + nframes, xfer, jack->hw_ptr, jack->appl_ptr); ATOMIC_WRITE(&jack->xrun_detected, false); return 0; } @@ -353,6 +371,7 @@ static int snd_pcm_jack_prepare(snd_pcm_ioplug_t *io) snd_pcm_sw_params_t *swparams; int err; + ATOMIC_WRITE(&jack->appl_ptr, 0); ATOMIC_WRITE(&jack->hw_ptr, 0); ATOMIC_WRITE(&jack->xrun_detected, false); @@ -521,6 +540,7 @@ static snd_pcm_ioplug_callback_t jack_pcm_callback = { .start = snd_pcm_jack_start, .stop = snd_pcm_jack_stop, .pointer = snd_pcm_jack_pointer, + .transfer = snd_pcm_jack_transfer, .prepare = snd_pcm_jack_prepare, .drain = snd_pcm_jack_drain, .poll_revents = snd_pcm_jack_poll_revents, From 79a1cdd736c88602a14626e01424643702c007bc Mon Sep 17 00:00:00 2001 From: Timo Wischer Date: Wed, 17 Jan 2018 11:32:49 +0100 Subject: [PATCH 12/13] jack: Do not use snd_pcm_ioplug_mmap_areas() in JACK thread because it accesses io->state and a read to this variable can result in an invalid value because there is no synchronisation used. Signed-off-by: Timo Wischer --- jack/pcm_jack.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index 9a8dd65..022f716 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -48,6 +48,7 @@ #define ATOMIC_READ(VARP) atomic_load((VARP)) typedef _Atomic snd_pcm_state_t atomic_snd_pcm_state_t; typedef _Atomic snd_pcm_uframes_t atomic_snd_pcm_uframes_t; +typedef _Atomic snd_pcm_channel_area_t atomic_snd_pcm_channel_area_t; #else #define ATOMIC_WRITE(VARP, VAL) \ @@ -57,6 +58,7 @@ typedef _Atomic snd_pcm_uframes_t atomic_snd_pcm_uframes_t; typedef volatile int atomic_bool; typedef volatile snd_pcm_state_t atomic_snd_pcm_state_t; typedef volatile snd_pcm_uframes_t atomic_snd_pcm_uframes_t; +typedef volatile snd_pcm_channel_area_t atomic_snd_pcm_channel_area_t; #endif @@ -82,6 +84,8 @@ typedef struct { atomic_snd_pcm_state_t state; atomic_snd_pcm_uframes_t appl_ptr; atomic_snd_pcm_uframes_t hw_ptr; + /** the areas provided by the ALSA library from the user application */ + const atomic_snd_pcm_channel_area_t *alsa_areas; atomic_bool xrun_detected; /* variables used by JACK thread @@ -268,6 +272,7 @@ static snd_pcm_sframes_t snd_pcm_jack_transfer(snd_pcm_ioplug_t *io, */ const snd_pcm_uframes_t forwarded_appl_ptr = io->appl_ptr + size; ATOMIC_WRITE(&jack->appl_ptr, forwarded_appl_ptr); + ATOMIC_WRITE(&(jack->alsa_areas), (atomic_snd_pcm_channel_area_t*)areas); return size; } @@ -290,7 +295,8 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) state = ATOMIC_READ(&jack->state); if (state == SND_PCM_STATE_RUNNING || state == SND_PCM_STATE_DRAINING) { - const snd_pcm_channel_area_t *areas = snd_pcm_ioplug_mmap_areas(io); + const snd_pcm_channel_area_t *alsa_areas = + (snd_pcm_channel_area_t*)ATOMIC_READ(&jack->alsa_areas); while (xfer < nframes) { snd_pcm_uframes_t frames = nframes - xfer; @@ -314,9 +320,9 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) for (channel = 0; channel < io->channels; channel++) { if (io->stream == SND_PCM_STREAM_PLAYBACK) - snd_pcm_area_copy(&jack->areas[channel], xfer, &areas[channel], offset, frames, io->format); + snd_pcm_area_copy(&jack->areas[channel], xfer, &alsa_areas[channel], offset, frames, io->format); else - snd_pcm_area_copy(&areas[channel], offset, &jack->areas[channel], xfer, frames, io->format); + snd_pcm_area_copy(&alsa_areas[channel], offset, &jack->areas[channel], xfer, frames, io->format); } hw_ptr += frames; From 3c6f0b3e761b664930b78f687a687737c9820aa3 Mon Sep 17 00:00:00 2001 From: Timo Wischer Date: Tue, 16 Jan 2018 16:04:36 +0100 Subject: [PATCH 13/13] jack: Rename jack->areas to jack_areas jack_areas contains the pointer to the JACK buffers Signed-off-by: Timo Wischer --- jack/pcm_jack.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index 022f716..3c266a5 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -93,7 +93,7 @@ typedef struct { * not changed by the ALSA thread as long as the JACK thread is active. * Therefore no locking is required. */ - snd_pcm_channel_area_t *areas; + snd_pcm_channel_area_t *jack_areas; jack_port_t **ports; } snd_pcm_jack_t; @@ -203,7 +203,7 @@ static void snd_pcm_jack_free(snd_pcm_jack_t *jack) close(jack->fd); if (jack->io.poll_fd >= 0) close(jack->io.poll_fd); - free(jack->areas); + free(jack->jack_areas); free(jack->ports); free(jack); } @@ -286,10 +286,10 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) snd_pcm_state_t state; for (channel = 0; channel < io->channels; channel++) { - jack->areas[channel].addr = + jack->jack_areas[channel].addr = jack_port_get_buffer (jack->ports[channel], nframes); - jack->areas[channel].first = 0; - jack->areas[channel].step = jack->sample_bits; + jack->jack_areas[channel].first = 0; + jack->jack_areas[channel].step = jack->sample_bits; } state = ATOMIC_READ(&jack->state); @@ -320,9 +320,9 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) for (channel = 0; channel < io->channels; channel++) { if (io->stream == SND_PCM_STREAM_PLAYBACK) - snd_pcm_area_copy(&jack->areas[channel], xfer, &alsa_areas[channel], offset, frames, io->format); + snd_pcm_area_copy(&jack->jack_areas[channel], xfer, &alsa_areas[channel], offset, frames, io->format); else - snd_pcm_area_copy(&alsa_areas[channel], offset, &jack->areas[channel], xfer, frames, io->format); + snd_pcm_area_copy(&alsa_areas[channel], offset, &jack->jack_areas[channel], xfer, frames, io->format); } hw_ptr += frames; @@ -339,7 +339,7 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) if (io->stream == SND_PCM_STREAM_PLAYBACK) { const unsigned int samples = nframes - xfer; for (channel = 0; channel < io->channels; channel++) - snd_pcm_area_silence(&jack->areas[channel], xfer, samples, io->format); + snd_pcm_area_silence(&jack->jack_areas[channel], xfer, samples, io->format); } if (state == SND_PCM_STATE_PREPARED) { @@ -689,8 +689,8 @@ static int snd_pcm_jack_open(snd_pcm_t **pcmp, const char *name, return -ENOENT; } - jack->areas = calloc(jack->channels, sizeof(snd_pcm_channel_area_t)); - if (! jack->areas) { + jack->jack_areas = calloc(jack->channels, sizeof(snd_pcm_channel_area_t)); + if (! jack->jack_areas) { snd_pcm_jack_free(jack); return -ENOMEM; }