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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx>
---
  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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux