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

On Tue, 17 Apr 2018 05:01:13 +0200, Takashi Sakamoto wrote:

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@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.

Right, snd_pcm_status() needs an additional check of snd_pcm_running(). The revised patch is below.

thanks,

Takashi

-- 8< --

Subject: [PATCH v2 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay()

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@suse.de --- sound/core/pcm_native.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index eddb0cd6d1eb..7585444352df 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,8 @@ 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_running(substream) ? + snd_pcm_calc_delay(substream) : 0; status->avail_max = runtime->avail_max; status->overrange = runtime->overrange; runtime->avail_max = 0; @@ -2655,19 +2656,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; }