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

[PATCH 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>
---
 .../kernel-api/writing-an-alsa-driver.rst     | 14 +--
 include/sound/pcm.h                           | 14 +--
 include/uapi/sound/asound.h                   |  9 +-
 sound/core/pcm_lib.c                          | 86 ++++++++-----------
 sound/core/pcm_local.h                        |  3 +-
 sound/core/pcm_native.c                       |  6 +-
 6 files changed, 62 insertions(+), 70 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..f2834a016473 100644
--- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
+++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
@@ -1577,14 +1577,16 @@ 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;		/* msx size of silence pre-fill */
           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..f20400bb7032 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..50aa1d98873f 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -429,9 +429,12 @@ 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..b074c5b926db 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -42,70 +42,58 @@ 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 appl_ptr = READ_ONCE(runtime->control->appl_ptr);
+	snd_pcm_sframes_t hw_avail, added, noise_dist;
 	snd_pcm_uframes_t frames, ofs, transfer;
 	int err;
 
+	// 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;
+
+	added = appl_ptr - runtime->silence_start;
+	if (added) {
+		if (added < 0)
+			added += runtime->boundary;
+		if ((snd_pcm_uframes_t)added < runtime->silence_filled)
+			runtime->silence_filled -= added;
+		else
+			runtime->silence_filled = 0;
+		runtime->silence_start = appl_ptr;
+	}
+
+	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 ((snd_pcm_sframes_t) 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 ((snd_pcm_sframes_t) 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 +427,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 +444,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 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. This patch makes it work out of the box.

Rather than figuring out the right threshold (which is one period plus
the card-specific FIFO size plus some IRQ jitter), we use "top-up" mode.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/core/pcm_lib.c    | 5 +++--
 sound/core/pcm_native.c | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index b074c5b926db..1dd870a2093d 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -71,7 +71,8 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream)
 	}
 
 	noise_dist = hw_avail + runtime->silence_filled;
-	if (runtime->silence_size < runtime->boundary) {
+	if (runtime->silence_size < runtime->boundary &&
+	    runtime->state != SNDRV_PCM_STATE_DRAINING) {
 		frames = runtime->silence_threshold - noise_dist;
 		if ((snd_pcm_sframes_t) frames <= 0)
 			return;
@@ -445,7 +446,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 1/2] ALSA: pcm: rewrite snd_pcm_playback_silence()

[Takashi Iwai]

On Wed, 05 Apr 2023 22:12:18 +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>

Applied both patches now to for-next branch.


thanks,

Takashi

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

[Jaroslav Kysela]

On 05. 04. 23 22:12, 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. This patch makes it work out of the box.
> 
> Rather than figuring out the right threshold (which is one period plus
> the card-specific FIFO size plus some IRQ jitter), we use "top-up" mode.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

I think that it was really bad decision to apply this patch without a broader 
discussion. When we designed the API, we knew about described problems and we 
decided to keep this up to applications. The silencing may not help in all 
cases where the PCM samples ends with a high volume. A volume ramping should 
be used and it's an application job. Also, silencing touches the DMA buffer 
which may not be desired. And lastly drivers can handle draining correctly 
(stop at the exact position - see substream->ops->trigger with 
SNDRV_PCM_TRIGGER_DRAIN argument).

I would create a new API extension for this (new ioctl or sw_params flag), but 
the default behaviour should be retained.

I will try to review the first patch too, but my time is limited over Easter.

					Jaroslav

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

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

[Takashi Iwai]

On Sat, 08 Apr 2023 01:58:21 +0200,
Jaroslav Kysela wrote:
> 
> On 05. 04. 23 22:12, 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. This patch makes it work out of the box.
> > 
> > Rather than figuring out the right threshold (which is one period plus
> > the card-specific FIFO size plus some IRQ jitter), we use "top-up" mode.
> > 
> > Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> 
> I think that it was really bad decision to apply this patch without a
> broader discussion. When we designed the API, we knew about described
> problems and we decided to keep this up to applications. The silencing
> may not help in all cases where the PCM samples ends with a high
> volume. A volume ramping should be used and it's an application
> job. Also, silencing touches the DMA buffer which may not be
> desired. And lastly drivers can handle draining correctly (stop at the
> exact position - see substream->ops->trigger with
> SNDRV_PCM_TRIGGER_DRAIN argument).
> 
> I would create a new API extension for this (new ioctl or sw_params
> flag), but the default behaviour should be retained.
> 
> I will try to review the first patch too, but my time is limited over Easter.

OK, thanks.  I'll drop the patch meanwhile.

Applying the silencing blindly might be an overkill, indeed, although
this could be seen as an easy solution.  Let's see.


Takashi

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

[Oswald Buddenhagen]

On Sat, Apr 08, 2023 at 01:58:21AM +0200, Jaroslav Kysela wrote:
>On 05. 04. 23 22:12, 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. This patch makes it work out of the box.
>> 
>I think that it was really bad decision to apply this patch without a 
>broader discussion.

>When we designed the API, we knew about described problems and we 
>decided to keep this up to applications.
>
i ran into no documentation of either the problems nor the decisions and 
their implications for the user.

>The silencing may not help in all cases where the PCM samples ends with 
>a high volume.
>
that would just create a slight crack, which isn't any different from a 
"regular" sudden stop.

> A volume ramping should be used and it's an application job.
>
imo, that entirely misses the point - the volume is most likely already 
zero at the end of the buffer. that doesn't mean that it's ok to play 
the samples again where the volume might not be *quite* zero yet.

>Also, silencing touches the DMA buffer which may not be desired.
>
hypothetically, yes. but practically? why would anyone want to play the 
same samples after draining? draining is most likely followed by closing 
the device. and even if not, in most cases (esp. where draining would 
actually make sense) one wouldn't play a fixed pattern that could be 
just re-used, so one would have to re-fill the buffer prior to starting 
again anyway. never mind the effort necessary to track the state of the 
buffer instead of just re-filling it. so for all practical purposes, 
already played samples can be considered undefined data and thus safe to 
overwrite.

>And lastly drivers can handle draining correctly (stop at the exact 
>position - see substream->ops->trigger with SNDRV_PCM_TRIGGER_DRAIN 
>argument).
>
yeah. hypothetically. afaict, there is exactly one driver which supports 
this. most (older) hardware wouldn't even have the capability to do such 
precise timing without external help.

On Sat, Apr 08, 2023 at 07:55:48AM +0200, Takashi Iwai wrote:
>Applying the silencing blindly might be an overkill, indeed, although
>this could be seen as an easy solution.  Let's see.
>
i don't think that "overkill" is right here. someone has to do the 
silencing for draining to be useful at all, and so the question is only 
who that should be. my argument is that not auto-silencing is 
*extremely* unexpected, and thus just bad api. i'm pretty certain that 
about 99% of the usages of DRAIN start out missing this, and most never 
get fixed.

imo, if any api is added, it should be to opt *out* of auto-silencing.  
but i don't think this makes any sense; there would be ~zero users of 
this ever.

regards

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

[Jaroslav Kysela]

On 05. 04. 23 22:12, 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.

I don't follow what you refer here. The old code uses 
snd_pcm_playback_hw_avail() thus new hw_ptr for the threshold mode, too.

> 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>
> ---
>   .../kernel-api/writing-an-alsa-driver.rst     | 14 +--
>   include/sound/pcm.h                           | 14 +--
>   include/uapi/sound/asound.h                   |  9 +-
>   sound/core/pcm_lib.c                          | 86 ++++++++-----------
>   sound/core/pcm_local.h                        |  3 +-
>   sound/core/pcm_native.c                       |  6 +-
>   6 files changed, 62 insertions(+), 70 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..f2834a016473 100644
> --- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
> +++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
> @@ -1577,14 +1577,16 @@ 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;		/* msx size of silence pre-fill */
>             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..f20400bb7032 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

I would use traditional C-style comment style here (to match other descriptions).

>   	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..50aa1d98873f 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -429,9 +429,12 @@ 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:

Traditional C-style comments.

> +	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..b074c5b926db 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -42,70 +42,58 @@ 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 appl_ptr = READ_ONCE(runtime->control->appl_ptr);
> +	snd_pcm_sframes_t hw_avail, added, noise_dist;
>   	snd_pcm_uframes_t frames, ofs, transfer;
>   	int err;
>   
> +	// 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.

> +
> +	added = appl_ptr - runtime->silence_start;
> +	if (added) {
> +		if (added < 0)
> +			added += runtime->boundary;
> +		if ((snd_pcm_uframes_t)added < runtime->silence_filled)
> +			runtime->silence_filled -= added;
> +		else
> +			runtime->silence_filled = 0;
> +		runtime->silence_start = appl_ptr;
> +	}
> +
> +	noise_dist = hw_avail + runtime->silence_filled;
>   	if (runtime->silence_size < runtime->boundary) {
> -		snd_pcm_sframes_t noise_dist, n;

...

> -		if (noise_dist >= (snd_pcm_sframes_t) runtime->silence_threshold)
> -			return;
>   		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;

>   		if (frames > runtime->silence_size)
>   			frames = runtime->silence_size;
>   	} else {
> -		if (new_hw_ptr == ULONG_MAX) {	/* initialization */

...

> -		frames = runtime->buffer_size - runtime->silence_filled;
> +		frames = runtime->buffer_size - noise_dist;
> +		if ((snd_pcm_sframes_t) frames <= 0)
> +			return;

Similar thing, move the check before the frames assignment.

						Jaroslav

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

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

[Jaroslav Kysela]

On 08. 04. 23 9:24, Oswald Buddenhagen wrote:
> On Sat, Apr 08, 2023 at 01:58:21AM +0200, Jaroslav Kysela wrote:
>> On 05. 04. 23 22:12, 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. This patch makes it work out of the box.
>>>
>> I think that it was really bad decision to apply this patch without a
>> broader discussion.
> 
>> When we designed the API, we knew about described problems and we
>> decided to keep this up to applications.
>>
> i ran into no documentation of either the problems nor the decisions and
> their implications for the user.

The documentation may be improved, but the "period transfers" are described.

>> The silencing may not help in all cases where the PCM samples ends with
>> a high volume.
>>
> that would just create a slight crack, which isn't any different from a
> "regular" sudden stop.
> 
>> A volume ramping should be used and it's an application job.
>>
> imo, that entirely misses the point - the volume is most likely already
> zero at the end of the buffer. that doesn't mean that it's ok to play
> the samples again where the volume might not be *quite* zero yet.
> 
>> Also, silencing touches the DMA buffer which may not be desired.
>>
> hypothetically, yes. but practically? why would anyone want to play the
> same samples after draining? draining is most likely followed by closing
> the device. and even if not, in most cases (esp. where draining would
> actually make sense) one wouldn't play a fixed pattern that could be
> just re-used, so one would have to re-fill the buffer prior to starting
> again anyway. never mind the effort necessary to track the state of the
> buffer instead of just re-filling it. so for all practical purposes,
> already played samples can be considered undefined data and thus safe to
> overwrite.

The buffers can be mmaped so used directly for application and hardware. I 
don't really feel that it's a good thing to modify this buffer for playback 
when the application has not requested for that.

>> And lastly drivers can handle draining correctly (stop at the exact
>> position - see substream->ops->trigger with SNDRV_PCM_TRIGGER_DRAIN
>> argument).
>>
> yeah. hypothetically. afaict, there is exactly one driver which supports
> this. most (older) hardware wouldn't even have the capability to do such
> precise timing without external help.

Most hardware has FIFO and most drivers uses the DMA position, so I think that 
the interrupt -> stop DMA latency may be covered with this FIFO in most cases.

But I would really keep this on the driver code to handle this rather than do 
the forced silencing.

> On Sat, Apr 08, 2023 at 07:55:48AM +0200, Takashi Iwai wrote:
>> Applying the silencing blindly might be an overkill, indeed, although
>> this could be seen as an easy solution.  Let's see.
>>
> i don't think that "overkill" is right here. someone has to do the
> silencing for draining to be useful at all, and so the question is only
> who that should be. my argument is that not auto-silencing is
> *extremely* unexpected, and thus just bad api. i'm pretty certain that
> about 99% of the usages of DRAIN start out missing this, and most never
> get fixed.

Again, I would improve the documentation. Also, the silencing is controlled 
using sw_params, so applications may request the silencing before drain.

> imo, if any api is added, it should be to opt *out* of auto-silencing.
> but i don't think this makes any sense; there would be ~zero users of
> this ever.

Lastly, I think that you cannot call updated snd_pcm_playback_silence() 
function with runtime->silence_size == 0.

         if (runtime->silence_size < runtime->boundary) {
                 frames = runtime->silence_threshold - noise_dist;
                 if ((snd_pcm_sframes_t) frames <= 0)
                         return;
                 if (frames > runtime->silence_size)
                         frames = runtime->silence_size;
         } else {


The frames variable will be 0, so your code will do nothing.

			Jaroslav

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

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

[Oswald Buddenhagen]

On Tue, Apr 11, 2023 at 01:09:59PM +0200, Jaroslav Kysela wrote:
>On 08. 04. 23 9:24, Oswald Buddenhagen wrote:
>>> Also, silencing touches the DMA buffer which may not be desired.
>>>
>> hypothetically, yes. but practically? [...]
>
>The buffers can be mmaped so used directly for application and 
>hardware.
>
yes, and they are owned by the hardware/driver. an application would 
know better than doing with them anything they were not designed for.

>>> And lastly drivers can handle draining correctly (stop at the exact
>>> position - see substream->ops->trigger with SNDRV_PCM_TRIGGER_DRAIN
>>> argument).
>>>
>> yeah. hypothetically. afaict, there is exactly one driver which supports
>> this. most (older) hardware wouldn't even have the capability to do such
>> precise timing without external help.
>
>Most hardware has FIFO and most drivers uses the DMA position, so I think that 
>the interrupt -> stop DMA latency may be covered with this FIFO in most cases.
>
on most hardware it would be quite a stunt to re-program the buffer 
pointers on the fly to enable a mid-period interrupt. and given the 
reliability problems insisted on by takashi in the other thread, the 
approach seems questionable at best. and that's still ignoring the 
effort of migrating tens (hundreds?) of drivers.

>Again, I would improve the documentation.
>
no amount of documentation will fix a bad api. it's just not how humans 
work.

>the silencing is controlled using sw_params, so applications may 
>request the silencing before drain.
>
yeah, they could, but they don't, and most won't ever.

you're arguing for not doing a very practical and simple change that 
will fix a lot of user code at once, for the sake of preventing an 
entirely hypothetical and implausible problem. that is not a good 
trade-off.

>Lastly, I think that you cannot call updated snd_pcm_playback_silence() 
>function with runtime->silence_size == 0.
>
>         if (runtime->silence_size < runtime->boundary) {
>
you missed the hunk that adjusts the code accordingly.

regards

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

[Jaroslav Kysela]

On 11. 04. 23 15:57, Oswald Buddenhagen wrote:
> On Tue, Apr 11, 2023 at 01:09:59PM +0200, Jaroslav Kysela wrote:
>> On 08. 04. 23 9:24, Oswald Buddenhagen wrote:
>>>> Also, silencing touches the DMA buffer which may not be desired.
>>>>
>>> hypothetically, yes. but practically? [...]
>>
>> The buffers can be mmaped so used directly for application and
>> hardware.
>>
> yes, and they are owned by the hardware/driver. an application would
> know better than doing with them anything they were not designed for.
> 
>>>> And lastly drivers can handle draining correctly (stop at the exact
>>>> position - see substream->ops->trigger with SNDRV_PCM_TRIGGER_DRAIN
>>>> argument).
>>>>
>>> yeah. hypothetically. afaict, there is exactly one driver which supports
>>> this. most (older) hardware wouldn't even have the capability to do such
>>> precise timing without external help.
>>
>> Most hardware has FIFO and most drivers uses the DMA position, so I think that
>> the interrupt -> stop DMA latency may be covered with this FIFO in most cases.
>>
> on most hardware it would be quite a stunt to re-program the buffer
> pointers on the fly to enable a mid-period interrupt. and given the
> reliability problems insisted on by takashi in the other thread, the
> approach seems questionable at best. and that's still ignoring the
> effort of migrating tens (hundreds?) of drivers.

I said to not change things at the moment. Drivers may be revised at some 
point. If we have large buffers, the silencing may consume many CPU ticks. If 
the driver already behaves nicely for the drain operation, this is an extra 
step which should be avoided. This can be handled using new snd_pcm_ops, of 
course.

You're using a hammer to fix a little issue.

>> Again, I would improve the documentation.
>>
> no amount of documentation will fix a bad api. it's just not how humans
> work.

Which code does not fill the last period? Alsa utilities do it. We can 
eventually handle this in alsa-lib.

>> the silencing is controlled using sw_params, so applications may
>> request the silencing before drain.
>>
> yeah, they could, but they don't, and most won't ever.
> 
> you're arguing for not doing a very practical and simple change that
> will fix a lot of user code at once, for the sake of preventing an
> entirely hypothetical and implausible problem. that is not a good
> trade-off.

I'm arguing that we should not do anything extra with the buffers until the 
application requests that. That's the clear API context.

>> Lastly, I think that you cannot call updated snd_pcm_playback_silence()
>> function with runtime->silence_size == 0.
>>
>>          if (runtime->silence_size < runtime->boundary) {
>>
> you missed the hunk that adjusts the code accordingly.

Ohh, yes. You're right.

If we allow modification of the PCM buffer, I think that we should:

- Do not modify the buffer for drivers already working with the
   appl_ptr data (end position) only.
- Handle the situation with the large buffer; it may make sense
   to change the "wait" operation from the end-of-period interrupt to time
   scheduler and stop the drain more early when the end-of-valid data condition
   is fulfilled.
- Increase the protocol version.

But as I wrote, I would make those extensions configurable 
(SNDRV_PCM_HW_PARAMS_DRAIN_ALLOW_SILENCE). It can be turned on by default.

					Jaroslav

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

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

[Oswald Buddenhagen]

On Tue, Apr 11, 2023 at 04:48:58PM +0200, Jaroslav Kysela wrote:
>You're using a hammer to fix a little issue.
>
yes, but at the time it seemed like a rather small hammer to me.

if large buffers are actually a thing (what for?), then the fill could 
be limited to two periods or something. it would make the code uglier, 
though.

>Which code does not fill the last period?
>
a lot, i imagine - doing that is rather counter-intuitive when using the 
write() access method.

also, just the last period is not enough, due to the fifo, and possibly 
delayed/lost irqs.

>>> the silencing is controlled using sw_params, so applications may
>>> request the silencing before drain.
>>>
>> yeah, they could, but they don't, and most won't ever.
>> 
>> you're arguing for not doing a very practical and simple change that
>> will fix a lot of user code at once, for the sake of preventing an
>> entirely hypothetical and implausible problem. that is not a good
>> trade-off.
>
>I'm arguing that we should not do anything extra with the buffers until 
>the application requests that.

>That's the clear API context.
>
no, it's not. you cannot assume this to be understood as the central 
guiding principle which trumps more immediate issues. people use an api 
to solve a specific problem, and they want to do that with the least 
effort possible. no-one is going to read the whole docu top to bottom 
and remember every caveat. if it appears to work, people will just call 
it a day, and that's exactly what will be the case with the use of DRAIN 
(one needs a somewhat specific configuration and content to even notice 
that there is a problem).

>If we allow modification of the PCM buffer, I think that we should:
>
>- Do not modify the buffer for drivers already working with the
>   appl_ptr data (end position) only.
>
i suppose that should be detected by the drain callback being set up?

>- Handle the situation with the large buffer; it may make sense
>   to change the "wait" operation from the end-of-period interrupt to time
>   scheduler and stop the drain more early when the end-of-valid data condition
>   is fulfilled.
>
i don't understand what you're asking for.

>- Increase the protocol version.
>
>But as I wrote, I would make those extensions configurable 
>(SNDRV_PCM_HW_PARAMS_DRAIN_ALLOW_SILENCE). It can be turned on by default.
>
i have no clue what would be involved in doing that. to me that sounds 
like overkill (solving a non-issue), and goes waaaay beyond what i 
expected to invest into this issue (really, i just wanted to verify that 
the emu10k1 fixes work, and accidentally discovered that there is a 
mid-layer issue that affects user space, as the pyalsaaudio lib i'm 
using doesn't handle it).

regards

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

[Jaroslav Kysela]

On 11. 04. 23 18:50, Oswald Buddenhagen wrote:

>> If we allow modification of the PCM buffer, I think that we should:
>>
>> - Do not modify the buffer for drivers already working with the
>>    appl_ptr data (end position) only.
>>
> i suppose that should be detected by the drain callback being set up?

Yes, but it would be probably better to add a default silencing callback with 
a warning to notify authors of drivers to review and eventually correct the 
behavior.

>> - Handle the situation with the large buffer; it may make sense
>>    to change the "wait" operation from the end-of-period interrupt to time
>>    scheduler and stop the drain more early when the end-of-valid data condition
>>    is fulfilled.
>>
> i don't understand what you're asking for.

Use jiffies/timeout instead waiting to the interrupt. In this case, the stop 
may be called earlier (in the middle of period). In this case the silenced 
area may be much smaller.

>> - Increase the protocol version.
>>
>> But as I wrote, I would make those extensions configurable
>> (SNDRV_PCM_HW_PARAMS_DRAIN_ALLOW_SILENCE). It can be turned on by default.
>>
> i have no clue what would be involved in doing that. to me that sounds
> like overkill (solving a non-issue), and goes waaaay beyond what i
> expected to invest into this issue (really, i just wanted to verify that
> the emu10k1 fixes work, and accidentally discovered that there is a
> mid-layer issue that affects user space, as the pyalsaaudio lib i'm
> using doesn't handle it).

OK. I don't think that it's a pyalsaudio job to resolve the issue with the 
minimal transfer chunk / period (which you set / know before the transfer is 
initiated).

				Jaroslav

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

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

[Takashi Iwai]

On Tue, 11 Apr 2023 19:23:17 +0200,
Jaroslav Kysela wrote:
> 
> On 11. 04. 23 18:50, Oswald Buddenhagen wrote:
> 
> >> If we allow modification of the PCM buffer, I think that we should:
> >> 
> >> - Do not modify the buffer for drivers already working with the
> >>    appl_ptr data (end position) only.
> >> 
> > i suppose that should be detected by the drain callback being set up?
> 
> Yes, but it would be probably better to add a default silencing
> callback with a warning to notify authors of drivers to review and
> eventually correct the behavior.
> 
> >> - Handle the situation with the large buffer; it may make sense
> >>    to change the "wait" operation from the end-of-period interrupt to time
> >>    scheduler and stop the drain more early when the end-of-valid data condition
> >>    is fulfilled.
> >> 
> > i don't understand what you're asking for.
> 
> Use jiffies/timeout instead waiting to the interrupt. In this case,
> the stop may be called earlier (in the middle of period). In this case
> the silenced area may be much smaller.

Does this difference matter so much?  I guess you're concerned about
the performance, right?  This sounds a bit too complex just for the
simple purpose...

> >> - Increase the protocol version.
> >> 
> >> But as I wrote, I would make those extensions configurable
> >> (SNDRV_PCM_HW_PARAMS_DRAIN_ALLOW_SILENCE). It can be turned on by default.
> >> 
> > i have no clue what would be involved in doing that. to me that sounds
> > like overkill (solving a non-issue), and goes waaaay beyond what i
> > expected to invest into this issue (really, i just wanted to verify that
> > the emu10k1 fixes work, and accidentally discovered that there is a
> > mid-layer issue that affects user space, as the pyalsaaudio lib i'm
> > using doesn't handle it).
> 
> OK. I don't think that it's a pyalsaudio job to resolve the issue with
> the minimal transfer chunk / period (which you set / know before the
> transfer is initiated).

I'm thinking whether we need to change anything in the kernel side for
this at all.  Can't it be changed rather in alsa-lib side instead?


Takashi

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

[Oswald Buddenhagen]

On Wed, Apr 12, 2023 at 09:54:54AM +0200, Takashi Iwai wrote:
>I'm thinking whether we need to change anything in the kernel side for
>this at all.  Can't it be changed rather in alsa-lib side instead?
>
it could, but it would be a lot uglier. user space would have to do a 
"man-in-the-middle attack" on the data, while in the kernel we can just 
slightly modify the consumer. this would be particularly obvious in the 
case of write() access.

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

[Oswald Buddenhagen]

[-- 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

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

[Takashi Iwai]

On Wed, 12 Apr 2023 10:04:31 +0200,
Oswald Buddenhagen wrote:
> 
> On Wed, Apr 12, 2023 at 09:54:54AM +0200, Takashi Iwai wrote:
> > I'm thinking whether we need to change anything in the kernel side for
> > this at all.  Can't it be changed rather in alsa-lib side instead?
> > 
> it could, but it would be a lot uglier. user space would have to do a
> "man-in-the-middle attack" on the data, while in the kernel we can
> just slightly modify the consumer. this would be particularly obvious
> in the case of write() access.

But basically it'd be like fiddling sw_params temporarily for
draining, I suppose?  And the "attack" here can be taken too
seriously; the whole PCM operation can be somehow interfered if a
process may have the access to the PCM device, and changing sw_params
itself must not introduce too much trouble.


thanks,

Takashi

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

[Oswald Buddenhagen]

On Wed, Apr 12, 2023 at 12:37:56PM +0200, Takashi Iwai wrote:
>On Wed, 12 Apr 2023 10:04:31 +0200,
>Oswald Buddenhagen wrote:
>> 
>> On Wed, Apr 12, 2023 at 09:54:54AM +0200, Takashi Iwai wrote:
>> > I'm thinking whether we need to change anything in the kernel side for
>> > this at all.  Can't it be changed rather in alsa-lib side instead?
>> > 
>> it could, but it would be a lot uglier. user space would have to do a
>> "man-in-the-middle attack" on the data, while in the kernel we can
>> just slightly modify the consumer. this would be particularly obvious
>> in the case of write() access.
>
>But basically it'd be like fiddling sw_params temporarily for
>draining, I suppose?
>
err, right - i was still assuming manual padding.
i actually tried temporarily changing the params (and pondered 
introducing "shadow" params) when i was doing the kernel patch, but that 
was a lot uglier than what i did in the end. i think it would be even 
worse in user space due to the need to support async operation.

regards

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

[Jaroslav Kysela]

On 12. 04. 23 12:33, Oswald Buddenhagen wrote:
> 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.)

I will correct that it will make sense where hw_ptr is nearby boundary 
(boundary - buffer_size ... boundary - 1) and appl_ptr is cropped using 
boundary (0 ... buffer_size). But because appl_ptr can be set by application 
without any kernel side correction, it may be possible to check if the 
appl_ptr is in 0 ... boundary range before any use. Sorry for the confusion.

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

It seems that you have answer to everything. My suggestion is perfectly 
readable (is the requested silence threshold fulfilled? or is the noise 
distance greater than the whole buffer / buffer_size?).

					Jaroslav

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

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

[Jaroslav Kysela]

On 12. 04. 23 12:37, Takashi Iwai wrote:
> On Wed, 12 Apr 2023 10:04:31 +0200,
> Oswald Buddenhagen wrote:
>>
>> On Wed, Apr 12, 2023 at 09:54:54AM +0200, Takashi Iwai wrote:
>>> I'm thinking whether we need to change anything in the kernel side for
>>> this at all.  Can't it be changed rather in alsa-lib side instead?
>>>
>> it could, but it would be a lot uglier. user space would have to do a
>> "man-in-the-middle attack" on the data, while in the kernel we can
>> just slightly modify the consumer. this would be particularly obvious
>> in the case of write() access.
> 
> But basically it'd be like fiddling sw_params temporarily for
> draining, I suppose?  And the "attack" here can be taken too
> seriously; the whole PCM operation can be somehow interfered if a
> process may have the access to the PCM device, and changing sw_params
> itself must not introduce too much trouble.

This looks like a sane proposal, but some drivers does not require the 
silencing at all, so we can probably skip this step for them (new 
SNDRV_PCM_INFO_PERFECT_DRAIN flag?).

The other not-yet-discussed option is to just print an warning in alsa-lib 
that the residue samples may be played (when no silencing / period size align 
is used). Then introduce a new helper function to setup silencing for the 
drivers without new SNDRV_PCM_INFO_PERFECT_DRAIN flag set.

						Jaroslav

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

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

[Takashi Iwai]

On Wed, 12 Apr 2023 21:59:28 +0200,
Jaroslav Kysela wrote:
> 
> On 12. 04. 23 12:37, Takashi Iwai wrote:
> > On Wed, 12 Apr 2023 10:04:31 +0200,
> > Oswald Buddenhagen wrote:
> >> 
> >> On Wed, Apr 12, 2023 at 09:54:54AM +0200, Takashi Iwai wrote:
> >>> I'm thinking whether we need to change anything in the kernel side for
> >>> this at all.  Can't it be changed rather in alsa-lib side instead?
> >>> 
> >> it could, but it would be a lot uglier. user space would have to do a
> >> "man-in-the-middle attack" on the data, while in the kernel we can
> >> just slightly modify the consumer. this would be particularly obvious
> >> in the case of write() access.
> > 
> > But basically it'd be like fiddling sw_params temporarily for
> > draining, I suppose?  And the "attack" here can be taken too
> > seriously; the whole PCM operation can be somehow interfered if a
> > process may have the access to the PCM device, and changing sw_params
> > itself must not introduce too much trouble.
> 
> This looks like a sane proposal, but some drivers does not require the
> silencing at all, so we can probably skip this step for them (new
> SNDRV_PCM_INFO_PERFECT_DRAIN flag?).

Sure, we should apply it only conditionally.  Also, we may skip the
workaround for applications accessing directly via mmap as default.
Also we may provide a flag in asoundrc config, if any, too.
The implementation in alsa-lib is more flexible in this regard.

> The other not-yet-discussed option is to just print an warning in
> alsa-lib that the residue samples may be played (when no silencing /
> period size align is used). Then introduce a new helper function to
> setup silencing for the drivers without new
> SNDRV_PCM_INFO_PERFECT_DRAIN flag set.

Hm, I don't think this would be useful.  Spewing warnings are rather
annoying or confusing for end users, and I bet the old applications
wouldn't be fixed even with such annoyance.


thanks,

Takashi

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

[Oswald Buddenhagen]

On Wed, Apr 12, 2023 at 09:23:23PM +0200, Jaroslav Kysela wrote:
>On 12. 04. 23 12:33, Oswald Buddenhagen wrote:
>> On Tue, Apr 11, 2023 at 12:47:26PM +0200, Jaroslav Kysela wrote:
>>> On 05. 04. 23 22:12, Oswald Buddenhagen wrote:
>>>> +	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.
>>>
>I will correct that it will make sense where hw_ptr is nearby boundary 
>(boundary - buffer_size ... boundary - 1) and appl_ptr is cropped using 
>boundary (0 ... buffer_size).
>
no, that's the case where it goes negative.
because it's a subtraction, it plain cannot go over the boundary when 
both inputs are bounded.
due to the remaining correction, it could still go "very big" in an 
underrun condition, as the comment in the patch says.
we should discuss the implications of the latter for the 
snd_pcm_*_avail() functions separately (the apidoc doesn't make the 
contract clear at all).

>But because appl_ptr can be set by application without any kernel side 
>correction, it may be possible to check if the appl_ptr is in 0 ...  
>boundary range before any use.
>
that should be rather obviously done *somewhere*, as otherwise appl_ptr 
can often be even slightly above 2*boundary, at which point the above 
correction (and many alike) wouldn't even work. but for the sake of 
efficiency, that should be done asap, so when it is set or the boundary 
changes. no?

>>>>    		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?".
>
>It seems that you have answer to everything.
>
only to the parts that i actually reply to ...

>(is the requested silence threshold fulfilled? or is the noise distance 
>greater than the whole buffer / buffer_size?).
>
but why would you want to approach it that way? it's just an extra step 
to think through. to reinforce the point: if the compiler is any good, 
then your variant will be optimized into mine.

regards

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

[Oswald Buddenhagen]

On Thu, Apr 13, 2023 at 07:42:06AM +0200, Takashi Iwai wrote:
>On Wed, 12 Apr 2023 21:59:28 +0200,
>Jaroslav Kysela wrote:
>> This looks like a sane proposal, but some drivers does not require 
>> the
>> silencing at all, so we can probably skip this step for them (new
>> SNDRV_PCM_INFO_PERFECT_DRAIN flag?).
>
>Sure, we should apply it only conditionally.
>
i don't think the extra complexity is justified. with my improved 
proposal, we're talking about a cost of filling two periods per draining 
op, which aren't exactly super-frequent.

>Also, we may skip the
>workaround for applications accessing directly via mmap as default.
>
no, because one may easily miss that more than one period is required.  
also, i think that forgetting it entirely is an easy mistake to make, 
even if it's harder with mmap than with write.

regards

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

[Takashi Iwai]

On Thu, 13 Apr 2023 12:16:04 +0200,
Oswald Buddenhagen wrote:
> 
> On Thu, Apr 13, 2023 at 07:42:06AM +0200, Takashi Iwai wrote:
> > On Wed, 12 Apr 2023 21:59:28 +0200,
> > Jaroslav Kysela wrote:
> >> This looks like a sane proposal, but some drivers does not require
> >> the
> >> silencing at all, so we can probably skip this step for them (new
> >> SNDRV_PCM_INFO_PERFECT_DRAIN flag?).
> > 
> > Sure, we should apply it only conditionally.
> > 
> i don't think the extra complexity is justified. with my improved
> proposal, we're talking about a cost of filling two periods per
> draining op, which aren't exactly super-frequent.

I'm not much fan of introducing a new flag for that behavior, either.

> > Also, we may skip the
> > workaround for applications accessing directly via mmap as default.
> > 
> no, because one may easily miss that more than one period is required.
> also, i think that forgetting it entirely is an easy mistake to make,
> even if it's harder with mmap than with write.

I don't agree with that point -- if application wants the access only
via mmap (without any write actions via alsa-lib functions), the
buffer and data management relies fully on the application itself.
Manipulating the data *silently* is no good action for such
applications.  For them, the drain simply means to stop at the certain
point.  OTOH, for the explicit write, basically alsa-lib / kernel is
responsible for the buffer / data management, and the workaround can
be applied.

That's I mentioned to "apply conditionally".  There are cases where we
shouldn't touch the data at all.  For the usual cases with the
standard write, we may apply it.


thanks,

Takashi

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

[Oswald Buddenhagen]

On Thu, Apr 13, 2023 at 12:28:34PM +0200, Takashi Iwai wrote:
>On Thu, 13 Apr 2023 12:16:04 +0200, Oswald Buddenhagen wrote:
>> On Thu, Apr 13, 2023 at 07:42:06AM +0200, Takashi Iwai wrote:
>> > Also, we may skip the
>> > workaround for applications accessing directly via mmap as default.
>> > 
>> no, because one may easily miss that more than one period is required.
>> also, i think that forgetting it entirely is an easy mistake to make,
>> even if it's harder with mmap than with write.
>
>I don't agree with that point -- if application wants the access only
>via mmap (without any write actions via alsa-lib functions), the
>buffer and data management relies fully on the application itself.
>Manipulating the data *silently* is no good action for such
>applications.

>For them, the drain simply means to stop at the certain point.
>
i don't think that's true. if an app wants to control things finely, it 
would just use start/stop and manage the timing itself. draining otoh is 
a convenient fire-and-forget operation. that makes it easy to miss the 
finer details, which is why i'm so insistent that it should just work 
out of the box.

if you exclude mmapped devices in kernel, you exclude plughw with 
emulated write(), so you'd have to add yet more code to compensate for 
that. and doing it all in user space is yet more code. for all i can 
tell, it's really just layers of complexity to solve a non-problem.

regards

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

[Takashi Iwai]

On Thu, 13 Apr 2023 13:10:34 +0200,
Oswald Buddenhagen wrote:
> 
> On Thu, Apr 13, 2023 at 12:28:34PM +0200, Takashi Iwai wrote:
> > On Thu, 13 Apr 2023 12:16:04 +0200, Oswald Buddenhagen wrote:
> >> On Thu, Apr 13, 2023 at 07:42:06AM +0200, Takashi Iwai wrote:
> >> > Also, we may skip the
> >> > workaround for applications accessing directly via mmap as default.
> >> > no, because one may easily miss that more than one period is
> >> required.
> >> also, i think that forgetting it entirely is an easy mistake to make,
> >> even if it's harder with mmap than with write.
> > 
> > I don't agree with that point -- if application wants the access only
> > via mmap (without any write actions via alsa-lib functions), the
> > buffer and data management relies fully on the application itself.
> > Manipulating the data *silently* is no good action for such
> > applications.
> 
> > For them, the drain simply means to stop at the certain point.
> > 
> i don't think that's true. if an app wants to control things finely,
> it would just use start/stop and manage the timing itself. draining
> otoh is a convenient fire-and-forget operation. that makes it easy to
> miss the finer details, which is why i'm so insistent that it should
> just work out of the box.

Sure, but that's still no excuse to ignore the possibility blindly.

> if you exclude mmapped devices in kernel, you exclude plughw with
> emulated write(), so you'd have to add yet more code to compensate for
> that.

No, I wrote "if application wants the access only via mmap (without
any write actions via alsa-lib functions)".  So if application writes
via plugin write(), we should apply the workaround, too.

> and doing it all in user space is yet more code. for all i can
> tell, it's really just layers of complexity to solve a non-problem.

I don't get it: we're talking about the sw_params call in alsa-lib's
drain function, and how can it be *so* complex...?


Takashi

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

[Oswald Buddenhagen]

On Thu, Apr 13, 2023 at 02:06:49PM +0200, Takashi Iwai wrote:
>On Thu, 13 Apr 2023 13:10:34 +0200, Oswald Buddenhagen wrote:
>> i don't think that's true. if an app wants to control things finely,
>> it would just use start/stop and manage the timing itself. draining
>> otoh is a convenient fire-and-forget operation. that makes it easy to
>> miss the finer details, which is why i'm so insistent that it should
>> just work out of the box.
>
>Sure, but that's still no excuse to ignore the possibility blindly.
>
it's not blindly. it's after considering it, and concluding that it's a 
hypothetical problem at best.

we could of course do a survey of actually existing publicly accessible 
code, to quantify the trade-off between apps fixed and apps broken. i 
actually sort of tried that ...

first thing is that i found lots of stackoverflow answers and similar, 
and *none* of them even mentioned the need to clear the rest of the 
buffer. i found a bunch of libs, and none of the apidocs mentioned it in 
the parts i read. i found one cross-platform how-to which did actually 
mention it. yay.

code search was trickier, with rather predictable results:
basically all hits for drain() were immediately followed by close().
i found some simple cases of write+drain, and none of them did any 
additional padding. this includes alsa's own pcm example. but never 
mind, we're in agreement about this case.
most other code was some abstraction, so it would be impossible to asses 
the bigger picture quickly.
that would be even more the case for apps that use mmap. so i won't even 
try to provide actual data.
one thing to consider here is that most of the code are cross-platform 
abstractions. under such circumstances, it seems kinda inconceivable 
that the higher level code would make any assumptions about buffer space 
that has not been filled with fresh samples.

>> and doing it all in user space is yet more code. for all i can
>> tell, it's really just layers of complexity to solve a non-problem.
>
>I don't get it: we're talking about the sw_params call in alsa-lib's
>drain function, and how can it be *so* complex...?
>
the "drain function" bit is critical here, because it kind of implies 
resetting it, potentially asynchronously. but if we add a specific bit 
to the kernel to enable it, then it can be actually set already when the 
device is set up, and the user space would be rather simple. however, 
that would overall be still a lot more code than doing it 
unconditionally, and fully in kernel.

regards

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

[Takashi Iwai]

On Thu, 13 Apr 2023 16:59:02 +0200,
Oswald Buddenhagen wrote:
> 
> On Thu, Apr 13, 2023 at 02:06:49PM +0200, Takashi Iwai wrote:
> > On Thu, 13 Apr 2023 13:10:34 +0200, Oswald Buddenhagen wrote:
> >> i don't think that's true. if an app wants to control things finely,
> >> it would just use start/stop and manage the timing itself. draining
> >> otoh is a convenient fire-and-forget operation. that makes it easy to
> >> miss the finer details, which is why i'm so insistent that it should
> >> just work out of the box.
> > 
> > Sure, but that's still no excuse to ignore the possibility blindly.
> > 
> it's not blindly. it's after considering it, and concluding that it's
> a hypothetical problem at best.
> 
> we could of course do a survey of actually existing publicly
> accessible code, to quantify the trade-off between apps fixed and apps
> broken. i actually sort of tried that ...
> 
> first thing is that i found lots of stackoverflow answers and similar,
> and *none* of them even mentioned the need to clear the rest of the
> buffer. i found a bunch of libs, and none of the apidocs mentioned it
> in the parts i read. i found one cross-platform how-to which did
> actually mention it. yay.
> 
> code search was trickier, with rather predictable results:
> basically all hits for drain() were immediately followed by close().
> i found some simple cases of write+drain, and none of them did any
> additional padding. this includes alsa's own pcm example. but never
> mind, we're in agreement about this case.
> most other code was some abstraction, so it would be impossible to
> asses the bigger picture quickly.
> that would be even more the case for apps that use mmap. so i won't
> even try to provide actual data.
> one thing to consider here is that most of the code are cross-platform
> abstractions. under such circumstances, it seems kinda inconceivable
> that the higher level code would make any assumptions about buffer
> space that has not been filled with fresh samples.

Those whole context should have been mentioned before the
discussion...  But still we'd better survey the actual usage for the
decision.

ATM, I still hesitate taking the behavior change in the kernel,
*iff* it can be worked around differently.
While the mmap situation is admittedly a corner case, the point of
alsa-lib implementation is the flexibility.  And, your implementation
means to modify the mmapped data silently, which never happened in the
past -- this is another side of coin of fixing in API side, and we
don't know the side-effect to any wild applications.  Some additional
configuration or flexible workaround might be required, and that's
often harder to apply in the kernel.

And, another concern is, as already discussed, whether we really have
to apply it unconditionally at every draining call.  Obviously the
workaround is superfluous for the drivers like USB-audio, which never
overrun without the filled data.

> >> and doing it all in user space is yet more code. for all i can
> >> tell, it's really just layers of complexity to solve a non-problem.
> > 
> > I don't get it: we're talking about the sw_params call in alsa-lib's
> > drain function, and how can it be *so* complex...?
> > 
> the "drain function" bit is critical here, because it kind of implies
> resetting it, potentially asynchronously. but if we add a specific bit
> to the kernel to enable it, then it can be actually set already when
> the device is set up, and the user space would be rather
> simple. however, that would overall be still a lot more code than
> doing it unconditionally, and fully in kernel.

Indeed we might want to take the kernel-side fix in the end, but let's
check things a bit more beforehand.


BTW, I guess that one missing piece in your patch is the case where
the drain is called at the moment of fully filled data.  You added
snd_pcm_playback_silence() at snd_pcm_do_drain_init(), but in this
scenario, the call wouldn't do anything at this moment.  But
snd_pcm_playback_silence() won't be called afterwards because
runtime->silence_size = 0.  So this workaround won't take effect in
that case, no?


thanks,

Takashi

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

[Oswald Buddenhagen]

On Fri, Apr 14, 2023 at 10:26:26AM +0200, Takashi Iwai wrote:
>Indeed we might want to take the kernel-side fix in the end, but let's
>check things a bit more beforehand.
>
i'll post updated patches soon, then you can sleep over it. :-D

(i'm being a bit slow, because i'm also developing tooling for 
maintaining stacked branches and (re-)posting them, and of course i want 
to try it out with real data.)

>BTW, I guess that one missing piece in your patch is the case where
>the drain is called at the moment of fully filled data.  You added
>snd_pcm_playback_silence() at snd_pcm_do_drain_init(), but in this
>scenario, the call wouldn't do anything at this moment.  But
>snd_pcm_playback_silence() won't be called afterwards because
>runtime->silence_size = 0.  So this workaround won't take effect in
>that case, no?
>
the hunk in snd_pcm_update_hw_ptr0() should take care of that.

regards

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

[Takashi Iwai]

On Fri, 14 Apr 2023 10:56:10 +0200,
Oswald Buddenhagen wrote:
> 
> On Fri, Apr 14, 2023 at 10:26:26AM +0200, Takashi Iwai wrote:
> > BTW, I guess that one missing piece in your patch is the case where
> > the drain is called at the moment of fully filled data.  You added
> > snd_pcm_playback_silence() at snd_pcm_do_drain_init(), but in this
> > scenario, the call wouldn't do anything at this moment.  But
> > snd_pcm_playback_silence() won't be called afterwards because
> > runtime->silence_size = 0.  So this workaround won't take effect in
> > that case, no?
> > 
> the hunk in snd_pcm_update_hw_ptr0() should take care of that.

OK, I see.  Thanks.


Takashi