[PATCH] Add async support to ioplug

[PATCH] Add async support to ioplug

[Takashi Iwai]

Hi,

the patch below adds a framework to support async on ioplug.  Now
an external ioplug plugin can implement appropriate async methods if
possible.

The async callback is just a copy of snd_pcm_async().  When sig >= 0,
activate the async, and with a negative value, deactivate async.

The test patch to alsa-plugins will follow.


Takashi

---
diff -r 8b22022a861d include/local.h
--- a/include/local.h	Fri May 09 16:02:02 2008 +0200
+++ b/include/local.h	Tue May 20 09:02:35 2008 +0200
@@ -149,7 +149,13 @@
 	void *private_data;
 	struct list_head glist;
 	struct list_head hlist;
+	int no_signal;
 };
+
+int _snd_async_add_handler(snd_async_handler_t **handler, int fd, 
+			   snd_async_callback_t callback,
+			   void *private_data, int no_signal);
+void _snd_async_call_handlers(int fd);
 
 typedef enum _snd_set_mode {
 	SND_CHANGE,
diff -r 8b22022a861d include/pcm_ioplug.h
--- a/include/pcm_ioplug.h	Fri May 09 16:02:02 2008 +0200
+++ b/include/pcm_ioplug.h	Tue May 20 09:02:35 2008 +0200
@@ -66,7 +66,7 @@
  */
 #define SND_PCM_IOPLUG_VERSION_MAJOR	1	/**< Protocol major version */
 #define SND_PCM_IOPLUG_VERSION_MINOR	0	/**< Protocol minor version */
-#define SND_PCM_IOPLUG_VERSION_TINY	1	/**< Protocol tiny version */
+#define SND_PCM_IOPLUG_VERSION_TINY	2	/**< Protocol tiny version */
 /**
  * IO-plugin protocol version
  */
@@ -114,6 +114,10 @@
 	unsigned int rate;		/**< rate; filled after hw_params is called */
 	snd_pcm_uframes_t period_size;	/**< period size; filled after hw_params is called */
 	snd_pcm_uframes_t buffer_size;	/**< buffer size; filled after hw_params is called */
+	/**
+	 * fields added by version 1.0.2
+	 */
+	unsigned int no_signal_async;	/**< async support without signal */
 };
 
 /** Callback table of ioplug */
@@ -189,6 +193,10 @@
 	 * get the delay for the running PCM; optional
 	 */
 	int (*delay)(snd_pcm_ioplug_t *io, snd_pcm_sframes_t *delayp);
+	/**
+	 * set up async handler; optional; added in version 1.0.2
+	 */
+	int (*async)(snd_pcm_ioplug_t *io, int sig, pid_t pid);
 };
 
 
@@ -212,6 +220,9 @@
 /* change PCM status */
 int snd_pcm_ioplug_set_state(snd_pcm_ioplug_t *ioplug, snd_pcm_state_t state);
 
+/* call async handlers */
+void snd_pcm_ioplug_call_async_handlers(snd_pcm_ioplug_t *ioplug);
+
 /** \} */
 
 #endif /* __ALSA_PCM_IOPLUG_H */
diff -r 8b22022a861d src/async.c
--- a/src/async.c	Fri May 09 16:02:02 2008 +0200
+++ b/src/async.c	Tue May 20 09:02:35 2008 +0200
@@ -48,19 +48,48 @@
 #endif
 
 static LIST_HEAD(snd_async_handlers);
+static int sig_count;
 
-static void snd_async_handler(int signo ATTRIBUTE_UNUSED, siginfo_t *siginfo, void *context ATTRIBUTE_UNUSED)
+#ifndef DOC_HIDDEN
+void _snd_async_call_handlers(int fd)
 {
-	int fd;
 	struct list_head *i;
-	//assert(siginfo->si_code == SI_SIGIO);
-	fd = siginfo->si_fd;
 	list_for_each(i, &snd_async_handlers) {
 		snd_async_handler_t *h = list_entry(i, snd_async_handler_t, glist);
 		if (h->fd == fd && h->callback)
 			h->callback(h);
 	}
 }
+#endif
+
+static void snd_async_handler(int signo ATTRIBUTE_UNUSED, siginfo_t *siginfo, void *context ATTRIBUTE_UNUSED)
+{
+	int fd;
+	//assert(siginfo->si_code == SI_SIGIO);
+	fd = siginfo->si_fd;
+	_snd_async_call_handlers(fd);
+}
+
+#ifndef DOC_HIDDEN
+int _snd_async_add_handler(snd_async_handler_t **handler, int fd, 
+			   snd_async_callback_t callback,
+			   void *private_data, int no_signal)
+{
+	snd_async_handler_t *h;
+	assert(handler);
+	h = malloc(sizeof(*h));
+	if (!h)
+		return -ENOMEM;
+	h->fd = fd;
+	h->callback = callback;
+	h->private_data = private_data;
+	list_add_tail(&h->glist, &snd_async_handlers);
+	INIT_LIST_HEAD(&h->hlist);
+	h->no_signal = no_signal;
+	*handler = h;
+	return 0;
+}
+#endif
 
 /**
  * \brief Registers an async handler.
@@ -94,20 +123,13 @@
 int snd_async_add_handler(snd_async_handler_t **handler, int fd, 
 			  snd_async_callback_t callback, void *private_data)
 {
-	snd_async_handler_t *h;
-	int was_empty;
-	assert(handler);
-	h = malloc(sizeof(*h));
-	if (!h)
-		return -ENOMEM;
-	h->fd = fd;
-	h->callback = callback;
-	h->private_data = private_data;
-	was_empty = list_empty(&snd_async_handlers);
-	list_add_tail(&h->glist, &snd_async_handlers);
-	INIT_LIST_HEAD(&h->hlist);
-	*handler = h;
-	if (was_empty) {
+	int err;
+
+	err = _snd_async_add_handler(handler, fd, callback, private_data, 0);
+	if (err < 0)
+		return err;
+
+	if (!sig_count) {
 		int err;
 		struct sigaction act;
 		memset(&act, 0, sizeof(act));
@@ -116,10 +138,15 @@
 		sigemptyset(&act.sa_mask);
 		err = sigaction(snd_async_signo, &act, NULL);
 		if (err < 0) {
+			err = -errno;
 			SYSERR("sigaction");
-			return -errno;
+			list_del(&(*handler)->glist);
+			free(*handler);
+			*handler = NULL;
+			return err;
 		}
 	}
+	sig_count++;
 	return 0;
 }
 
@@ -133,15 +160,18 @@
 	int err = 0;
 	assert(handler);
 	list_del(&handler->glist);
-	if (list_empty(&snd_async_handlers)) {
-		struct sigaction act;
-		memset(&act, 0, sizeof(act));
-		act.sa_flags = 0;
-		act.sa_handler = SIG_DFL;
-		err = sigaction(snd_async_signo, &act, NULL);
-		if (err < 0) {
-			SYSERR("sigaction");
-			return -errno;
+	if (!handler->no_signal) {
+		sig_count--;
+		if (!sig_count) {
+			struct sigaction act;
+			memset(&act, 0, sizeof(act));
+			act.sa_flags = 0;
+			act.sa_handler = SIG_DFL;
+			err = sigaction(snd_async_signo, &act, NULL);
+			if (err < 0) {
+				SYSERR("sigaction");
+				/* return -errno; */
+			}
 		}
 	}
 	if (handler->type == SND_ASYNC_HANDLER_GENERIC)
diff -r 8b22022a861d src/pcm/pcm.c
--- a/src/pcm/pcm.c	Fri May 09 16:02:02 2008 +0200
+++ b/src/pcm/pcm.c	Tue May 20 09:02:36 2008 +0200
@@ -1984,8 +1984,10 @@
 	int err;
 	int was_empty;
 	snd_async_handler_t *h;
-	err = snd_async_add_handler(&h, _snd_pcm_async_descriptor(pcm),
-				    callback, private_data);
+
+	err = _snd_async_add_handler(&h, _snd_pcm_async_descriptor(pcm),
+				     callback, private_data,
+				     pcm->no_signal_async);
 	if (err < 0)
 		return err;
 	h->type = SND_ASYNC_HANDLER_PCM;
diff -r 8b22022a861d src/pcm/pcm_ioplug.c
--- a/src/pcm/pcm_ioplug.c	Fri May 09 16:02:02 2008 +0200
+++ b/src/pcm/pcm_ioplug.c	Tue May 20 09:02:36 2008 +0200
@@ -698,11 +698,15 @@
 	return 0;
 }
 
-static int snd_pcm_ioplug_async(snd_pcm_t *pcm ATTRIBUTE_UNUSED,
-				int sig ATTRIBUTE_UNUSED,
-				pid_t pid ATTRIBUTE_UNUSED)
+static int snd_pcm_ioplug_async(snd_pcm_t *pcm, int sig, pid_t pid)
 {
-	return -ENOSYS;
+	ioplug_priv_t *io = pcm->private_data;
+
+	if (io->data->version >= 0x010002 &&
+	    io->data->callback->async)
+		return io->data->callback->async(io->data, sig, pid);
+	else
+		return -ENOSYS;
 }
 
 static int snd_pcm_ioplug_munmap(snd_pcm_t *pcm ATTRIBUTE_UNUSED)
@@ -1037,6 +1041,9 @@
 	ioplug->pcm->poll_events = ioplug->poll_events;
 	ioplug->pcm->monotonic = (ioplug->flags & SND_PCM_IOPLUG_FLAG_MONOTONIC) != 0;
 	ioplug->pcm->mmap_rw = ioplug->mmap_rw;
+	if (ioplug->version >= 0x010002)
+		ioplug->pcm->no_signal_async = ioplug->no_signal_async;
+
 	return 0;
 }
 
@@ -1070,3 +1077,16 @@
 	ioplug->state = state;
 	return 0;
 }
+
+/**
+ * \brief Call the associated async handlers
+ * \param ioplug the ioplug handle
+ *
+ * Call the async handlers associated with the given ioplug.
+ * This function is usually called from the plugin's async handling
+ * routine appropriately.
+ */
+void snd_pcm_ioplug_call_async_handlers(snd_pcm_ioplug_t *ioplug)
+{
+	_snd_async_call_handlers(ioplug->poll_fd);
+}
diff -r 8b22022a861d src/pcm/pcm_local.h
--- a/src/pcm/pcm_local.h	Fri May 09 16:02:02 2008 +0200
+++ b/src/pcm/pcm_local.h	Tue May 20 09:02:36 2008 +0200
@@ -220,6 +220,7 @@
 					 * use the mmaped buffer of the slave
 					 */
 	unsigned int donot_close: 1;	/* don't close this PCM */
+	unsigned int no_signal_async :1; /* async without signal */
 	snd_pcm_channel_info_t *mmap_channels;
 	snd_pcm_channel_area_t *running_areas;
 	snd_pcm_channel_area_t *stopped_areas;

[PATCH] Add async support to pulse and jack plugins

[Takashi Iwai]

This patch adds async support to pulse and jack plugins.
I tested only jack plugin, so far.  The pulse might be buggy due to
possible races.

Anyway, as you can see, the implementation is pretty simple for a
plugin with a parallel thread like jack or pulse.  For other cases
like oss plugin, it might be more difficult.


Takashi

---
diff -r 0833aa56dbe2 jack/pcm_jack.c
--- a/jack/pcm_jack.c	Tue May 13 13:16:06 2008 +0200
+++ b/jack/pcm_jack.c	Tue May 20 13:07:57 2008 +0200
@@ -48,6 +48,8 @@
 
 	jack_port_t **ports;
 	jack_client_t *client;
+
+	int call_async;
 } snd_pcm_jack_t;
 
 static void snd_pcm_jack_free(snd_pcm_jack_t *jack)
@@ -90,6 +92,18 @@
 	*revents = pfds[0].revents;
 	return 0;
 }
+
+#if SND_PCM_IOPLUG_VERSION >= 0x010002
+static int snd_pcm_jack_async(snd_pcm_ioplug_t *io, int sig, pid_t pid)
+{
+	snd_pcm_jack_t *jack = io->private_data;
+	if (sig < 0)
+		jack->call_async = 0;
+	else
+		jack->call_async = 1;
+	return 0;
+}
+#endif
 
 static snd_pcm_sframes_t snd_pcm_jack_pointer(snd_pcm_ioplug_t *io)
 {
@@ -145,6 +159,11 @@
 
 	write(jack->fd, buf, 1); /* for polling */
 
+#if SND_PCM_IOPLUG_VERSION >= 0x010002
+	/* call async handlers if needed */
+	if (jack->call_async)
+		snd_pcm_ioplug_call_async_handlers(io);
+#endif
 	return 0;
 }
 
@@ -238,6 +257,9 @@
 	.pointer = snd_pcm_jack_pointer,
 	.prepare = snd_pcm_jack_prepare,
 	.poll_revents = snd_pcm_jack_poll_revents,
+#if SND_PCM_IOPLUG_VERSION >= 0x010002
+	.async = snd_pcm_jack_async,
+#endif
 };
 
 #define ARRAY_SIZE(ary)	(sizeof(ary)/sizeof(ary[0]))
@@ -372,6 +394,9 @@
 	jack->io.poll_fd = fd[1];
 	jack->io.poll_events = POLLIN;
 	jack->io.mmap_rw = 1;
+#if SND_PCM_IOPLUG_VERSION >= 0x010002
+	jack->io.no_signal_async = 1;
+#endif
 
 	err = snd_pcm_ioplug_create(&jack->io, name, stream, mode);
 	if (err < 0) {
diff -r 0833aa56dbe2 pulse/pcm_pulse.c
--- a/pulse/pcm_pulse.c	Tue May 13 13:16:06 2008 +0200
+++ b/pulse/pcm_pulse.c	Tue May 20 13:07:57 2008 +0200
@@ -45,6 +45,8 @@
     pa_sample_spec ss;
     unsigned int frame_size;
     pa_buffer_attr buffer_attr;
+
+    int call_async;
 } snd_pcm_pulse_t;
 
 static void update_ptr(snd_pcm_pulse_t *pcm)
@@ -365,6 +367,11 @@
     assert(pcm->p);
 
     pulse_poll_activate(pcm->p);
+
+#if SND_PCM_IOPLUG_VERSION >= 0x010002
+    if (pcm->call_async)
+      snd_pcm_ioplug_call_async_handlers(&pcm->io);
+#endif
 }
 
 static void stream_underrun_cb(pa_stream *p, void *userdata) {
@@ -446,6 +453,18 @@
 
 	return err;
 }
+
+#if SND_PCM_IOPLUG_VERSION >= 0x010002
+static int pulse_pcm_async(snd_pcm_ioplug_t *io, int sig, pid_t pid)
+{
+  snd_pcm_pulse_t *pcm = io->private_data;
+  if (sig < 0)
+    pcm->call_async = 0;
+  else
+    pcm->call_async = 1;
+  return 0;
+}
+#endif
 
 static int pulse_prepare(snd_pcm_ioplug_t *io)
 {
@@ -620,6 +639,9 @@
     .prepare = pulse_prepare,
     .hw_params = pulse_hw_params,
     .close = pulse_close,
+#if SND_PCM_IOPLUG_VERSION >= 0x010002
+    .async = pulse_pcm_async,
+#endif
 };
 
 
@@ -635,6 +657,9 @@
     .prepare = pulse_prepare,
     .hw_params = pulse_hw_params,
     .close = pulse_close,
+#if SND_PCM_IOPLUG_VERSION >= 0x010002
+    .async = pulse_pcm_async,
+#endif
 };
 
 
@@ -746,6 +771,9 @@
 	pcm->io.poll_fd = -1;
 	pcm->io.poll_events = 0;
 	pcm->io.mmap_rw = 0;
+#if SND_PCM_IOPLUG_VERSION >= 0x010002
+	pcm->io.no_signal_async = 1;
+#endif
 	pcm->io.callback = stream == SND_PCM_STREAM_PLAYBACK ?
 		&pulse_playback_callback : &pulse_capture_callback;
 	pcm->io.private_data = pcm;

Re: [PATCH] Add async support to ioplug

[Lennart Poettering]

On Tue, 20.05.08 13:10, Takashi Iwai (tiwai@suse.de) wrote:

> Hi,
> 
> the patch below adds a framework to support async on ioplug.  Now
> an external ioplug plugin can implement appropriate async methods if
> possible.
> 
> The async callback is just a copy of snd_pcm_async().  When sig >= 0,
> activate the async, and with a negative value, deactivate async.
> 
> The test patch to alsa-plugins will follow.

Humm, I am not really happy about this. 

Firstly I thing that async is a really bad thing anyway. Signal handler
segmantics are very different from normal process semantics, i.e. a
lot of API functions are not reentrant and not signal handler
safe. Also, a lot of people implementing signal handlers don't
save/restore errno the way the should (libasound being one example
btw, as I just checked, this needs fixing). It's a fact: implementing
signal handlers is difficult, and people almost never get it right. I
mean, because it is so difficult there's even a valgrind module that
helps identifying problems with signal handlers!  

Also, signals are a shared resource where no reservation scheme
exists. If one library in a process takes posession of a signal then
it might overwrite the previous signal handler, there is no sane way
to detect that. Flash 9 for example registers an alsa async handler,
which causes SIGIO to be overwritten. It's sheer luck that noone else
in the Firefox process wants to use SIGIO, because that would clash
with Flash -- and this would not even be detected! And SIGIO is used
for a lot of stuff, among them the whole aio subsystem, dnotify alsa
and a few others. To emphasize this: if you use alsa async, you cannot
use aio or dnotify in the same process!

Signal delivery for usage in event loops on Linux is slow. Much slower
than poll() based event loop approaches (somewhere on the web is a detailed
comparison of poll() loops vs. RT sig based event loops in regards to
throughput, I can dig that up if someone wants to see that.)

In summary: async doesn't give any benefits. However, it creates quite
a few problems:

In the case of flash it just signals a semaphore from the async
handler which is slept on by the sound loop thread. This is really not
a clean design, anyway. They should directly sleep on the audio
fds. With Flash 10 they responded to criticism of mine. It doesn't use
async anymore and is those compatible with PA and other ioplug
modules.

What I want to say is: using signals is bad style. Don't use
signals. I do believe the best would be to remove async support from
ALSA entirely. Grepping through google code search I couldn't find a
single program using alsa async where using it was actually a good
idea. Adding even further async support to ALSA does not strike me as
a good idea. I believe the plan must be to remove async, not extend
its support. And such a plan I think would be perfectly in line with
what happens on the kernel front anyway. We now have stuff like
signalfd(), explicitly because signal handlers are so ugly. For aio
people are looking for alternatives without sig handlers, too. And
yes, one of the reasons dnotify got replaced by inotify is that it
used SIGIO.

What however strikes me particularly questionnable about this patch is
that this way ioplug async handlers are not called from signal handler
context, instead they are called directly from process context. This
is really problematic. Firstly, if people start to use async on ioplug
a lot they will very likely produce code that only runs fine from
normal process context and will break horribly from signal handler
process (e.g. use synchronization based on mutexes). More importantly
however, if a program uses signal masks to make sure that the async
handler is run from a very specific thread or temporarily disabled for
criticial sections this will be breaking for ioplug drivers. 

If you really think that extending async to iolpug is a good idea then
this should probably done by using sigqueue() to manually enqueue
SIGIO and thus make sure the async events pass the usual signal
delivery logic.

When I analyzed the Flash+PA incompatibility I though about extending
ioplug for async myself. But in the end I thought that enqueing SIGIOs
from userspace was so ugly it would never have a chance to go upstream.

Again, please let's drop async altogether. It's error-prone, easy
to misuse, and especially Flash 9 (which I assume is the reason you
came up with the patch) is a prominent misuser of it, so with Flash as
only reason to do this I think the whole idea is void.

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: [PATCH] Add async support to ioplug

[Takashi Iwai]

Lennart, you don't need lengthy explanation :)  I was already
convinced that async is evil; that's why I didn't implement it in
ioplug...

At Thu, 22 May 2008 00:16:43 +0200,
Lennart Poettering wrote:
> 
> Again, please let's drop async altogether. It's error-prone, easy
> to misuse, and especially Flash 9 (which I assume is the reason you
> came up with the patch) is a prominent misuser of it, so with Flash as
> only reason to do this I think the whole idea is void.

IIRC, the wine code used async, too.  I guess it was already
fixed, though.

It was just a 10 minutes test code, and I'm willing to drop it if
flash9 doesn't work with pulseaudio anyway.


thanks,

Takashi