| Message ID | 1521118587-6319-2-git-send-email-twischer@de.adit-jv.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 15 Mar 2018 13:56:27 +0100, <twischer@de.adit-jv.com> wrote: > > From: Timo Wischer <twischer@de.adit-jv.com> > > This variable will be used to exchange the status of the stream between > the ALSA and JACK thread. In future commits it will also be used to signal > DRAINING state from the ALSA to JACK thread and to signal XRUN state form > the JACK to ALSA thread. > > Therefore this internal state variable is not always in sync with > io->state. In addition the internal state variable will be updated after > the corresponding callback function was processed and not before the > callback function was called (e.g. PREPARE and RUNNING). Well, the fact that the state change is done for PREPARE before the plugin callback can be seen as a generic bug of ioplug. The best would be to fix in ioplug side. And, the rest is RUNNING state. Is there anything else? For RUNNING, instead of keeping an internal shadow state, a saner way would be to have a jack->running bool flag indicating whether it's really running. thanks, Takashi
Hello Takashi, > Well, the fact that the state change is done for PREPARE before the > plugin callback can be seen as a generic bug of ioplug. The best > would be to fix in ioplug side. This change could possibly brake other existing IO plugins. Do you think we should really change it? > For RUNNING, instead of keeping an internal shadow state, a saner way > would be to have a jack->running bool flag indicating whether it's > really running. With the following pending commits [1] and [2] we would than have 3 additional variables to indicate - running - xruns - draining My idea was to avoid this variables and use the already existing snd_pcm_state enum. With further commits which I have not yet sent I want to make the JACK plugin thread save. But instead of using locking I am using atomic lock-less access (to avoid blocking of the JACK daemon). Therefore I cannot call snd_pcm_state() to get the internal state. I have to forward the status by my own to the JACK thread. Due to that I need an internal variable which will be atomically accessed and represents the state. (I think it is not intended by ALSA to extend the ALSA library to update the internal ALSA state atomically so that it can be read atomically from any thread. Therefore I have not yet another idea how to transfer the state to the JACK thread with atomic lock-less access.) I could replace the assignments to the internal state in all IO plug callback functions in the JACK plugin with jack->state = io->state But I am not sure if this is really helpful for the understanding. [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-March/132725.html [2] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-March/132726.html Best regards Timo Wischer Advanced Driver Information Technology GmbH Engineering Software Base (ADITG/ESB) Robert-Bosch-Str. 200 31139 Hildesheim Germany Tel. +49 5121 49 6938 Fax +49 5121 49 6999 twischer@de.adit-jv.com ADIT is a joint venture company of Robert Bosch GmbH/Robert Bosch Car Multimedia GmbH and DENSO Corporation Sitz: Hildesheim, Registergericht: Amtsgericht Hildesheim HRB 3438 Geschäftsführung: Wilhelm Grabow, Ken Yaguchi
On Thu, 15 Mar 2018 15:05:39 +0100, Wischer, Timo (ADITG/ESB) wrote: > > Hello Takashi, > > > Well, the fact that the state change is done for PREPARE before the > > plugin callback can be seen as a generic bug of ioplug. The best > > would be to fix in ioplug side. > > This change could possibly brake other existing IO plugins. > Do you think we should really change it? We need to check it, of course. But the behavior of the prepare function is certainly different from others, so it's more likely a bug. > > For RUNNING, instead of keeping an internal shadow state, a saner way > > would be to have a jack->running bool flag indicating whether it's > > really running. > > With the following pending commits [1] and [2] we would than have 3 additional variables to indicate > - running That's this patch, and... > - xruns This should be notified to ioplug via snd_pcm_ioplug_set_state(). > - draining This is also a thing to be fixed in ioplug side caller side. The ioplug should set DRAINING state before calling the callback. > My idea was to avoid this variables and use the already existing snd_pcm_state enum. > With further commits which I have not yet sent I want to make the JACK plugin thread save. > But instead of using locking I am using atomic lock-less access (to avoid blocking of the JACK daemon). > Therefore I cannot call snd_pcm_state() to get the internal state. > I have to forward the status by my own to the JACK thread. > Due to that I need an internal variable which will be atomically accessed and represents the state. > (I think it is not intended by ALSA to extend the ALSA library to update the internal ALSA state atomically > so that it can be read atomically from any thread. Therefore I have not yet another idea how to transfer the state to the JACK thread with atomic lock-less access.) > I could replace the assignments to the internal state in all IO plug callback functions in the JACK plugin with > jack->state = io->state > But I am not sure if this is really helpful for the understanding. That'd be great if everything is ready, but this can't be a reason to add a shadow state and modify the code as in the patch for now. thanks, Takashi
> We need to check it, of course. But the behavior of the prepare > function is certainly different from others, so it's more likely a > bug. I could provide a fix for the IO plug API for prepare and draining. > This should be notified to ioplug via snd_pcm_ioplug_set_state(). Without an additional variable snd_pcm_ioplug_set_state() has to be called from the JACK thread. But snd_pcm_ioplug_set_state() is not thread safe. > That'd be great if everything is ready, but this can't be a reason to > add a shadow state and modify the code as in the patch for now. At the end I would like to use functions similar to READ_ONCE() and WRITE_ONCE() to access the state (known from the Linux kernel [1]). But I think this should not be part of the ALSA library because it is for most IO plugins not required. Therefore I need to have an internal copy of the state which will only be accessed with READ_ONCE() and WRITE_ONCE(). You are right, it is not yet required but at the end I have to implement it. Do you think we should implement such a synchronization into the ALSA lib? [1] https://www.kernel.org/doc/Documentation/memory-barriers.txt Best regards Timo Wischer Advanced Driver Information Technology GmbH Engineering Software Base (ADITG/ESB) Robert-Bosch-Str. 200 31139 Hildesheim Germany Tel. +49 5121 49 6938 Fax +49 5121 49 6999 twischer@de.adit-jv.com ADIT is a joint venture company of Robert Bosch GmbH/Robert Bosch Car Multimedia GmbH and DENSO Corporation Sitz: Hildesheim, Registergericht: Amtsgericht Hildesheim HRB 3438 Geschäftsführung: Wilhelm Grabow, Ken Yaguchi
On Thu, 15 Mar 2018 16:07:31 +0100, Wischer, Timo (ADITG/ESB) wrote: > > > We need to check it, of course. But the behavior of the prepare > > function is certainly different from others, so it's more likely a > > bug. > > I could provide a fix for the IO plug API for prepare and draining. Yes, these are very trivial changes. > > This should be notified to ioplug via snd_pcm_ioplug_set_state(). > > Without an additional variable snd_pcm_ioplug_set_state() has to be called from the JACK thread. > But snd_pcm_ioplug_set_state() is not thread safe. Well, but your internal state copy is also not thread safe as of this patch, after all. > > That'd be great if everything is ready, but this can't be a reason to > > add a shadow state and modify the code as in the patch for now. > > At the end I would like to use functions similar to READ_ONCE() and WRITE_ONCE() to access the state (known from the Linux kernel [1]). > But I think this should not be part of the ALSA library because it is for most IO plugins not required. > Therefore I need to have an internal copy of the state which will only be accessed with READ_ONCE() and WRITE_ONCE(). > > You are right, it is not yet required but at the end I have to implement it. > > Do you think we should implement such a synchronization into the ALSA lib? > > [1] https://www.kernel.org/doc/Documentation/memory-barriers.txt I really would like to avoid that, at least, not introducing any dependency into alsa-lib. The usage inside a plugin is another question, and it would depend on the implementation details. A dependency in plugin may cause a messy conflict when different versions are used in a same application. So, in general, it'd be best for a plugin to be self-contained. Takashi
Hi Takashi, > Yes, these are very trivial changes. See attached the required patch for the IO plug API. > Well, but your internal state copy is also not thread safe as of this > patch, after all. I will introduce additional variables for the synchronisation between the JACK thread and ALSA thread with the thread safety patches. Best regards Timo
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);