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