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