On Fri, 05 May 2023 09:38:11 +0200, Jaroslav Kysela wrote:
The incremental silencing was broken with the threshold mode. The silenced area was smaller than expected in some cases. The updated area starts at runtime->silence_start + runtime->silence_filled position not only at runtime->silence_start in this mode.
Unify the runtime->silence_start use for all cases (threshold and top-up).
Suggested-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de Signed-off-by: Jaroslav Kysela perex@perex.cz
While this change itself follows the original code, and is fine from the code-refactoring POV.
But, the difficulty of the code (even after this patch) is that the filling behavior is completely different between the threshold and the fill-up modes, and we still try to use the similar code. When reconsidering what we actually need, you can notice that, in the fill-up mode, we don't have to keep tracking silence_start and silence_size at all.
Namely, in the fill-up mode, what we need are: - at init, fill silence in the unused buffer:
ofs = runtime->control->appl_ptr % runtime->buffer_size; frames = snd_pcm_playback_avail(runtime); fill_silence_in_loop(ofs, frames);
- at each incremental hw_ptr update, fill the area with silence:
ofs = runtime->status->hw_ptr % runtime->buffer_size; frames = new_hw_ptr - runtime->status->hw_ptr; if (frames < 0) frames += runtime->boundary; fill_silence_in_loop(ofs, frames);
That's all, and far simpler than keeping silence_start and silence_filled. (I really had hard time to understand why filling at silence_start + silence_filled in the incremental mode works correctly...)
I might have overlooked something and there can be a bit more room for optimization, but the point is that unifying the code for two behavior isn't always good. Treating separately can be sometimes easier.
thanks,
Takashi