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

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

[Oswald Buddenhagen]

The auto-silencer supports two modes: "thresholded" to fill up "just
enough", and "top-up" to fill up "as much as possible". The two modes
used rather distinct code paths, which this patch unifies. The only
remaining distinction is how much we actually want to fill.

This fixes a bug in thresholded mode, where we failed to use new_hw_ptr,
resulting in under-fill.

Top-up mode is now more well-behaved and much easier to understand in
corner cases.

This also updates comments in the proximity of silencing-related data
structures.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

---
v2:
- removed useless boundary check
- got rid of casts by using a signed type for deltas.
  i did not adjust the style of the conditionals, because it's not clear
  whether the hangup was actually over that, or merely over the casts.
- dropped use of C99 comments where the surroundings suggest it.
  (in the case of the interspersed multi-line comments, that doesn't
  look like an improvement to me at all ...)
- swapped the `added` and `hw_avail` calculation blocks to reduce
  subsequent churn. it's more logical that way anyway.
---
 .../kernel-api/writing-an-alsa-driver.rst     | 17 ++--
 include/sound/pcm.h                           | 14 +--
 include/uapi/sound/asound.h                   | 11 ++-
 sound/core/pcm_lib.c                          | 86 ++++++++-----------
 sound/core/pcm_local.h                        |  3 +-
 sound/core/pcm_native.c                       |  6 +-
 6 files changed, 66 insertions(+), 71 deletions(-)

diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
index a368529e8ed3..e37d9dba320d 100644
--- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
+++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
@@ -1577,14 +1577,19 @@ are the contents of this file:
           unsigned int period_step;
           unsigned int sleep_min;		/* min ticks to sleep */
           snd_pcm_uframes_t start_threshold;
-          snd_pcm_uframes_t stop_threshold;
-          snd_pcm_uframes_t silence_threshold; /* Silence filling happens when
-                                                  noise is nearest than this */
-          snd_pcm_uframes_t silence_size;	/* Silence filling size */
+          /*
+           * The following two thresholds alleviate playback buffer underruns; when
+           * hw_avail drops below the threshold, the respective action is triggered:
+           */
+          snd_pcm_uframes_t stop_threshold;	/* - stop playback */
+          snd_pcm_uframes_t silence_threshold;	/* - pre-fill buffer with silence */
+          snd_pcm_uframes_t silence_size;       /* max size of silence pre-fill; when >= boundary,
+                                                 * fill played area with silence immediately */
           snd_pcm_uframes_t boundary;	/* pointers wrap point */
   
-          snd_pcm_uframes_t silenced_start;
-          snd_pcm_uframes_t silenced_size;
+          /* internal data of auto-silencer */
+          snd_pcm_uframes_t silence_start; /* starting pointer to silence area */
+          snd_pcm_uframes_t silence_filled; /* size filled with silence */
   
           snd_pcm_sync_id_t sync;		/* hardware synchronization ID */
   
diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 27040b472a4f..19f564606ac4 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -378,18 +378,18 @@ struct snd_pcm_runtime {
 	unsigned int rate_den;
 	unsigned int no_period_wakeup: 1;
 
-	/* -- SW params -- */
-	int tstamp_mode;		/* mmap timestamp is updated */
+	/* -- SW params; see struct snd_pcm_sw_params for comments -- */
+	int tstamp_mode;
   	unsigned int period_step;
 	snd_pcm_uframes_t start_threshold;
 	snd_pcm_uframes_t stop_threshold;
-	snd_pcm_uframes_t silence_threshold; /* Silence filling happens when
-						noise is nearest than this */
-	snd_pcm_uframes_t silence_size;	/* Silence filling size */
-	snd_pcm_uframes_t boundary;	/* pointers wrap point */
+	snd_pcm_uframes_t silence_threshold;
+	snd_pcm_uframes_t silence_size;
+	snd_pcm_uframes_t boundary;
 
+	/* internal data of auto-silencer */
 	snd_pcm_uframes_t silence_start; /* starting pointer to silence area */
-	snd_pcm_uframes_t silence_filled; /* size filled with silence */
+	snd_pcm_uframes_t silence_filled; /* already filled part of silence area */
 
 	union snd_pcm_sync_id sync;	/* hardware synchronization ID */
 
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index de6810e94abe..a255a891eb81 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -429,9 +429,14 @@ struct snd_pcm_sw_params {
 	snd_pcm_uframes_t avail_min;		/* min avail frames for wakeup */
 	snd_pcm_uframes_t xfer_align;		/* obsolete: xfer size need to be a multiple */
 	snd_pcm_uframes_t start_threshold;	/* min hw_avail frames for automatic start */
-	snd_pcm_uframes_t stop_threshold;	/* min avail frames for automatic stop */
-	snd_pcm_uframes_t silence_threshold;	/* min distance from noise for silence filling */
-	snd_pcm_uframes_t silence_size;		/* silence block size */
+	/*
+	 * The following two thresholds alleviate playback buffer underruns; when
+	 * hw_avail drops below the threshold, the respective action is triggered:
+	 */
+	snd_pcm_uframes_t stop_threshold;	/* - stop playback */
+	snd_pcm_uframes_t silence_threshold;	/* - pre-fill buffer with silence */
+	snd_pcm_uframes_t silence_size;		/* max size of silence pre-fill; when >= boundary,
+						 * fill played area with silence immediately */
 	snd_pcm_uframes_t boundary;		/* pointers wrap point */
 	unsigned int proto;			/* protocol version */
 	unsigned int tstamp_type;		/* timestamp type (req. proto >= 2.0.12) */
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 02fd65993e7e..5bb2129cceac 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -42,70 +42,56 @@ static int fill_silence_frames(struct snd_pcm_substream *substream,
  *
  * when runtime->silence_size >= runtime->boundary - fill processed area with silence immediately
  */
-void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_uframes_t new_hw_ptr)
+void snd_pcm_playback_silence(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	snd_pcm_uframes_t frames, ofs, transfer;
+	snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr);
+	snd_pcm_sframes_t added, hw_avail, frames;
+	snd_pcm_uframes_t noise_dist, ofs, transfer;
 	int err;
 
+	added = appl_ptr - runtime->silence_start;
+	if (added) {
+		if (added < 0)
+			added += runtime->boundary;
+		if (added < runtime->silence_filled)
+			runtime->silence_filled -= added;
+		else
+			runtime->silence_filled = 0;
+		runtime->silence_start = appl_ptr;
+	}
+
+	// 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;
+
+	noise_dist = hw_avail + runtime->silence_filled;
 	if (runtime->silence_size < runtime->boundary) {
-		snd_pcm_sframes_t noise_dist, n;
-		snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr);
-		if (runtime->silence_start != appl_ptr) {
-			n = appl_ptr - runtime->silence_start;
-			if (n < 0)
-				n += runtime->boundary;
-			if ((snd_pcm_uframes_t)n < runtime->silence_filled)
-				runtime->silence_filled -= n;
-			else
-				runtime->silence_filled = 0;
-			runtime->silence_start = appl_ptr;
-		}
-		if (runtime->silence_filled >= runtime->buffer_size)
-			return;
-		noise_dist = snd_pcm_playback_hw_avail(runtime) + runtime->silence_filled;
-		if (noise_dist >= (snd_pcm_sframes_t) runtime->silence_threshold)
-			return;
 		frames = runtime->silence_threshold - noise_dist;
+		if (frames <= 0)
+			return;
 		if (frames > runtime->silence_size)
 			frames = runtime->silence_size;
 	} else {
-		if (new_hw_ptr == ULONG_MAX) {	/* initialization */
-			snd_pcm_sframes_t avail = snd_pcm_playback_hw_avail(runtime);
-			if (avail > runtime->buffer_size)
-				avail = runtime->buffer_size;
-			runtime->silence_filled = avail > 0 ? avail : 0;
-			runtime->silence_start = (runtime->status->hw_ptr +
-						  runtime->silence_filled) %
-						 runtime->boundary;
-		} else {
-			ofs = runtime->status->hw_ptr;
-			frames = new_hw_ptr - ofs;
-			if ((snd_pcm_sframes_t)frames < 0)
-				frames += runtime->boundary;
-			runtime->silence_filled -= frames;
-			if ((snd_pcm_sframes_t)runtime->silence_filled < 0) {
-				runtime->silence_filled = 0;
-				runtime->silence_start = new_hw_ptr;
-			} else {
-				runtime->silence_start = ofs;
-			}
-		}
-		frames = runtime->buffer_size - runtime->silence_filled;
+		frames = runtime->buffer_size - noise_dist;
+		if (frames <= 0)
+			return;
 	}
+
 	if (snd_BUG_ON(frames > runtime->buffer_size))
 		return;
-	if (frames == 0)
-		return;
-	ofs = runtime->silence_start % runtime->buffer_size;
-	while (frames > 0) {
+	ofs = (runtime->silence_start + runtime->silence_filled) % runtime->buffer_size;
+	do {
 		transfer = ofs + frames > runtime->buffer_size ? runtime->buffer_size - ofs : frames;
 		err = fill_silence_frames(substream, ofs, transfer);
 		snd_BUG_ON(err < 0);
 		runtime->silence_filled += transfer;
 		frames -= transfer;
 		ofs = 0;
-	}
+	} while (frames > 0);
 	snd_pcm_dma_buffer_sync(substream, SNDRV_DMA_SYNC_DEVICE);
 }
 
@@ -439,10 +425,6 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 		return 0;
 	}
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
-	    runtime->silence_size > 0)
-		snd_pcm_playback_silence(substream, new_hw_ptr);
-
 	if (in_interrupt) {
 		delta = new_hw_ptr - runtime->hw_ptr_interrupt;
 		if (delta < 0)
@@ -460,6 +442,10 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 		runtime->hw_ptr_wrap += runtime->boundary;
 	}
 
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
+	    runtime->silence_size > 0)
+		snd_pcm_playback_silence(substream);
+
 	update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
 
 	return snd_pcm_update_state(substream, runtime);
diff --git a/sound/core/pcm_local.h b/sound/core/pcm_local.h
index ecb21697ae3a..42fe3a4e9154 100644
--- a/sound/core/pcm_local.h
+++ b/sound/core/pcm_local.h
@@ -29,8 +29,7 @@ int snd_pcm_update_state(struct snd_pcm_substream *substream,
 			 struct snd_pcm_runtime *runtime);
 int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream);
 
-void snd_pcm_playback_silence(struct snd_pcm_substream *substream,
-			      snd_pcm_uframes_t new_hw_ptr);
+void snd_pcm_playback_silence(struct snd_pcm_substream *substream);
 
 static inline snd_pcm_uframes_t
 snd_pcm_avail(struct snd_pcm_substream *substream)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 331380c2438b..0e3e7997dc58 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -958,7 +958,7 @@ static int snd_pcm_sw_params(struct snd_pcm_substream *substream,
 	if (snd_pcm_running(substream)) {
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
 		    runtime->silence_size > 0)
-			snd_pcm_playback_silence(substream, ULONG_MAX);
+			snd_pcm_playback_silence(substream);
 		err = snd_pcm_update_state(substream, runtime);
 	}
 	snd_pcm_stream_unlock_irq(substream);
@@ -1455,7 +1455,7 @@ static void snd_pcm_post_start(struct snd_pcm_substream *substream,
 	__snd_pcm_set_state(runtime, state);
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
 	    runtime->silence_size > 0)
-		snd_pcm_playback_silence(substream, ULONG_MAX);
+		snd_pcm_playback_silence(substream);
 	snd_pcm_timer_notify(substream, SNDRV_TIMER_EVENT_MSTART);
 }
 
@@ -1916,7 +1916,7 @@ static void snd_pcm_post_reset(struct snd_pcm_substream *substream,
 	runtime->control->appl_ptr = runtime->status->hw_ptr;
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
 	    runtime->silence_size > 0)
-		snd_pcm_playback_silence(substream, ULONG_MAX);
+		snd_pcm_playback_silence(substream);
 	snd_pcm_stream_unlock_irq(substream);
 }
 
-- 
2.40.0.152.g15d061e6df

[PATCH v2 2/2] ALSA: pcm: auto-fill buffer with silence when draining playback

[Oswald Buddenhagen]

Draining will always playback somewhat beyond the end of the filled
buffer. This would produce artifacts if the user did not set up the
auto-silencing machinery, which is an extremely easy mistake to make, as
the API strongly suggests convenient fire-and-forget semantics. This
patch makes it work out of the box.

Applying this unconditionally is uncontroversial for RW_ACCESS, as the
buffer is fully controlled by the kernel in this case, which a) makes
failure to set up silencing even more likely and b) no detrimental
effects on user space are possible.

MMAP_ACCESS is a different matter:
- It can be argued that the user can be expected to know that the buffer
  needs to be padded one way or another. I dispute that; of the numerous
  resources I surveyed, only one mentioned this. Draining is a
  convenience function also in the mmap case - an application that wants
  to control things finely would just use start/stop and manage the
  timing itself.
- It can be argued that it's a bad thing to overwrite a buffer the user
  has access to without them explicitly requesting it. While technically
  true, I think that's only a hypothetical issue - applications can be
  expected to treat consumed samples as undefined data:
  - The cases where playing back the same samples would be even useful
    and practical are extremely limited.
  - Most user code uses cross-platform/-API abstractions, which makes it
    even less likely that they would get the idea that it's OK to re-use
    buffered samples.

So I think the trade-off between fixing numerous applications and
potentially breaking some is skewed towards the former to the point that
it's not even a question.

We do the filling even if the driver supports exact draining
(SNDRV_PCM_TRIGGER_DRAIN), because a) the cost of filling two periods
from time to time is negligible, so it's not worth complicating the code
and b) so the behavior is consistent between drivers, so hypothetical
problems with the mmap case would be easier to reproduce.

It would be possible to add an opt-in API to the kernel and leave
actually enabling it to alsa-lib. However, this would add significant
overall complexity, for no obvious gain.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

---
v2:
- fill only up to two periods, to avoid undue load with big buffers
- added discussion to commit message
---
 sound/core/pcm_lib.c    | 47 +++++++++++++++++++++++++----------------
 sound/core/pcm_native.c |  3 ++-
 2 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 5bb2129cceac..b8940ceeaedb 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -61,24 +61,35 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream)
 		runtime->silence_start = appl_ptr;
 	}
 
-	// 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;
+	if (runtime->state == SNDRV_PCM_STATE_DRAINING) {
+		// We actually need only the next period boundary plus the FIFO size
+		// plus some slack for IRQ delays, but it's not worth calculating that.
+		frames = runtime->period_size * 2 - runtime->silence_filled;
+		if (frames <= 0)
+			return;
+		// Impossible, unless the buffer has only one period.
+		if (frames > runtime->buffer_size)
+			frames = runtime->buffer_size;
+	} else  {
+		// 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;
 
-	noise_dist = hw_avail + runtime->silence_filled;
-	if (runtime->silence_size < runtime->boundary) {
-		frames = runtime->silence_threshold - noise_dist;
-		if (frames <= 0)
-			return;
-		if (frames > runtime->silence_size)
-			frames = runtime->silence_size;
-	} else {
-		frames = runtime->buffer_size - noise_dist;
-		if (frames <= 0)
-			return;
+		noise_dist = hw_avail + runtime->silence_filled;
+		if (runtime->silence_size < runtime->boundary) {
+			frames = runtime->silence_threshold - noise_dist;
+			if (frames <= 0)
+				return;
+			if (frames > runtime->silence_size)
+				frames = runtime->silence_size;
+		} else {
+			frames = runtime->buffer_size - noise_dist;
+			if (frames <= 0)
+				return;
+		}
 	}
 
 	if (snd_BUG_ON(frames > runtime->buffer_size))
@@ -443,7 +454,7 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 	}
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
-	    runtime->silence_size > 0)
+	    (runtime->silence_size > 0 || runtime->state == SNDRV_PCM_STATE_DRAINING))
 		snd_pcm_playback_silence(substream);
 
 	update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 0e3e7997dc58..6ecb6a733606 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1454,7 +1454,7 @@ static void snd_pcm_post_start(struct snd_pcm_substream *substream,
 							    runtime->rate;
 	__snd_pcm_set_state(runtime, state);
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
-	    runtime->silence_size > 0)
+	    (runtime->silence_size > 0 || state == SNDRV_PCM_STATE_DRAINING))
 		snd_pcm_playback_silence(substream);
 	snd_pcm_timer_notify(substream, SNDRV_TIMER_EVENT_MSTART);
 }
@@ -2045,6 +2045,7 @@ static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream,
 			break;
 		case SNDRV_PCM_STATE_RUNNING:
 			__snd_pcm_set_state(runtime, SNDRV_PCM_STATE_DRAINING);
+			snd_pcm_playback_silence(substream);
 			break;
 		case SNDRV_PCM_STATE_XRUN:
 			__snd_pcm_set_state(runtime, SNDRV_PCM_STATE_SETUP);
-- 
2.40.0.152.g15d061e6df

Re: [PATCH v2 2/2] ALSA: pcm: auto-fill buffer with silence when draining playback

[Jaroslav Kysela]

On 20. 04. 23 13:33, Oswald Buddenhagen wrote:
> Draining will always playback somewhat beyond the end of the filled
> buffer. This would produce artifacts if the user did not set up the
> auto-silencing machinery, which is an extremely easy mistake to make, as
> the API strongly suggests convenient fire-and-forget semantics. This
> patch makes it work out of the box.

NACK. The initial implementation should be put to alsa-lib as discussed.

				Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH v2 2/2] ALSA: pcm: auto-fill buffer with silence when draining playback

[Oswald Buddenhagen]

On Fri, Apr 21, 2023 at 11:33:35AM +0200, Jaroslav Kysela wrote:
>On 20. 04. 23 13:33, Oswald Buddenhagen wrote:
>> Draining will always playback somewhat beyond the end of the filled
>> buffer. This would produce artifacts if the user did not set up the
>> auto-silencing machinery, which is an extremely easy mistake to make, as
>> the API strongly suggests convenient fire-and-forget semantics. This
>> patch makes it work out of the box.
>
>NACK. The initial implementation should be put to alsa-lib as discussed.
>
as discussed, a user-space only implementation based on the current 
kernel api is not reasonable:
it could either enable auto-silencing on device open (which would be 
unreasonably expensive) or it could enable it on drain (and disable it 
once draining is done, which would be unreasonably complex due to 
needing to handle asynchronous draining completion).

so we would at least need a kernel api to enable silence-on-drain, which 
user space could apply on device open.
this would be easy enough to do, but i really don't see a point in 
adding that complexity, given that it should be always enabled, lest it 
won't have much of a real-world impact.

fwiw, i just realized that the argument against touching the mmap'd 
buffer is even weaker with the updated implementation, as it only clears 
as much as user space would have to clear anyway (pedantically, i could 
round it down to the end of a period rather than filling two whole 
periods, if that's the bit that convinces you).

regards

Re: [PATCH v2 2/2] ALSA: pcm: auto-fill buffer with silence when draining playback

[Jaroslav Kysela]

On 21. 04. 23 12:04, Oswald Buddenhagen wrote:
> On Fri, Apr 21, 2023 at 11:33:35AM +0200, Jaroslav Kysela wrote:
>> On 20. 04. 23 13:33, Oswald Buddenhagen wrote:
>>> Draining will always playback somewhat beyond the end of the filled
>>> buffer. This would produce artifacts if the user did not set up the
>>> auto-silencing machinery, which is an extremely easy mistake to make, as
>>> the API strongly suggests convenient fire-and-forget semantics. This
>>> patch makes it work out of the box.
>>
>> NACK. The initial implementation should be put to alsa-lib as discussed.
>>
> as discussed, a user-space only implementation based on the current
> kernel api is not reasonable:
> it could either enable auto-silencing on device open (which would be
> unreasonably expensive) or it could enable it on drain (and disable it
> once draining is done, which would be unreasonably complex due to
> needing to handle asynchronous draining completion).

I doubt. We should consider all solutions. The drain ends with the SETUP 
state, thus the application must call prepare again. We can restore the 
sw_params there for all types of i/o access (if the app does not reset 
sw_params itself). We can just set the silence_size (sw_params) in 
snd_pcm_hw_drain() and it's all.

Also, an interrupt can be "lost" or "merged" only for the small periods where 
the system is not able to handle the fast interrupts. For large periods, we 
should not assume that any of the interrupt is lost. Otherwise, it would break 
many things and the driver is really broken in this case. So the drain fill 
size should be updated for the big periods like "fill_align_to_last_period + 
100ms" or so.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

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

[Jaroslav Kysela]

On 20. 04. 23 13:33, Oswald Buddenhagen wrote:
> The auto-silencer supports two modes: "thresholded" to fill up "just
> enough", and "top-up" to fill up "as much as possible". The two modes
> used rather distinct code paths, which this patch unifies. The only
> remaining distinction is how much we actually want to fill.
> 
> This fixes a bug in thresholded mode, where we failed to use new_hw_ptr,
> resulting in under-fill.
> 
> Top-up mode is now more well-behaved and much easier to understand in
> corner cases.
> 
> This also updates comments in the proximity of silencing-related data
> structures.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

Looks much better. Thanks.

Reviewed-by: Jaroslav Kysela <perex@perex.cz>

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

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

[Takashi Iwai]

On Thu, 20 Apr 2023 13:33:23 +0200,
Oswald Buddenhagen wrote:
> 
> The auto-silencer supports two modes: "thresholded" to fill up "just
> enough", and "top-up" to fill up "as much as possible". The two modes
> used rather distinct code paths, which this patch unifies. The only
> remaining distinction is how much we actually want to fill.
> 
> This fixes a bug in thresholded mode, where we failed to use new_hw_ptr,
> resulting in under-fill.
> 
> Top-up mode is now more well-behaved and much easier to understand in
> corner cases.
> 
> This also updates comments in the proximity of silencing-related data
> structures.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> 
> ---
> v2:
> - removed useless boundary check
> - got rid of casts by using a signed type for deltas.
>   i did not adjust the style of the conditionals, because it's not clear
>   whether the hangup was actually over that, or merely over the casts.
> - dropped use of C99 comments where the surroundings suggest it.
>   (in the case of the interspersed multi-line comments, that doesn't
>   look like an improvement to me at all ...)
> - swapped the `added` and `hw_avail` calculation blocks to reduce
>   subsequent churn. it's more logical that way anyway.

Applied this one.  For the second patch, let's discuss further.


thanks,

Takashi