[alsa-devel] [PATCH] alsa-plugins: fix polling and recovering in alsa-jack plugin
Takashi Iwai
tiwai at suse.de
Fri May 9 07:34:15 CEST 2014
At Fri, 09 May 2014 04:00:50 +0400,
Sergey wrote:
>
> At Mon 05 May 2014 Takashi Iwai wrote:
>
> >> This patch fixes polling in alsa-to-jack plugin.
> >> It makes poll()+_poll_revents() return correct values
> >> whenever there's something to read/write.
> >>
> >> Additionally jack_deactivate() code makes it survive snd_pcm_recover().
> >>
> >> It works for the code example I posted before, aplay, arecord, mozilla, etc.
> >>
> >> Comments and suggestions are welcome.
> >
> > Thanks for the patch. The basic idea looks good, but some suggestions
> > below.
>
> Thank you for your response and comments.
>
> > You don't need inline in general. Compilers should be smart enough
> > nowadays.
>
> Uninlined.
>
> > Please follow the coding style in that file. The brace is placed in
> > the same line as if ().
>
> Missed that, sorry. Fixed.
>
> > So, it's intended to clear all pending writes, right?
> > If so, do reads in loop. Then you don't have to use such a big
> > buffer. The size of 32 or such should be enough.
>
> Done.
>
> > The function names are a bit too ambiguous and hard to understand what
> > they are really doing only from the names.
>
> How about pcm_poll_block() and pcm_poll_unblock()?
A bit better, but not really. You can give a comment to each function
to explain what they do. Then you might find better names while
explaining functionality to yourself.
> >> + poll_unblocked(io); /* unblock socket for initial polling if needed */
> >
> > Is this really needed?
>
> Playback pcm accepts writes since _prepare(), so poll() is unblocked there.
> Without it playback pcm gets poll() blocked by default and a code like:
> _open();
> _set_params();
> while (still_playing) {
> if (poll() > 0 && _revents() == 0 && revents&POLLOUT)
> _write();
> }
> never starts writing.
Then do it only for the playback. For capture, the poll shouldn't
return at prepare state.
> >> +
> >> + if (jack->activated) {
> >> + jack_deactivate(jack->client);
> >> + jack->activated = 0;
> >> + }
> >
> > This is same as just calling snd_pcm_jack_stop().
>
> Right. To put snd_pcm_jack_stop() call in snd_pcm_jack_prepare()
> I have to move snd_pcm_jack_prepare() below snd_pcm_jack_stop(),
> that makes patch larger.
You can declare a function.
thanks,
Takashi
>
> Patch updated and ready for more comments.
>
> ---
> diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c
> index 7a8b24d..837e15c 100644
> --- a/jack/pcm_jack.c
> +++ b/jack/pcm_jack.c
> @@ -42,6 +42,7 @@ typedef struct {
> unsigned int num_ports;
> unsigned int hw_ptr;
> unsigned int sample_bits;
> + snd_pcm_uframes_t min_avail;
>
> unsigned int channels;
> snd_pcm_channel_area_t *areas;
> @@ -50,6 +51,38 @@ typedef struct {
> jack_client_t *client;
> } snd_pcm_jack_t;
>
> +static int pcm_poll_block(snd_pcm_ioplug_t *io)
> +{
> + static char buf[32];
> + snd_pcm_sframes_t avail;
> + snd_pcm_jack_t *jack = io->private_data;
> +
> + if (io->state == SND_PCM_STATE_RUNNING) {
> + avail = snd_pcm_avail_update(io->pcm);
> + if (avail >= 0 && avail < jack->min_avail) {
> + while (read(io->poll_fd, &buf, sizeof(buf)) == sizeof(buf));
> + return 1;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int pcm_poll_unblock(snd_pcm_ioplug_t *io)
> +{
> + static char buf[1];
> + snd_pcm_sframes_t avail;
> + snd_pcm_jack_t *jack = io->private_data;
> +
> + avail = snd_pcm_avail_update(io->pcm);
> + if (avail < 0 || avail >= jack->min_avail) {
> + write(jack->fd, &buf, 1);
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> static void snd_pcm_jack_free(snd_pcm_jack_t *jack)
> {
> if (jack) {
> @@ -81,14 +114,10 @@ static int snd_pcm_jack_poll_revents(snd_pcm_ioplug_t *io,
> struct pollfd *pfds, unsigned int nfds,
> unsigned short *revents)
> {
> - static char buf[1];
> -
> assert(pfds && nfds == 1 && revents);
>
> - read(pfds[0].fd, buf, 1);
> -
> *revents = pfds[0].revents & ~(POLLIN | POLLOUT);
> - if (pfds[0].revents & POLLIN)
> + if (pfds[0].revents & POLLIN && !pcm_poll_block(io))
> *revents |= (io->stream == SND_PCM_STREAM_PLAYBACK) ? POLLOUT : POLLIN;
> return 0;
> }
> @@ -105,7 +134,6 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io)
> snd_pcm_jack_t *jack = io->private_data;
> const snd_pcm_channel_area_t *areas;
> snd_pcm_uframes_t xfer = 0;
> - static char buf[1];
> unsigned int channel;
>
> for (channel = 0; channel < io->channels; channel++) {
> @@ -145,44 +173,11 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io)
> xfer += frames;
> }
>
> - write(jack->fd, buf, 1); /* for polling */
> + pcm_poll_unblock(io); /* unblock socket for polling if needed */
>
> return 0;
> }
>
> -static int snd_pcm_jack_prepare(snd_pcm_ioplug_t *io)
> -{
> - snd_pcm_jack_t *jack = io->private_data;
> - unsigned int i;
> -
> - jack->hw_ptr = 0;
> -
> - if (jack->ports)
> - return 0;
> -
> - 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) {
> -
> - 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);
> - return 0;
> -}
> -
> static int snd_pcm_jack_start(snd_pcm_ioplug_t *io)
> {
> snd_pcm_jack_t *jack = io->private_data;
> @@ -233,6 +228,54 @@ static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io)
> return 0;
> }
>
> +static int snd_pcm_jack_prepare(snd_pcm_ioplug_t *io)
> +{
> + snd_pcm_jack_t *jack = io->private_data;
> + unsigned int i;
> + snd_pcm_sw_params_t *swparams;
> + int err;
> +
> + jack->hw_ptr = 0;
> +
> + jack->min_avail = io->period_size;
> + snd_pcm_sw_params_alloca(&swparams);
> + err = snd_pcm_sw_params_current(io->pcm, swparams);
> + if (err == 0) {
> + snd_pcm_sw_params_get_avail_min(swparams, &jack->min_avail);
> + }
> +
> + /* deactivate jack connections if this is XRUN recovery */
> + snd_pcm_jack_stop(io);
> +
> + /* playback pcm initially accepts writes, poll must be unblocked */
> + pcm_poll_unblock(io);
> +
> + if (jack->ports)
> + return 0;
> +
> + 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) {
> +
> + 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);
> + return 0;
> +}
> +
> static snd_pcm_ioplug_callback_t jack_pcm_callback = {
> .close = snd_pcm_jack_close,
> .start = snd_pcm_jack_start,
> --
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
More information about the Alsa-devel
mailing list