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