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

Message ID 1521118587-6319-2-git-send-email-twischer@de.adit-jv.com
State New
Headers show

Commit Message

Timo Wischer March 15, 2018, 12:56 p.m. UTC
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>

Comments

Takashi Iwai March 15, 2018, 1:14 p.m. UTC | #1
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
Timo Wischer March 15, 2018, 2:05 p.m. UTC | #2
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
Takashi Iwai March 15, 2018, 2:20 p.m. UTC | #3
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
Timo Wischer March 15, 2018, 3:07 p.m. UTC | #4
> 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
Takashi Iwai March 15, 2018, 3:49 p.m. UTC | #5
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
Timo Wischer March 16, 2018, 10:02 a.m. UTC | #6
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
diff mbox

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