diff mbox series

[alsa-lib,1/4] pcm: hw: setup explicit silencing for snd_pcm_drain by default

Message ID 20230502115010.986325-2-perex@perex.cz (mailing list archive)
State New, archived
Headers show
Series pcm: hw: implement explicit silencing for snd_pcm_drain | expand

Commit Message

Jaroslav Kysela May 2, 2023, 11:50 a.m. UTC
Some applications may not follow the period_size transfer blocks and
also the driver developers may not follow the consequeces of the
access beyond valid samples in the playback DMA buffer.

To avoid clicks, fill a little silence at the end of the playback
ring buffer when the snd_pcm_drain() is called.

Related: https://lore.kernel.org/alsa-devel/20230420113324.877164-2-oswald.buddenhagen@gmx.de/
Related: https://lore.kernel.org/alsa-devel/20230405201219.2197789-2-oswald.buddenhagen@gmx.de/
Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
 src/pcm/pcm.c       | 33 ++++++++++++++++++-------------
 src/pcm/pcm_hw.c    | 47 +++++++++++++++++++++++++++++++++++++++++++++
 src/pcm/pcm_local.h |  4 ++++
 3 files changed, 71 insertions(+), 13 deletions(-)

Comments

Oswald Buddenhagen May 3, 2023, 11:20 a.m. UTC | #1
On Tue, May 02, 2023 at 01:50:07PM +0200, Jaroslav Kysela wrote:
>Some applications may not follow the period_size transfer blocks and
>also the driver developers may not follow the consequeces of the
>access beyond valid samples in the playback DMA buffer.
>
i find this way too vague.

>To avoid clicks, fill a little silence at the end of the playback

>ring buffer when the snd_pcm_drain() is called.
>
no 'the' here.
(see 
https://www.eltconcourse.com/training/inservice/lexicogrammar/article_system.html 
for more than you ever wanted to know about articles.)

>--- a/src/pcm/pcm_hw.c
>+++ b/src/pcm/pcm_hw.c
>@@ -718,7 +732,40 @@ static int snd_pcm_hw_drop(snd_pcm_t *pcm)
> static int snd_pcm_hw_drain(snd_pcm_t *pcm)
> {
> 	snd_pcm_hw_t *hw = pcm->private_data;
>+	snd_pcm_sw_params_t sw_params;
>+	snd_pcm_uframes_t silence_size;
> 	int err;
>+

>+	if (pcm->stream != SND_PCM_STREAM_PLAYBACK)
>+		goto __skip_silence;
>
arguably, you should use the inverse condition and a block instead of a 
goto.
if this is a measure to keep the indentation down, factoring it out to a 
separate snd_pcm_hw_auto_silence() function should do the job.

>+	/* compute end silence size, align to period size + extra time */
>+	snd_pcm_sw_params_current_no_lock(pcm, &sw_params);
>
if you swap these lines here, there will be less churn in the followup
patch.
in the comment, better use a colon instead of a comma.

>+	if ((pcm->boundary % pcm->period_size) == 0) {
>+		silence_size = pcm->period_size - (*pcm->appl.ptr % pcm->period_size);
>+		if (silence_size == pcm->period_size)
>+			silence_size = 0;
>
you can avoid the condition by rewriting it like this:

   silence_size = pcm->period_size - ((*pcm->appl.ptr + pcm->period_size - 1) % pcm->period_size) - 1;

(it may be possible to simplify this further, but this makes my head 
hurt ...)

>+	} else {
>+		/* it not not easy to compute the period cross point
>
"it is not"
"crossing point"

>+		 * in this case because period is not aligned to the boundary
>
"the period"

>+		 * - use the full range (one period) in this case
>+		 */
>+		silence_size = pcm->period_size;
>+	}
>+	silence_size += pcm->rate / 10;	/* 1/10th of second */
>+	if (sw_params.silence_size < silence_size) {
>+		/* fill the silence soon as possible (in the bellow ioctl
>
"as soon as possible"
"in the ioctl below"

>+		 * or the next period wake up)
>+		 */
>+		sw_params.silence_threshold = pcm->buffer_size;
>+		sw_params.silence_size = silence_size;
>
so at this point i got the thought "huh, that can exceed the buffer 
size. is that ok?" ...
and yes, it is. but ...

the kernel doesn't check silence_threshold, so user space can trigger 
the snd_BUG_ON() in snd_pcm_playback_silence(). whoops.
(yes, this predates my patch.)
i'm not sure what the best way to deal with this is. anyway, different 
tree, different patch.

regards
Oswald Buddenhagen May 3, 2023, 8:19 p.m. UTC | #2
On Wed, May 03, 2023 at 01:20:37PM +0200, Oswald Buddenhagen wrote:
>On Tue, May 02, 2023 at 01:50:07PM +0200, Jaroslav Kysela wrote:
>>+		 * or the next period wake up)
>>+		 */
>>+		sw_params.silence_threshold = pcm->buffer_size;
>>+		sw_params.silence_size = silence_size;
>>
>so at this point i got the thought "huh, that can exceed the buffer 
>size. is that ok?" ...
>and yes, it is. but ...
>
>the kernel doesn't check silence_threshold, so user space can trigger 
>the snd_BUG_ON() in snd_pcm_playback_silence(). whoops.
>(yes, this predates my patch.)
>i'm not sure what the best way to deal with this is. anyway, different 
>tree, different patch.

actually, that analysis is garbage, because i didn't look at enough 
context. :}

the kernel _does_ check the values in snd_pcm_sw_params(), which means 
that silence_size exceeding silence_threshold would lead to EINVAL, and 
therefore silencing being broken. this will inevitably happen with small 
buffer sizes, where the 1/10th sec extension dominates.

as snd_pcm_sw_params() checks the parameters (and snd_pcm_hw_params() 
resets the sw params to defaults, so inverse calling order cannot bypass 
it), the concern about the crash is invalid. phew.

regards
Jaroslav Kysela May 3, 2023, 8:31 p.m. UTC | #3
On 03. 05. 23 22:19, Oswald Buddenhagen wrote:
> On Wed, May 03, 2023 at 01:20:37PM +0200, Oswald Buddenhagen wrote:
>> On Tue, May 02, 2023 at 01:50:07PM +0200, Jaroslav Kysela wrote:
>>> +		 * or the next period wake up)
>>> +		 */
>>> +		sw_params.silence_threshold = pcm->buffer_size;
>>> +		sw_params.silence_size = silence_size;
>>>
>> so at this point i got the thought "huh, that can exceed the buffer
>> size. is that ok?" ...
>> and yes, it is. but ...
>>
>> the kernel doesn't check silence_threshold, so user space can trigger
>> the snd_BUG_ON() in snd_pcm_playback_silence(). whoops.
>> (yes, this predates my patch.)
>> i'm not sure what the best way to deal with this is. anyway, different
>> tree, different patch.
> 
> actually, that analysis is garbage, because i didn't look at enough
> context. :}
> 
> the kernel _does_ check the values in snd_pcm_sw_params(), which means
> that silence_size exceeding silence_threshold would lead to EINVAL, and
> therefore silencing being broken. this will inevitably happen with small
> buffer sizes, where the 1/10th sec extension dominates.

Yes, I overlooked this. Thanks. Fixed in:

https://github.com/alsa-project/alsa-lib/commit/58077e2f0d896ff848f3551518b1d9fc6c97d695

					Jaroslav
Oswald Buddenhagen May 5, 2023, 6:56 p.m. UTC | #4
On Tue, May 02, 2023 at 01:50:07PM +0200, Jaroslav Kysela wrote:
>--- a/src/pcm/pcm_hw.c
>+++ b/src/pcm/pcm_hw.c
>@@ -718,7 +732,40 @@ static int snd_pcm_hw_drop(snd_pcm_t *pcm)
> static int snd_pcm_hw_drain(snd_pcm_t *pcm)
> {
> 	snd_pcm_hw_t *hw = pcm->private_data;
>+	snd_pcm_sw_params_t sw_params;
>+	snd_pcm_uframes_t silence_size;
> 	int err;
>+
>+	if (pcm->stream != SND_PCM_STREAM_PLAYBACK)
>+		goto __skip_silence;
>+	/* compute end silence size, align to period size + extra time */
>+	snd_pcm_sw_params_current_no_lock(pcm, &sw_params);
>+	if ((pcm->boundary % pcm->period_size) == 0) {
>+		silence_size = pcm->period_size - (*pcm->appl.ptr % pcm->period_size);
>+		if (silence_size == pcm->period_size)
>+			silence_size = 0;
>+	} else {
>+		/* it not not easy to compute the period cross point
>+		 * in this case because period is not aligned to the boundary
>+		 * - use the full range (one period) in this case
>+		 */
>+		silence_size = pcm->period_size;
>+	}
>+	silence_size += pcm->rate / 10;	/* 1/10th of second */
>+	if (sw_params.silence_size < silence_size) {
>+		/* fill the silence soon as possible (in the bellow ioctl
>+		 * or the next period wake up)
>+		 */
>+		sw_params.silence_threshold = pcm->buffer_size;
>+		sw_params.silence_size = silence_size;
>
i was reworking my kernel patch along these lines (it's easier to deploy 
when the kernel is the only thing you're hacking on), and ran into this 
behavior:

check thresholded silence fill, sil start 0, sil fill 0, app 66000
now sil start 66000, sil fill 0
noise dist 23997
drain raw fill 0
drain extended fill 4800
frames 3
filling 3 at 18000
period elapsed
check thresholded silence fill, sil start 66000, sil fill 3, app 66000
now sil start 66000, sil fill 3
noise dist 18000
drain raw fill 0
drain extended fill 4800
frames 4800
filling 4800 at 18003
period elapsed
check thresholded silence fill, sil start 66000, sil fill 4803, app 66000
now sil start 66000, sil fill 4803
noise dist 16800
drain raw fill 0
drain extended fill 4800
frames 4800
filling 1197 at 22803
filling 3603 at 0
period elapsed
check thresholded silence fill, sil start 66000, sil fill 9603, app 66000
now sil start 66000, sil fill 9603
noise dist 15600
drain raw fill 0
drain extended fill 4800
frames 4800
filling 4800 at 3603
period elapsed
check thresholded silence fill, sil start 66000, sil fill 14403, app 66000
now sil start 66000, sil fill 14403
noise dist 6755399441055758400

what you're seeing is this: when the drain is issued, the buffer is 
initially full, and after every played period some padding is added, 
totalling way beyond what was intended.
this doesn't break anything, but it's a bit inefficient, and just ugly.

this is a result of silence_threshold being the buffer size.
and setting it to the silence_size of course doesn't work reliably when 
that is smaller than the period size.

while pondering how to fix that, my thoughts returned to my yet unaired 
gripe with the silencing parameters being anything but intuitive. would 
you mind explaining why they are like that?

why not interpret silence_size as the minimum number of playable samples 
(that is, noise_dist in the kernel code) that must be maintained at all 
times, and simply ignore silence_threshold?

unless i'm missing something, this is exactly what one would want for 
maintaining underrun resilience, which i think is the purpose of the 
thresholded silence fill mode (at least my comment updates which claim 
so were accepted).

and doing that would "magically" fix your patch.

can you think of any plausible use case that this would break?

but i guess you'll be paranoid about backwards compat anyway, so an 
alternative proposal would be using silence_threshold == ULONG_MAX to 
trigger the new semantics. of course this would have the downside that 
it wouldn't "magically" fix your code (and i suspect some other code as 
well), and kernel version dependent behavior would have to be coded for 
the (presumably) common usage.

regards
diff mbox series

Patch

diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
index 2b966d44..88b13ed4 100644
--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -6167,6 +6167,25 @@  int snd_pcm_hw_params_get_min_align(const snd_pcm_hw_params_t *params, snd_pcm_u
 	return 0;
 }
 
+#ifndef DOXYGEN
+void snd_pcm_sw_params_current_no_lock(snd_pcm_t *pcm, snd_pcm_sw_params_t *params)
+{
+	params->proto = SNDRV_PCM_VERSION;
+	params->tstamp_mode = pcm->tstamp_mode;
+	params->tstamp_type = pcm->tstamp_type;
+	params->period_step = pcm->period_step;
+	params->sleep_min = 0;
+	params->avail_min = pcm->avail_min;
+	sw_set_period_event(params, pcm->period_event);
+	params->xfer_align = 1;
+	params->start_threshold = pcm->start_threshold;
+	params->stop_threshold = pcm->stop_threshold;
+	params->silence_threshold = pcm->silence_threshold;
+	params->silence_size = pcm->silence_size;
+	params->boundary = pcm->boundary;
+}
+#endif
+
 /**
  * \brief Return current software configuration for a PCM
  * \param pcm PCM handle
@@ -6183,19 +6202,7 @@  int snd_pcm_sw_params_current(snd_pcm_t *pcm, snd_pcm_sw_params_t *params)
 		return -EIO;
 	}
 	__snd_pcm_lock(pcm); /* forced lock due to pcm field changes */
-	params->proto = SNDRV_PCM_VERSION;
-	params->tstamp_mode = pcm->tstamp_mode;
-	params->tstamp_type = pcm->tstamp_type;
-	params->period_step = pcm->period_step;
-	params->sleep_min = 0;
-	params->avail_min = pcm->avail_min;
-	sw_set_period_event(params, pcm->period_event);
-	params->xfer_align = 1;
-	params->start_threshold = pcm->start_threshold;
-	params->stop_threshold = pcm->stop_threshold;
-	params->silence_threshold = pcm->silence_threshold;
-	params->silence_size = pcm->silence_size;
-	params->boundary = pcm->boundary;
+	snd_pcm_sw_params_current_no_lock(pcm, params);
 	__snd_pcm_unlock(pcm);
 	return 0;
 }
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index daa3e1ff..d8f32bd9 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -98,6 +98,8 @@  typedef struct {
 	bool mmap_control_fallbacked;
 	struct snd_pcm_sync_ptr *sync_ptr;
 
+	bool prepare_reset_sw_params;
+
 	int period_event;
 	snd_timer_t *period_timer;
 	struct pollfd period_timer_pfd;
@@ -534,6 +536,7 @@  static int snd_pcm_hw_sw_params(snd_pcm_t *pcm, snd_pcm_sw_params_t * params)
 		SYSMSG("SNDRV_PCM_IOCTL_SW_PARAMS failed (%i)", err);
 		goto out;
 	}
+	hw->prepare_reset_sw_params = false;
 	if ((snd_pcm_tstamp_type_t) params->tstamp_type != pcm->tstamp_type) {
 		if (hw->version < SNDRV_PROTOCOL_VERSION(2, 0, 12)) {
 			int on = (snd_pcm_tstamp_type_t) params->tstamp_type ==
@@ -660,7 +663,18 @@  static int snd_pcm_hw_hwsync(snd_pcm_t *pcm)
 static int snd_pcm_hw_prepare(snd_pcm_t *pcm)
 {
 	snd_pcm_hw_t *hw = pcm->private_data;
+	snd_pcm_sw_params_t sw_params;
 	int fd = hw->fd, err;
+
+	if (hw->prepare_reset_sw_params) {
+		snd_pcm_sw_params_current_no_lock(pcm, &sw_params);
+		if (ioctl(hw->fd, SNDRV_PCM_IOCTL_SW_PARAMS, sw_params) < 0) {
+			err = -errno;
+			SYSMSG("SNDRV_PCM_IOCTL_SW_PARAMS failed (%i)", err);
+			return err;
+		}
+		hw->prepare_reset_sw_params = false;
+	}
 	if (ioctl(fd, SNDRV_PCM_IOCTL_PREPARE) < 0) {
 		err = -errno;
 		SYSMSG("SNDRV_PCM_IOCTL_PREPARE failed (%i)", err);
@@ -718,7 +732,40 @@  static int snd_pcm_hw_drop(snd_pcm_t *pcm)
 static int snd_pcm_hw_drain(snd_pcm_t *pcm)
 {
 	snd_pcm_hw_t *hw = pcm->private_data;
+	snd_pcm_sw_params_t sw_params;
+	snd_pcm_uframes_t silence_size;
 	int err;
+
+	if (pcm->stream != SND_PCM_STREAM_PLAYBACK)
+		goto __skip_silence;
+	/* compute end silence size, align to period size + extra time */
+	snd_pcm_sw_params_current_no_lock(pcm, &sw_params);
+	if ((pcm->boundary % pcm->period_size) == 0) {
+		silence_size = pcm->period_size - (*pcm->appl.ptr % pcm->period_size);
+		if (silence_size == pcm->period_size)
+			silence_size = 0;
+	} else {
+		/* it not not easy to compute the period cross point
+		 * in this case because period is not aligned to the boundary
+		 * - use the full range (one period) in this case
+		 */
+		silence_size = pcm->period_size;
+	}
+	silence_size += pcm->rate / 10;	/* 1/10th of second */
+	if (sw_params.silence_size < silence_size) {
+		/* fill the silence soon as possible (in the bellow ioctl
+		 * or the next period wake up)
+		 */
+		sw_params.silence_threshold = pcm->buffer_size;
+		sw_params.silence_size = silence_size;
+		if (ioctl(hw->fd, SNDRV_PCM_IOCTL_SW_PARAMS, &sw_params) < 0) {
+			err = -errno;
+			SYSMSG("SNDRV_PCM_IOCTL_SW_PARAMS failed (%i)", err);
+			return err;
+		}
+		hw->prepare_reset_sw_params = true;
+	}
+__skip_silence:
 	if (ioctl(hw->fd, SNDRV_PCM_IOCTL_DRAIN) < 0) {
 		err = -errno;
 		SYSMSG("SNDRV_PCM_IOCTL_DRAIN failed (%i)", err);
diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index ae0c44bf..4a859cd1 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -366,6 +366,8 @@  struct _snd_pcm {
 	snd1_pcm_hw_param_get_max
 #define snd_pcm_hw_param_name		\
 	snd1_pcm_hw_param_name
+#define snd_pcm_sw_params_current_no_lock \
+	snd1_pcm_sw_params_current_no_lock
 
 int snd_pcm_new(snd_pcm_t **pcmp, snd_pcm_type_t type, const char *name,
 		snd_pcm_stream_t stream, int mode);
@@ -390,6 +392,8 @@  void snd_pcm_mmap_appl_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames);
 void snd_pcm_mmap_hw_backward(snd_pcm_t *pcm, snd_pcm_uframes_t frames);
 void snd_pcm_mmap_hw_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames);
 
+void snd_pcm_sw_params_current_no_lock(snd_pcm_t *pcm, snd_pcm_sw_params_t *params);
+
 snd_pcm_sframes_t snd_pcm_mmap_writei(snd_pcm_t *pcm, const void *buffer, snd_pcm_uframes_t size);
 snd_pcm_sframes_t snd_pcm_mmap_readi(snd_pcm_t *pcm, void *buffer, snd_pcm_uframes_t size);
 snd_pcm_sframes_t snd_pcm_mmap_writen(snd_pcm_t *pcm, void **bufs, snd_pcm_uframes_t size);