diff mbox series

[2/2] ALSA: pcm: auto-fill buffer with silence when draining playback

Message ID 20230405201219.2197789-2-oswald.buddenhagen@gmx.de (mailing list archive)
State Superseded
Headers show
Series [1/2] ALSA: pcm: rewrite snd_pcm_playback_silence() | expand

Commit Message

Oswald Buddenhagen April 5, 2023, 8:12 p.m. UTC
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(-)

Comments

Jaroslav Kysela April 7, 2023, 11:58 p.m. UTC | #1
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
Takashi Iwai April 8, 2023, 5:55 a.m. UTC | #2
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
Oswald Buddenhagen April 8, 2023, 7:24 a.m. UTC | #3
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
Jaroslav Kysela April 11, 2023, 11:09 a.m. UTC | #4
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
Oswald Buddenhagen April 11, 2023, 1:57 p.m. UTC | #5
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
Jaroslav Kysela April 11, 2023, 2:48 p.m. UTC | #6
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
Oswald Buddenhagen April 11, 2023, 4:50 p.m. UTC | #7
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
Jaroslav Kysela April 11, 2023, 5:23 p.m. UTC | #8
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
Takashi Iwai April 12, 2023, 7:54 a.m. UTC | #9
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
Oswald Buddenhagen April 12, 2023, 8:04 a.m. UTC | #10
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.
Takashi Iwai April 12, 2023, 10:37 a.m. UTC | #11
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
Oswald Buddenhagen April 12, 2023, 11:38 a.m. UTC | #12
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
Jaroslav Kysela April 12, 2023, 7:59 p.m. UTC | #13
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
Takashi Iwai April 13, 2023, 5:42 a.m. UTC | #14
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
Oswald Buddenhagen April 13, 2023, 10:16 a.m. UTC | #15
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
Takashi Iwai April 13, 2023, 10:28 a.m. UTC | #16
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
Oswald Buddenhagen April 13, 2023, 11:10 a.m. UTC | #17
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
Takashi Iwai April 13, 2023, 12:06 p.m. UTC | #18
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
Oswald Buddenhagen April 13, 2023, 2:59 p.m. UTC | #19
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
Takashi Iwai April 14, 2023, 8:26 a.m. UTC | #20
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
Oswald Buddenhagen April 14, 2023, 8:56 a.m. UTC | #21
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
Takashi Iwai April 14, 2023, 9:28 a.m. UTC | #22
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
diff mbox series

Patch

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);