[-,JACK,plugin,4/4] jack: Support snd_pcm_drain()
diff mbox

Message ID 1519910048-19732-5-git-send-email-twischer@de.adit-jv.com
State New
Headers show

Commit Message

Timo Wischer March 1, 2018, 1:14 p.m. UTC
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>

Comments

Jaroslav Kysela March 1, 2018, 1:41 p.m. UTC | #1
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
Timo Wischer March 16, 2018, 2:44 p.m. UTC | #2
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
Jaroslav Kysela March 16, 2018, 3:11 p.m. UTC | #3
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.
>
Timo Wischer March 16, 2018, 3:52 p.m. UTC | #4
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
Timo Wischer March 21, 2018, 4:22 p.m. UTC | #5
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
Takashi Iwai March 21, 2018, 4:34 p.m. UTC | #6
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
Timo Wischer March 21, 2018, 4:47 p.m. UTC | #7
> 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
Takashi Iwai March 21, 2018, 4:52 p.m. UTC | #8
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
Timo Wischer March 21, 2018, 5:02 p.m. UTC | #9
>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?
Takashi Iwai March 21, 2018, 5:07 p.m. UTC | #10
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

Patch
diff mbox

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