On Thu, 04 May 2023 15:00:07 +0200,
Oswald Buddenhagen wrote:
>
> Turns out that we cannot rely on the application pointer being updated
> in top-up mode, as its primary purpose is to remain operational in
> free-running mode as used by dmix.
>
> So we logically revert some bits from commit 9f656705c5faa ("ALSA: pcm:
> rewrite snd_pcm_playback_silence()"), but we retain the bug fixes and
> try to make the code paths congruent.
>
> Reported-by: Jeff Chua <jeff.chua.linux@xxxxxxxxx>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@xxxxxx>
Honestly speaking, this is really hard to review. As most of changes
here are the revert of the previous commit, I'd rather like to get it
reverted whole once, and re-apply the proper fix gradually.
The auto-silence function is really messy and fragile, and we can't
follow the code changes easily if the things go from left to right
and return again.
That said, don't mix the fix and the code refactoring and the revert
into a pot, but let's separate them.
Through a quick glance over the patch, my concern is how
runtime->silence_start is handled. In the older code, silence_start
is the starting offset, while the offset in the current tree is
silence_start + silence_filled.
Is it really OK to reset the silence_start always when it's updated
(in silent_size==boundary case) as in this patch?
thanks,
Takashi
> ---
> sound/core/pcm_lib.c | 82 +++++++++++++++++++++++++++--------------
> sound/core/pcm_local.h | 3 +-
> sound/core/pcm_native.c | 6 +--
> 3 files changed, 60 insertions(+), 31 deletions(-)
>
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index d21c73944efd..cd5f2ef14ab4 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -42,41 +42,69 @@ 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_sframes_t hw_avail, frames;
> snd_pcm_uframes_t noise_dist, 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) {
> + snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr);
> + snd_pcm_sframes_t 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;
> + }
> +
> + if (new_hw_ptr == ULONG_MAX) // initialization
> + new_hw_ptr = runtime->status->hw_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 - new_hw_ptr;
> + if (hw_avail < 0)
> + hw_avail += runtime->boundary;
> +
> + noise_dist = hw_avail + runtime->silence_filled;
> frames = runtime->silence_threshold - noise_dist;
> if (frames <= 0)
> return;
> if (frames > runtime->silence_size)
> frames = runtime->silence_size;
> } else {
> - frames = runtime->buffer_size - noise_dist;
> + // This filling mode aims at free-running mode (used for example by dmix),
> + // which doesn't update the application pointer.
> + snd_pcm_uframes_t hw_ptr = runtime->status->hw_ptr;
> + if (new_hw_ptr == ULONG_MAX) { // initialization
> + // Usually, this is entered while stopped, before data is queued,
> + // so both pointers are expected to be zero.
> + hw_avail = runtime->control->appl_ptr - hw_ptr;
> + if (hw_avail < 0)
> + hw_avail += runtime->boundary;
> + // In free-running mode, appl_ptr will be zero even while running,
> + // so we end up with a huge number. There is no useful way to
> + // handle this, so we just clear the whole buffer.
> + runtime->silence_filled = hw_avail > runtime->buffer_size ? 0 : hw_avail;
> + runtime->silence_start = hw_ptr;
> + } else {
> + snd_pcm_sframes_t played = new_hw_ptr - hw_ptr;
> + if (played) {
> + if (played < 0)
> + played += runtime->boundary;
> + if (played < runtime->silence_filled)
> + runtime->silence_filled -= played;
> + else // This may happen due to a reset.
> + runtime->silence_filled = 0;
> + runtime->silence_start = new_hw_ptr;
> + }
> + }
> + frames = runtime->buffer_size - runtime->silence_filled;
> if (frames <= 0)
> return;
> }
> @@ -425,6 +453,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 +474,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 3d0c4a5b701b..94185267a7b9 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);
> }
>
> --
> 2.40.0.152.g15d061e6df
>
