Re: [PATCH 1/2] ALSA: pcm: rewrite snd_pcm_playback_silence()

[Oswald Buddenhagen <oswald.buddenhagen@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 1995 bytes --]

On Tue, Apr 11, 2023 at 12:47:26PM +0200, Jaroslav Kysela wrote:
>On 05. 04. 23 22:12, Oswald Buddenhagen wrote:
>> This fixes a bug in thresholded mode, where we failed to use 
>> new_hw_ptr,
>> resulting in under-fill.
>
>I don't follow what you refer here. The old code uses 
>snd_pcm_playback_hw_avail()
>
yes

>thus new hw_ptr for the threshold mode, too.
>
not before my patch. the silencer was called before the new pointer was 
stored. it had to be, as otherwise the delta for top-up mode could not 
be calculated.

>> +	// This will "legitimately" turn negative on underrun, and will be mangled
>> +	// into a huge number by the boundary crossing handling. The initial state
>> +	// might also be not quite sane. The code below MUST account for these cases.
>> +	hw_avail = appl_ptr - runtime->status->hw_ptr;
>> +	if (hw_avail < 0)
>> +		hw_avail += runtime->boundary;

>> +	else if ((snd_pcm_uframes_t) hw_avail >= runtime->boundary)
>> +		hw_avail -= runtime->boundary;
>
>If hw_avail is above runtime->boundary then the initial condition is totaly 
>bogus. I would use snd_BUG_ON() and direct return here.
>
this is only there as a result of inlining 
snd_pcm_playback_hw_avail()/snd_pcm_playback_avail() somewhat 
mindlessly. the check does indeed make no sense, so i'll just drop it.
(the broader lesson of this is the attached patch. i can re-post it 
separately if you like it.)

>>   		frames = runtime->silence_threshold - noise_dist;
>> +		if ((snd_pcm_sframes_t) frames <= 0)
>> +			return;
>
>The retyping does not look good here. Could we move the if before frames 
>assignment like:
>
>   if (runtime->silence_threshold <= noise_dist)
>     return;
>   frames = runtime->silence_threshold - noise_dist;
>
dunno, i don't like it - it's more noisy and imo it loses 
expressiveness, as the question we're asking is "how many frames do we 
need to fill?".
note that due to use of unsigned types in the runtime struct, such 
retyping is rather common in comparisons.

regards

[-- Attachment #2: 0001-ALSA-pcm-reshuffle-implementations-of-snd_pcm_playba.patch --]
[-- Type: text/x-diff, Size: 2635 bytes --]

>From 02b303326b1be1ddb52afb38a384d26d79fa8b53 Mon Sep 17 00:00:00 2001
From: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Date: Wed, 12 Apr 2023 12:28:49 +0200
Subject: [PATCH] ALSA: pcm: reshuffle implementations of
 snd_pcm_playback_{,hw}_avail()

Implementing snd_pcm_playback_hw_avail() in terms of
snd_pcm_playback_avail() doesn't actually make sense - it should be the
other way around, the opposite of the respective functions for capture.
This makes the code clearer by illustrating the inverse data flow
better, and saving a conditional.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 include/sound/pcm.h | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index f20400bb7032..81854813a567 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -783,22 +783,31 @@ static inline size_t snd_pcm_lib_period_bytes(struct snd_pcm_substream *substrea
 	return frames_to_bytes(runtime, runtime->period_size);
 }
 
+/**
+ * snd_pcm_playback_hw_avail - Get the queued space for playback
+ * @runtime: PCM runtime instance
+ *
+ * Return: available frame size
+ */
+static inline snd_pcm_sframes_t snd_pcm_playback_hw_avail(struct snd_pcm_runtime *runtime)
+{
+	snd_pcm_sframes_t avail = runtime->control->appl_ptr - runtime->status->hw_ptr;
+	if (avail < 0)
+		avail += runtime->boundary;
+	return avail;
+}
+
 /**
  * snd_pcm_playback_avail - Get the available (writable) space for playback
  * @runtime: PCM runtime instance
  *
  * Result is between 0 ... (boundary - 1)
  *
  * Return: available frame size
  */
 static inline snd_pcm_uframes_t snd_pcm_playback_avail(struct snd_pcm_runtime *runtime)
 {
-	snd_pcm_sframes_t avail = runtime->status->hw_ptr + runtime->buffer_size - runtime->control->appl_ptr;
-	if (avail < 0)
-		avail += runtime->boundary;
-	else if ((snd_pcm_uframes_t) avail >= runtime->boundary)
-		avail -= runtime->boundary;
-	return avail;
+	return runtime->buffer_size - snd_pcm_playback_hw_avail(runtime);
 }
 
 /**
@@ -817,17 +826,6 @@ static inline snd_pcm_uframes_t snd_pcm_capture_avail(struct snd_pcm_runtime *ru
 	return avail;
 }
 
-/**
- * snd_pcm_playback_hw_avail - Get the queued space for playback
- * @runtime: PCM runtime instance
- *
- * Return: available frame size
- */
-static inline snd_pcm_sframes_t snd_pcm_playback_hw_avail(struct snd_pcm_runtime *runtime)
-{
-	return runtime->buffer_size - snd_pcm_playback_avail(runtime);
-}
-
 /**
  * snd_pcm_capture_hw_avail - Get the free space for capture
  * @runtime: PCM runtime instance
-- 
2.40.0.152.g15d061e6df