[PATCH v3] ALSA: pcm: auto-fill buffer with silence when draining playback

[PATCH v3] 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.

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

---
you are NOT expected to apply this. i just needed it for my testing
(it's easier to deploy as i'm hacking on the kernel anyway) and wanted
to post it for posterity.

v3:
- rebased to updated silencing code
- intro period alignment to reduce redundant fill
- cut down commit message again, as it's disabled by default now
v2:
- fill only up to two periods, to avoid undue load with big buffers
- added discussion to commit message
---
 sound/core/pcm_lib.c    | 35 ++++++++++++++++++++++++++++++++++-
 sound/core/pcm_native.c |  3 ++-
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 9c121a921b04..46e63e653789 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -67,6 +67,11 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram
 	snd_pcm_uframes_t frames, ofs, transfer;
 	int err;
 
+	if (runtime->silence_size == 0 &&
+	    (runtime->state != SNDRV_PCM_STATE_DRAINING ||
+	     (runtime->info & SNDRV_PCM_HW_PARAMS_NO_DRAIN_SILENCE) ||
+	     (runtime->hw.info & SNDRV_PCM_INFO_PERFECT_DRAIN)))
+		return;
 	if (runtime->silence_size < runtime->boundary) {
 		snd_pcm_sframes_t noise_dist;
 		snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr);
@@ -80,6 +85,33 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram
 			noise_dist += runtime->boundary;
 		/* total noise distance */
 		noise_dist += runtime->silence_filled;
+		if (runtime->state == SNDRV_PCM_STATE_DRAINING) {
+			snd_pcm_uframes_t slack = runtime->rate / 10;
+			snd_pcm_sframes_t threshold;
+			snd_pcm_uframes_t ps = runtime->period_size;
+			snd_pcm_uframes_t silence_size = ps;
+			// Round down to start of next period. This is disabled
+			// if the period count is not integer.
+			if (runtime->periods * ps == runtime->buffer_size)
+				silence_size = ps - (appl_ptr + ps - 1) % ps - 1;
+			// Add overshoot to accomodate FIFOs and IRQ delays.
+			// The default 1/10th secs is very generous. But more than one
+			// period doesn't make sense; the driver would set the minimum
+			// period size accordingly.
+			slack = min(slack, ps);
+			silence_size += slack;
+			// This catches the periods == 1 case.
+			silence_size = min(silence_size, runtime->buffer_size);
+
+			threshold = ps + slack;
+			if (noise_dist >= threshold)
+				return;
+			frames = threshold - noise_dist;
+			if (frames > silence_size)
+				frames = silence_size;
+
+			goto avoid_reindent;
+		}
 		if (noise_dist >= (snd_pcm_sframes_t) runtime->silence_threshold)
 			return;
 		frames = runtime->silence_threshold - noise_dist;
@@ -118,6 +150,7 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram
 		 */
 		frames = runtime->buffer_size - runtime->silence_filled;
 	}
+avoid_reindent:
 	if (snd_BUG_ON(frames > runtime->buffer_size))
 		return;
 	if (frames == 0)
@@ -465,7 +498,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, new_hw_ptr);
 
 	if (in_interrupt) {
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 39a65d1415ab..913dae449ba0 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, ULONG_MAX);
 	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, ULONG_MAX);
 			break;
 		case SNDRV_PCM_STATE_XRUN:
 			__snd_pcm_set_state(runtime, SNDRV_PCM_STATE_SETUP);
-- 
2.40.0.152.g15d061e6df

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

[Jaroslav Kysela]

On 10. 05. 23 18:29, 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.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

NAK. Already implemented in alsa-lib which is enough for the first 
implementation. This patch also does not set the perfect drain flag nor 
handles the silence suppression for the user space (double fill) [1].

				Jaroslav

[1] https://lore.kernel.org/alsa-devel/20230502115536.986900-1-perex@perex.cz/

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

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

[Oswald Buddenhagen]

On Wed, May 10, 2023 at 06:54:57PM +0200, Jaroslav Kysela wrote:
>NAK. Already implemented in alsa-lib which is enough for the first 
>implementation.
>
did you read the part below the cut-off line? ;-)

>> you are NOT expected to apply this. i just needed it for my testing
>> (it's easier to deploy as i'm hacking on the kernel anyway) and wanted
>> to post it for posterity.

this patch should serve as a template for fixing the bug in the 
user-space implementation i reported a few days back. i'll post a patch 
sometimes soon if you don't beat me to it.

also, i think the drain_silence config should be re-interpreted as 
overriding the default 1/10th sec "overshoot", rather than nonsensically 
the total delay irrespective of period size.

regards