Re: [PATCH 3/5] ALSA: pcm: fix playback silence - correct the incremental silencing

[Takashi Iwai <tiwai@suse.de>]

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