diff mbox

alsa-plugins: fix polling and recovering in alsa-jack plugin

Message ID 1399593650.837833135@f94.i.mail.ru (mailing list archive)
State Accepted
Delegated to: Takashi Iwai
Headers show

Commit Message

Sergey May 9, 2014, midnight UTC 
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()?

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

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

Patch updated and ready for more comments.

---

Comments

Takashi Iwai May 9, 2014, 5:34 a.m. UTC | #1 
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@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
diff mbox

Patch

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,