From patchwork Fri May 9 00:00:50 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sergey X-Patchwork-Id: 4139651 X-Patchwork-Delegate: tiwai@suse.de Return-Path: X-Original-To: patchwork-alsa-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 0FF3F9F1E1 for ; Fri, 9 May 2014 00:01:17 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 07E23202EC for ; Fri, 9 May 2014 00:01:16 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) by mail.kernel.org (Postfix) with ESMTP id 4CFED202E9 for ; Fri, 9 May 2014 00:01:13 +0000 (UTC) Received: by alsa0.perex.cz (Postfix, from userid 1000) id B3324265554; Fri, 9 May 2014 02:01:11 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Spam-Status: No, score=-0.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, FREEMAIL_FROM,FROM_EXCESS_BASE64,NO_DNS_FOR_FROM,T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=no version=3.3.1 Received: from alsa0.perex.cz (localhost [IPv6:::1]) by alsa0.perex.cz (Postfix) with ESMTP id 963C8265524; Fri, 9 May 2014 02:01:00 +0200 (CEST) X-Original-To: alsa-devel@alsa-project.org Delivered-To: alsa-devel@alsa-project.org Received: by alsa0.perex.cz (Postfix, from userid 1000) id D4E37265531; Fri, 9 May 2014 02:00:59 +0200 (CEST) Received: from f94.i.mail.ru (f94.i.mail.ru [94.100.185.169]) by alsa0.perex.cz (Postfix) with ESMTP id CDBBF261A81 for ; Fri, 9 May 2014 02:00:51 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=mail.ru; s=mail2; h=References:In-Reply-To:Content-Transfer-Encoding:Content-Type:Message-ID:Reply-To:Date:Mime-Version:Subject:To:From; bh=MZROb7045pMTmH/dePhm7bLNOCT767VFmuDANY5Fl8Y=; b=BzyqYN+5Km/WXT4eSE4EtmRkvg/sMmGpcHUnmewJ+a75cCmLHKszjil7oZNSzXos4jqh538bSagDuOb1zik0/MLMzsjp8/yW7ypV3IvEOfZxDU8BC7y1qjIjd7R0sdglXGU1f2kIqLR5g8ndDEUB+5StfnTlSw6R+K+arsx5G3s=; Received: from mail by f94.i.mail.ru with local (envelope-from ) id 1WiYF8-0000pV-Sr for alsa-devel@alsa-project.org; Fri, 09 May 2014 04:00:51 +0400 Received: from [85.198.169.60] by e.mail.ru with HTTP; Fri, 09 May 2014 04:00:50 +0400 From: =?UTF-8?B?U2VyZ2V5?= To: =?UTF-8?B?YWxzYS1kZXZlbA==?= Mime-Version: 1.0 X-Mailer: Mail.Ru Mailer 1.0 X-Originating-IP: [85.198.169.60] Date: Fri, 09 May 2014 04:00:50 +0400 X-Priority: 3 (Normal) Message-ID: <1399593650.837833135@f94.i.mail.ru> X-Mras: Ok X-Spam: undefined In-Reply-To: References: <1399056393.47049129@f181.i.mail.ru> Subject: Re: [alsa-devel] =?utf-8?q?=5BPATCH=5D_alsa-plugins=3A_fix_polling_an?= =?utf-8?q?d_recovering_in_alsa-jack_plugin?= X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: =?UTF-8?B?U2VyZ2V5?= List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org X-Virus-Scanned: ClamAV using ClamSMTP At Mon 05 May 2014 Takashi Iwai wrote: >> 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. Thank you for your response and comments. > You don't need inline in general. Compilers should be smart enough > nowadays. Uninlined. > Please follow the coding style in that file. The brace is placed in > the same line as if (). Missed that, sorry. Fixed. > 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. > 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()? >> + 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. >> + >> + 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. diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c index 7a8b24d..837e15c 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,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,