[-,JACK,1/1] jack: Do not Xrun the ALSA buffer

Commit Message

Timo Wischer March 2, 2018, 4:21 p.m. UTC

From: Timo Wischer <[e-mail]>

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 <[e-mail]>

Comments

Timo Wischer March 13, 2018, 8:34 a.m. UTC | #1

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

Takashi Iwai March 13, 2018, 9:20 p.m. UTC | #2

On Tue, 13 Mar 2018 09:34:41 +0100,
<[e-mail]> 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

Timo Wischer March 15, 2018, 3:09 p.m. UTC | #3

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. [phone]
Fax [phone]
[e-mail]

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

Takashi Iwai March 15, 2018, 3:13 p.m. UTC | #4

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?
> 
> [1] http://git.alsa-project.org/?p=alsa-lib.git;a=summary

Sorry, forgot to push.  Now pushed out.


thanks,

Takashi

Patch

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;