diff mbox series

[alsa-lib,2/4] pcm: hw: add drain_silence configuration keyword

Message ID 20230502115010.986325-3-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
# Add silence in drain (-1 = auto /default/, 0 = off, > 0 silenced frames)
  [drain_silence INT]

Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
 src/pcm/pcm_hw.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

Comments

Oswald Buddenhagen May 3, 2023, 11:24 a.m. UTC | #1
On Tue, May 02, 2023 at 01:50:08PM +0200, Jaroslav Kysela wrote:
>  # Add silence in drain (-1 = auto /default/, 0 = off, > 0 silenced frames)
>  [drain_silence INT]
>
i find this wholly inadequate as a description.
specifically, it's missing a motivation.

and how would one use this in a meaningful way, given that the actual 
silence size is dependent on the period size and preferably the pointer 
alignment?

what i could imagine _hypothetically_ making sense is making the 1/10th 
sec "overshoot" configurable, as it's hardware-dependent. but in 
practice, i don't see how that would be actually useful, as the cost of 
doing too much is negligible, and the default you chose seems more than 
safe enough.

regards
Jaroslav Kysela May 3, 2023, 2:22 p.m. UTC | #2
On 03. 05. 23 13:24, Oswald Buddenhagen wrote:
> On Tue, May 02, 2023 at 01:50:08PM +0200, Jaroslav Kysela wrote:
>>   # Add silence in drain (-1 = auto /default/, 0 = off, > 0 silenced frames)
>>   [drain_silence INT]
>>
> i find this wholly inadequate as a description.
> specifically, it's missing a motivation.
> 
> and how would one use this in a meaningful way, given that the actual
> silence size is dependent on the period size and preferably the pointer
> alignment?
> 
> what i could imagine _hypothetically_ making sense is making the 1/10th
> sec "overshoot" configurable, as it's hardware-dependent. but in
> practice, i don't see how that would be actually useful, as the cost of
> doing too much is negligible, and the default you chose seems more than
> safe enough.

The positive value is a bit bonus. I just picked an easy understandable way. 
But looking to this issue for the second time, I changed the meaning for the 
positive value to milliseconds. In this way, it's time/rate related.

The main purpose for this option is to turn the code off. The other silence 
size calculation schemes may be described by another configuration field in 
future on demand.

Thanks for the review of all patches - I picked some proposals and pushed 
changes to the alsa-lib repository:

https://github.com/alsa-project/alsa-lib/commits/master

					Jaroslav
Oswald Buddenhagen May 3, 2023, 3:39 p.m. UTC | #3
On Wed, May 03, 2023 at 04:22:03PM +0200, Jaroslav Kysela wrote:
>On 03. 05. 23 13:24, Oswald Buddenhagen wrote:
>> what i could imagine _hypothetically_ making sense is making the 
>> 1/10th
>> sec "overshoot" configurable, as it's hardware-dependent. but in
>> practice, i don't see how that would be actually useful, as the cost of
>> doing too much is negligible, and the default you chose seems more than
>> safe enough.
>
>The positive value is a bit bonus. I just picked an easy understandable way. 
>But looking to this issue for the second time, I changed the meaning for the 
>positive value to milliseconds. In this way, it's time/rate related.
>
i think it's a bad idea to add "bonus" features that have no clear use 
case. it's basically dead code, and you can't use these values for 
something actually useful later.

>Thanks for the review of all patches - I picked some proposals and 
>pushed changes to the alsa-lib repository:
>
well, and you ignored some of them for no obvious reason.

generally, i don't think maintainers should be exempt from replying to 
comments and posting v2+ patchsets.

regards
diff mbox series

Patch

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index d8f32bd9..30ff843c 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -111,6 +111,7 @@  typedef struct {
 		int max;
 	} rates;
 	int channels;
+	int drain_silence;
 	/* for chmap */
 	unsigned int chmap_caps;
 	snd_pcm_chmap_query_t **chmap_override;
@@ -738,8 +739,16 @@  static int snd_pcm_hw_drain(snd_pcm_t *pcm)
 
 	if (pcm->stream != SND_PCM_STREAM_PLAYBACK)
 		goto __skip_silence;
-	/* compute end silence size, align to period size + extra time */
+	if (hw->drain_silence == 0)
+		goto __skip_silence;
 	snd_pcm_sw_params_current_no_lock(pcm, &sw_params);
+	if (hw->drain_silence > 0) {
+		silence_size = hw->drain_silence;
+		if (silence_size > pcm->buffer_size)
+			silence_size = pcm->buffer_size;
+		goto __manual_silence;
+	}
+	/* compute end silence size, align to period size + extra time */
 	if ((pcm->boundary % pcm->period_size) == 0) {
 		silence_size = pcm->period_size - (*pcm->appl.ptr % pcm->period_size);
 		if (silence_size == pcm->period_size)
@@ -752,6 +761,7 @@  static int snd_pcm_hw_drain(snd_pcm_t *pcm)
 		silence_size = pcm->period_size;
 	}
 	silence_size += pcm->rate / 10;	/* 1/10th of second */
+__manual_silence:
 	if (sw_params.silence_size < silence_size) {
 		/* fill the silence soon as possible (in the bellow ioctl
 		 * or the next period wake up)
@@ -1818,6 +1828,7 @@  pcm.name {
 	[rate INT]		# Restrict only to the given rate
 	  or [rate [INT INT]]	# Restrict only to the given rate range (min max)
 	[chmap MAP]		# Override channel maps; MAP is a string array
+	[drain_silence INT]	# Add silence in drain (-1 = auto /default/, 0 = off, > 0 silenced frames)
 }
 \endcode
 
@@ -1850,7 +1861,7 @@  int _snd_pcm_hw_open(snd_pcm_t **pcmp, const char *name,
 	long card = -1, device = 0, subdevice = -1;
 	const char *str;
 	int err, sync_ptr_ioctl = 0;
-	int min_rate = 0, max_rate = 0, channels = 0;
+	int min_rate = 0, max_rate = 0, channels = 0, drain_silence = -1;
 	snd_pcm_format_t format = SND_PCM_FORMAT_UNKNOWN;
 	snd_config_t *n;
 	int nonblock = 1; /* non-block per default */
@@ -1991,6 +2002,16 @@  int _snd_pcm_hw_open(snd_pcm_t **pcmp, const char *name,
 			}
 			continue;
 		}
+		if (strcmp(id, "drain_silence") == 0) {
+			long val;
+			err = snd_config_get_integer(n, &val);
+			if (err < 0) {
+				SNDERR("Invalid type for %s", id);
+				goto fail;
+			}
+			drain_silence = val;
+			continue;
+		}
 		SNDERR("Unknown field %s", id);
 		err = -EINVAL;
 		goto fail;
@@ -2033,6 +2054,7 @@  int _snd_pcm_hw_open(snd_pcm_t **pcmp, const char *name,
 	}
 	if (chmap)
 		hw->chmap_override = chmap;
+	hw->drain_silence = drain_silence;
 
 	return 0;