| Message ID | 20230505073813.1219175-4-perex@perex.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rewrite snd_pcm_playback_silence() again | expand |
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
On Fri, May 05, 2023 at 11:57:00AM +0200, Takashi Iwai wrote: >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. > what is slightly confusing is that we're kinda abusing silence_filled to mean silence_filled + real_samples. i can add a comment to that effect, but i don't think it's worth tearing apart the paths. regards
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 8a01aeda2213..cfdb4aa5f6fa 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -78,26 +78,24 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram if (frames > runtime->silence_size) frames = runtime->silence_size; } else { - if (new_hw_ptr == ULONG_MAX) { /* initialization */ + if (new_hw_ptr == ULONG_MAX) { + /* initialization, fill silence to whole unused buffer */ snd_pcm_sframes_t avail = snd_pcm_playback_hw_avail(runtime); if (avail > runtime->buffer_size) avail = runtime->buffer_size; runtime->silence_filled = avail > 0 ? avail : 0; - runtime->silence_start = (runtime->status->hw_ptr + - runtime->silence_filled) % - runtime->boundary; + runtime->silence_start = runtime->status->hw_ptr; } else { - ofs = runtime->status->hw_ptr; - frames = new_hw_ptr - ofs; + /* top-up mode (appl_ptr is not required) */ + /* silence the played area immediately */ + frames = new_hw_ptr - runtime->status->hw_ptr; if ((snd_pcm_sframes_t)frames < 0) frames += runtime->boundary; - runtime->silence_filled -= frames; - if ((snd_pcm_sframes_t)runtime->silence_filled < 0) { + if ((snd_pcm_uframes_t)frames < runtime->silence_filled) + runtime->silence_filled -= frames; + else runtime->silence_filled = 0; - runtime->silence_start = new_hw_ptr; - } else { - runtime->silence_start = ofs; - } + runtime->silence_start = new_hw_ptr; } frames = runtime->buffer_size - runtime->silence_filled; } @@ -105,7 +103,7 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram return; if (frames == 0) return; - ofs = runtime->silence_start % runtime->buffer_size; + ofs = (runtime->silence_start + runtime->silence_filled) % runtime->buffer_size; while (frames > 0) { transfer = ofs + frames > runtime->buffer_size ? runtime->buffer_size - ofs : frames; err = fill_silence_frames(substream, ofs, transfer);
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> --- sound/core/pcm_lib.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)