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
