19.11.2010 19:22, Torben Hohn wrote:
the use of the socketpair for synchronisation is not correct. jack does not allow blocking calls in the process_callback, the socket buffers are quite small, and seem to overflow under some conditions.
Some remarks.
First, your patch preserves the incorrect code for the case when eventfd is not available. While this is not a regression, I think you can do better (possibly in a separate patch).
Second, I disagree that socketpair()-based synchronization cannot be made correct and thus that eventfd is needed for correctness. It might be needed for efficiency, though.
the result was write(2) blocking, and jack kicking the client.
So why not just make the file descriptor non-blocking? This way, instead of blocking the write(2), the socket pair would lose data when it gets too many writes. No problem - the reader will still see its file descriptor as active, eat a byte from the socket in snd_pcm_jack_poll_revents() and presumably do something to fill the audio buffer.
(jack needs to be running in RT mode, with a pretty low period size (<=128) but this really is the normal usecase)
this patch uses eventfd(2) when its available to mitigate the problem.
Signed-off-by: Torben Hohntorbenh@gmx.de
configure.in | 5 +++++ jack/pcm_jack.c | 30 +++++++++++++++++++++++++++--- 2 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/configure.in b/configure.in index 5d71cae..9fa7f27 100644 --- a/configure.in +++ b/configure.in @@ -21,6 +21,11 @@ AC_CHECK_LIB(asound, snd_pcm_ioplug_create,, AC_ARG_ENABLE([jack], AS_HELP_STRING([--disable-jack], [Disable building of JACK plugin]))
+AC_CHECK_HEADER([sys/eventfd.h], [HAVE_EVENTFD=yes], [HAVE_EVENTFD=no]) +if test "$HAVE_EVENTFD" = "yes"; then
- AC_DEFINE(HAVE_EVENTFD, 1,"eventfd is available")
+fi
- if test "x$enable_jack" != "xno"; then PKG_CHECK_MODULES(JACK, jack>= 0.98, [HAVE_JACK=yes], [HAVE_JACK=no]) fi
diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index 3370a26..83ac56a 100644 --- a/jack/pcm_jack.c +++ b/jack/pcm_jack.c @@ -20,6 +20,8 @@
*/
+#include "config.h"
- #include<byteswap.h> #include<sys/shm.h> #include<sys/types.h>
@@ -28,6 +30,10 @@ #include<alsa/asoundlib.h> #include<alsa/pcm_external.h>
+#ifdef HAVE_EVENTFD +#include<sys/eventfd.h> +#endif
- typedef enum _jack_format { SND_PCM_JACK_FORMAT_RAW } snd_pcm_jack_format_t;
@@ -63,8 +69,10 @@ static void snd_pcm_jack_free(snd_pcm_jack_t *jack) } if (jack->fd>= 0) close(jack->fd); +#ifndef HAVE_EVENTFD if (jack->io.poll_fd>= 0) close(jack->io.poll_fd); +#endif free(jack->areas); free(jack); } @@ -81,11 +89,19 @@ static int snd_pcm_jack_poll_revents(snd_pcm_ioplug_t *io ATTRIBUTE_UNUSED, struct pollfd *pfds, unsigned int nfds, unsigned short *revents) { +#ifdef HAVE_EVENTFD
- eventfd_t buf[1];
+#else static char buf[1];
+#endif
- assert(pfds&& nfds == 1&& revents);
- read(pfds[0].fd, buf, 1);
- read(pfds[0].fd, buf, sizeof(buf));
+#ifdef HAVE_EVENTFD
- buf[0] -= 1;
- write(pfds[0].fd, buf, sizeof(buf));
+#endif
*revents = pfds[0].revents; return 0; @@ -103,7 +119,11 @@ 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; +#ifdef HAVE_EVENTFD
- static eventfd_t buf[1] = { 0x00000001 };
+#else static char buf[1]; +#endif unsigned int channel;
for (channel = 0; channel< io->channels; channel++) { @@ -143,7 +163,7 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io) xfer += frames; }
- write(jack->fd, buf, 1); /* for polling */
write(jack->fd, buf, sizeof(buf)); /* for polling */
return 0; }
@@ -361,7 +381,11 @@ static int snd_pcm_jack_open(snd_pcm_t **pcmp, const char *name, return -ENOMEM; }
+#ifdef HAVE_EVENTFD
- fd[0] = fd[1] = eventfd(0, 0);
+#else socketpair(AF_LOCAL, SOCK_STREAM, 0, fd); +#endif
jack->fd = fd[0];