Re: [PATCH RESEND] pcm_rate: Do not discard slave reported delay in status result.

[Takashi Iwai <tiwai@suse.de>]

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 <consult.awy@gmail.com>
> 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 <consult.awy@gmail.com>
> ---
>  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