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

[Oswald Buddenhagen <oswald.buddenhagen@gmx.de>]

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