[alsa-devel] [PATCH 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay()

[Takashi Sakamoto]

Hi,

On Apr 16 2018 21:14, Takashi Iwai wrote:
> Yet another slight code cleanup: there are two places where
> calculating the PCM delay, and they can be unified in a single
> helper.  It reduces the multiple open codes.
> 
> Signed-off-by: Takashi Iwai <tiwai at suse.de>
> ---
>   sound/core/pcm_native.c | 36 +++++++++++++++---------------------
>   1 file changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index eddb0cd6d1eb..81bbe0b3284b 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -857,6 +857,18 @@ static int snd_pcm_sw_params_user(struct snd_pcm_substream *substream,
>   	return err;
>   }
>   
> +static inline snd_pcm_uframes_t
> +snd_pcm_calc_delay(struct snd_pcm_substream *substream)
> +{
> +	snd_pcm_uframes_t delay;
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		delay = snd_pcm_playback_hw_avail(substream->runtime);
> +	else
> +		delay = snd_pcm_capture_avail(substream->runtime);
> +	return delay + substream->runtime->delay;
> +}
> +
>   int snd_pcm_status(struct snd_pcm_substream *substream,
>   		   struct snd_pcm_status *status)
>   {
> @@ -909,19 +921,7 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
>   	status->appl_ptr = runtime->control->appl_ptr;
>   	status->hw_ptr = runtime->status->hw_ptr;
>   	status->avail = snd_pcm_avail(substream);
> -	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> -		if (runtime->status->state == SNDRV_PCM_STATE_RUNNING ||
> -		    runtime->status->state == SNDRV_PCM_STATE_DRAINING) {
> -			status->delay = runtime->buffer_size - status->avail;
> -			status->delay += runtime->delay;
> -		} else
> -			status->delay = 0;
> -	} else {
> -		if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)
> -			status->delay = status->avail + runtime->delay;
> -		else
> -			status->delay = 0;
> -	}
> +	status->delay = snd_pcm_calc_delay(substream);
>   	status->avail_max = runtime->avail_max;
>   	status->overrange = runtime->overrange;
>   	runtime->avail_max = 0;
> @@ -2655,19 +2655,13 @@ static int snd_pcm_hwsync(struct snd_pcm_substream *substream)
>   		
>   static snd_pcm_sframes_t snd_pcm_delay(struct snd_pcm_substream *substream)
>   {
> -	struct snd_pcm_runtime *runtime = substream->runtime;
>   	int err;
>   	snd_pcm_sframes_t n = 0;
>   
>   	snd_pcm_stream_lock_irq(substream);
>   	err = do_pcm_hwsync(substream);
> -	if (!err) {
> -		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> -			n = snd_pcm_playback_hw_avail(runtime);
> -		else
> -			n = snd_pcm_capture_avail(runtime);
> -		n += runtime->delay;
> -	}
> +	if (!err)
> +		n = snd_pcm_calc_delay(substream);
>   	snd_pcm_stream_unlock_irq(substream);
>   	return err < 0 ? err : n;
>   }

When any PCM substream is under running state, this patchset is good.
Below is short descriptions of calculation in each case.

snd_pcm_status() for playback on running
  * delay = runtime->buffer_size - status->avail 
(=snd_pcm_playback_hw_avail())
  * delay += runtime->delay

snd_pcm_status() for capture on running
  * delay = status->avail (= snd_pcm_avail() = snd_pcm_capture_avail())
  * delay += runtime->delay

snd_pcm_delay() for playback
  * delay = snd_pcm_playback_hw_avail()
  * delay += rutime_delay

snd_pcm_delay() for capture
  * delay = snd_pcm_capture_avail(runtime)
  * delay += runtime->delay


However, under non-running states, I have some suspicion, because
originally 'snd_pcm_status()' take 0 in the states while your code
manage to calculate it.

(sound/core/pcm_native.c)
911    if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
912        status->avail = snd_pcm_playback_avail(runtime);
913        if (runtime->status->state == SNDRV_PCM_STATE_RUNNING ||
914            runtime->status->state == SNDRV_PCM_STATE_DRAINING) {
                ...
917        } else
918            status->delay = 0;
919        } else {
920            status->avail = snd_pcm_capture_avail(runtime);
921            if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)
                    ...
923            else
924                status->delay = 0;
925        }

I think this patch is based on an assumption that hw_ptr is zeroed
after stopping PCM substream. On the other hand, I cannot find codes
which reset hw_ptr to zero during lifetime of PCM runtime. Any
calculation of delay with hw_ptr could return non-zero value in
non-running states. This patch can change behaviour of PCM core in a 
view of userspace applications, in my understanding.


Regards

Takashi Sakamoto