[PATCH - JACK PCM plugin] jack: Do not Xrun the ALSA buffer

[PATCH - JACK PCM plugin] jack: Do not Xrun the ALSA buffer

[twischer]

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.html


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

Re: [PATCH - JACK PCM plugin] jack: Do not Xrun the ALSA buffer

[Wischer, Timo (ADITG/ESB)]

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.html
[2] http://git.alsa-project.org/?p=alsa-plugins.git;a=commit;h=3e6ace6fe045c580dc5490d87eff6b616f7769ef
[3] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-January/130987.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: 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.html


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

Re: [PATCH - JACK PCM plugin] jack: Do not Xrun the ALSA buffer

[Takashi Iwai]

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

[PATCH - JACK PCM plugin] Xrun handling

[twischer]

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!?

[PATCH - JACK plugin 1/4] jack: Do not Xrun the ALSA buffer

[twischer]

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;
-- 
2.7.4

[PATCH - JACK plugin 2/4] jack: Use internal snd_pcm_state to reduce amount of additional variables

[twischer]

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);
-- 
2.7.4

[PATCH - JACK plugin 3/4] jack: Report Xruns to user application

[twischer]

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 */
-- 
2.7.4

[PATCH - JACK plugin 4/4] jack: Support snd_pcm_drain()

[twischer]

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,
 };
 
-- 
2.7.4

Re: [PATCH - JACK plugin 4/4] jack: Support snd_pcm_drain()

[Jaroslav Kysela]

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.

Re: [PATCH - JACK PCM plugin] Xrun handling

[Takashi Iwai]

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

Re: [PATCH - JACK plugin 1/4] jack: Do not Xrun the ALSA buffer

[Takashi Iwai]

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);
> +				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);
> +		}
> +	}
> +

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

Re: [PATCH - JACK plugin 2/4] jack: Use internal snd_pcm_state to reduce amount of additional variables

[Takashi Iwai]

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

[PATCH - JACK plugin 1/4] jack: Do not Xrun the ALSA buffer

[twischer]

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

[PATCH - PCM 2/3] pcm: ioplug: Provide hw_avail helper function for plugins

[twischer]

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)
-- 
2.7.4

[PATCH - PCM 3/3] pcm: Provide areas_copy function which handles buffer wrap around

[twischer]

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));
-- 
2.7.4

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

[twischer]

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;
-- 
2.7.4

[PATCH - JACK plugin] jack: Do not Xrun the ALSA buffer

[twischer]

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

[PATCH - PCM 1/2] pcm: ioplug: Provide hw_avail helper function for plugins

[twischer]

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)
-- 
2.7.4

[PATCH - PCM 2/2] pcm: Provide areas_copy function which handles buffer wrap around

[twischer]

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));
-- 
2.7.4

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

[twischer]

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 */
 
-- 
2.7.4

Re: [PATCH - JACK plugin] jack: Do not Xrun the ALSA buffer

[Takashi Iwai]

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

[PATCH - JACK plugin] jack: Use internal snd_pcm_state to reduce amount of additional variables

[twischer]

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

[PATCH - JACK 1/1] jack: Use internal snd_pcm_state to reduce amount of additional variables

[twischer]

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);
-- 
2.7.4

Re: [PATCH - JACK 1/1] jack: Use internal snd_pcm_state to reduce amount of additional variables

[Takashi Iwai]

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

Re: [PATCH - JACK 1/1] jack: Use internal snd_pcm_state to reduce amount of additional variables

[Wischer, Timo (ADITG/ESB)]

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

Re: [PATCH - JACK 1/1] jack: Use internal snd_pcm_state to reduce amount of additional variables

[Takashi Iwai]

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

Re: [PATCH - JACK 1/1] jack: Use internal snd_pcm_state to reduce amount of additional variables

[Wischer, Timo (ADITG/ESB)]

> 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

Re: [PATCH - JACK plugin] jack: Do not Xrun the ALSA buffer

[Wischer, Timo (ADITG/ESB)]

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

Re: [PATCH - JACK plugin] jack: Do not Xrun the ALSA buffer

[Takashi Iwai]

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

Re: [PATCH - JACK 1/1] jack: Use internal snd_pcm_state to reduce amount of additional variables

[Takashi Iwai]

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

[PATCH - IOPLUG] Update prepare and draining state correctly

[twischer]

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

[PATCH - IOPLUG 1/1] pcm: ioplug: update Prepare and draining state correctly

[twischer]

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);
-- 
2.7.4

Re: [PATCH - IOPLUG 1/1] pcm: ioplug: update Prepare and draining state correctly

[Takashi Iwai]

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
> 

[PATCH - IOPLUG 1/1] pcm: ioplug: update prepare and draining state correctly

[twischer]

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);
-- 
2.7.4

Re: [PATCH - IOPLUG 1/1] pcm: ioplug: update prepare and draining state correctly

[Takashi Iwai]

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

[PATCH - JACK plugin 3/4] jack: Report Xruns to user application

[twischer]

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

[PATCH - JACK 1/1] jack: Report Xruns to user application

[twischer]

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);
-- 
2.7.4

Re: [PATCH - JACK 1/1] jack: Report Xruns to user application

[Takashi Iwai]

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

Re: [PATCH - JACK plugin 4/4] jack: Support snd_pcm_drain()

[Wischer, Timo (ADITG/ESB)]

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=8c0ed4836365afb53c0cbce796a5d39c2d05a3d7;hb=07a17bd5a50289e2fdb2714a4e39f38f41811558#l504

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.

Re: [PATCH - JACK 1/1] jack: Report Xruns to user application

[Wischer, Timo (ADITG/ESB)]

> 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

Re: [PATCH - JACK 1/1] jack: Report Xruns to user application

[Takashi Iwai]

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

Re: [PATCH - JACK plugin 4/4] jack: Support snd_pcm_drain()

[Jaroslav Kysela]

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=8c0ed4836365afb53c0cbce796a5d39c2d05a3d7;hb=07a17bd5a50289e2fdb2714a4e39f38f41811558#l504
> 
> 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.
> 


-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH - JACK plugin 4/4] jack: Support snd_pcm_drain()

[Wischer, Timo (ADITG/ESB)]

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

Re: [PATCH - JACK plugin 4/4] jack: Support snd_pcm_drain()

[Wischer, Timo (ADITG/ESB)]

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

Re: [PATCH - JACK plugin 4/4] jack: Support snd_pcm_drain()

[Takashi Iwai]

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

Re: [PATCH - JACK plugin 4/4] jack: Support snd_pcm_drain()

[Wischer, Timo (ADITG/ESB)]

> 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

Re: [PATCH - JACK plugin 4/4] jack: Support snd_pcm_drain()

[Takashi Iwai]

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

Re: [PATCH - JACK plugin 4/4] jack: Support snd_pcm_drain()

[Wischer, Timo (ADITG/ESB)]

>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?

Re: [PATCH - JACK plugin 4/4] jack: Support snd_pcm_drain()

[Takashi Iwai]

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