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

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.

---

jack/pcm_jack.c +1 -0

index 7a8b24d..a39fea5 100644
@@ -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;

jack/pcm_jack.c +19 -7

+{
+	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;
 

Quoted 4 lines

This patch fixes polling in alsa-to-jack plugin. It makes poll()+_poll_revents() return correct values whenever there's something to read/write.

Quoted 5 lines

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.

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

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

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.

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

Quoted 1 line

+ poll_unblocked(io); /* unblock socket for initial polling if needed */

Is this really needed?

Review

Quoted 5 lines

(quoted diff) +5 -0

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

This is same as just calling snd_pcm_jack_stop().

Quoted 4 lines · At Mon 05 May 2014 Takashi Iwai wrote:

Quoted 4 lines

This patch fixes polling in alsa-to-jack plugin. It makes poll()+_poll_revents() return correct values whenever there's something to read/write.

Quoted 8 lines

Quoted 5 lines

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.

Quoted 2 lines

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

Uninlined.

Quoted 2 lines

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

Missed that, sorry. Fixed.

Quoted 3 lines

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.

Quoted 2 lines

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()?

Quoted 3 lines

Quoted 1 line

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

Quoted 7 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

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

---

jack/pcm_jack.c +83 -40

index 7a8b24d..837e15c 100644
@@ -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,

Signature · 4 lines

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 1 line

Playback pcm accepts writes since _prepare(), so poll() is unblocked there.

Then do it only for the playback. For capture, the poll shouldn't return at prepare state.

Quoted 3 lines

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.