[PATCH 0/5] rewrite snd_pcm_playback_silence() again
This is a split of changes for both patches (the first with the missing top-up mode + later fix with mixups) proposed by Oswald. The aim was to pick only real code changes.
Only the first two patches after revert fixes the current silencing issues. The last two are just cleanups with the extra optimization in the last patch moving the common code to a function.
Cc: Oswald Buddenhagen oswald.buddenhagen@gmx.de Cc: Jeff Chua jeff.chua.linux@gmail.com
Jaroslav Kysela (5): ALSA: pcm: Revert "ALSA: pcm: rewrite snd_pcm_playback_silence()" ALSA: pcm: fix playback silence - use the actual new_hw_ptr for the threshold mode ALSA: pcm: fix playback silence - correct the incremental silencing ALSA: pcm: playback silence - remove extra code ALSA: pcm: playback silence - move silence variables updates to separate function
sound/core/pcm_lib.c | 91 +++++++++++++++++++++++++---------------- sound/core/pcm_local.h | 3 +- sound/core/pcm_native.c | 6 +-- 3 files changed, 61 insertions(+), 39 deletions(-)
This reverts commit 9f656705c5faa18afb26d922cfc64f9fd103c38d.
There is a regression (the top-up mode). Unfortunately, the patch provided from the author of this commit is not easy for the review.
Keep the update and new comments in headers.
Signed-off-by: Jaroslav Kysela perex@perex.cz --- sound/core/pcm_lib.c | 86 ++++++++++++++++++++++++----------------- sound/core/pcm_local.h | 3 +- sound/core/pcm_native.c | 6 +-- 3 files changed, 55 insertions(+), 40 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index d21c73944efd..af1eb136feb0 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -42,56 +42,70 @@ static int fill_silence_frames(struct snd_pcm_substream *substream, * * when runtime->silence_size >= runtime->boundary - fill processed area with silence immediately */ -void snd_pcm_playback_silence(struct snd_pcm_substream *substream) +void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_uframes_t new_hw_ptr) { struct snd_pcm_runtime *runtime = substream->runtime; - snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr); - snd_pcm_sframes_t added, hw_avail, frames; - snd_pcm_uframes_t noise_dist, ofs, transfer; + snd_pcm_uframes_t frames, ofs, transfer; int err;
- added = appl_ptr - runtime->silence_start; - if (added) { - if (added < 0) - added += runtime->boundary; - if (added < runtime->silence_filled) - runtime->silence_filled -= added; - else - runtime->silence_filled = 0; - runtime->silence_start = appl_ptr; - } - - // This will "legitimately" turn negative on underrun, and will be mangled - // into a huge number by the boundary crossing handling. The initial state - // might also be not quite sane. The code below MUST account for these cases. - hw_avail = appl_ptr - runtime->status->hw_ptr; - if (hw_avail < 0) - hw_avail += runtime->boundary; - - noise_dist = hw_avail + runtime->silence_filled; if (runtime->silence_size < runtime->boundary) { - frames = runtime->silence_threshold - noise_dist; - if (frames <= 0) + snd_pcm_sframes_t noise_dist, n; + snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr); + if (runtime->silence_start != appl_ptr) { + n = appl_ptr - runtime->silence_start; + if (n < 0) + n += runtime->boundary; + if ((snd_pcm_uframes_t)n < runtime->silence_filled) + runtime->silence_filled -= n; + else + runtime->silence_filled = 0; + runtime->silence_start = appl_ptr; + } + if (runtime->silence_filled >= runtime->buffer_size) + return; + noise_dist = snd_pcm_playback_hw_avail(runtime) + runtime->silence_filled; + if (noise_dist >= (snd_pcm_sframes_t) runtime->silence_threshold) return; + frames = runtime->silence_threshold - noise_dist; if (frames > runtime->silence_size) frames = runtime->silence_size; } else { - frames = runtime->buffer_size - noise_dist; - if (frames <= 0) - return; + if (new_hw_ptr == ULONG_MAX) { /* initialization */ + 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; + } else { + ofs = runtime->status->hw_ptr; + frames = new_hw_ptr - ofs; + if ((snd_pcm_sframes_t)frames < 0) + frames += runtime->boundary; + runtime->silence_filled -= frames; + if ((snd_pcm_sframes_t)runtime->silence_filled < 0) { + runtime->silence_filled = 0; + runtime->silence_start = new_hw_ptr; + } else { + runtime->silence_start = ofs; + } + } + frames = runtime->buffer_size - runtime->silence_filled; } - if (snd_BUG_ON(frames > runtime->buffer_size)) return; - ofs = (runtime->silence_start + runtime->silence_filled) % runtime->buffer_size; - do { + if (frames == 0) + return; + ofs = runtime->silence_start % runtime->buffer_size; + while (frames > 0) { transfer = ofs + frames > runtime->buffer_size ? runtime->buffer_size - ofs : frames; err = fill_silence_frames(substream, ofs, transfer); snd_BUG_ON(err < 0); runtime->silence_filled += transfer; frames -= transfer; ofs = 0; - } while (frames > 0); + } snd_pcm_dma_buffer_sync(substream, SNDRV_DMA_SYNC_DEVICE); }
@@ -425,6 +439,10 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, return 0; }
+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && + runtime->silence_size > 0) + snd_pcm_playback_silence(substream, new_hw_ptr); + if (in_interrupt) { delta = new_hw_ptr - runtime->hw_ptr_interrupt; if (delta < 0) @@ -442,10 +460,6 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, runtime->hw_ptr_wrap += runtime->boundary; }
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && - runtime->silence_size > 0) - snd_pcm_playback_silence(substream); - update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
return snd_pcm_update_state(substream, runtime); diff --git a/sound/core/pcm_local.h b/sound/core/pcm_local.h index 42fe3a4e9154..ecb21697ae3a 100644 --- a/sound/core/pcm_local.h +++ b/sound/core/pcm_local.h @@ -29,7 +29,8 @@ int snd_pcm_update_state(struct snd_pcm_substream *substream, struct snd_pcm_runtime *runtime); int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream);
-void snd_pcm_playback_silence(struct snd_pcm_substream *substream); +void snd_pcm_playback_silence(struct snd_pcm_substream *substream, + snd_pcm_uframes_t new_hw_ptr);
static inline snd_pcm_uframes_t snd_pcm_avail(struct snd_pcm_substream *substream) diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 91c87cdb786e..39a65d1415ab 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -958,7 +958,7 @@ static int snd_pcm_sw_params(struct snd_pcm_substream *substream, if (snd_pcm_running(substream)) { if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && runtime->silence_size > 0) - snd_pcm_playback_silence(substream); + snd_pcm_playback_silence(substream, ULONG_MAX); err = snd_pcm_update_state(substream, runtime); } snd_pcm_stream_unlock_irq(substream); @@ -1455,7 +1455,7 @@ static void snd_pcm_post_start(struct snd_pcm_substream *substream, __snd_pcm_set_state(runtime, state); if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && runtime->silence_size > 0) - snd_pcm_playback_silence(substream); + snd_pcm_playback_silence(substream, ULONG_MAX); snd_pcm_timer_notify(substream, SNDRV_TIMER_EVENT_MSTART); }
@@ -1916,7 +1916,7 @@ static void snd_pcm_post_reset(struct snd_pcm_substream *substream, runtime->control->appl_ptr = runtime->status->hw_ptr; if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && runtime->silence_size > 0) - snd_pcm_playback_silence(substream); + snd_pcm_playback_silence(substream, ULONG_MAX); snd_pcm_stream_unlock_irq(substream); }
On Fri, 05 May 2023 09:38:09 +0200, Jaroslav Kysela wrote:
This reverts commit 9f656705c5faa18afb26d922cfc64f9fd103c38d.
There is a regression (the top-up mode). Unfortunately, the patch provided from the author of this commit is not easy for the review.
Keep the update and new comments in headers.
Signed-off-by: Jaroslav Kysela perex@perex.cz
Better to add Reported-by and the reference for the original thread reporting the regression, as well as the Fixes tag.
I can cook up it by myself, too.
Takashi
On Fri, May 05, 2023 at 11:31:16AM +0200, Takashi Iwai wrote:
On Fri, 05 May 2023 09:38:09 +0200, Jaroslav Kysela wrote:
Signed-off-by: Jaroslav Kysela perex@perex.cz
Better to add Reported-by and the reference for the original thread reporting the regression,
i'll post a slight rework of the series shortly, where i'll include this.
as well as the Fixes tag.
that seems pointless for a revert, as all the info is already included anyway, no?
regards
On Fri, 05 May 2023 11:38:45 +0200, Oswald Buddenhagen wrote:
On Fri, May 05, 2023 at 11:31:16AM +0200, Takashi Iwai wrote:
On Fri, 05 May 2023 09:38:09 +0200, Jaroslav Kysela wrote:
Signed-off-by: Jaroslav Kysela perex@perex.cz
Better to add Reported-by and the reference for the original thread reporting the regression,
i'll post a slight rework of the series shortly, where i'll include this.
as well as the Fixes tag.
that seems pointless for a revert, as all the info is already included anyway, no?
People tend to look at Fixes tag to judge whether the commit can fix something real or not. For example, we (at SUSE) regularly check Fixes tag to pick up the missing fixing. In the case of a revert, it doesn't mean always a fix. So it's not mandatory, but would be still helpful.
thanks,
Takashi
The snd_pcm_playback_hw_avail() function uses runtime->status->hw_ptr. Unfortunately, in case when we call this function from snd_pcm_update_hw_ptr0(), this variable contains the previous hardware pointer. Use the new_hw_ptr argument to calculate hw_avail (filled samples by the user space) to correct the threshold comparison.
The new_hw_ptr argument may also be set to ULONG_MAX which means the initialization phase. In this case, use runtime->status->hw_ptr.
Suggested-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de Signed-off-by: Jaroslav Kysela perex@perex.cz --- sound/core/pcm_lib.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index af1eb136feb0..8a01aeda2213 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -63,7 +63,15 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram } if (runtime->silence_filled >= runtime->buffer_size) return; - noise_dist = snd_pcm_playback_hw_avail(runtime) + runtime->silence_filled; + /* initialization outside pointer updates */ + if (new_hw_ptr == ULONG_MAX) + new_hw_ptr = runtime->status->hw_ptr; + /* get hw_avail with the boundary crossing */ + noise_dist = appl_ptr - new_hw_ptr; + if (noise_dist < 0) + noise_dist += runtime->boundary; + /* total noise distance */ + noise_dist += runtime->silence_filled; if (noise_dist >= (snd_pcm_sframes_t) runtime->silence_threshold) return; frames = runtime->silence_threshold - noise_dist;
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(-)
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);
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
The removed condition handles de facto only one situation where runtime->silence_filled variable is equal to runtime->buffer_size, because this variable cannot go over the buffer size. This case is catched also with the next condition for the noise distance and required threshold comparison.
Suggested-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de Signed-off-by: Jaroslav Kysela perex@perex.cz --- sound/core/pcm_lib.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index cfdb4aa5f6fa..952f0d807124 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -61,8 +61,6 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram runtime->silence_filled = 0; runtime->silence_start = appl_ptr; } - if (runtime->silence_filled >= runtime->buffer_size) - return; /* initialization outside pointer updates */ if (new_hw_ptr == ULONG_MAX) new_hw_ptr = runtime->status->hw_ptr;
There is a common code in the threshold and top-up mode tracking the added (already silenced) samples. Move this code to one place to enhance the readability.
Signed-off-by: Jaroslav Kysela perex@perex.cz --- sound/core/pcm_lib.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 952f0d807124..6b0601fec832 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -33,6 +33,25 @@ static int fill_silence_frames(struct snd_pcm_substream *substream, snd_pcm_uframes_t off, snd_pcm_uframes_t frames);
+ +static inline void silence_update(struct snd_pcm_runtime *runtime, + snd_pcm_uframes_t ptr, + snd_pcm_uframes_t new_ptr) +{ + snd_pcm_sframes_t n; + + if (ptr == new_ptr) + return; + n = new_ptr - ptr; + if (n < 0) + n += runtime->boundary; + if ((snd_pcm_uframes_t)n < runtime->silence_filled) + runtime->silence_filled -= n; + else + runtime->silence_filled = 0; + runtime->silence_start = new_ptr; +} + /* * fill ring buffer with silence * runtime->silence_start: starting pointer to silence area @@ -49,18 +68,9 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram int err;
if (runtime->silence_size < runtime->boundary) { - snd_pcm_sframes_t noise_dist, n; + snd_pcm_sframes_t noise_dist; snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr); - if (runtime->silence_start != appl_ptr) { - n = appl_ptr - runtime->silence_start; - if (n < 0) - n += runtime->boundary; - if ((snd_pcm_uframes_t)n < runtime->silence_filled) - runtime->silence_filled -= n; - else - runtime->silence_filled = 0; - runtime->silence_start = appl_ptr; - } + silence_update(runtime, runtime->silence_start, appl_ptr); /* initialization outside pointer updates */ if (new_hw_ptr == ULONG_MAX) new_hw_ptr = runtime->status->hw_ptr; @@ -86,14 +96,7 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram } else { /* 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; - if ((snd_pcm_uframes_t)frames < runtime->silence_filled) - runtime->silence_filled -= frames; - else - runtime->silence_filled = 0; - runtime->silence_start = new_hw_ptr; + silence_update(runtime, runtime->status->hw_ptr, new_hw_ptr); } frames = runtime->buffer_size - runtime->silence_filled; }
On Fri, 05 May 2023 09:38:08 +0200, Jaroslav Kysela wrote:
This is a split of changes for both patches (the first with the missing top-up mode + later fix with mixups) proposed by Oswald. The aim was to pick only real code changes.
Only the first two patches after revert fixes the current silencing issues. The last two are just cleanups with the extra optimization in the last patch moving the common code to a function.
Cc: Oswald Buddenhagen oswald.buddenhagen@gmx.de Cc: Jeff Chua jeff.chua.linux@gmail.com
Jaroslav Kysela (5): ALSA: pcm: Revert "ALSA: pcm: rewrite snd_pcm_playback_silence()" ALSA: pcm: fix playback silence - use the actual new_hw_ptr for the threshold mode ALSA: pcm: fix playback silence - correct the incremental silencing ALSA: pcm: playback silence - remove extra code ALSA: pcm: playback silence - move silence variables updates to separate function
Thanks, this makes it much easier to digest the whole changes!
I watch out for a while whether any objection comes up, then apply patches in today -- or if we didn't reach to consensus, I'll pick up only the first revert patch for 6.4-rc1, at least.
Takashi
participants (3)
-
Jaroslav Kysela -
Oswald Buddenhagen -
Takashi Iwai