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

Message ID 1399056393.47049129@f181.i.mail.ru
State Accepted
Delegated to: Takashi Iwai
Headers show

Commit Message

Sergey May 2, 2014, 6:46 p.m. UTC
Hello.

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.

---

Comments

Takashi Iwai May 5, 2014, 3:56 p.m. UTC | #1
At Fri, 02 May 2014 22:46:33 +0400,
Sergey wrote:
> 
> Hello.
> 
> 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.

> 
> ---
> diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c
> index 7a8b24d..a39fea5 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,41 @@ typedef struct {
>  	jack_client_t *client;
>  } snd_pcm_jack_t;
>  
> +static inline int poll_blocked(snd_pcm_ioplug_t *io)

You don't need inline in general.  Compilers should be smart enough
nowadays.

> +{
> +	static char buf[65536];
> +	snd_pcm_sframes_t avail;
> +	snd_pcm_jack_t *jack = io->private_data;
> +
> +	if (io->state == SND_PCM_STATE_RUNNING)
> +	{

Please follow the coding style in that file.  The brace is placed in
the same line as if ().

> +		avail = snd_pcm_avail_update(io->pcm);
> +		if (avail >= 0 && avail < jack->min_avail)
> +		{
> +			read(io->poll_fd, &buf, sizeof buf);

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.

> +			return 1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static inline int poll_unblocked(snd_pcm_ioplug_t *io)

The function names are a bit too ambiguous and hard to understand what
they are really doing only from the names.

> +{
> +	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 +117,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 && !poll_blocked(io))
>  		*revents |= (io->stream == SND_PCM_STREAM_PLAYBACK) ? POLLOUT : POLLIN;
>  	return 0;
>  }
> @@ -105,7 +137,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,7 +176,7 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io)
>  		xfer += frames;
>  	}
>  
> -	write(jack->fd, buf, 1); /* for polling */
> +	poll_unblocked(io); /* unblock socket for polling if needed */
>  
>  	return 0;
>  }
> @@ -154,9 +185,25 @@ 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);
> +	}
> +
> +	poll_unblocked(io); /* unblock socket for initial polling if needed */

Is this really needed?

> +
> +	if (jack->activated) {
> +		jack_deactivate(jack->client);
> +		jack->activated = 0;
> +	}

This is same as just calling snd_pcm_jack_stop().


thanks,

Takashi

Patch
diff mbox

diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c
index 7a8b24d..a39fea5 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,41 @@  typedef struct {
 	jack_client_t *client;
 } snd_pcm_jack_t;
 
+static inline int poll_blocked(snd_pcm_ioplug_t *io)
+{
+	static char buf[65536];
+	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)
+		{
+			read(io->poll_fd, &buf, sizeof buf);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
+static inline int poll_unblocked(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 +117,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 && !poll_blocked(io))
 		*revents |= (io->stream == SND_PCM_STREAM_PLAYBACK) ? POLLOUT : POLLIN;
 	return 0;
 }
@@ -105,7 +137,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,7 +176,7 @@  snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io)
 		xfer += frames;
 	}
 
-	write(jack->fd, buf, 1); /* for polling */
+	poll_unblocked(io); /* unblock socket for polling if needed */
 
 	return 0;
 }
@@ -154,9 +185,25 @@  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);
+	}
+
+	poll_unblocked(io); /* unblock socket for initial polling if needed */
+
+	if (jack->activated) {
+		jack_deactivate(jack->client);
+		jack->activated = 0;
+	}
+
 	if (jack->ports)
 		return 0;