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 Hohn<torbenh@xxxxxx>
> ---
> 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];
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
