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@xxxxxx> > Signed-off-by: Jaroslav Kysela <perex@xxxxxxxx> 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
