[PATCH 1/2] ALSA: pcm: rewrite snd_pcm_playback_silence()
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(-)
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); }
Draining will always playback somewhat beyond the end of the filled buffer. This would produce artifacts if the user did not set up the auto-silencing machinery. This patch makes it work out of the box.
Rather than figuring out the right threshold (which is one period plus the card-specific FIFO size plus some IRQ jitter), we use "top-up" mode.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/core/pcm_lib.c | 5 +++-- sound/core/pcm_native.c | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index b074c5b926db..1dd870a2093d 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -71,7 +71,8 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream) }
noise_dist = hw_avail + runtime->silence_filled; - if (runtime->silence_size < runtime->boundary) { + if (runtime->silence_size < runtime->boundary && + runtime->state != SNDRV_PCM_STATE_DRAINING) { frames = runtime->silence_threshold - noise_dist; if ((snd_pcm_sframes_t) frames <= 0) return; @@ -445,7 +446,7 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, }
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && - runtime->silence_size > 0) + (runtime->silence_size > 0 || runtime->state == SNDRV_PCM_STATE_DRAINING)) snd_pcm_playback_silence(substream);
update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp); diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 0e3e7997dc58..6ecb6a733606 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1454,7 +1454,7 @@ static void snd_pcm_post_start(struct snd_pcm_substream *substream, runtime->rate; __snd_pcm_set_state(runtime, state); if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && - runtime->silence_size > 0) + (runtime->silence_size > 0 || state == SNDRV_PCM_STATE_DRAINING)) snd_pcm_playback_silence(substream); snd_pcm_timer_notify(substream, SNDRV_TIMER_EVENT_MSTART); } @@ -2045,6 +2045,7 @@ static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, break; case SNDRV_PCM_STATE_RUNNING: __snd_pcm_set_state(runtime, SNDRV_PCM_STATE_DRAINING); + snd_pcm_playback_silence(substream); break; case SNDRV_PCM_STATE_XRUN: __snd_pcm_set_state(runtime, SNDRV_PCM_STATE_SETUP);
On 05. 04. 23 22:12, Oswald Buddenhagen wrote:
Draining will always playback somewhat beyond the end of the filled buffer. This would produce artifacts if the user did not set up the auto-silencing machinery. This patch makes it work out of the box.
Rather than figuring out the right threshold (which is one period plus the card-specific FIFO size plus some IRQ jitter), we use "top-up" mode.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de
I think that it was really bad decision to apply this patch without a broader discussion. When we designed the API, we knew about described problems and we decided to keep this up to applications. The silencing may not help in all cases where the PCM samples ends with a high volume. A volume ramping should be used and it's an application job. Also, silencing touches the DMA buffer which may not be desired. And lastly drivers can handle draining correctly (stop at the exact position - see substream->ops->trigger with SNDRV_PCM_TRIGGER_DRAIN argument).
I would create a new API extension for this (new ioctl or sw_params flag), but the default behaviour should be retained.
I will try to review the first patch too, but my time is limited over Easter.
Jaroslav
On Sat, 08 Apr 2023 01:58:21 +0200, Jaroslav Kysela wrote:
On 05. 04. 23 22:12, Oswald Buddenhagen wrote:
Draining will always playback somewhat beyond the end of the filled buffer. This would produce artifacts if the user did not set up the auto-silencing machinery. This patch makes it work out of the box.
Rather than figuring out the right threshold (which is one period plus the card-specific FIFO size plus some IRQ jitter), we use "top-up" mode.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de
I think that it was really bad decision to apply this patch without a broader discussion. When we designed the API, we knew about described problems and we decided to keep this up to applications. The silencing may not help in all cases where the PCM samples ends with a high volume. A volume ramping should be used and it's an application job. Also, silencing touches the DMA buffer which may not be desired. And lastly drivers can handle draining correctly (stop at the exact position - see substream->ops->trigger with SNDRV_PCM_TRIGGER_DRAIN argument).
I would create a new API extension for this (new ioctl or sw_params flag), but the default behaviour should be retained.
I will try to review the first patch too, but my time is limited over Easter.
OK, thanks. I'll drop the patch meanwhile.
Applying the silencing blindly might be an overkill, indeed, although this could be seen as an easy solution. Let's see.
Takashi
On Sat, Apr 08, 2023 at 01:58:21AM +0200, Jaroslav Kysela wrote:
On 05. 04. 23 22:12, Oswald Buddenhagen wrote:
Draining will always playback somewhat beyond the end of the filled buffer. This would produce artifacts if the user did not set up the auto-silencing machinery. This patch makes it work out of the box.
I think that it was really bad decision to apply this patch without a broader discussion.
When we designed the API, we knew about described problems and we decided to keep this up to applications.
i ran into no documentation of either the problems nor the decisions and their implications for the user.
The silencing may not help in all cases where the PCM samples ends with a high volume.
that would just create a slight crack, which isn't any different from a "regular" sudden stop.
A volume ramping should be used and it's an application job.
imo, that entirely misses the point - the volume is most likely already zero at the end of the buffer. that doesn't mean that it's ok to play the samples again where the volume might not be *quite* zero yet.
Also, silencing touches the DMA buffer which may not be desired.
hypothetically, yes. but practically? why would anyone want to play the same samples after draining? draining is most likely followed by closing the device. and even if not, in most cases (esp. where draining would actually make sense) one wouldn't play a fixed pattern that could be just re-used, so one would have to re-fill the buffer prior to starting again anyway. never mind the effort necessary to track the state of the buffer instead of just re-filling it. so for all practical purposes, already played samples can be considered undefined data and thus safe to overwrite.
And lastly drivers can handle draining correctly (stop at the exact position - see substream->ops->trigger with SNDRV_PCM_TRIGGER_DRAIN argument).
yeah. hypothetically. afaict, there is exactly one driver which supports this. most (older) hardware wouldn't even have the capability to do such precise timing without external help.
On Sat, Apr 08, 2023 at 07:55:48AM +0200, Takashi Iwai wrote:
Applying the silencing blindly might be an overkill, indeed, although this could be seen as an easy solution. Let's see.
i don't think that "overkill" is right here. someone has to do the silencing for draining to be useful at all, and so the question is only who that should be. my argument is that not auto-silencing is *extremely* unexpected, and thus just bad api. i'm pretty certain that about 99% of the usages of DRAIN start out missing this, and most never get fixed.
imo, if any api is added, it should be to opt *out* of auto-silencing. but i don't think this makes any sense; there would be ~zero users of this ever.
regards
On 08. 04. 23 9:24, Oswald Buddenhagen wrote:
On Sat, Apr 08, 2023 at 01:58:21AM +0200, Jaroslav Kysela wrote:
On 05. 04. 23 22:12, Oswald Buddenhagen wrote:
Draining will always playback somewhat beyond the end of the filled buffer. This would produce artifacts if the user did not set up the auto-silencing machinery. This patch makes it work out of the box.
I think that it was really bad decision to apply this patch without a broader discussion.
When we designed the API, we knew about described problems and we decided to keep this up to applications.
i ran into no documentation of either the problems nor the decisions and their implications for the user.
The documentation may be improved, but the "period transfers" are described.
The silencing may not help in all cases where the PCM samples ends with a high volume.
that would just create a slight crack, which isn't any different from a "regular" sudden stop.
A volume ramping should be used and it's an application job.
imo, that entirely misses the point - the volume is most likely already zero at the end of the buffer. that doesn't mean that it's ok to play the samples again where the volume might not be *quite* zero yet.
Also, silencing touches the DMA buffer which may not be desired.
hypothetically, yes. but practically? why would anyone want to play the same samples after draining? draining is most likely followed by closing the device. and even if not, in most cases (esp. where draining would actually make sense) one wouldn't play a fixed pattern that could be just re-used, so one would have to re-fill the buffer prior to starting again anyway. never mind the effort necessary to track the state of the buffer instead of just re-filling it. so for all practical purposes, already played samples can be considered undefined data and thus safe to overwrite.
The buffers can be mmaped so used directly for application and hardware. I don't really feel that it's a good thing to modify this buffer for playback when the application has not requested for that.
And lastly drivers can handle draining correctly (stop at the exact position - see substream->ops->trigger with SNDRV_PCM_TRIGGER_DRAIN argument).
yeah. hypothetically. afaict, there is exactly one driver which supports this. most (older) hardware wouldn't even have the capability to do such precise timing without external help.
Most hardware has FIFO and most drivers uses the DMA position, so I think that the interrupt -> stop DMA latency may be covered with this FIFO in most cases.
But I would really keep this on the driver code to handle this rather than do the forced silencing.
On Sat, Apr 08, 2023 at 07:55:48AM +0200, Takashi Iwai wrote:
Applying the silencing blindly might be an overkill, indeed, although this could be seen as an easy solution. Let's see.
i don't think that "overkill" is right here. someone has to do the silencing for draining to be useful at all, and so the question is only who that should be. my argument is that not auto-silencing is *extremely* unexpected, and thus just bad api. i'm pretty certain that about 99% of the usages of DRAIN start out missing this, and most never get fixed.
Again, I would improve the documentation. Also, the silencing is controlled using sw_params, so applications may request the silencing before drain.
imo, if any api is added, it should be to opt *out* of auto-silencing. but i don't think this makes any sense; there would be ~zero users of this ever.
Lastly, I think that you cannot call updated snd_pcm_playback_silence() function with runtime->silence_size == 0.
if (runtime->silence_size < runtime->boundary) { frames = runtime->silence_threshold - noise_dist; if ((snd_pcm_sframes_t) frames <= 0) return; if (frames > runtime->silence_size) frames = runtime->silence_size; } else {
The frames variable will be 0, so your code will do nothing.
Jaroslav
On Tue, Apr 11, 2023 at 01:09:59PM +0200, Jaroslav Kysela wrote:
On 08. 04. 23 9:24, Oswald Buddenhagen wrote:
Also, silencing touches the DMA buffer which may not be desired.
hypothetically, yes. but practically? [...]
The buffers can be mmaped so used directly for application and hardware.
yes, and they are owned by the hardware/driver. an application would know better than doing with them anything they were not designed for.
And lastly drivers can handle draining correctly (stop at the exact position - see substream->ops->trigger with SNDRV_PCM_TRIGGER_DRAIN argument).
yeah. hypothetically. afaict, there is exactly one driver which supports this. most (older) hardware wouldn't even have the capability to do such precise timing without external help.
Most hardware has FIFO and most drivers uses the DMA position, so I think that the interrupt -> stop DMA latency may be covered with this FIFO in most cases.
on most hardware it would be quite a stunt to re-program the buffer pointers on the fly to enable a mid-period interrupt. and given the reliability problems insisted on by takashi in the other thread, the approach seems questionable at best. and that's still ignoring the effort of migrating tens (hundreds?) of drivers.
Again, I would improve the documentation.
no amount of documentation will fix a bad api. it's just not how humans work.
the silencing is controlled using sw_params, so applications may request the silencing before drain.
yeah, they could, but they don't, and most won't ever.
you're arguing for not doing a very practical and simple change that will fix a lot of user code at once, for the sake of preventing an entirely hypothetical and implausible problem. that is not a good trade-off.
Lastly, I think that you cannot call updated snd_pcm_playback_silence() function with runtime->silence_size == 0.
if (runtime->silence_size < runtime->boundary) {
you missed the hunk that adjusts the code accordingly.
regards
On 11. 04. 23 15:57, Oswald Buddenhagen wrote:
On Tue, Apr 11, 2023 at 01:09:59PM +0200, Jaroslav Kysela wrote:
On 08. 04. 23 9:24, Oswald Buddenhagen wrote:
Also, silencing touches the DMA buffer which may not be desired.
hypothetically, yes. but practically? [...]
The buffers can be mmaped so used directly for application and hardware.
yes, and they are owned by the hardware/driver. an application would know better than doing with them anything they were not designed for.
And lastly drivers can handle draining correctly (stop at the exact position - see substream->ops->trigger with SNDRV_PCM_TRIGGER_DRAIN argument).
yeah. hypothetically. afaict, there is exactly one driver which supports this. most (older) hardware wouldn't even have the capability to do such precise timing without external help.
Most hardware has FIFO and most drivers uses the DMA position, so I think that the interrupt -> stop DMA latency may be covered with this FIFO in most cases.
on most hardware it would be quite a stunt to re-program the buffer pointers on the fly to enable a mid-period interrupt. and given the reliability problems insisted on by takashi in the other thread, the approach seems questionable at best. and that's still ignoring the effort of migrating tens (hundreds?) of drivers.
I said to not change things at the moment. Drivers may be revised at some point. If we have large buffers, the silencing may consume many CPU ticks. If the driver already behaves nicely for the drain operation, this is an extra step which should be avoided. This can be handled using new snd_pcm_ops, of course.
You're using a hammer to fix a little issue.
Again, I would improve the documentation.
no amount of documentation will fix a bad api. it's just not how humans work.
Which code does not fill the last period? Alsa utilities do it. We can eventually handle this in alsa-lib.
the silencing is controlled using sw_params, so applications may request the silencing before drain.
yeah, they could, but they don't, and most won't ever.
you're arguing for not doing a very practical and simple change that will fix a lot of user code at once, for the sake of preventing an entirely hypothetical and implausible problem. that is not a good trade-off.
I'm arguing that we should not do anything extra with the buffers until the application requests that. That's the clear API context.
Lastly, I think that you cannot call updated snd_pcm_playback_silence() function with runtime->silence_size == 0.
if (runtime->silence_size < runtime->boundary) {you missed the hunk that adjusts the code accordingly.
Ohh, yes. You're right.
If we allow modification of the PCM buffer, I think that we should:
- Do not modify the buffer for drivers already working with the appl_ptr data (end position) only. - Handle the situation with the large buffer; it may make sense to change the "wait" operation from the end-of-period interrupt to time scheduler and stop the drain more early when the end-of-valid data condition is fulfilled. - Increase the protocol version.
But as I wrote, I would make those extensions configurable (SNDRV_PCM_HW_PARAMS_DRAIN_ALLOW_SILENCE). It can be turned on by default.
Jaroslav
On Tue, Apr 11, 2023 at 04:48:58PM +0200, Jaroslav Kysela wrote:
You're using a hammer to fix a little issue.
yes, but at the time it seemed like a rather small hammer to me.
if large buffers are actually a thing (what for?), then the fill could be limited to two periods or something. it would make the code uglier, though.
Which code does not fill the last period?
a lot, i imagine - doing that is rather counter-intuitive when using the write() access method.
also, just the last period is not enough, due to the fifo, and possibly delayed/lost irqs.
the silencing is controlled using sw_params, so applications may request the silencing before drain.
yeah, they could, but they don't, and most won't ever.
you're arguing for not doing a very practical and simple change that will fix a lot of user code at once, for the sake of preventing an entirely hypothetical and implausible problem. that is not a good trade-off.
I'm arguing that we should not do anything extra with the buffers until the application requests that.
That's the clear API context.
no, it's not. you cannot assume this to be understood as the central guiding principle which trumps more immediate issues. people use an api to solve a specific problem, and they want to do that with the least effort possible. no-one is going to read the whole docu top to bottom and remember every caveat. if it appears to work, people will just call it a day, and that's exactly what will be the case with the use of DRAIN (one needs a somewhat specific configuration and content to even notice that there is a problem).
If we allow modification of the PCM buffer, I think that we should:
- Do not modify the buffer for drivers already working with the appl_ptr data (end position) only.
i suppose that should be detected by the drain callback being set up?
- Handle the situation with the large buffer; it may make sense to change the "wait" operation from the end-of-period interrupt to time scheduler and stop the drain more early when the end-of-valid data condition is fulfilled.
i don't understand what you're asking for.
- Increase the protocol version.
But as I wrote, I would make those extensions configurable (SNDRV_PCM_HW_PARAMS_DRAIN_ALLOW_SILENCE). It can be turned on by default.
i have no clue what would be involved in doing that. to me that sounds like overkill (solving a non-issue), and goes waaaay beyond what i expected to invest into this issue (really, i just wanted to verify that the emu10k1 fixes work, and accidentally discovered that there is a mid-layer issue that affects user space, as the pyalsaaudio lib i'm using doesn't handle it).
regards
On 11. 04. 23 18:50, Oswald Buddenhagen wrote:
If we allow modification of the PCM buffer, I think that we should:
- Do not modify the buffer for drivers already working with the appl_ptr data (end position) only.
i suppose that should be detected by the drain callback being set up?
Yes, but it would be probably better to add a default silencing callback with a warning to notify authors of drivers to review and eventually correct the behavior.
- Handle the situation with the large buffer; it may make sense to change the "wait" operation from the end-of-period interrupt to time scheduler and stop the drain more early when the end-of-valid data condition is fulfilled.
i don't understand what you're asking for.
Use jiffies/timeout instead waiting to the interrupt. In this case, the stop may be called earlier (in the middle of period). In this case the silenced area may be much smaller.
- Increase the protocol version.
But as I wrote, I would make those extensions configurable (SNDRV_PCM_HW_PARAMS_DRAIN_ALLOW_SILENCE). It can be turned on by default.
i have no clue what would be involved in doing that. to me that sounds like overkill (solving a non-issue), and goes waaaay beyond what i expected to invest into this issue (really, i just wanted to verify that the emu10k1 fixes work, and accidentally discovered that there is a mid-layer issue that affects user space, as the pyalsaaudio lib i'm using doesn't handle it).
OK. I don't think that it's a pyalsaudio job to resolve the issue with the minimal transfer chunk / period (which you set / know before the transfer is initiated).
Jaroslav
On Tue, 11 Apr 2023 19:23:17 +0200, Jaroslav Kysela wrote:
On 11. 04. 23 18:50, Oswald Buddenhagen wrote:
If we allow modification of the PCM buffer, I think that we should:
- Do not modify the buffer for drivers already working with the appl_ptr data (end position) only.
i suppose that should be detected by the drain callback being set up?
Yes, but it would be probably better to add a default silencing callback with a warning to notify authors of drivers to review and eventually correct the behavior.
- Handle the situation with the large buffer; it may make sense to change the "wait" operation from the end-of-period interrupt to time scheduler and stop the drain more early when the end-of-valid data condition is fulfilled.
i don't understand what you're asking for.
Use jiffies/timeout instead waiting to the interrupt. In this case, the stop may be called earlier (in the middle of period). In this case the silenced area may be much smaller.
Does this difference matter so much? I guess you're concerned about the performance, right? This sounds a bit too complex just for the simple purpose...
- Increase the protocol version.
But as I wrote, I would make those extensions configurable (SNDRV_PCM_HW_PARAMS_DRAIN_ALLOW_SILENCE). It can be turned on by default.
i have no clue what would be involved in doing that. to me that sounds like overkill (solving a non-issue), and goes waaaay beyond what i expected to invest into this issue (really, i just wanted to verify that the emu10k1 fixes work, and accidentally discovered that there is a mid-layer issue that affects user space, as the pyalsaaudio lib i'm using doesn't handle it).
OK. I don't think that it's a pyalsaudio job to resolve the issue with the minimal transfer chunk / period (which you set / know before the transfer is initiated).
I'm thinking whether we need to change anything in the kernel side for this at all. Can't it be changed rather in alsa-lib side instead?
Takashi
On Wed, Apr 12, 2023 at 09:54:54AM +0200, Takashi Iwai wrote:
I'm thinking whether we need to change anything in the kernel side for this at all. Can't it be changed rather in alsa-lib side instead?
it could, but it would be a lot uglier. user space would have to do a "man-in-the-middle attack" on the data, while in the kernel we can just slightly modify the consumer. this would be particularly obvious in the case of write() access.
On Wed, 12 Apr 2023 10:04:31 +0200, Oswald Buddenhagen wrote:
On Wed, Apr 12, 2023 at 09:54:54AM +0200, Takashi Iwai wrote:
I'm thinking whether we need to change anything in the kernel side for this at all. Can't it be changed rather in alsa-lib side instead?
it could, but it would be a lot uglier. user space would have to do a "man-in-the-middle attack" on the data, while in the kernel we can just slightly modify the consumer. this would be particularly obvious in the case of write() access.
But basically it'd be like fiddling sw_params temporarily for draining, I suppose? And the "attack" here can be taken too seriously; the whole PCM operation can be somehow interfered if a process may have the access to the PCM device, and changing sw_params itself must not introduce too much trouble.
thanks,
Takashi
On Wed, Apr 12, 2023 at 12:37:56PM +0200, Takashi Iwai wrote:
On Wed, 12 Apr 2023 10:04:31 +0200, Oswald Buddenhagen wrote:
On Wed, Apr 12, 2023 at 09:54:54AM +0200, Takashi Iwai wrote:
I'm thinking whether we need to change anything in the kernel side for this at all. Can't it be changed rather in alsa-lib side instead?
it could, but it would be a lot uglier. user space would have to do a "man-in-the-middle attack" on the data, while in the kernel we can just slightly modify the consumer. this would be particularly obvious in the case of write() access.
But basically it'd be like fiddling sw_params temporarily for draining, I suppose?
err, right - i was still assuming manual padding. i actually tried temporarily changing the params (and pondered introducing "shadow" params) when i was doing the kernel patch, but that was a lot uglier than what i did in the end. i think it would be even worse in user space due to the need to support async operation.
regards
On 12. 04. 23 12:37, Takashi Iwai wrote:
On Wed, 12 Apr 2023 10:04:31 +0200, Oswald Buddenhagen wrote:
On Wed, Apr 12, 2023 at 09:54:54AM +0200, Takashi Iwai wrote:
I'm thinking whether we need to change anything in the kernel side for this at all. Can't it be changed rather in alsa-lib side instead?
it could, but it would be a lot uglier. user space would have to do a "man-in-the-middle attack" on the data, while in the kernel we can just slightly modify the consumer. this would be particularly obvious in the case of write() access.
But basically it'd be like fiddling sw_params temporarily for draining, I suppose? And the "attack" here can be taken too seriously; the whole PCM operation can be somehow interfered if a process may have the access to the PCM device, and changing sw_params itself must not introduce too much trouble.
This looks like a sane proposal, but some drivers does not require the silencing at all, so we can probably skip this step for them (new SNDRV_PCM_INFO_PERFECT_DRAIN flag?).
The other not-yet-discussed option is to just print an warning in alsa-lib that the residue samples may be played (when no silencing / period size align is used). Then introduce a new helper function to setup silencing for the drivers without new SNDRV_PCM_INFO_PERFECT_DRAIN flag set.
Jaroslav
On Wed, 12 Apr 2023 21:59:28 +0200, Jaroslav Kysela wrote:
On 12. 04. 23 12:37, Takashi Iwai wrote:
On Wed, 12 Apr 2023 10:04:31 +0200, Oswald Buddenhagen wrote:
On Wed, Apr 12, 2023 at 09:54:54AM +0200, Takashi Iwai wrote:
I'm thinking whether we need to change anything in the kernel side for this at all. Can't it be changed rather in alsa-lib side instead?
it could, but it would be a lot uglier. user space would have to do a "man-in-the-middle attack" on the data, while in the kernel we can just slightly modify the consumer. this would be particularly obvious in the case of write() access.
But basically it'd be like fiddling sw_params temporarily for draining, I suppose? And the "attack" here can be taken too seriously; the whole PCM operation can be somehow interfered if a process may have the access to the PCM device, and changing sw_params itself must not introduce too much trouble.
This looks like a sane proposal, but some drivers does not require the silencing at all, so we can probably skip this step for them (new SNDRV_PCM_INFO_PERFECT_DRAIN flag?).
Sure, we should apply it only conditionally. Also, we may skip the workaround for applications accessing directly via mmap as default. Also we may provide a flag in asoundrc config, if any, too. The implementation in alsa-lib is more flexible in this regard.
The other not-yet-discussed option is to just print an warning in alsa-lib that the residue samples may be played (when no silencing / period size align is used). Then introduce a new helper function to setup silencing for the drivers without new SNDRV_PCM_INFO_PERFECT_DRAIN flag set.
Hm, I don't think this would be useful. Spewing warnings are rather annoying or confusing for end users, and I bet the old applications wouldn't be fixed even with such annoyance.
thanks,
Takashi
On Thu, Apr 13, 2023 at 07:42:06AM +0200, Takashi Iwai wrote:
On Wed, 12 Apr 2023 21:59:28 +0200, Jaroslav Kysela wrote:
This looks like a sane proposal, but some drivers does not require the silencing at all, so we can probably skip this step for them (new SNDRV_PCM_INFO_PERFECT_DRAIN flag?).
Sure, we should apply it only conditionally.
i don't think the extra complexity is justified. with my improved proposal, we're talking about a cost of filling two periods per draining op, which aren't exactly super-frequent.
Also, we may skip the workaround for applications accessing directly via mmap as default.
no, because one may easily miss that more than one period is required. also, i think that forgetting it entirely is an easy mistake to make, even if it's harder with mmap than with write.
regards
On Thu, 13 Apr 2023 12:16:04 +0200, Oswald Buddenhagen wrote:
On Thu, Apr 13, 2023 at 07:42:06AM +0200, Takashi Iwai wrote:
On Wed, 12 Apr 2023 21:59:28 +0200, Jaroslav Kysela wrote:
This looks like a sane proposal, but some drivers does not require the silencing at all, so we can probably skip this step for them (new SNDRV_PCM_INFO_PERFECT_DRAIN flag?).
Sure, we should apply it only conditionally.
i don't think the extra complexity is justified. with my improved proposal, we're talking about a cost of filling two periods per draining op, which aren't exactly super-frequent.
I'm not much fan of introducing a new flag for that behavior, either.
Also, we may skip the workaround for applications accessing directly via mmap as default.
no, because one may easily miss that more than one period is required. also, i think that forgetting it entirely is an easy mistake to make, even if it's harder with mmap than with write.
I don't agree with that point -- if application wants the access only via mmap (without any write actions via alsa-lib functions), the buffer and data management relies fully on the application itself. Manipulating the data *silently* is no good action for such applications. For them, the drain simply means to stop at the certain point. OTOH, for the explicit write, basically alsa-lib / kernel is responsible for the buffer / data management, and the workaround can be applied.
That's I mentioned to "apply conditionally". There are cases where we shouldn't touch the data at all. For the usual cases with the standard write, we may apply it.
thanks,
Takashi
On Thu, Apr 13, 2023 at 12:28:34PM +0200, Takashi Iwai wrote:
On Thu, 13 Apr 2023 12:16:04 +0200, Oswald Buddenhagen wrote:
On Thu, Apr 13, 2023 at 07:42:06AM +0200, Takashi Iwai wrote:
Also, we may skip the workaround for applications accessing directly via mmap as default.
no, because one may easily miss that more than one period is required. also, i think that forgetting it entirely is an easy mistake to make, even if it's harder with mmap than with write.
I don't agree with that point -- if application wants the access only via mmap (without any write actions via alsa-lib functions), the buffer and data management relies fully on the application itself. Manipulating the data *silently* is no good action for such applications.
For them, the drain simply means to stop at the certain point.
i don't think that's true. if an app wants to control things finely, it would just use start/stop and manage the timing itself. draining otoh is a convenient fire-and-forget operation. that makes it easy to miss the finer details, which is why i'm so insistent that it should just work out of the box.
if you exclude mmapped devices in kernel, you exclude plughw with emulated write(), so you'd have to add yet more code to compensate for that. and doing it all in user space is yet more code. for all i can tell, it's really just layers of complexity to solve a non-problem.
regards
On Thu, 13 Apr 2023 13:10:34 +0200, Oswald Buddenhagen wrote:
On Thu, Apr 13, 2023 at 12:28:34PM +0200, Takashi Iwai wrote:
On Thu, 13 Apr 2023 12:16:04 +0200, Oswald Buddenhagen wrote:
On Thu, Apr 13, 2023 at 07:42:06AM +0200, Takashi Iwai wrote:
Also, we may skip the workaround for applications accessing directly via mmap as default. no, because one may easily miss that more than one period is
required. also, i think that forgetting it entirely is an easy mistake to make, even if it's harder with mmap than with write.
I don't agree with that point -- if application wants the access only via mmap (without any write actions via alsa-lib functions), the buffer and data management relies fully on the application itself. Manipulating the data *silently* is no good action for such applications.
For them, the drain simply means to stop at the certain point.
i don't think that's true. if an app wants to control things finely, it would just use start/stop and manage the timing itself. draining otoh is a convenient fire-and-forget operation. that makes it easy to miss the finer details, which is why i'm so insistent that it should just work out of the box.
Sure, but that's still no excuse to ignore the possibility blindly.
if you exclude mmapped devices in kernel, you exclude plughw with emulated write(), so you'd have to add yet more code to compensate for that.
No, I wrote "if application wants the access only via mmap (without any write actions via alsa-lib functions)". So if application writes via plugin write(), we should apply the workaround, too.
and doing it all in user space is yet more code. for all i can tell, it's really just layers of complexity to solve a non-problem.
I don't get it: we're talking about the sw_params call in alsa-lib's drain function, and how can it be *so* complex...?
Takashi
On Thu, Apr 13, 2023 at 02:06:49PM +0200, Takashi Iwai wrote:
On Thu, 13 Apr 2023 13:10:34 +0200, Oswald Buddenhagen wrote:
i don't think that's true. if an app wants to control things finely, it would just use start/stop and manage the timing itself. draining otoh is a convenient fire-and-forget operation. that makes it easy to miss the finer details, which is why i'm so insistent that it should just work out of the box.
Sure, but that's still no excuse to ignore the possibility blindly.
it's not blindly. it's after considering it, and concluding that it's a hypothetical problem at best.
we could of course do a survey of actually existing publicly accessible code, to quantify the trade-off between apps fixed and apps broken. i actually sort of tried that ...
first thing is that i found lots of stackoverflow answers and similar, and *none* of them even mentioned the need to clear the rest of the buffer. i found a bunch of libs, and none of the apidocs mentioned it in the parts i read. i found one cross-platform how-to which did actually mention it. yay.
code search was trickier, with rather predictable results: basically all hits for drain() were immediately followed by close(). i found some simple cases of write+drain, and none of them did any additional padding. this includes alsa's own pcm example. but never mind, we're in agreement about this case. most other code was some abstraction, so it would be impossible to asses the bigger picture quickly. that would be even more the case for apps that use mmap. so i won't even try to provide actual data. one thing to consider here is that most of the code are cross-platform abstractions. under such circumstances, it seems kinda inconceivable that the higher level code would make any assumptions about buffer space that has not been filled with fresh samples.
and doing it all in user space is yet more code. for all i can tell, it's really just layers of complexity to solve a non-problem.
I don't get it: we're talking about the sw_params call in alsa-lib's drain function, and how can it be *so* complex...?
the "drain function" bit is critical here, because it kind of implies resetting it, potentially asynchronously. but if we add a specific bit to the kernel to enable it, then it can be actually set already when the device is set up, and the user space would be rather simple. however, that would overall be still a lot more code than doing it unconditionally, and fully in kernel.
regards
On Thu, 13 Apr 2023 16:59:02 +0200, Oswald Buddenhagen wrote:
On Thu, Apr 13, 2023 at 02:06:49PM +0200, Takashi Iwai wrote:
On Thu, 13 Apr 2023 13:10:34 +0200, Oswald Buddenhagen wrote:
i don't think that's true. if an app wants to control things finely, it would just use start/stop and manage the timing itself. draining otoh is a convenient fire-and-forget operation. that makes it easy to miss the finer details, which is why i'm so insistent that it should just work out of the box.
Sure, but that's still no excuse to ignore the possibility blindly.
it's not blindly. it's after considering it, and concluding that it's a hypothetical problem at best.
we could of course do a survey of actually existing publicly accessible code, to quantify the trade-off between apps fixed and apps broken. i actually sort of tried that ...
first thing is that i found lots of stackoverflow answers and similar, and *none* of them even mentioned the need to clear the rest of the buffer. i found a bunch of libs, and none of the apidocs mentioned it in the parts i read. i found one cross-platform how-to which did actually mention it. yay.
code search was trickier, with rather predictable results: basically all hits for drain() were immediately followed by close(). i found some simple cases of write+drain, and none of them did any additional padding. this includes alsa's own pcm example. but never mind, we're in agreement about this case. most other code was some abstraction, so it would be impossible to asses the bigger picture quickly. that would be even more the case for apps that use mmap. so i won't even try to provide actual data. one thing to consider here is that most of the code are cross-platform abstractions. under such circumstances, it seems kinda inconceivable that the higher level code would make any assumptions about buffer space that has not been filled with fresh samples.
Those whole context should have been mentioned before the discussion... But still we'd better survey the actual usage for the decision.
ATM, I still hesitate taking the behavior change in the kernel, *iff* it can be worked around differently. While the mmap situation is admittedly a corner case, the point of alsa-lib implementation is the flexibility. And, your implementation means to modify the mmapped data silently, which never happened in the past -- this is another side of coin of fixing in API side, and we don't know the side-effect to any wild applications. Some additional configuration or flexible workaround might be required, and that's often harder to apply in the kernel.
And, another concern is, as already discussed, whether we really have to apply it unconditionally at every draining call. Obviously the workaround is superfluous for the drivers like USB-audio, which never overrun without the filled data.
and doing it all in user space is yet more code. for all i can tell, it's really just layers of complexity to solve a non-problem.
I don't get it: we're talking about the sw_params call in alsa-lib's drain function, and how can it be *so* complex...?
the "drain function" bit is critical here, because it kind of implies resetting it, potentially asynchronously. but if we add a specific bit to the kernel to enable it, then it can be actually set already when the device is set up, and the user space would be rather simple. however, that would overall be still a lot more code than doing it unconditionally, and fully in kernel.
Indeed we might want to take the kernel-side fix in the end, but let's check things a bit more beforehand.
BTW, I guess that one missing piece in your patch is the case where the drain is called at the moment of fully filled data. You added snd_pcm_playback_silence() at snd_pcm_do_drain_init(), but in this scenario, the call wouldn't do anything at this moment. But snd_pcm_playback_silence() won't be called afterwards because runtime->silence_size = 0. So this workaround won't take effect in that case, no?
thanks,
Takashi
On Fri, Apr 14, 2023 at 10:26:26AM +0200, Takashi Iwai wrote:
Indeed we might want to take the kernel-side fix in the end, but let's check things a bit more beforehand.
i'll post updated patches soon, then you can sleep over it. :-D
(i'm being a bit slow, because i'm also developing tooling for maintaining stacked branches and (re-)posting them, and of course i want to try it out with real data.)
BTW, I guess that one missing piece in your patch is the case where the drain is called at the moment of fully filled data. You added snd_pcm_playback_silence() at snd_pcm_do_drain_init(), but in this scenario, the call wouldn't do anything at this moment. But snd_pcm_playback_silence() won't be called afterwards because runtime->silence_size = 0. So this workaround won't take effect in that case, no?
the hunk in snd_pcm_update_hw_ptr0() should take care of that.
regards
On Fri, 14 Apr 2023 10:56:10 +0200, Oswald Buddenhagen wrote:
On Fri, Apr 14, 2023 at 10:26:26AM +0200, Takashi Iwai wrote:
BTW, I guess that one missing piece in your patch is the case where the drain is called at the moment of fully filled data. You added snd_pcm_playback_silence() at snd_pcm_do_drain_init(), but in this scenario, the call wouldn't do anything at this moment. But snd_pcm_playback_silence() won't be called afterwards because runtime->silence_size = 0. So this workaround won't take effect in that case, no?
the hunk in snd_pcm_update_hw_ptr0() should take care of that.
OK, I see. Thanks.
Takashi
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 05. 04. 23 22:12, 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.
I don't follow what you refer here. The old code uses snd_pcm_playback_hw_avail() thus new hw_ptr for the threshold mode, too.
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(-)
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 whennoise 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-silencersnd_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
I would use traditional C-style comment style here (to match other descriptions).
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:
Traditional C-style comments.
- 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 */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;
If hw_avail is above runtime->boundary then the initial condition is totaly bogus. I would use snd_BUG_ON() and direct return here.
- 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;elseruntime->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;
...
if (noise_dist >= (snd_pcm_sframes_t) runtime->silence_threshold)return;
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;
if (frames > runtime->silence_size) frames = runtime->silence_size;} else {
if (new_hw_ptr == ULONG_MAX) { /* initialization */
...
frames = runtime->buffer_size - runtime->silence_filled;
frames = runtime->buffer_size - noise_dist;if ((snd_pcm_sframes_t) frames <= 0)return;
Similar thing, move the check before the frames assignment.
Jaroslav
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
participants (3)
-
Jaroslav Kysela -
Oswald Buddenhagen -
Takashi Iwai