[alsa-devel] [PATCH - JACK PCM plugin] jack: Do not Xrun the ALSA buffer
From: Timo Wischer twischer@de.adit-jv.com
when the JACK thread is requesting too many audio frames
Playback: Without this commit the ALSA audio buffer will be played with endless repeats as long as the user application has not provided new audio data. Therefore this garbage will be played as long as the user application has not called snd_pcm_stop() after an Xrun. With this fix the rest of the JACK buffer will be filled with silence.
Capture: Without this commit the audio data in the ALSA buffer would be overwritten. With this commit the new data from the JACK buffer will not be copied. Therefore the existing data in the ALSA buffer will not be overwritten.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com ---
This patch is depending on http://mailman.alsa-project.org/pipermail/alsa-devel/2018-January/130986.htm...
diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index c22a5d0..a4f35f4 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -54,6 +54,68 @@ typedef struct {
static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io);
+ +static inline snd_pcm_uframes_t snd_pcm_jack_playback_avail(const snd_pcm_ioplug_t* const io, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) +{ + const snd_pcm_jack_t* const 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 = hw_ptr + io->buffer_size - 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(const snd_pcm_ioplug_t* const io, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) +{ + const snd_pcm_jack_t* const 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 = hw_ptr - appl_ptr; + if (avail < 0) + avail += jack->boundary; + return avail; +} + +static inline snd_pcm_uframes_t snd_pcm_jack_avail(const snd_pcm_ioplug_t* const io, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) +{ + return (io->stream == SND_PCM_STREAM_PLAYBACK) ? + snd_pcm_jack_playback_avail(io, hw_ptr, appl_ptr) : + snd_pcm_jack_capture_avail(io, hw_ptr, appl_ptr); +} + +static inline snd_pcm_uframes_t snd_pcm_jack_hw_avail(const snd_pcm_ioplug_t* const io, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) +{ + /* available data/space which can be transfered by the user application */ + const snd_pcm_uframes_t user_avail = snd_pcm_jack_avail(io, hw_ptr, + appl_ptr); + + if (user_avail > io->buffer_size) { + /* there was an Xrun */ + return 0; + } + /* 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]; @@ -154,7 +216,6 @@ 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 hw_ptr; - const snd_pcm_channel_area_t *areas; snd_pcm_uframes_t xfer = 0; unsigned int channel; @@ -164,40 +225,57 @@ 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; - } - }
hw_ptr = jack->hw_ptr; - areas = snd_pcm_ioplug_mmap_areas(io); - - while (xfer < nframes) { - snd_pcm_uframes_t frames = nframes - xfer; - snd_pcm_uframes_t offset = hw_ptr % io->buffer_size; - snd_pcm_uframes_t cont = io->buffer_size - offset; - - if (cont < frames) - frames = cont; + if (io->state == SND_PCM_STATE_RUNNING) { + 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 = 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, hw_ptr, + io->appl_ptr); + + /* 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 + */ + 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); + 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); + hw_ptr += frames; + if (hw_ptr >= jack->boundary) + hw_ptr -= jack->boundary; + xfer += frames; } - - hw_ptr += frames; - if (hw_ptr >= jack->boundary) - hw_ptr -= jack->boundary; - xfer += frames; } jack->hw_ptr = hw_ptr;
+ /* 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 snd_pcm_uframes_t samples = nframes - xfer; + for (channel = 0; channel < io->channels; channel++) + snd_pcm_area_silence(&jack->areas[channel], xfer, samples, io->format); + } + } + pcm_poll_unblock_check(io); /* unblock socket for polling if needed */
return 0;
Hello all,
Due to merged [1] (see [2]) [3] can be merged without conflicts, now.
@Takashi: Thanks for merging [2].
Please give a feedback if there is anything which has to be changed.
The biggest change in snd_pcm_jack_process_cb() occurs due to the movement of the snd_pcm_area_silence() call to the end of this function. This is required to fill the buffer with silence if there is not enough audio data available for JACK.
[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-January/130986.htm... [2] http://git.alsa-project.org/?p=alsa-plugins.git;a=commit;h=3e6ace6fe045c580d... [3] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-January/130987.htm...
Best regards
Timo Wischer
Advanced Driver Information Technology GmbH Engineering Software Base (ADITG/ESB) Robert-Bosch-Str. 200 31139 Hildesheim Germany
Tel. +49 5121 49 6938 Fax +49 5121 49 6999 twischer@de.adit-jv.com
ADIT is a joint venture company of Robert Bosch GmbH/Robert Bosch Car Multimedia GmbH and DENSO Corporation Sitz: Hildesheim, Registergericht: Amtsgericht Hildesheim HRB 3438 Geschäftsführung: Wilhelm Grabow, Ken Yaguchi
________________________________________ From: Wischer, Timo (ADITG/ESB) Sent: Wednesday, January 24, 2018 12:53 PM To: patch@alsa-project.org Cc: alsa-devel@alsa-project.org; Wischer, Timo (ADITG/ESB) Subject: [PATCH - JACK PCM plugin] jack: Do not Xrun the ALSA buffer
From: Timo Wischer twischer@de.adit-jv.com
when the JACK thread is requesting too many audio frames
Playback: Without this commit the ALSA audio buffer will be played with endless repeats as long as the user application has not provided new audio data. Therefore this garbage will be played as long as the user application has not called snd_pcm_stop() after an Xrun. With this fix the rest of the JACK buffer will be filled with silence.
Capture: Without this commit the audio data in the ALSA buffer would be overwritten. With this commit the new data from the JACK buffer will not be copied. Therefore the existing data in the ALSA buffer will not be overwritten.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com ---
This patch is depending on http://mailman.alsa-project.org/pipermail/alsa-devel/2018-January/130986.htm...
diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index c22a5d0..a4f35f4 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -54,6 +54,68 @@ typedef struct {
static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io);
+ +static inline snd_pcm_uframes_t snd_pcm_jack_playback_avail(const snd_pcm_ioplug_t* const io, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) +{ + const snd_pcm_jack_t* const 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 = hw_ptr + io->buffer_size - 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(const snd_pcm_ioplug_t* const io, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) +{ + const snd_pcm_jack_t* const 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 = hw_ptr - appl_ptr; + if (avail < 0) + avail += jack->boundary; + return avail; +} + +static inline snd_pcm_uframes_t snd_pcm_jack_avail(const snd_pcm_ioplug_t* const io, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) +{ + return (io->stream == SND_PCM_STREAM_PLAYBACK) ? + snd_pcm_jack_playback_avail(io, hw_ptr, appl_ptr) : + snd_pcm_jack_capture_avail(io, hw_ptr, appl_ptr); +} + +static inline snd_pcm_uframes_t snd_pcm_jack_hw_avail(const snd_pcm_ioplug_t* const io, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) +{ + /* available data/space which can be transfered by the user application */ + const snd_pcm_uframes_t user_avail = snd_pcm_jack_avail(io, hw_ptr, + appl_ptr); + + if (user_avail > io->buffer_size) { + /* there was an Xrun */ + return 0; + } + /* 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]; @@ -154,7 +216,6 @@ 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 hw_ptr; - const snd_pcm_channel_area_t *areas; snd_pcm_uframes_t xfer = 0; unsigned int channel;
@@ -164,40 +225,57 @@ 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; - } - }
hw_ptr = jack->hw_ptr; - areas = snd_pcm_ioplug_mmap_areas(io); - - while (xfer < nframes) { - snd_pcm_uframes_t frames = nframes - xfer; - snd_pcm_uframes_t offset = hw_ptr % io->buffer_size; - snd_pcm_uframes_t cont = io->buffer_size - offset; - - if (cont < frames) - frames = cont; + if (io->state == SND_PCM_STATE_RUNNING) { + 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 = 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, hw_ptr, + io->appl_ptr); + + /* 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 + */ + 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); + 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); + hw_ptr += frames; + if (hw_ptr >= jack->boundary) + hw_ptr -= jack->boundary; + xfer += frames; } - - hw_ptr += frames; - if (hw_ptr >= jack->boundary) - hw_ptr -= jack->boundary; - xfer += frames; } jack->hw_ptr = hw_ptr;
+ /* 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 snd_pcm_uframes_t samples = nframes - xfer; + for (channel = 0; channel < io->channels; channel++) + snd_pcm_area_silence(&jack->areas[channel], xfer, samples, io->format); + } + } + pcm_poll_unblock_check(io); /* unblock socket for polling if needed */
return 0; -- 2.7.4
On Mon, 26 Feb 2018 09:14:11 +0100, Wischer, Timo (ADITG/ESB) wrote:
Hello all,
Due to merged [1] (see [2]) [3] can be merged without conflicts, now.
@Takashi: Thanks for merging [2].
Please give a feedback if there is anything which has to be changed.
Could you simply resubmit the patches? In your way, the real topic and the patch is buried deeply.
thanks,
Takashi
From: Timo Wischer twischer@de.adit-jv.com
when the JACK thread is requesting too many audio frames
Playback: Without this commit the ALSA audio buffer will be played with endless repeats as long as the user application has not provided new audio data. Therefore this garbage will be played as long as the user application has not called snd_pcm_stop() after an Xrun. With this fix the rest of the JACK buffer will be filled with silence.
Capture: Without this commit the audio data in the ALSA buffer would be overwritten. With this commit the new data from the JACK buffer will not be copied. Therefore the existing data in the ALSA buffer will not be overwritten.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index c547cbb..c3ed9fb 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -54,6 +54,68 @@ typedef struct {
static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io);
+ +static inline snd_pcm_uframes_t snd_pcm_jack_playback_avail(const snd_pcm_ioplug_t* const io, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) +{ + const snd_pcm_jack_t* const 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 = hw_ptr + io->buffer_size - 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(const snd_pcm_ioplug_t* const io, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) +{ + const snd_pcm_jack_t* const 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 = hw_ptr - appl_ptr; + if (avail < 0) + avail += jack->boundary; + return avail; +} + +static inline snd_pcm_uframes_t snd_pcm_jack_avail(const snd_pcm_ioplug_t* const io, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) +{ + return (io->stream == SND_PCM_STREAM_PLAYBACK) ? + snd_pcm_jack_playback_avail(io, hw_ptr, appl_ptr) : + snd_pcm_jack_capture_avail(io, hw_ptr, appl_ptr); +} + +static inline snd_pcm_uframes_t snd_pcm_jack_hw_avail(const snd_pcm_ioplug_t* const io, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) +{ + /* available data/space which can be transfered by the user application */ + const snd_pcm_uframes_t user_avail = snd_pcm_jack_avail(io, hw_ptr, + appl_ptr); + + if (user_avail > io->buffer_size) { + /* there was an Xrun */ + return 0; + } + /* 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]; @@ -144,7 +206,6 @@ 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 hw_ptr; - const snd_pcm_channel_area_t *areas; snd_pcm_uframes_t xfer = 0; unsigned int channel; @@ -154,40 +215,57 @@ 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; - } - }
hw_ptr = jack->hw_ptr; - areas = snd_pcm_ioplug_mmap_areas(io); - - while (xfer < nframes) { - snd_pcm_uframes_t frames = nframes - xfer; - snd_pcm_uframes_t offset = hw_ptr % io->buffer_size; - snd_pcm_uframes_t cont = io->buffer_size - offset; - - if (cont < frames) - frames = cont; + if (io->state == SND_PCM_STATE_RUNNING) { + 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 = 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, hw_ptr, + io->appl_ptr); + + /* 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 + */ + 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); + 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); + hw_ptr += frames; + if (hw_ptr >= jack->boundary) + hw_ptr -= jack->boundary; + xfer += frames; } - - hw_ptr += frames; - if (hw_ptr >= jack->boundary) - hw_ptr -= jack->boundary; - xfer += frames; } jack->hw_ptr = hw_ptr;
+ /* 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 snd_pcm_uframes_t samples = nframes - xfer; + for (channel = 0; channel < io->channels; channel++) + snd_pcm_area_silence(&jack->areas[channel], xfer, samples, io->format); + } + } + pcm_poll_unblock_check(io); /* unblock socket for polling if needed */
return 0;
On Thu, 01 Mar 2018 14:14:05 +0100, twischer@de.adit-jv.com wrote:
From: Timo Wischer twischer@de.adit-jv.com
when the JACK thread is requesting too many audio frames
Playback: Without this commit the ALSA audio buffer will be played with endless repeats as long as the user application has not provided new audio data. Therefore this garbage will be played as long as the user application has not called snd_pcm_stop() after an Xrun. With this fix the rest of the JACK buffer will be filled with silence.
Capture: Without this commit the audio data in the ALSA buffer would be overwritten. With this commit the new data from the JACK buffer will not be copied. Therefore the existing data in the ALSA buffer will not be overwritten.
Please try to format the text a bit nicer. These line breaks appear like you're writing a poem :)
Now about the code changes:
+static inline snd_pcm_uframes_t snd_pcm_jack_playback_avail(const snd_pcm_ioplug_t* const io,
const snd_pcm_uframes_t hw_ptr,const snd_pcm_uframes_t appl_ptr)
You don't need to put inline unless it's really mandatory.
+{
- const snd_pcm_jack_t* const 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 = hw_ptr + io->buffer_size - 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(const snd_pcm_ioplug_t* const io,
const snd_pcm_uframes_t hw_ptr,const snd_pcm_uframes_t appl_ptr)+{
- const snd_pcm_jack_t* const 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 = hw_ptr - appl_ptr;
- if (avail < 0)
avail += jack->boundary;- return avail;
+}
+static inline snd_pcm_uframes_t snd_pcm_jack_avail(const snd_pcm_ioplug_t* const io,
const snd_pcm_uframes_t hw_ptr,const snd_pcm_uframes_t appl_ptr)+{
- return (io->stream == SND_PCM_STREAM_PLAYBACK) ?
snd_pcm_jack_playback_avail(io, hw_ptr, appl_ptr) :snd_pcm_jack_capture_avail(io, hw_ptr, appl_ptr);+}
+static inline snd_pcm_uframes_t snd_pcm_jack_hw_avail(const snd_pcm_ioplug_t* const io,
const snd_pcm_uframes_t hw_ptr,const snd_pcm_uframes_t appl_ptr)+{
- /* available data/space which can be transfered by the user application */
- const snd_pcm_uframes_t user_avail = snd_pcm_jack_avail(io, hw_ptr,
appl_ptr);- if (user_avail > io->buffer_size) {
/* there was an Xrun */return 0;- }
- /* available data/space which can be transfered by the DMA */
- return io->buffer_size - user_avail;
+}
Hm, this whole stuff would fit better in ioplug code itself instead of open-coding in each plugin. Maybe providing a new helper function?
static int pcm_poll_block_check(snd_pcm_ioplug_t *io) { static char buf[32]; @@ -144,7 +206,6 @@ 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 hw_ptr;
- const snd_pcm_channel_area_t *areas; snd_pcm_uframes_t xfer = 0; unsigned int channel;
@@ -154,40 +215,57 @@ 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;}}
hw_ptr = jack->hw_ptr;
areas = snd_pcm_ioplug_mmap_areas(io);
while (xfer < nframes) {
snd_pcm_uframes_t frames = nframes - xfer;snd_pcm_uframes_t offset = hw_ptr % io->buffer_size;snd_pcm_uframes_t cont = io->buffer_size - offset;if (cont < frames)frames = cont;
- if (io->state == SND_PCM_STATE_RUNNING) {
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 = 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, hw_ptr,io->appl_ptr);/* 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*/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);elsesnd_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);elsesnd_pcm_area_copy(&areas[channel], offset, &jack->areas[channel], xfer, frames, io->format);
hw_ptr += frames;if (hw_ptr >= jack->boundary)hw_ptr -= jack->boundary;xfer += frames;
hw_ptr += frames;if (hw_ptr >= jack->boundary)hw_ptr -= jack->boundary;xfer += frames;
- /* 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 snd_pcm_uframes_t samples = nframes - xfer;for (channel = 0; channel < io->channels; channel++)snd_pcm_area_silence(&jack->areas[channel], xfer, samples, io->format);}- }
Ditto. Basically the whole procedure here is some common pattern, so it wouldn't be too bad to have it in ioplug side.
thanks,
Takashi
Hello Takashi,
The following patches introduce two helper functions: - snd_pcm_ioplug_hw_avail() Provides the available frames for the hardware/DMA - snd_pcm_areas_copy_wrap() considers the buffer_size to split the copy operation into several parts
With the usage of this helper functions the complexity of the JACK plugin is reduced. Please have a look and give some feedback.
I have not yet compiled nor tested it.
Best regards
Timo
From: Timo Wischer twischer@de.adit-jv.com
This function can be called without calling snd_pcm_avail_update().
The call to snd_pcm_avail_update() can take some time. Therefore some developers would not like to call it from a real-time context (e.g. from JACK client context).
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
diff --git a/include/pcm_ioplug.h b/include/pcm_ioplug.h index e75f973..778d9c6 100644 --- a/include/pcm_ioplug.h +++ b/include/pcm_ioplug.h @@ -234,6 +234,9 @@ int snd_pcm_ioplug_set_param_list(snd_pcm_ioplug_t *io, int type, unsigned int n /* change PCM status */ int snd_pcm_ioplug_set_state(snd_pcm_ioplug_t *ioplug, snd_pcm_state_t state);
+/* calucalte the available frames */ +snd_pcm_uframes_t snd_pcm_ioplug_hw_avail(const snd_pcm_ioplug_t* const ioplug, const snd_pcm_uframes_t hw_ptr, const snd_pcm_uframes_t appl_ptr); + /** } */
#endif /* __ALSA_PCM_IOPLUG_H */ diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c index 9970646..a0cb778 100644 --- a/src/pcm/pcm_ioplug.c +++ b/src/pcm/pcm_ioplug.c @@ -1148,3 +1148,27 @@ int snd_pcm_ioplug_set_state(snd_pcm_ioplug_t *ioplug, snd_pcm_state_t state) ioplug->state = state; return 0; } + +/** + * \brief Get the available frames. This function can be used to calculate the + * the available frames before calling #snd_pcm_avail_update() + * \param ioplug the ioplug handle + * \param hw_ptr hardware pointer in frames + * \param appl_ptr application pointer in frames + * \return available frames for the hardware + */ +snd_pcm_uframes_t snd_pcm_ioplug_hw_avail(const snd_pcm_ioplug_t* const ioplug, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) +{ + /* available data/space which can be transfered by the user application */ + const snd_pcm_uframes_t user_avail = __snd_pcm_avail(ioplug->pcm, hw_ptr, + appl_ptr); + + if (user_avail > ioplug->pcm->buffer_size) { + /* there was an Xrun */ + return 0; + } + /* available data/space which can be transfered by the DMA */ + return ioplug->pcm->buffer_size - user_avail; +} diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index 3d95e17..b695b92 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -467,10 +467,12 @@ static inline int snd_pcm_check_error(snd_pcm_t *pcm, int err) return err; }
-static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm) +static inline snd_pcm_uframes_t __snd_pcm_playback_avail(snd_pcm_t *pcm, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) { snd_pcm_sframes_t avail; - avail = *pcm->hw.ptr + pcm->buffer_size - *pcm->appl.ptr; + avail = hw_ptr + pcm->buffer_size - appl_ptr; if (avail < 0) avail += pcm->boundary; else if ((snd_pcm_uframes_t) avail >= pcm->boundary) @@ -478,21 +480,40 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm) return avail; }
-static inline snd_pcm_uframes_t snd_pcm_mmap_capture_avail(snd_pcm_t *pcm) +static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm) +{ + return __snd_pcm_playback_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr); +} + +static inline snd_pcm_uframes_t __snd_pcm_capture_avail(snd_pcm_t *pcm, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) { snd_pcm_sframes_t avail; - avail = *pcm->hw.ptr - *pcm->appl.ptr; + avail = hw_ptr - appl_ptr; if (avail < 0) avail += pcm->boundary; return avail; }
-static inline snd_pcm_uframes_t snd_pcm_mmap_avail(snd_pcm_t *pcm) +static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm) +{ + return __snd_pcm_capture_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr); +} + +static inline snd_pcm_uframes_t __snd_pcm_avail(snd_pcm_t *pcm, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) { if (pcm->stream == SND_PCM_STREAM_PLAYBACK) - return snd_pcm_mmap_playback_avail(pcm); + return snd_pcm_playback_avail(pcm, hw_ptr, appl_ptr); else - return snd_pcm_mmap_capture_avail(pcm); + return snd_pcm_capture_avail(pcm, hw_ptr, appl_ptr); +} + +static inline snd_pcm_uframes_t snd_pcm_mmap_avail(snd_pcm_t *pcm) +{ + return __snd_pcm_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr); }
static inline snd_pcm_sframes_t snd_pcm_mmap_playback_hw_avail(snd_pcm_t *pcm)
From: Timo Wischer twischer@de.adit-jv.com
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
diff --git a/include/pcm.h b/include/pcm.h index 2619c8c..d880a1c 100644 --- a/include/pcm.h +++ b/include/pcm.h @@ -1147,6 +1147,9 @@ int snd_pcm_area_copy(const snd_pcm_channel_area_t *dst_channel, snd_pcm_uframes int snd_pcm_areas_copy(const snd_pcm_channel_area_t *dst_channels, snd_pcm_uframes_t dst_offset, const snd_pcm_channel_area_t *src_channels, snd_pcm_uframes_t src_offset, unsigned int channels, snd_pcm_uframes_t frames, snd_pcm_format_t format); +int snd_pcm_areas_copy_wrap(const snd_pcm_channel_area_t *dst_channels, snd_pcm_uframes_t dst_offset, const snd_pcm_uframes_t dst_size, + const snd_pcm_channel_area_t *src_channels, snd_pcm_uframes_t src_offset, const snd_pcm_uframes_t src_size, + const unsigned int channels, snd_pcm_uframes_t frames, const snd_pcm_format_t format);
/** } */
diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c index d53ed98..77898db 100644 --- a/src/pcm/pcm.c +++ b/src/pcm/pcm.c @@ -3289,6 +3289,36 @@ int snd_pcm_areas_copy(const snd_pcm_channel_area_t *dst_areas, snd_pcm_uframes_ return 0; }
+int snd_pcm_areas_copy_wrap(const snd_pcm_channel_area_t *dst_channels, snd_pcm_uframes_t dst_offset, const snd_pcm_uframes_t dst_size, + const snd_pcm_channel_area_t *src_channels, snd_pcm_uframes_t src_offset, const snd_pcm_uframes_t src_size, + const unsigned int channels, snd_pcm_uframes_t frames, const snd_pcm_format_t format) +{ + while (frames > 0) { + int err; + snd_pcm_uframes_t xfer = frames; + /* do not write above the destination buffer */ + if ( (dst_offset + xfer) > dst_size ) + xfer = dst_size - dst_offset; + /* do not read from above the source buffer */ + if ( (src_offset + xfer) > src_size ) + xfer = src_size - src_offset; + err = snd_pcm_areas_copy(dst_channels, dst_offset, src_channels, + src_offset, channels, xfer, format); + if (err < 0) + return err; + + dst_offset += xfer; + if (dst_offset >= dst_size) + dst_offset = 0; + src_offset += xfer; + if (src_offset >= src_size) + src_offset = 0; + frames -= xfer; + } + + return 0; +} + static void dump_one_param(snd_pcm_hw_params_t *params, unsigned int k, snd_output_t *out) { snd_output_printf(out, "%s: ", snd_pcm_hw_param_name(k));
From: Timo Wischer twischer@de.adit-jv.com
when the JACK thread is requesting too many audio frames
Playback: Without this commit the ALSA audio buffer will be played with endless repeats as long as the user application has not provided new audio data. Therefore this garbage will be played as long as the user application has not called snd_pcm_stop() after an Xrun. With this fix the rest of the JACK buffer will be filled with silence.
Capture: Without this commit the audio data in the ALSA buffer would be overwritten. With this commit the new data from the JACK buffer will not be copied. Therefore the existing data in the ALSA buffer will not be overwritten.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index c547cbb..18425e9 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -54,6 +54,7 @@ typedef struct {
static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io);
+ static int pcm_poll_block_check(snd_pcm_ioplug_t *io) { static char buf[32]; @@ -144,7 +145,6 @@ 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 hw_ptr; - const snd_pcm_channel_area_t *areas; snd_pcm_uframes_t xfer = 0; unsigned int channel; @@ -154,40 +154,43 @@ 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; - } - }
hw_ptr = jack->hw_ptr; - areas = snd_pcm_ioplug_mmap_areas(io); + if (io->state == SND_PCM_STATE_RUNNING) { + const snd_pcm_channel_area_t *areas = snd_pcm_ioplug_mmap_areas(io); + + snd_pcm_uframes_t hw_avail = snd_pcm_ioplug_hw_avail(io, hw_ptr, + io->appl_ptr);
- while (xfer < nframes) { - snd_pcm_uframes_t frames = nframes - xfer; - snd_pcm_uframes_t offset = hw_ptr % io->buffer_size; - snd_pcm_uframes_t cont = io->buffer_size - offset; + if (hw_avail > 0) { + snd_pcm_uframes_t frames = nframes - xfer; + snd_pcm_uframes_t offset = hw_ptr % io->buffer_size;
- 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); + snd_pcm_areas_copy_wrap(jack->areas, xfer, io->buffer_size, areas, offset, io->buffer_size, io->channels, frames, io->format); else - snd_pcm_area_copy(&areas[channel], offset, &jack->areas[channel], xfer, frames, io->format); + snd_pcm_areas_copy_wrap(areas, offset, io->buffer_size, jack->areas, xfer, io->buffer_size, io->channels, frames, io->format); + + hw_ptr += frames; + if (hw_ptr >= jack->boundary) + hw_ptr -= jack->boundary; + xfer += frames; } - - hw_ptr += frames; - if (hw_ptr >= jack->boundary) - hw_ptr -= jack->boundary; - xfer += frames; } jack->hw_ptr = hw_ptr;
+ /* 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 snd_pcm_uframes_t frames = nframes - xfer; + snd_pcm_areas_silence(jack->areas, io->channels, xfer, frames, io->format); + } + } + pcm_poll_unblock_check(io); /* unblock socket for polling if needed */
return 0;
Hello Takashi,
Now I have compiled and tested the patchies successfully. Some minor adaption where required. I have incorporated all your findings.
Please try to format the text a bit nicer. These line breaks appear like you're writing a poem :)i
Done.
You don't need to put inline unless it's really mandatory.
Done.
Hm, this whole stuff would fit better in ioplug code itself instead of open-coding in each plugin. Maybe providing a new helper function?
I am providing a new helper function called snd_pcm_ioplug_hw_avail() and I am reusing internally already existing functions.
Ditto. Basically the whole procedure here is some common pattern, so it wouldn't be too bad to have it in ioplug side.
I have provided a new area copy function which takes care about the buffer wrap around. This function is called snd_pcm_areas_copy_wrap(). This reduces the complexity of the JACK IO plug a lot. I have also compared the new implementation with other ALSA IO plugins. I could not found more simuilarities because not all plugins are creating a snd_pcm_channel_area_t for the DMA/HW buffer. Therefore functions like snd_pcm_areas_silence() and snd_pcm_areas_copy() cannot be used. For example the pulse IO plug is also not using snd_pcm_channel_area_t for the pulse buffer.
I hope you are fine with these changes, now. If not it would be great if you could give me a feedback in time.
Best regards
Timo
From: Timo Wischer twischer@de.adit-jv.com
This function can be called without calling snd_pcm_avail_update().
The call to snd_pcm_avail_update() can take some time. Therefore some developers would not like to call it from a real-time context (e.g. from JACK client context).
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
diff --git a/include/pcm_ioplug.h b/include/pcm_ioplug.h index e75f973..c1310e3 100644 --- a/include/pcm_ioplug.h +++ b/include/pcm_ioplug.h @@ -234,6 +234,11 @@ int snd_pcm_ioplug_set_param_list(snd_pcm_ioplug_t *io, int type, unsigned int n /* change PCM status */ int snd_pcm_ioplug_set_state(snd_pcm_ioplug_t *ioplug, snd_pcm_state_t state);
+/* calucalte the available frames */ +snd_pcm_uframes_t snd_pcm_ioplug_hw_avail(const snd_pcm_ioplug_t * const ioplug, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr); + /** } */
#endif /* __ALSA_PCM_IOPLUG_H */ diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c index 9970646..af223a1 100644 --- a/src/pcm/pcm_ioplug.c +++ b/src/pcm/pcm_ioplug.c @@ -1148,3 +1148,29 @@ int snd_pcm_ioplug_set_state(snd_pcm_ioplug_t *ioplug, snd_pcm_state_t state) ioplug->state = state; return 0; } + +/** + * \brief Get the available frames. This function can be used to calculate the + * the available frames before calling #snd_pcm_avail_update() + * \param ioplug the ioplug handle + * \param hw_ptr hardware pointer in frames + * \param appl_ptr application pointer in frames + * \return available frames for the hardware + */ +snd_pcm_uframes_t snd_pcm_ioplug_hw_avail(const snd_pcm_ioplug_t * const ioplug, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) +{ + /* available data/space which can be transferred by the user + * application + */ + const snd_pcm_uframes_t user_avail = __snd_pcm_avail(ioplug->pcm, + hw_ptr, appl_ptr); + + if (user_avail > ioplug->pcm->buffer_size) { + /* there was an Xrun */ + return 0; + } + /* available data/space which can be transferred by the DMA */ + return ioplug->pcm->buffer_size - user_avail; +} diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index 3d95e17..d52229d 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -467,10 +467,12 @@ static inline int snd_pcm_check_error(snd_pcm_t *pcm, int err) return err; }
-static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm) +static inline snd_pcm_uframes_t __snd_pcm_playback_avail(snd_pcm_t *pcm, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) { snd_pcm_sframes_t avail; - avail = *pcm->hw.ptr + pcm->buffer_size - *pcm->appl.ptr; + avail = hw_ptr + pcm->buffer_size - appl_ptr; if (avail < 0) avail += pcm->boundary; else if ((snd_pcm_uframes_t) avail >= pcm->boundary) @@ -478,21 +480,40 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm) return avail; }
-static inline snd_pcm_uframes_t snd_pcm_mmap_capture_avail(snd_pcm_t *pcm) +static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm) +{ + return __snd_pcm_playback_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr); +} + +static inline snd_pcm_uframes_t __snd_pcm_capture_avail(snd_pcm_t *pcm, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) { snd_pcm_sframes_t avail; - avail = *pcm->hw.ptr - *pcm->appl.ptr; + avail = hw_ptr - appl_ptr; if (avail < 0) avail += pcm->boundary; return avail; }
-static inline snd_pcm_uframes_t snd_pcm_mmap_avail(snd_pcm_t *pcm) +static inline snd_pcm_uframes_t snd_pcm_mmap_capture_avail(snd_pcm_t *pcm) +{ + return __snd_pcm_capture_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr); +} + +static inline snd_pcm_uframes_t __snd_pcm_avail(snd_pcm_t *pcm, + const snd_pcm_uframes_t hw_ptr, + const snd_pcm_uframes_t appl_ptr) { if (pcm->stream == SND_PCM_STREAM_PLAYBACK) - return snd_pcm_mmap_playback_avail(pcm); + return __snd_pcm_playback_avail(pcm, hw_ptr, appl_ptr); else - return snd_pcm_mmap_capture_avail(pcm); + return __snd_pcm_capture_avail(pcm, hw_ptr, appl_ptr); +} + +static inline snd_pcm_uframes_t snd_pcm_mmap_avail(snd_pcm_t *pcm) +{ + return __snd_pcm_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr); }
static inline snd_pcm_sframes_t snd_pcm_mmap_playback_hw_avail(snd_pcm_t *pcm)
From: Timo Wischer twischer@de.adit-jv.com
The already existing areas_copy functions do not care about the end of the source and destination buffer. Therefore the caller has to take care that the requested offset+size is not exceeding any buffer limit.
This additional function will take care about the end of an buffer and will continue at the beginning of the buffer. For example this is required when copying between buffers with different sizes (not multiple of). This is often the case in IO plugins like the JACK plugin.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
diff --git a/include/pcm.h b/include/pcm.h index 2619c8c..e2a5343 100644 --- a/include/pcm.h +++ b/include/pcm.h @@ -1147,6 +1147,15 @@ int snd_pcm_area_copy(const snd_pcm_channel_area_t *dst_channel, snd_pcm_uframes int snd_pcm_areas_copy(const snd_pcm_channel_area_t *dst_channels, snd_pcm_uframes_t dst_offset, const snd_pcm_channel_area_t *src_channels, snd_pcm_uframes_t src_offset, unsigned int channels, snd_pcm_uframes_t frames, snd_pcm_format_t format); +int snd_pcm_areas_copy_wrap(const snd_pcm_channel_area_t *dst_channels, + snd_pcm_uframes_t dst_offset, + const snd_pcm_uframes_t dst_size, + const snd_pcm_channel_area_t *src_channels, + snd_pcm_uframes_t src_offset, + const snd_pcm_uframes_t src_size, + const unsigned int channels, + snd_pcm_uframes_t frames, + const snd_pcm_format_t format);
/** } */
diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c index d53ed98..ed47cb5 100644 --- a/src/pcm/pcm.c +++ b/src/pcm/pcm.c @@ -3289,6 +3289,55 @@ int snd_pcm_areas_copy(const snd_pcm_channel_area_t *dst_areas, snd_pcm_uframes_ return 0; }
+/** + * \brief Copy one or more areas + * \param dst_areas destination areas specification (one for each channel) + * \param dst_offset offset in frames inside destination area + * \param dst_size size in frames of the destination buffer + * \param src_areas source areas specification (one for each channel) + * \param src_offset offset in frames inside source area + * \param dst_size size in frames of the source buffer + * \param channels channels count + * \param frames frames to copy + * \param format PCM sample format + * \return 0 on success otherwise a negative error code + */ +int snd_pcm_areas_copy_wrap(const snd_pcm_channel_area_t *dst_channels, + snd_pcm_uframes_t dst_offset, + const snd_pcm_uframes_t dst_size, + const snd_pcm_channel_area_t *src_channels, + snd_pcm_uframes_t src_offset, + const snd_pcm_uframes_t src_size, + const unsigned int channels, + snd_pcm_uframes_t frames, + const snd_pcm_format_t format) +{ + while (frames > 0) { + int err; + snd_pcm_uframes_t xfer = frames; + /* do not write above the destination buffer */ + if ((dst_offset + xfer) > dst_size) + xfer = dst_size - dst_offset; + /* do not read from above the source buffer */ + if ((src_offset + xfer) > src_size) + xfer = src_size - src_offset; + err = snd_pcm_areas_copy(dst_channels, dst_offset, src_channels, + src_offset, channels, xfer, format); + if (err < 0) + return err; + + dst_offset += xfer; + if (dst_offset >= dst_size) + dst_offset = 0; + src_offset += xfer; + if (src_offset >= src_size) + src_offset = 0; + frames -= xfer; + } + + return 0; +} + static void dump_one_param(snd_pcm_hw_params_t *params, unsigned int k, snd_output_t *out) { snd_output_printf(out, "%s: ", snd_pcm_hw_param_name(k));
From: Timo Wischer twischer@de.adit-jv.com
when the JACK thread is requesting too many audio frames
Playback: Without this commit the ALSA audio buffer will be played with endless repeats as long as the user application has not provided new audio data. Therefore this garbage will be played as long as the user application has not called snd_pcm_stop() after an Xrun. With this fix the rest of the JACK buffer will be filled with silence.
Capture: Without this commit the audio data in the ALSA buffer would be overwritten. With this commit the new data from the JACK buffer will not be copied. Therefore the existing data in the ALSA buffer will not be overwritten.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index c547cbb..7c7c230 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -54,6 +54,7 @@ typedef struct {
static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io);
+ static int pcm_poll_block_check(snd_pcm_ioplug_t *io) { static char buf[32]; @@ -143,8 +144,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; - snd_pcm_uframes_t hw_ptr; - const snd_pcm_channel_area_t *areas; snd_pcm_uframes_t xfer = 0; unsigned int channel; @@ -154,39 +153,50 @@ 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; - } - }
- hw_ptr = jack->hw_ptr; - areas = snd_pcm_ioplug_mmap_areas(io); + if (io->state == SND_PCM_STATE_RUNNING) { + snd_pcm_uframes_t hw_ptr = jack->hw_ptr; + const snd_pcm_uframes_t hw_avail = snd_pcm_ioplug_hw_avail(io, hw_ptr, + io->appl_ptr);
- while (xfer < nframes) { - snd_pcm_uframes_t frames = nframes - xfer; - snd_pcm_uframes_t offset = hw_ptr % io->buffer_size; - snd_pcm_uframes_t cont = io->buffer_size - offset; + if (hw_avail > 0) { + const snd_pcm_channel_area_t *areas = snd_pcm_ioplug_mmap_areas(io); + const snd_pcm_uframes_t offset = hw_ptr % io->buffer_size;
- if (cont < frames) - frames = cont; + xfer = nframes; + if (xfer > hw_avail) + xfer = 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); + snd_pcm_areas_copy_wrap(jack->areas, 0, nframes, + areas, offset, + io->buffer_size, + io->channels, xfer, + io->format); else - snd_pcm_area_copy(&areas[channel], offset, &jack->areas[channel], xfer, frames, io->format); + snd_pcm_areas_copy_wrap(areas, offset, + io->buffer_size, + jack->areas, 0, nframes, + io->channels, xfer, + io->format); + + hw_ptr += xfer; + if (hw_ptr >= jack->boundary) + hw_ptr -= jack->boundary; + jack->hw_ptr = hw_ptr; + } + } + + /* 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 snd_pcm_uframes_t frames = nframes - xfer; + + snd_pcm_areas_silence(jack->areas, io->channels, xfer, + frames, io->format); } - - hw_ptr += frames; - if (hw_ptr >= jack->boundary) - hw_ptr -= jack->boundary; - xfer += frames; } - jack->hw_ptr = hw_ptr;
pcm_poll_unblock_check(io); /* unblock socket for polling if needed */
On Tue, 13 Mar 2018 09:34:41 +0100, twischer@de.adit-jv.com wrote:
Hello Takashi,
Now I have compiled and tested the patchies successfully. Some minor adaption where required. I have incorporated all your findings.
Please try to format the text a bit nicer. These line breaks appear like you're writing a poem :)i
Done.
You don't need to put inline unless it's really mandatory.
Done.
Hm, this whole stuff would fit better in ioplug code itself instead of open-coding in each plugin. Maybe providing a new helper function?
I am providing a new helper function called snd_pcm_ioplug_hw_avail() and I am reusing internally already existing functions.
Ditto. Basically the whole procedure here is some common pattern, so it wouldn't be too bad to have it in ioplug side.
I have provided a new area copy function which takes care about the buffer wrap around. This function is called snd_pcm_areas_copy_wrap(). This reduces the complexity of the JACK IO plug a lot. I have also compared the new implementation with other ALSA IO plugins. I could not found more simuilarities because not all plugins are creating a snd_pcm_channel_area_t for the DMA/HW buffer. Therefore functions like snd_pcm_areas_silence() and snd_pcm_areas_copy() cannot be used. For example the pulse IO plug is also not using snd_pcm_channel_area_t for the pulse buffer.
I hope you are fine with these changes, now. If not it would be great if you could give me a feedback in time.
Thanks, now applied both patches to alsa-lib and the patch to jack plugin. For alsa-plugins, we'll need the update of configure.ac to check the latest alsa-lib version, but let's do it later at bumping actually the version.
Takashi
Hello Takashi,
Thanks, now applied both patches to alsa-lib and the patch to jack plugin.
I fear you missed the ALSA lib changes [1], right?
[1] http://git.alsa-project.org/?p=alsa-lib.git;a=summary
Best regards
Timo Wischer
Advanced Driver Information Technology GmbH Engineering Software Base (ADITG/ESB) Robert-Bosch-Str. 200 31139 Hildesheim Germany
Tel. +49 5121 49 6938 Fax +49 5121 49 6999 twischer@de.adit-jv.com
ADIT is a joint venture company of Robert Bosch GmbH/Robert Bosch Car Multimedia GmbH and DENSO Corporation Sitz: Hildesheim, Registergericht: Amtsgericht Hildesheim HRB 3438 Geschäftsführung: Wilhelm Grabow, Ken Yaguchi
On Thu, 15 Mar 2018 16:09:41 +0100, Wischer, Timo (ADITG/ESB) wrote:
Hello Takashi,
Thanks, now applied both patches to alsa-lib and the patch to jack plugin.
I fear you missed the ALSA lib changes [1], right?
Sorry, forgot to push. Now pushed out.
thanks,
Takashi
From: Timo Wischer twischer@de.adit-jv.com
This variable will be used to exchange the status of the stream between the ALSA and JACK thread. In future commits it will also be used to signal DRAINING state from the ALSA to JACK thread and to signal XRUN state form the JACK to ALSA thread
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index c3ed9fb..0dff0d2 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -36,7 +36,11 @@ typedef struct { snd_pcm_ioplug_t io;
int fd; - int activated; /* jack is activated? */ + + /* This variable is not always in sync with io->state + * because it will be accessed by multiple threads. + */ + snd_pcm_state_t state;
char **port_names; unsigned int num_ports; @@ -122,8 +126,8 @@ static int pcm_poll_block_check(snd_pcm_ioplug_t *io) snd_pcm_sframes_t avail; snd_pcm_jack_t *jack = io->private_data;
- if (io->state == SND_PCM_STATE_RUNNING || - (io->state == SND_PCM_STATE_PREPARED && io->stream == SND_PCM_STREAM_CAPTURE)) { + if (jack->state == SND_PCM_STATE_RUNNING || + (jack->state == SND_PCM_STATE_PREPARED && io->stream == SND_PCM_STREAM_CAPTURE)) { avail = snd_pcm_avail_update(io->pcm); if (avail >= 0 && avail < jack->min_avail) { while (read(io->poll_fd, &buf, sizeof(buf)) == sizeof(buf)) @@ -217,7 +221,7 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) }
hw_ptr = jack->hw_ptr; - if (io->state == SND_PCM_STATE_RUNNING) { + if (jack->state == SND_PCM_STATE_RUNNING) { const snd_pcm_channel_area_t *areas = snd_pcm_ioplug_mmap_areas(io);
while (xfer < nframes) { @@ -297,29 +301,31 @@ static int snd_pcm_jack_prepare(snd_pcm_ioplug_t *io) else pcm_poll_block_check(io); /* block capture pcm if that's XRUN recovery */
- if (jack->ports) - return 0; + if (!jack->ports) { + jack->ports = calloc(io->channels, sizeof(jack_port_t*));
- jack->ports = calloc(io->channels, sizeof(jack_port_t*)); - - for (i = 0; i < io->channels; i++) { - char port_name[32]; - if (io->stream == SND_PCM_STREAM_PLAYBACK) { + for (i = 0; i < io->channels; i++) { + char port_name[32]; + if (io->stream == SND_PCM_STREAM_PLAYBACK) {
- sprintf(port_name, "out_%03d", i); - jack->ports[i] = jack_port_register(jack->client, port_name, - JACK_DEFAULT_AUDIO_TYPE, - JackPortIsOutput, 0); - } else { - sprintf(port_name, "in_%03d", i); - jack->ports[i] = jack_port_register(jack->client, port_name, - JACK_DEFAULT_AUDIO_TYPE, - JackPortIsInput, 0); + sprintf(port_name, "out_%03d", i); + jack->ports[i] = jack_port_register(jack->client, port_name, + JACK_DEFAULT_AUDIO_TYPE, + JackPortIsOutput, 0); + } else { + sprintf(port_name, "in_%03d", i); + jack->ports[i] = jack_port_register(jack->client, port_name, + JACK_DEFAULT_AUDIO_TYPE, + JackPortIsInput, 0); + } } + + jack_set_process_callback(jack->client, + (JackProcessCallback)snd_pcm_jack_process_cb, io); }
- jack_set_process_callback(jack->client, - (JackProcessCallback)snd_pcm_jack_process_cb, io); + jack->state = SND_PCM_STATE_PREPARED; + return 0; }
@@ -327,12 +333,11 @@ static int snd_pcm_jack_start(snd_pcm_ioplug_t *io) { snd_pcm_jack_t *jack = io->private_data; unsigned int i; + int err = 0; 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; @@ -345,21 +350,27 @@ static int snd_pcm_jack_start(snd_pcm_ioplug_t *io) } if (jack_connect(jack->client, src, dst)) { fprintf(stderr, "cannot connect %s to %s\n", src, dst); - return -EIO; + err = -EIO; + break; } } } + + /* do not start forwarding audio samples before the ports are connected */ + jack->state = SND_PCM_STATE_RUNNING; - return 0; + return err; }
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 = jack->state; + + if (state == SND_PCM_STATE_RUNNING || + state == SND_PCM_STATE_DRAINING || + state == SND_PCM_STATE_XRUN) { jack_deactivate(jack->client); - jack->activated = 0; } #if 0 unsigned i; @@ -370,6 +381,9 @@ static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io) } } #endif + + jack->state = SND_PCM_STATE_SETUP; + return 0; }
@@ -481,6 +495,7 @@ static int snd_pcm_jack_open(snd_pcm_t **pcmp, const char *name,
jack->fd = -1; jack->io.poll_fd = -1; + jack->state = SND_PCM_STATE_OPEN;
err = parse_ports(jack, stream == SND_PCM_STREAM_PLAYBACK ? playback_conf : capture_conf);
On Thu, 01 Mar 2018 14:14:06 +0100, twischer@de.adit-jv.com wrote:
From: Timo Wischer twischer@de.adit-jv.com
This variable will be used to exchange the status of the stream between the ALSA and JACK thread. In future commits it will also be used to signal DRAINING state from the ALSA to JACK thread and to signal XRUN state form the JACK to ALSA thread
So in which situation would io->state and jack->state differ with each other? Only DRAINING and XRUN, or in other cases?
Takashi
Hello Takashi,
So in which situation would io->state and jack->state differ with each other? Only DRAINING and XRUN, or in other cases?
I have extended the commit message:
Therefore this internal state variable is not always in sync with io->state. In addition the internal state variable will be updated after the corresponding callback function was processed and not before the callback function was called (e.g. PREPARE and RUNNING).
Therefore the internal state will always contain the state which is currently active. It will keep the old state as long as the transition to the new one was successfully finished
See attached the updated patch which can be applied without conflicts.
Best regards,
Timo
From: Timo Wischer twischer@de.adit-jv.com
This variable will be used to exchange the status of the stream between the ALSA and JACK thread. In future commits it will also be used to signal DRAINING state from the ALSA to JACK thread and to signal XRUN state form the JACK to ALSA thread.
Therefore this internal state variable is not always in sync with io->state. In addition the internal state variable will be updated after the corresponding callback function was processed and not before the callback function was called (e.g. PREPARE and RUNNING).
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index 7c7c230..5932ca2 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -36,7 +36,11 @@ typedef struct { snd_pcm_ioplug_t io;
int fd; - int activated; /* jack is activated? */ + + /* This variable is not always in sync with io->state + * because it will be accessed by multiple threads. + */ + snd_pcm_state_t state;
char **port_names; unsigned int num_ports; @@ -61,8 +65,8 @@ static int pcm_poll_block_check(snd_pcm_ioplug_t *io) snd_pcm_sframes_t avail; snd_pcm_jack_t *jack = io->private_data;
- if (io->state == SND_PCM_STATE_RUNNING || - (io->state == SND_PCM_STATE_PREPARED && io->stream == SND_PCM_STREAM_CAPTURE)) { + if (jack->state == SND_PCM_STATE_RUNNING || + (jack->state == SND_PCM_STATE_PREPARED && io->stream == SND_PCM_STREAM_CAPTURE)) { avail = snd_pcm_avail_update(io->pcm); if (avail >= 0 && avail < jack->min_avail) { while (read(io->poll_fd, &buf, sizeof(buf)) == sizeof(buf)) @@ -154,7 +158,7 @@ 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 (jack->state == SND_PCM_STATE_RUNNING) { snd_pcm_uframes_t hw_ptr = jack->hw_ptr; const snd_pcm_uframes_t hw_avail = snd_pcm_ioplug_hw_avail(io, hw_ptr, io->appl_ptr); @@ -229,29 +233,31 @@ static int snd_pcm_jack_prepare(snd_pcm_ioplug_t *io) else pcm_poll_block_check(io); /* block capture pcm if that's XRUN recovery */
- if (jack->ports) - return 0; - - jack->ports = calloc(io->channels, sizeof(jack_port_t*)); + if (!jack->ports) { + jack->ports = calloc(io->channels, sizeof(jack_port_t*));
- for (i = 0; i < io->channels; i++) { - char port_name[32]; - if (io->stream == SND_PCM_STREAM_PLAYBACK) { + for (i = 0; i < io->channels; i++) { + char port_name[32]; + if (io->stream == SND_PCM_STREAM_PLAYBACK) {
- sprintf(port_name, "out_%03d", i); - jack->ports[i] = jack_port_register(jack->client, port_name, - JACK_DEFAULT_AUDIO_TYPE, - JackPortIsOutput, 0); - } else { - sprintf(port_name, "in_%03d", i); - jack->ports[i] = jack_port_register(jack->client, port_name, - JACK_DEFAULT_AUDIO_TYPE, - JackPortIsInput, 0); + sprintf(port_name, "out_%03d", i); + jack->ports[i] = jack_port_register(jack->client, port_name, + JACK_DEFAULT_AUDIO_TYPE, + JackPortIsOutput, 0); + } else { + sprintf(port_name, "in_%03d", i); + jack->ports[i] = jack_port_register(jack->client, port_name, + JACK_DEFAULT_AUDIO_TYPE, + JackPortIsInput, 0); + } } + + jack_set_process_callback(jack->client, + (JackProcessCallback)snd_pcm_jack_process_cb, io); }
- jack_set_process_callback(jack->client, - (JackProcessCallback)snd_pcm_jack_process_cb, io); + jack->state = SND_PCM_STATE_PREPARED; + return 0; }
@@ -259,12 +265,11 @@ static int snd_pcm_jack_start(snd_pcm_ioplug_t *io) { snd_pcm_jack_t *jack = io->private_data; unsigned int i; + int err = 0; 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; @@ -277,21 +282,27 @@ static int snd_pcm_jack_start(snd_pcm_ioplug_t *io) } if (jack_connect(jack->client, src, dst)) { fprintf(stderr, "cannot connect %s to %s\n", src, dst); - return -EIO; + err = -EIO; + break; } } } + + /* do not start forwarding audio samples before the ports are connected */ + jack->state = SND_PCM_STATE_RUNNING; - return 0; + return err; }
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 = jack->state; + + if (state == SND_PCM_STATE_RUNNING || + state == SND_PCM_STATE_DRAINING || + state == SND_PCM_STATE_XRUN) { jack_deactivate(jack->client); - jack->activated = 0; } #if 0 unsigned i; @@ -302,6 +313,9 @@ static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io) } } #endif + + jack->state = SND_PCM_STATE_SETUP; + return 0; }
@@ -413,6 +427,7 @@ static int snd_pcm_jack_open(snd_pcm_t **pcmp, const char *name,
jack->fd = -1; jack->io.poll_fd = -1; + jack->state = SND_PCM_STATE_OPEN;
err = parse_ports(jack, stream == SND_PCM_STREAM_PLAYBACK ? playback_conf : capture_conf);
On Thu, 15 Mar 2018 13:56:27 +0100, twischer@de.adit-jv.com wrote:
From: Timo Wischer twischer@de.adit-jv.com
This variable will be used to exchange the status of the stream between the ALSA and JACK thread. In future commits it will also be used to signal DRAINING state from the ALSA to JACK thread and to signal XRUN state form the JACK to ALSA thread.
Therefore this internal state variable is not always in sync with io->state. In addition the internal state variable will be updated after the corresponding callback function was processed and not before the callback function was called (e.g. PREPARE and RUNNING).
Well, the fact that the state change is done for PREPARE before the plugin callback can be seen as a generic bug of ioplug. The best would be to fix in ioplug side.
And, the rest is RUNNING state. Is there anything else? For RUNNING, instead of keeping an internal shadow state, a saner way would be to have a jack->running bool flag indicating whether it's really running.
thanks,
Takashi
Hello Takashi,
Well, the fact that the state change is done for PREPARE before the plugin callback can be seen as a generic bug of ioplug. The best would be to fix in ioplug side.
This change could possibly brake other existing IO plugins. Do you think we should really change it?
For RUNNING, instead of keeping an internal shadow state, a saner way would be to have a jack->running bool flag indicating whether it's really running.
With the following pending commits [1] and [2] we would than have 3 additional variables to indicate - running - xruns - draining
My idea was to avoid this variables and use the already existing snd_pcm_state enum.
With further commits which I have not yet sent I want to make the JACK plugin thread save. But instead of using locking I am using atomic lock-less access (to avoid blocking of the JACK daemon). Therefore I cannot call snd_pcm_state() to get the internal state. I have to forward the status by my own to the JACK thread. Due to that I need an internal variable which will be atomically accessed and represents the state. (I think it is not intended by ALSA to extend the ALSA library to update the internal ALSA state atomically so that it can be read atomically from any thread. Therefore I have not yet another idea how to transfer the state to the JACK thread with atomic lock-less access.)
I could replace the assignments to the internal state in all IO plug callback functions in the JACK plugin with jack->state = io->state But I am not sure if this is really helpful for the understanding.
[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-March/132725.html [2] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-March/132726.html
Best regards
Timo Wischer
Advanced Driver Information Technology GmbH Engineering Software Base (ADITG/ESB) Robert-Bosch-Str. 200 31139 Hildesheim Germany
Tel. +49 5121 49 6938 Fax +49 5121 49 6999 twischer@de.adit-jv.com
ADIT is a joint venture company of Robert Bosch GmbH/Robert Bosch Car Multimedia GmbH and DENSO Corporation Sitz: Hildesheim, Registergericht: Amtsgericht Hildesheim HRB 3438 Geschäftsführung: Wilhelm Grabow, Ken Yaguchi
________________________________________ From: Takashi Iwai [tiwai@suse.de] Sent: Thursday, March 15, 2018 2:14 PM To: Wischer, Timo (ADITG/ESB) Cc: alsa-devel@alsa-project.org Subject: Re: [PATCH - JACK 1/1] jack: Use internal snd_pcm_state to reduce amount of additional variables
On Thu, 15 Mar 2018 13:56:27 +0100, twischer@de.adit-jv.com wrote:
From: Timo Wischer twischer@de.adit-jv.com
This variable will be used to exchange the status of the stream between the ALSA and JACK thread. In future commits it will also be used to signal DRAINING state from the ALSA to JACK thread and to signal XRUN state form the JACK to ALSA thread.
Therefore this internal state variable is not always in sync with io->state. In addition the internal state variable will be updated after the corresponding callback function was processed and not before the callback function was called (e.g. PREPARE and RUNNING).
Well, the fact that the state change is done for PREPARE before the plugin callback can be seen as a generic bug of ioplug. The best would be to fix in ioplug side.
And, the rest is RUNNING state. Is there anything else? For RUNNING, instead of keeping an internal shadow state, a saner way would be to have a jack->running bool flag indicating whether it's really running.
thanks,
Takashi
On Thu, 15 Mar 2018 15:05:39 +0100, Wischer, Timo (ADITG/ESB) wrote:
Hello Takashi,
Well, the fact that the state change is done for PREPARE before the plugin callback can be seen as a generic bug of ioplug. The best would be to fix in ioplug side.
This change could possibly brake other existing IO plugins. Do you think we should really change it?
We need to check it, of course. But the behavior of the prepare function is certainly different from others, so it's more likely a bug.
For RUNNING, instead of keeping an internal shadow state, a saner way would be to have a jack->running bool flag indicating whether it's really running.
With the following pending commits [1] and [2] we would than have 3 additional variables to indicate
- running
That's this patch, and...
- xruns
This should be notified to ioplug via snd_pcm_ioplug_set_state().
- draining
This is also a thing to be fixed in ioplug side caller side. The ioplug should set DRAINING state before calling the callback.
My idea was to avoid this variables and use the already existing snd_pcm_state enum. With further commits which I have not yet sent I want to make the JACK plugin thread save. But instead of using locking I am using atomic lock-less access (to avoid blocking of the JACK daemon). Therefore I cannot call snd_pcm_state() to get the internal state. I have to forward the status by my own to the JACK thread. Due to that I need an internal variable which will be atomically accessed and represents the state. (I think it is not intended by ALSA to extend the ALSA library to update the internal ALSA state atomically so that it can be read atomically from any thread. Therefore I have not yet another idea how to transfer the state to the JACK thread with atomic lock-less access.) I could replace the assignments to the internal state in all IO plug callback functions in the JACK plugin with jack->state = io->state But I am not sure if this is really helpful for the understanding.
That'd be great if everything is ready, but this can't be a reason to add a shadow state and modify the code as in the patch for now.
thanks,
Takashi
We need to check it, of course. But the behavior of the prepare function is certainly different from others, so it's more likely a bug.
I could provide a fix for the IO plug API for prepare and draining.
This should be notified to ioplug via snd_pcm_ioplug_set_state().
Without an additional variable snd_pcm_ioplug_set_state() has to be called from the JACK thread. But snd_pcm_ioplug_set_state() is not thread safe.
That'd be great if everything is ready, but this can't be a reason to add a shadow state and modify the code as in the patch for now.
At the end I would like to use functions similar to READ_ONCE() and WRITE_ONCE() to access the state (known from the Linux kernel [1]). But I think this should not be part of the ALSA library because it is for most IO plugins not required. Therefore I need to have an internal copy of the state which will only be accessed with READ_ONCE() and WRITE_ONCE().
You are right, it is not yet required but at the end I have to implement it.
Do you think we should implement such a synchronization into the ALSA lib?
[1] https://www.kernel.org/doc/Documentation/memory-barriers.txt
Best regards
Timo Wischer
Advanced Driver Information Technology GmbH Engineering Software Base (ADITG/ESB) Robert-Bosch-Str. 200 31139 Hildesheim Germany
Tel. +49 5121 49 6938 Fax +49 5121 49 6999 twischer@de.adit-jv.com
ADIT is a joint venture company of Robert Bosch GmbH/Robert Bosch Car Multimedia GmbH and DENSO Corporation Sitz: Hildesheim, Registergericht: Amtsgericht Hildesheim HRB 3438 Geschäftsführung: Wilhelm Grabow, Ken Yaguchi
On Thu, 15 Mar 2018 16:07:31 +0100, Wischer, Timo (ADITG/ESB) wrote:
We need to check it, of course. But the behavior of the prepare function is certainly different from others, so it's more likely a bug.
I could provide a fix for the IO plug API for prepare and draining.
Yes, these are very trivial changes.
This should be notified to ioplug via snd_pcm_ioplug_set_state().
Without an additional variable snd_pcm_ioplug_set_state() has to be called from the JACK thread. But snd_pcm_ioplug_set_state() is not thread safe.
Well, but your internal state copy is also not thread safe as of this patch, after all.
That'd be great if everything is ready, but this can't be a reason to add a shadow state and modify the code as in the patch for now.
At the end I would like to use functions similar to READ_ONCE() and WRITE_ONCE() to access the state (known from the Linux kernel [1]). But I think this should not be part of the ALSA library because it is for most IO plugins not required. Therefore I need to have an internal copy of the state which will only be accessed with READ_ONCE() and WRITE_ONCE().
You are right, it is not yet required but at the end I have to implement it.
Do you think we should implement such a synchronization into the ALSA lib?
[1] https://www.kernel.org/doc/Documentation/memory-barriers.txt
I really would like to avoid that, at least, not introducing any dependency into alsa-lib.
The usage inside a plugin is another question, and it would depend on the implementation details. A dependency in plugin may cause a messy conflict when different versions are used in a same application. So, in general, it'd be best for a plugin to be self-contained.
Takashi
Hi Takashi,
Yes, these are very trivial changes.
See attached the required patch for the IO plug API.
Well, but your internal state copy is also not thread safe as of this patch, after all.
I will introduce additional variables for the synchronisation between the JACK thread and ALSA thread with the thread safety patches.
Best regards
Timo
From: Timo Wischer twischer@de.adit-jv.com
PREPARED should only be set when it is done and it was successfully.
DRAINING should be signalled when starting to drain. There is no need to check if draining was successfully because it will change to drop (SETUP) in any case.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c index af223a1..e60b688 100644 --- a/src/pcm/pcm_ioplug.c +++ b/src/pcm/pcm_ioplug.c @@ -146,13 +146,18 @@ static int snd_pcm_ioplug_prepare(snd_pcm_t *pcm) ioplug_priv_t *io = pcm->private_data; int err = 0;
- io->data->state = SND_PCM_STATE_PREPARED; snd_pcm_ioplug_reset(pcm); if (io->data->callback->prepare) { snd_pcm_unlock(pcm); /* to avoid deadlock */ err = io->data->callback->prepare(io->data); snd_pcm_lock(pcm); } + if (err < 0) + return err; + + gettimestamp(&io->trigger_tstamp, pcm->tstamp_type); + io->data->state = SND_PCM_STATE_PREPARED; + return err; }
@@ -493,6 +498,10 @@ static int snd_pcm_ioplug_drain(snd_pcm_t *pcm)
if (io->data->state == SND_PCM_STATE_OPEN) return -EBADFD; + + gettimestamp(&io->trigger_tstamp, pcm->tstamp_type); + io->data->state = SND_PCM_STATE_DRAINING; + if (io->data->callback->drain) io->data->callback->drain(io->data); snd_pcm_lock(pcm);
On Fri, 16 Mar 2018 11:02:54 +0100, twischer@de.adit-jv.com wrote:
From: Timo Wischer twischer@de.adit-jv.com
PREPARED should only be set when it is done and it was successfully.
DRAINING should be signalled when starting to drain. There is no need to check if draining was successfully because it will change to drop (SETUP) in any case.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c index af223a1..e60b688 100644 --- a/src/pcm/pcm_ioplug.c +++ b/src/pcm/pcm_ioplug.c @@ -146,13 +146,18 @@ static int snd_pcm_ioplug_prepare(snd_pcm_t *pcm) ioplug_priv_t *io = pcm->private_data; int err = 0;
- io->data->state = SND_PCM_STATE_PREPARED; snd_pcm_ioplug_reset(pcm); if (io->data->callback->prepare) { snd_pcm_unlock(pcm); /* to avoid deadlock */ err = io->data->callback->prepare(io->data); snd_pcm_lock(pcm); }
- if (err < 0)
return err;- gettimestamp(&io->trigger_tstamp, pcm->tstamp_type);
The trigger_tstamp is updated only when triggered (START, STOP, PAUSE, etc). And this change wasn't even documented in the changelog.
- io->data->state = SND_PCM_STATE_PREPARED;
- return err;
}
@@ -493,6 +498,10 @@ static int snd_pcm_ioplug_drain(snd_pcm_t *pcm)
if (io->data->state == SND_PCM_STATE_OPEN) return -EBADFD;
- gettimestamp(&io->trigger_tstamp, pcm->tstamp_type);
Ditto.
thanks,
Takashi
- io->data->state = SND_PCM_STATE_DRAINING;
- if (io->data->callback->drain) io->data->callback->drain(io->data); snd_pcm_lock(pcm);
-- 2.7.4
From: Timo Wischer twischer@de.adit-jv.com
PREPARED should only be set when it is done and it was successfully.
DRAINING should be signalled when starting to drain. There is no need to check if draining was successfully because it will change to drop (SETUP) in any case.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
diff --git a/ChangeLog b/ChangeLog index 22df356..3dff7fb 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,4 @@ +* Set IO plug state to PREPARED after calling the IO plugin prepare callback * update to libtool 1.3.3
0.1.3 -> 0.2.0 diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c index af223a1..8c0ed48 100644 --- a/src/pcm/pcm_ioplug.c +++ b/src/pcm/pcm_ioplug.c @@ -146,13 +146,16 @@ static int snd_pcm_ioplug_prepare(snd_pcm_t *pcm) ioplug_priv_t *io = pcm->private_data; int err = 0;
- io->data->state = SND_PCM_STATE_PREPARED; snd_pcm_ioplug_reset(pcm); if (io->data->callback->prepare) { snd_pcm_unlock(pcm); /* to avoid deadlock */ err = io->data->callback->prepare(io->data); snd_pcm_lock(pcm); } + if (err < 0) + return err; + + io->data->state = SND_PCM_STATE_PREPARED; return err; }
@@ -493,6 +496,8 @@ static int snd_pcm_ioplug_drain(snd_pcm_t *pcm)
if (io->data->state == SND_PCM_STATE_OPEN) return -EBADFD; + + io->data->state = SND_PCM_STATE_DRAINING; if (io->data->callback->drain) io->data->callback->drain(io->data); snd_pcm_lock(pcm);
On Fri, 16 Mar 2018 11:20:46 +0100, twischer@de.adit-jv.com wrote:
From: Timo Wischer twischer@de.adit-jv.com
PREPARED should only be set when it is done and it was successfully.
DRAINING should be signalled when starting to drain. There is no need to check if draining was successfully because it will change to drop (SETUP) in any case.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
diff --git a/ChangeLog b/ChangeLog index 22df356..3dff7fb 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,4 @@ +* Set IO plug state to PREPARED after calling the IO plugin prepare callback
- update to libtool 1.3.3
0.1.3 -> 0.2.0
No, no, this wasn't what I meant. This ChangeLog file is actually dead, remaining only for historical reason.
In the previous patch, you didn't mention the exact change you've made in the patch description. That is, the behavior were silently modified. This must not be done, it fools the users.
So, if you want to change something, especially if it's about the behavior change, it has to be clearly documented in the patch description and/or in the code itself.
In anyway, no need for resend: I dropped the ChangeLog file change but applied the rest.
thanks,
Takashi
From: Timo Wischer twischer@de.adit-jv.com
Only increasing the hw_ptr is not sufficient because it will not be evaluated by the ALSA library to detect an Xrun.
In addition there is a raise where and Xrun detected by the JACK thread could not be detected in the ALSA thread. - In playback use case - The hw_ptr will be increased by the JACK thread (hw_ptr > appl_ptr => over run) - But the ALSA thread increases the appl_ptr before evaluating the hw_ptr - Therefore the hw_ptr < appl_ptr again - ALSA will not detect the over run which was already detected by the JACK thread
Therefore an additional variable is required to report an Xrun from the JACK thread to ALSA.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index 0dff0d2..79aa79b 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -198,6 +198,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->state == SND_PCM_STATE_XRUN) + return -EPIPE; + #ifdef SND_PCM_IOPLUG_FLAG_BOUNDARY_WA return jack->hw_ptr; #else @@ -268,6 +279,25 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) for (channel = 0; channel < io->channels; channel++) snd_pcm_area_silence(&jack->areas[channel], xfer, samples, io->format); } + + if (io->stream == SND_PCM_STREAM_PLAYBACK && + jack->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 an under run. + * In Capture use case the buffer should be filled + * and if the application is not fast enough + * to read the data in the buffer + * it is a valid over run. + */ + } else { + /* report Xrun to user application */ + jack->state = SND_PCM_STATE_XRUN; + } }
pcm_poll_unblock_check(io); /* unblock socket for polling if needed */
Hello all,
I have updated this patch. It can merged without conflicts, now. It is no longer requiring an internal state variable.
@Takashi: Thanks for your efforts so far. Possible it makes sense that you also have a look into this patch.
I would be happy about a feedback.
Best reagards
Timo
From: Timo Wischer twischer@de.adit-jv.com
Only increasing the hw_ptr is not sufficient because it will not be evaluated by the ALSA library to detect an Xrun.
In addition there is a raise where an Xrun detected by the JACK thread could not be detected in the ALSA thread. - In playback use case - The hw_ptr will be increased by the JACK thread (hw_ptr > appl_ptr => Xrun) - But the ALSA thread increases the appl_ptr before evaluating the hw_ptr - Therefore the hw_ptr < appl_ptr again - ALSA will not detect the Xrun which was already detected by the JACK thread
Therefore an additional variable is required to report an Xrun from the JACK thread to ALSA.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index 7c7c230..a655668 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -20,6 +20,7 @@ * */
+#include <stdbool.h> #include <byteswap.h> #include <sys/shm.h> #include <sys/types.h> @@ -50,6 +51,9 @@ typedef struct {
jack_port_t **ports; jack_client_t *client; + + /* JACK thread -> ALSA thread */ + bool xrun_detected; } snd_pcm_jack_t;
static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io); @@ -133,6 +137,9 @@ static snd_pcm_sframes_t snd_pcm_jack_pointer(snd_pcm_ioplug_t *io) { snd_pcm_jack_t *jack = io->private_data;
+ if (jack->xrun_detected) + return -EPIPE; + #ifdef SND_PCM_IOPLUG_FLAG_BOUNDARY_WA return jack->hw_ptr; #else @@ -196,6 +203,20 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) snd_pcm_areas_silence(jack->areas, io->channels, xfer, frames, 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 an Xrun. + */ + } else { + /* report Xrun to user application */ + jack->xrun_detected = true; + } }
pcm_poll_unblock_check(io); /* unblock socket for polling if needed */ @@ -211,6 +232,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);
On Fri, 16 Mar 2018 15:23:32 +0100, twischer@de.adit-jv.com wrote:
From: Timo Wischer twischer@de.adit-jv.com
Only increasing the hw_ptr is not sufficient because it will not be evaluated by the ALSA library to detect an Xrun.
In addition there is a raise where an Xrun detected by the JACK thread could not be detected in the ALSA thread.
- In playback use case
- The hw_ptr will be increased by the JACK thread (hw_ptr > appl_ptr => Xrun)
- But the ALSA thread increases the appl_ptr before evaluating the
hw_ptr
- Therefore the hw_ptr < appl_ptr again
- ALSA will not detect the Xrun which was already detected by the
JACK thread
Therefore an additional variable is required to report an Xrun from the JACK thread to ALSA.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
Thanks, I applied it now.
But, at the next time you send a patchset, please don't continue from the old thread. It's pretty confusing.
Takashi
But, at the next time you send a patchset, please don't continue from the old thread. It's pretty confusing.
Does it mean I should not replay-to any old mail and send a completely new mail? But how to mark that this patch was adapted and the updated one was applied?
Best regards
Timo Wischer
Advanced Driver Information Technology GmbH Engineering Software Base (ADITG/ESB) Robert-Bosch-Str. 200 31139 Hildesheim Germany
Tel. +49 5121 49 6938 Fax +49 5121 49 6999 twischer@de.adit-jv.com
ADIT is a joint venture company of Robert Bosch GmbH/Robert Bosch Car Multimedia GmbH and DENSO Corporation Sitz: Hildesheim, Registergericht: Amtsgericht Hildesheim HRB 3438 Geschäftsführung: Wilhelm Grabow, Ken Yaguchi
On Fri, 16 Mar 2018 15:47:04 +0100, Wischer, Timo (ADITG/ESB) wrote:
But, at the next time you send a patchset, please don't continue from the old thread. It's pretty confusing.
Does it mean I should not replay-to any old mail and send a completely new mail?
You can reply a mail to the old thread. But it shouldn't be the start of another patch series.
If you do submit a new patch series, do it from scratch. And if it's a revised patchset, put "v2" or such prefix, too.
Takashi
From: Timo Wischer twischer@de.adit-jv.com
Block on drain till available samples played
Without this commit the JACK thread will be stopped before the ALSA buffer was completely forwarded to the JACK daemon.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com
diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index 79aa79b..f457dc4 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -28,6 +28,17 @@ #include <alsa/asoundlib.h> #include <alsa/pcm_external.h>
+/* 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; @@ -146,7 +157,13 @@ 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->state == SND_PCM_STATE_DRAINING) { write(jack->fd, &buf, 1); return 1; } @@ -232,7 +249,8 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) }
hw_ptr = jack->hw_ptr; - if (jack->state == SND_PCM_STATE_RUNNING) { + if (jack->state == SND_PCM_STATE_RUNNING || + jack->state == SND_PCM_STATE_DRAINING) { const snd_pcm_channel_area_t *areas = snd_pcm_ioplug_mmap_areas(io);
while (xfer < nframes) { @@ -392,6 +410,61 @@ static int snd_pcm_jack_start(snd_pcm_ioplug_t *io) return err; }
+static int snd_pcm_jack_drain(snd_pcm_ioplug_t *io) +{ + snd_pcm_jack_t *jack = io->private_data; + const snd_pcm_state_t state = jack->state; + 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, jack->hw_ptr, io->appl_ptr) <= 0) { + /* No data pending. Nothing to drain. */ + return 0; + } + + /* start device if not yet done */ + if (state == SND_PCM_STATE_PREPARED) { + snd_pcm_jack_start(io); + } + + /* 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->state = SND_PCM_STATE_DRAINING; + + struct pollfd pfd; + pfd.fd = io->poll_fd; + pfd.events = io->poll_events | POLLERR | POLLNVAL; + + while (snd_pcm_jack_hw_avail(io, jack->hw_ptr, io->appl_ptr) > 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; @@ -423,6 +496,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, };
Dne 1.3.2018 v 14:14 twischer@de.adit-jv.com napsal(a):
From: Timo Wischer twischer@de.adit-jv.com
Block on drain till available samples played
Do you handle the non-blocking mode correctly here? It seems that this mode is completely ignored.
Jaroslav
Hello Jaroslav
Do you handle the non-blocking mode correctly here? It seems that this mode is completely ignored.
You are right. The non-blocking mode flag will be ignored here. But this case is also not covered by the IO plug API. snd_pcm_drop() is called immediately after the IO plugin drain callback returns [1]. The return value of the drain callback will be ignored.
Therefore the IO plug API has to be changed to support this. But I do not really see a use case for it. If you want to drain you also want to wait/block your application until all frames were played.
If you think it is required to change anything in this patch we have to clarify when drop should be called and when not. What is the sequence which the user would expect? Does the user need to call drop after drain is done or would it be called by snd_pcm_wait/snd_pcm_drain when it is not returning -EAGAIN?
[1] http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm_ioplug.c;h=...
Best regards
Timo Wischer
Advanced Driver Information Technology GmbH Engineering Software Base (ADITG/ESB) Robert-Bosch-Str. 200 31139 Hildesheim Germany
Tel. +49 5121 49 6938 Fax +49 5121 49 6999 twischer@de.adit-jv.com
ADIT is a joint venture company of Robert Bosch GmbH/Robert Bosch Car Multimedia GmbH and DENSO Corporation Sitz: Hildesheim, Registergericht: Amtsgericht Hildesheim HRB 3438 Geschäftsführung: Wilhelm Grabow, Ken Yaguchi
________________________________________ From: Jaroslav Kysela [perex@perex.cz] Sent: Thursday, March 01, 2018 2:42 PM To: Wischer, Timo (ADITG/ESB) Cc: ALSA development; Takashi Iwai Subject: Re: [alsa-devel] [PATCH - JACK plugin 4/4] jack: Support snd_pcm_drain()
Dne 1.3.2018 v 14:14 twischer@de.adit-jv.com napsal(a):
From: Timo Wischer twischer@de.adit-jv.com
Block on drain till available samples played
Do you handle the non-blocking mode correctly here? It seems that this mode is completely ignored.
Jaroslav
-- Jaroslav Kysela perex@perex.cz Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
Dne 16.3.2018 v 15:44 Wischer, Timo (ADITG/ESB) napsal(a):
Hello Jaroslav
Do you handle the non-blocking mode correctly here? It seems that this mode is completely ignored.
You are right. The non-blocking mode flag will be ignored here. But this case is also not covered by the IO plug API. snd_pcm_drop() is called immediately after the IO plugin drain callback returns [1]. The return value of the drain callback will be ignored.
Yes, it seems like a flaw in the ioplug core code.
Therefore the IO plug API has to be changed to support this. But I do not really see a use case for it.
But it changes the PCM API behaviour, so some apps might have trouble with it, because it might cause the unexpected task blocking. The correct behaviour is return with -EAGAIN and let application wait for the end-of-data using the file descriptor event (or build-in snd_pcm_wait() fcn which implements the poll() loop - see snd_pcm_wait_nocheck()).
If you want to drain you also want to wait/block your application until all frames were played.
But it does not imply that the drain should block. The app just notify the device that the rest of the data should be played.
If you think it is required to change anything in this patch we have to clarify when drop should be called and when not. What is the sequence which the user would expect? Does the user need to call drop after drain is done or would it be called by snd_pcm_wait/snd_pcm_drain when it is not returning -EAGAIN?
[1] http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm_ioplug.c;h=...
Best regards
Timo Wischer
Advanced Driver Information Technology GmbH Engineering Software Base (ADITG/ESB) Robert-Bosch-Str. 200 31139 Hildesheim Germany
Tel. +49 5121 49 6938 Fax +49 5121 49 6999 twischer@de.adit-jv.com
ADIT is a joint venture company of Robert Bosch GmbH/Robert Bosch Car Multimedia GmbH and DENSO Corporation Sitz: Hildesheim, Registergericht: Amtsgericht Hildesheim HRB 3438 Geschäftsführung: Wilhelm Grabow, Ken Yaguchi
From: Jaroslav Kysela [perex@perex.cz] Sent: Thursday, March 01, 2018 2:42 PM To: Wischer, Timo (ADITG/ESB) Cc: ALSA development; Takashi Iwai Subject: Re: [alsa-devel] [PATCH - JACK plugin 4/4] jack: Support snd_pcm_drain()
Dne 1.3.2018 v 14:14 twischer@de.adit-jv.com napsal(a):
From: Timo Wischer twischer@de.adit-jv.com
Block on drain till available samples played
Do you handle the non-blocking mode correctly here? It seems that this mode is completely ignored.
Jaroslav-- Jaroslav Kysela perex@perex.cz Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
Hi Jaroslav,
But it changes the PCM API behaviour, so some apps might have trouble with it, because it might cause the unexpected task blocking. The correct behaviour is return with -EAGAIN
I have checked all IO plugins (pulse, oss, a52, jack) only a52 could return -EAGAIN. oss and pulse do always block and it seems that they are not updating the file descriptor in case of draining. Therefore snd_pcm_wait() would block infinitely.
I think an application developer would not expect that he has to call snd_pcm_drop() after snd_pcm_drain(). Therefore snd_pcm_drop has to be called internally. (On a real hardware it will be handled in the kernel)
Therefore I think snd_pcm_drop() has to be called from snd_pcm_wait() before returning. But snd_pcm_wait callback is not available in the IO plug API. Therefore I think we have to call snd_pcm_drop() from snd_pcm_ioplug_poll_revents(). But this looks a little bit ugly for me. Do you have another idea?
One approach could be the following snd_pcm_ioplug_poll_revents() { ... if (draining) snd_pcm_drop() }
snd_pcm_ioplug_drain() { int err = 0 if (io->data->callback->drain) err = io->data->callback->drain(io->data); if (err != -EAGAIN) { snd_pcm_lock(pcm); err = snd_pcm_ioplug_drop(pcm); snd_pcm_unlock(pcm); } return err; }
But I am not aware of if this would brake any of the existing IO plugins. I would prefer to avoid these changes.
Best regards
Timo Wischer
Advanced Driver Information Technology GmbH Engineering Software Base (ADITG/ESB) Robert-Bosch-Str. 200 31139 Hildesheim Germany
Tel. +49 5121 49 6938 Fax +49 5121 49 6999 twischer@de.adit-jv.com
ADIT is a joint venture company of Robert Bosch GmbH/Robert Bosch Car Multimedia GmbH and DENSO Corporation Sitz: Hildesheim, Registergericht: Amtsgericht Hildesheim HRB 3438 Geschäftsführung: Wilhelm Grabow, Ken Yaguchi
Hi Jaroslav and Takashi,
What do you think about my "ugly" solution? Should I provide a patch for it or do we want to keep snd_pcm_drain() blocking as all other IO plugins it do?
Best regards
Timo Wischer
Advanced Driver Information Technology GmbH Engineering Software Base (ADITG/ESB) Robert-Bosch-Str. 200 31139 Hildesheim Germany
Tel. +49 5121 49 6938 Fax +49 5121 49 6999 twischer@de.adit-jv.com
ADIT is a joint venture company of Robert Bosch GmbH/Robert Bosch Car Multimedia GmbH and DENSO Corporation Sitz: Hildesheim, Registergericht: Amtsgericht Hildesheim HRB 3438 Geschäftsführung: Wilhelm Grabow, Ken Yaguchi
On Wed, 21 Mar 2018 17:22:27 +0100, Wischer, Timo (ADITG/ESB) wrote:
Hi Jaroslav and Takashi,
What do you think about my "ugly" solution? Should I provide a patch for it or do we want to keep snd_pcm_drain() blocking as all other IO plugins it do?
All ioplug plugins should behave equivalently like the real kernel driver, i.e. it should do -EAGAIN. And this check can be done even in snd_pcm_ioplug_drain(), so we can fix all in a shot.
thanks,
Takashi
And this check can be done even in snd_pcm_ioplug_drain(), so we can fix all in a shot.
For example the drain callback of the pulse IO plugin is blocking but we need to call this callback to inform the IO plugin that it should drain.
Therefore which solution do you have in your mind?
Best regards
Timo
On Wed, 21 Mar 2018 17:47:35 +0100, Wischer, Timo (ADITG/ESB) wrote:
And this check can be done even in snd_pcm_ioplug_drain(), so we can fix all in a shot.
For example the drain callback of the pulse IO plugin is blocking but we need to call this callback to inform the IO plugin that it should drain.
It means that the pulse plugin is buggy. And fixing in ioplug will fix the whole.
Takashi
It means that the pulse plugin is buggy. And fixing in ioplug will fix the whole.
Possibly it is to late but I have no idea how to change the IO plug API implementation that draining of all IO plugins is working without changing the IO plugins it self. When ever I am calling the drain callback of an IO plugin it is blocking but I have to call it to signal to the IO plugin that I want to drain.
Could you possibly provide an example how to fix it?
On Wed, 21 Mar 2018 18:02:18 +0100, Wischer, Timo (ADITG/ESB) wrote:
It means that the pulse plugin is buggy. And fixing in ioplug will fix the whole.
Possibly it is to late but I have no idea how to change the IO plug API implementation that draining of all IO plugins is working without changing the IO plugins it self. When ever I am calling the drain callback of an IO plugin it is blocking but I have to call it to signal to the IO plugin that I want to drain.
Why not poll()?
IOW, why ioplug must be handled specially regarding the non-blocking operation? The normal kernel driver behaves like that (returning -EAGAIN, and let apps to sync with poll()).
Takashi
On Thu, 01 Mar 2018 14:14:04 +0100, twischer@de.adit-jv.com wrote:
Hello Takashi,
Could you simply resubmit the patches? In your way, the real topic and the patch is buried deeply.
Hopefully this is what you are requesting!?
Yes, a sort of. At best, please give a cover letter as [0/N] for a short explanation of the whole series at the next time.
thanks,
Takashi
participants (4)
-
Jaroslav Kysela -
Takashi Iwai -
twischer＠de.adit-jv.com -
Wischer, Timo (ADITG/ESB)