| Message ID | 20230405201219.2197789-1-oswald.buddenhagen@gmx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] ALSA: pcm: rewrite snd_pcm_playback_silence() | expand |
On Wed, 05 Apr 2023 22:12:18 +0200, Oswald Buddenhagen wrote: > > The auto-silencer supports two modes: "thresholded" to fill up "just > enough", and "top-up" to fill up "as much as possible". The two modes > used rather distinct code paths, which this patch unifies. The only > remaining distinction is how much we actually want to fill. > > This fixes a bug in thresholded mode, where we failed to use new_hw_ptr, > resulting in under-fill. > > Top-up mode is now more well-behaved and much easier to understand in > corner cases. > > This also updates comments in the proximity of silencing-related data > structures. > > Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> Applied both patches now to for-next branch. thanks, Takashi
On Tue, Apr 11, 2023 at 12:47:26PM +0200, Jaroslav Kysela wrote: >On 05. 04. 23 22:12, Oswald Buddenhagen wrote: >> This fixes a bug in thresholded mode, where we failed to use >> new_hw_ptr, >> resulting in under-fill. > >I don't follow what you refer here. The old code uses >snd_pcm_playback_hw_avail() > yes >thus new hw_ptr for the threshold mode, too. > not before my patch. the silencer was called before the new pointer was stored. it had to be, as otherwise the delta for top-up mode could not be calculated. >> + // 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; >> + else if ((snd_pcm_uframes_t) hw_avail >= runtime->boundary) >> + hw_avail -= runtime->boundary; > >If hw_avail is above runtime->boundary then the initial condition is totaly >bogus. I would use snd_BUG_ON() and direct return here. > this is only there as a result of inlining snd_pcm_playback_hw_avail()/snd_pcm_playback_avail() somewhat mindlessly. the check does indeed make no sense, so i'll just drop it. (the broader lesson of this is the attached patch. i can re-post it separately if you like it.) >> frames = runtime->silence_threshold - noise_dist; >> + if ((snd_pcm_sframes_t) frames <= 0) >> + return; > >The retyping does not look good here. Could we move the if before frames >assignment like: > > if (runtime->silence_threshold <= noise_dist) > return; > frames = runtime->silence_threshold - noise_dist; > dunno, i don't like it - it's more noisy and imo it loses expressiveness, as the question we're asking is "how many frames do we need to fill?". note that due to use of unsigned types in the runtime struct, such retyping is rather common in comparisons. regards
On 12. 04. 23 12:33, Oswald Buddenhagen wrote: > On Tue, Apr 11, 2023 at 12:47:26PM +0200, Jaroslav Kysela wrote: >> On 05. 04. 23 22:12, Oswald Buddenhagen wrote: >>> This fixes a bug in thresholded mode, where we failed to use >>> new_hw_ptr, >>> resulting in under-fill. >> >> I don't follow what you refer here. The old code uses >> snd_pcm_playback_hw_avail() >> > yes > >> thus new hw_ptr for the threshold mode, too. >> > not before my patch. the silencer was called before the new pointer was > stored. it had to be, as otherwise the delta for top-up mode could not > be calculated. > >>> + // 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; > >>> + else if ((snd_pcm_uframes_t) hw_avail >= runtime->boundary) >>> + hw_avail -= runtime->boundary; >> >> If hw_avail is above runtime->boundary then the initial condition is totaly >> bogus. I would use snd_BUG_ON() and direct return here. >> > this is only there as a result of inlining > snd_pcm_playback_hw_avail()/snd_pcm_playback_avail() somewhat > mindlessly. the check does indeed make no sense, so i'll just drop it. > (the broader lesson of this is the attached patch. i can re-post it > separately if you like it.) I will correct that it will make sense where hw_ptr is nearby boundary (boundary - buffer_size ... boundary - 1) and appl_ptr is cropped using boundary (0 ... buffer_size). But because appl_ptr can be set by application without any kernel side correction, it may be possible to check if the appl_ptr is in 0 ... boundary range before any use. Sorry for the confusion. >>> frames = runtime->silence_threshold - noise_dist; >>> + if ((snd_pcm_sframes_t) frames <= 0) >>> + return; >> >> The retyping does not look good here. Could we move the if before frames >> assignment like: >> >> if (runtime->silence_threshold <= noise_dist) >> return; >> frames = runtime->silence_threshold - noise_dist; >> > dunno, i don't like it - it's more noisy and imo it loses > expressiveness, as the question we're asking is "how many frames do we > need to fill?". > note that due to use of unsigned types in the runtime struct, such > retyping is rather common in comparisons. It seems that you have answer to everything. My suggestion is perfectly readable (is the requested silence threshold fulfilled? or is the noise distance greater than the whole buffer / buffer_size?). Jaroslav
On Wed, Apr 12, 2023 at 09:23:23PM +0200, Jaroslav Kysela wrote: >On 12. 04. 23 12:33, Oswald Buddenhagen wrote: >> On Tue, Apr 11, 2023 at 12:47:26PM +0200, Jaroslav Kysela wrote: >>> On 05. 04. 23 22:12, Oswald Buddenhagen wrote: >>>> + else if ((snd_pcm_uframes_t) hw_avail >= runtime->boundary) >>>> + hw_avail -= runtime->boundary; >>> >>> If hw_avail is above runtime->boundary then the initial condition is totaly >>> bogus. I would use snd_BUG_ON() and direct return here. >>> >I will correct that it will make sense where hw_ptr is nearby boundary >(boundary - buffer_size ... boundary - 1) and appl_ptr is cropped using >boundary (0 ... buffer_size). > no, that's the case where it goes negative. because it's a subtraction, it plain cannot go over the boundary when both inputs are bounded. due to the remaining correction, it could still go "very big" in an underrun condition, as the comment in the patch says. we should discuss the implications of the latter for the snd_pcm_*_avail() functions separately (the apidoc doesn't make the contract clear at all). >But because appl_ptr can be set by application without any kernel side >correction, it may be possible to check if the appl_ptr is in 0 ... >boundary range before any use. > that should be rather obviously done *somewhere*, as otherwise appl_ptr can often be even slightly above 2*boundary, at which point the above correction (and many alike) wouldn't even work. but for the sake of efficiency, that should be done asap, so when it is set or the boundary changes. no? >>>> frames = runtime->silence_threshold - noise_dist; >>>> + if ((snd_pcm_sframes_t) frames <= 0) >>>> + return; >>> >>> The retyping does not look good here. Could we move the if before frames >>> assignment like: >>> >>> if (runtime->silence_threshold <= noise_dist) >>> return; >>> frames = runtime->silence_threshold - noise_dist; >>> >> dunno, i don't like it - it's more noisy and imo it loses >> expressiveness, as the question we're asking is "how many frames do we >> need to fill?". > >It seems that you have answer to everything. > only to the parts that i actually reply to ... >(is the requested silence threshold fulfilled? or is the noise distance >greater than the whole buffer / buffer_size?). > but why would you want to approach it that way? it's just an extra step to think through. to reinforce the point: if the compiler is any good, then your variant will be optimized into mine. regards
diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst index a368529e8ed3..f2834a016473 100644 --- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst +++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst @@ -1577,14 +1577,16 @@ are the contents of this file: unsigned int period_step; unsigned int sleep_min; /* min ticks to sleep */ snd_pcm_uframes_t start_threshold; - snd_pcm_uframes_t stop_threshold; - snd_pcm_uframes_t silence_threshold; /* Silence filling happens when - noise is nearest than this */ - snd_pcm_uframes_t silence_size; /* Silence filling size */ + // The following two thresholds alleviate playback buffer underruns; when + // hw_avail drops below the threshold, the respective action is triggered: + snd_pcm_uframes_t stop_threshold; /* stop playback */ + snd_pcm_uframes_t silence_threshold; /* pre-fill buffer with silence */ + snd_pcm_uframes_t silence_size; /* msx size of silence pre-fill */ snd_pcm_uframes_t boundary; /* pointers wrap point */ - snd_pcm_uframes_t silenced_start; - snd_pcm_uframes_t silenced_size; + // internal data of auto-silencer + snd_pcm_uframes_t silence_start; /* starting pointer to silence area */ + snd_pcm_uframes_t silence_filled; /* size filled with silence */ snd_pcm_sync_id_t sync; /* hardware synchronization ID */ diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 27040b472a4f..f20400bb7032 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -378,18 +378,18 @@ struct snd_pcm_runtime { unsigned int rate_den; unsigned int no_period_wakeup: 1; - /* -- SW params -- */ - int tstamp_mode; /* mmap timestamp is updated */ + /* -- SW params; see struct snd_pcm_sw_params for comments -- */ + int tstamp_mode; unsigned int period_step; snd_pcm_uframes_t start_threshold; snd_pcm_uframes_t stop_threshold; - snd_pcm_uframes_t silence_threshold; /* Silence filling happens when - noise is nearest than this */ - snd_pcm_uframes_t silence_size; /* Silence filling size */ - snd_pcm_uframes_t boundary; /* pointers wrap point */ + snd_pcm_uframes_t silence_threshold; + snd_pcm_uframes_t silence_size; + snd_pcm_uframes_t boundary; + // internal data of auto-silencer snd_pcm_uframes_t silence_start; /* starting pointer to silence area */ - snd_pcm_uframes_t silence_filled; /* size filled with silence */ + snd_pcm_uframes_t silence_filled; /* already filled part of silence area */ union snd_pcm_sync_id sync; /* hardware synchronization ID */ diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index de6810e94abe..50aa1d98873f 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -429,9 +429,12 @@ struct snd_pcm_sw_params { snd_pcm_uframes_t avail_min; /* min avail frames for wakeup */ snd_pcm_uframes_t xfer_align; /* obsolete: xfer size need to be a multiple */ snd_pcm_uframes_t start_threshold; /* min hw_avail frames for automatic start */ - snd_pcm_uframes_t stop_threshold; /* min avail frames for automatic stop */ - snd_pcm_uframes_t silence_threshold; /* min distance from noise for silence filling */ - snd_pcm_uframes_t silence_size; /* silence block size */ + // The following two thresholds alleviate playback buffer underruns; when + // hw_avail drops below the threshold, the respective action is triggered: + snd_pcm_uframes_t stop_threshold; /* stop playback */ + snd_pcm_uframes_t silence_threshold; /* pre-fill buffer with silence */ + snd_pcm_uframes_t silence_size; /* max size of silence pre-fill; when >= boundary, + * fill played area with silence immediately */ snd_pcm_uframes_t boundary; /* pointers wrap point */ unsigned int proto; /* protocol version */ unsigned int tstamp_type; /* timestamp type (req. proto >= 2.0.12) */ diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 02fd65993e7e..b074c5b926db 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -42,70 +42,58 @@ 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, snd_pcm_uframes_t new_hw_ptr) +void snd_pcm_playback_silence(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; + snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr); + snd_pcm_sframes_t hw_avail, added, noise_dist; snd_pcm_uframes_t frames, ofs, transfer; int err; + // 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; + else if ((snd_pcm_uframes_t) hw_avail >= runtime->boundary) + hw_avail -= runtime->boundary; + + added = appl_ptr - runtime->silence_start; + if (added) { + if (added < 0) + added += runtime->boundary; + if ((snd_pcm_uframes_t)added < runtime->silence_filled) + runtime->silence_filled -= added; + else + runtime->silence_filled = 0; + runtime->silence_start = appl_ptr; + } + + noise_dist = hw_avail + runtime->silence_filled; if (runtime->silence_size < runtime->boundary) { - 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 ((snd_pcm_sframes_t) frames <= 0) + return; if (frames > runtime->silence_size) frames = runtime->silence_size; } else { - 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; + frames = runtime->buffer_size - noise_dist; + if ((snd_pcm_sframes_t) frames <= 0) + return; } + if (snd_BUG_ON(frames > runtime->buffer_size)) return; - if (frames == 0) - return; - ofs = runtime->silence_start % runtime->buffer_size; - while (frames > 0) { + ofs = (runtime->silence_start + runtime->silence_filled) % runtime->buffer_size; + do { 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); } @@ -439,10 +427,6 @@ 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) @@ -460,6 +444,10 @@ 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 ecb21697ae3a..42fe3a4e9154 100644 --- a/sound/core/pcm_local.h +++ b/sound/core/pcm_local.h @@ -29,8 +29,7 @@ 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, - snd_pcm_uframes_t new_hw_ptr); +void snd_pcm_playback_silence(struct snd_pcm_substream *substream); 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 331380c2438b..0e3e7997dc58 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, ULONG_MAX); + snd_pcm_playback_silence(substream); 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, ULONG_MAX); + snd_pcm_playback_silence(substream); 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, ULONG_MAX); + snd_pcm_playback_silence(substream); snd_pcm_stream_unlock_irq(substream); }
The auto-silencer supports two modes: "thresholded" to fill up "just enough", and "top-up" to fill up "as much as possible". The two modes used rather distinct code paths, which this patch unifies. The only remaining distinction is how much we actually want to fill. This fixes a bug in thresholded mode, where we failed to use new_hw_ptr, resulting in under-fill. Top-up mode is now more well-behaved and much easier to understand in corner cases. This also updates comments in the proximity of silencing-related data structures. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- .../kernel-api/writing-an-alsa-driver.rst | 14 +-- include/sound/pcm.h | 14 +-- include/uapi/sound/asound.h | 9 +- sound/core/pcm_lib.c | 86 ++++++++----------- sound/core/pcm_local.h | 3 +- sound/core/pcm_native.c | 6 +- 6 files changed, 62 insertions(+), 70 deletions(-)