From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH RESEND] pcm_rate: Do not discard slave reported delay in status result. Date: Mon, 28 Nov 2016 20:11:14 +0100 Message-ID: References: <52e39131-9141-b466-e92a-ff19ffab6d3a@IEE.org> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id D5569266DD3 for ; Mon, 28 Nov 2016 20:11:15 +0100 (CET) In-Reply-To: <52e39131-9141-b466-e92a-ff19ffab6d3a@IEE.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 To: Alan Young Cc: alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org On Thu, 17 Nov 2016 18:12:57 +0100, Alan Young wrote: > > On 17/11/16 15:24, Alan Young wrote: > > > > Similar to recent dshare patch. > > > Update with sign-off Sorry for the late follow up, I've been drug by other things for too long. > >From 6f93ee59846d2c0058ef702c1fa68d723bfb14f6 Mon Sep 17 00:00:00 2001 > From: Alan Young > Date: Tue, 14 Jun 2016 10:15:01 +0100 > Subject: [PATCH] pcm_rate: Do not discard slave reported delay in status > result. > > snd_pcm_rate_status() gets the underlying status from the slave PCM. > This may contain a delay value that includes elements such as codec and > other transfer delays. Use this as the base for the returned delay > value, adjusted for any frames buffered locally (within the rate > plugin). > > Also update snd_pcm_rate_delay() similarly. > > Signed-off-by: Alan Young > --- > src/pcm/pcm_rate.c | 46 ++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 40 insertions(+), 6 deletions(-) > > diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c > index 6184def..15383ae 100644 > --- a/src/pcm/pcm_rate.c > +++ b/src/pcm/pcm_rate.c > @@ -559,10 +559,9 @@ snd_pcm_rate_read_areas1(snd_pcm_t *pcm, > pcm->channels, rate); > } > > -static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm) > +static inline void snd_pcm_rate_sync_hwptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_ptr) > { > snd_pcm_rate_t *rate = pcm->private_data; > - snd_pcm_uframes_t slave_hw_ptr = *rate->gen.slave->hw.ptr; > > if (pcm->stream != SND_PCM_STREAM_PLAYBACK) > return; > @@ -576,6 +575,12 @@ static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm) > rate->hw_ptr %= pcm->boundary; > } > > +static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm) > +{ > + snd_pcm_rate_t *rate = pcm->private_data; > + snd_pcm_rate_sync_hwptr0(pcm, *rate->gen.slave->hw.ptr); > +} > + > static int snd_pcm_rate_hwsync(snd_pcm_t *pcm) > { > snd_pcm_rate_t *rate = pcm->private_data; > @@ -586,10 +591,37 @@ static int snd_pcm_rate_hwsync(snd_pcm_t *pcm) > return 0; > } > > +static snd_pcm_uframes_t snd_pcm_rate_playback_internal_delay(snd_pcm_t *pcm) > +{ > + snd_pcm_rate_t *rate = pcm->private_data; > + > + if (rate->appl_ptr < rate->last_commit_ptr) { > + return rate->appl_ptr - rate->last_commit_ptr + pcm->boundary; > + } else { > + return rate->appl_ptr - rate->last_commit_ptr; > + } > +} > + > static int snd_pcm_rate_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp) > { > + snd_pcm_rate_t *rate = pcm->private_data; > + snd_pcm_sframes_t slave_delay; > + int err; > + > snd_pcm_rate_hwsync(pcm); > - *delayp = snd_pcm_mmap_hw_avail(pcm); > + > + err = snd_pcm_delay(rate->gen.slave, &slave_delay); > + if (err < 0) { > + return err; > + } > + > + if (pcm->stream == SND_PCM_STREAM_PLAYBACK) { > + *delayp = rate->ops.input_frames(rate->obj, slave_delay) > + + snd_pcm_rate_playback_internal_delay(pcm); > + } else { > + *delayp = rate->ops.output_frames(rate->obj, slave_delay) > + + snd_pcm_mmap_capture_hw_avail(pcm); > + } Hmm, I'm not 100% sure through a quick review whether it's the correct calculation. Maybe it's worth to give more comments either in the code or in the changelog. > return 0; > } > > @@ -1083,15 +1115,17 @@ static int snd_pcm_rate_status(snd_pcm_t *pcm, snd_pcm_status_t * status) > status->state = SND_PCM_STATE_RUNNING; > status->trigger_tstamp = rate->trigger_tstamp; > } > - snd_pcm_rate_sync_hwptr(pcm); > + snd_pcm_rate_sync_hwptr0(rate, status->hw_ptr); This can't work. I can fix it in my side, but OTOH, this made me wonder how you tested the patch... > status->appl_ptr = *pcm->appl.ptr; > status->hw_ptr = *pcm->hw.ptr; > if (pcm->stream == SND_PCM_STREAM_PLAYBACK) { > - status->delay = snd_pcm_mmap_playback_hw_avail(pcm); > + status->delay = rate->ops.input_frames(rate->obj, status->delay) > + + snd_pcm_rate_playback_internal_delay(pcm); > status->avail = snd_pcm_mmap_playback_avail(pcm); > status->avail_max = rate->ops.input_frames(rate->obj, status->avail_max); > } else { > - status->delay = snd_pcm_mmap_capture_hw_avail(pcm); > + status->delay = rate->ops.output_frames(rate->obj, status->delay) > + + snd_pcm_mmap_capture_hw_avail(pcm); > status->avail = snd_pcm_mmap_capture_avail(pcm); > status->avail_max = rate->ops.output_frames(rate->obj, status->avail_max); Why only playback needs the special handling while the capture doesn't? Again, some comments would be helpful for better understanding your changes. thanks, Takashi