Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
'Twas brillig, and David Henningsson at 14/09/10 07:28 did gyre and gimble: Perhaps the problem is that /bin/sh is not actually bash on your system? It is dash on Ubuntu systems. Yeah I think I remember someone saying that before which is what made me think of this as the problem :) Perhaps just changing the first line to: #!/bin/bash would cause it to tidy things up properly? Yes, that worked, thanks. Cool. OK, sounds reasonable. Do you think the patch I posted is OK with the 1330 time? I think it is good enough for now. If it turns out to be too little, we can adjust it later. OK, I've pushed patch to both master and stable queue now. I guess it's not super important as if it solves your original problem that kicked off this whole thread, then that's the main thing!! That PA can handle a stress test is important, but it's a different issue. I found another issue that perhaps could have caused snd_pcm_start() to not be called in alsa-sink, but it seems unrelated to this issue. I think it's still a valid fix tho', so have pushed that too. I think I'm quite happy with stable-queue just now. I'll try and do some more stress testing in due course, but failing any further issues, I think we can just call this 0.9.22 and be done with it (until the next set of fixes come in ;)) Col -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited [http://www.tribalogic.net/] Open Source: Mandriva Linux Contributor [http://www.mandriva.com/] PulseAudio Hacker [http://www.pulseaudio.org/] Trac Hacker [http://trac.edgewall.org/] ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
On 2010-09-13 13:03, Colin Guthrie wrote: 'Twas brillig, and David Henningsson at 13/09/10 11:14 did gyre and gimble: On 2010-09-04 14:10, Colin Guthrie wrote: I'd be interested as to whether anyone else can repeat this experiment and get similar results. Do you guys get a broken chordtest too (it's on the RedHat bug I mentioned at the beginning of this thread)? I have now tried to repeat the experiment. The chordtest.sh seems to be buggy in itself (the cleanup does not remove the gst-launch, which in turn had to be renamed to gst-launch-0.10 here). Yeah I have gst-launch-0.10 here too... not quite sure why, I'd have thought we could ditch the old 0.8 support by now but hey ho. (I don't follow gst dev super closely) I thought that the script trapped ctrl+c and killed any processes started. It seems to be clean for me. Perhaps the problem is that /bin/sh is not actually bash on your system? It is dash on Ubuntu systems. Perhaps just changing the first line to: #!/bin/bash would cause it to tidy things up properly? Yes, that worked, thanks. Anyway, the results were not encouraging - with tsched=0, pavucontrol, and - to syslog on, three tones were heard, then things went quiet - however, pulseaudio started to eat more and more memory. Quickly my machine started swapping and became unresponsive, so I killed PA. Besides that, when I looked at pavucontrol, only the meters of the first three were moving, the other ones were silent. My log got filled up with memblockq: pool full as well. I'm getting the feeling that this problem is something different, unrelated to DMA controller hardware. Interesting, can't say I noticed this, but I probably wasn't looking closely enough. My suggestion is that you should commit your proposed patch as it improves the situation compared to the current situation. If there are additional problems, let's nail them down separately. OK, sounds reasonable. Do you think the patch I posted is OK with the 1330 time? I think it is good enough for now. If it turns out to be too little, we can adjust it later. I guess it's not super important as if it solves your original problem that kicked off this whole thread, then that's the main thing!! That PA can handle a stress test is important, but it's a different issue. -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
On 2010-09-04 14:10, Colin Guthrie wrote: 'Twas brillig, and David Henningsson at 03/09/10 09:46 did gyre and gimble: 2010-09-02 16:06, pl bossart skrev: Agreed: You can pick those two patches, and then we add a third patch to both branches, which brings back the watermark for tsched devices and 20 ms for non-tsched. Assuming my suspicion is not disproved, of course. What does Pierre think of that? I don't want the watermark to be used for rewinds. The watermark is there for timer-based scheduling, so that you have enough time to wake-up from sleep and still refill the buffer. The rewinds happens when the processor is already awake, pulseaudio up and running and only the remix part needs to happen. Plus the watermark varies and the logic could really be improved. Also I think 20ms for rewinds is way too much. This will kill your actual latency. Imagine you have a low-latency app that starts, the first sample would be heard after at best 20ms. Not acceptable for speech or interactive sounds. But I agree that 256-bytes isn't fool-proof for heavy duty use cases such 8ch 192kHz 32-bit float. So how about we keep 256 bytes (1.33 ms for 48kHz) but add a 1.33 ms threshold to make sure we never rewind below. rewind_safeguard = max(256, pa_usec_to_bytes(1330)); This way you solve both the hardware issue (frequency independent) and leave enough headroom for the system to avoid underflows. Whether 1,33 ms or 20 ms is best - I assume your guess is as good as mine. Colin, feel free to go ahead with Pierre's suggestion - it's likely to be good enough. As for the watermark usage, I admit to not knowing enough of CPU scheduling and wake-up times to either prove Pierre right or wrong. OK, I've done this now. The patch is attached. It's based on stable-queue with the two previous patches cherry-picked first (and also Tanu's 0525807b63c11d3d71526cec553e8d80ad3f09cd which fixed a complier warning, but shouldn't get in the way) However, in testing this I had some problems. Likely this is due to me testing hard/more thoroughly than before. I found that using the attached patch fixed the chordtest.sh case for tsched=0, however, when running pavucontrol at the same time, everything started to go wrong pretty quickly (after two or three streams). When things when wrong, they generally stayed wrong. i.e. ctrl+c on the chordtest.sh kills all the streams, but if I rerun it, then the very first stream is generally cocked up. Interestingly a paplay seemed to work fine. So I changed the 1330 usec to 2 and tried again. This had slightly better results, but still broke the chordtest.sh case after three streams (fairly repeatable) when pavucontrol is running (unsurprisingly it also worked fine when pavucontrol was not running). The difference in this case however was the rerunning the test after an initial failure worked fine. The sound was back to normal on the first stream and only generally cocked up when it hit the third stream. So what does this test mean? pavucontrol obviously affects the latency of the sink due to it's VI meters. This obviously increases the likelihood of a rewind being triggered. So, with this in mind what values do you suggest we pick? I'd be interested as to whether anyone else can repeat this experiment and get similar results. Do you guys get a broken chordtest too (it's on the RedHat bug I mentioned at the beginning of this thread)? I have now tried to repeat the experiment. The chordtest.sh seems to be buggy in itself (the cleanup does not remove the gst-launch, which in turn had to be renamed to gst-launch-0.10 here). Anyway, the results were not encouraging - with tsched=0, pavucontrol, and - to syslog on, three tones were heard, then things went quiet - however, pulseaudio started to eat more and more memory. Quickly my machine started swapping and became unresponsive, so I killed PA. Besides that, when I looked at pavucontrol, only the meters of the first three were moving, the other ones were silent. My log got filled up with memblockq: pool full as well. I'm getting the feeling that this problem is something different, unrelated to DMA controller hardware. My suggestion is that you should commit your proposed patch as it improves the situation compared to the current situation. If there are additional problems, let's nail them down separately. -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
'Twas brillig, and David Henningsson at 13/09/10 11:14 did gyre and gimble: On 2010-09-04 14:10, Colin Guthrie wrote: 'Twas brillig, and David Henningsson at 03/09/10 09:46 did gyre and gimble: 2010-09-02 16:06, pl bossart skrev: Agreed: You can pick those two patches, and then we add a third patch to both branches, which brings back the watermark for tsched devices and 20 ms for non-tsched. Assuming my suspicion is not disproved, of course. What does Pierre think of that? I don't want the watermark to be used for rewinds. The watermark is there for timer-based scheduling, so that you have enough time to wake-up from sleep and still refill the buffer. The rewinds happens when the processor is already awake, pulseaudio up and running and only the remix part needs to happen. Plus the watermark varies and the logic could really be improved. Also I think 20ms for rewinds is way too much. This will kill your actual latency. Imagine you have a low-latency app that starts, the first sample would be heard after at best 20ms. Not acceptable for speech or interactive sounds. But I agree that 256-bytes isn't fool-proof for heavy duty use cases such 8ch 192kHz 32-bit float. So how about we keep 256 bytes (1.33 ms for 48kHz) but add a 1.33 ms threshold to make sure we never rewind below. rewind_safeguard = max(256, pa_usec_to_bytes(1330)); This way you solve both the hardware issue (frequency independent) and leave enough headroom for the system to avoid underflows. Whether 1,33 ms or 20 ms is best - I assume your guess is as good as mine. Colin, feel free to go ahead with Pierre's suggestion - it's likely to be good enough. As for the watermark usage, I admit to not knowing enough of CPU scheduling and wake-up times to either prove Pierre right or wrong. OK, I've done this now. The patch is attached. It's based on stable-queue with the two previous patches cherry-picked first (and also Tanu's 0525807b63c11d3d71526cec553e8d80ad3f09cd which fixed a complier warning, but shouldn't get in the way) However, in testing this I had some problems. Likely this is due to me testing hard/more thoroughly than before. I found that using the attached patch fixed the chordtest.sh case for tsched=0, however, when running pavucontrol at the same time, everything started to go wrong pretty quickly (after two or three streams). When things when wrong, they generally stayed wrong. i.e. ctrl+c on the chordtest.sh kills all the streams, but if I rerun it, then the very first stream is generally cocked up. Interestingly a paplay seemed to work fine. So I changed the 1330 usec to 2 and tried again. This had slightly better results, but still broke the chordtest.sh case after three streams (fairly repeatable) when pavucontrol is running (unsurprisingly it also worked fine when pavucontrol was not running). The difference in this case however was the rerunning the test after an initial failure worked fine. The sound was back to normal on the first stream and only generally cocked up when it hit the third stream. So what does this test mean? pavucontrol obviously affects the latency of the sink due to it's VI meters. This obviously increases the likelihood of a rewind being triggered. So, with this in mind what values do you suggest we pick? I'd be interested as to whether anyone else can repeat this experiment and get similar results. Do you guys get a broken chordtest too (it's on the RedHat bug I mentioned at the beginning of this thread)? I have now tried to repeat the experiment. The chordtest.sh seems to be buggy in itself (the cleanup does not remove the gst-launch, which in turn had to be renamed to gst-launch-0.10 here). Yeah I have gst-launch-0.10 here too... not quite sure why, I'd have thought we could ditch the old 0.8 support by now but hey ho. (I don't follow gst dev super closely) I thought that the script trapped ctrl+c and killed any processes started. It seems to be clean for me. Perhaps the problem is that /bin/sh is not actually bash on your system? Perhaps just changing the first line to: #!/bin/bash would cause it to tidy things up properly? Anyway, the results were not encouraging - with tsched=0, pavucontrol, and - to syslog on, three tones were heard, then things went quiet - however, pulseaudio started to eat more and more memory. Quickly my machine started swapping and became unresponsive, so I killed PA. Besides that, when I looked at pavucontrol, only the meters of the first three were moving, the other ones were silent. My log got filled up with memblockq: pool full as well. I'm getting the feeling that this problem is something different, unrelated to DMA controller hardware. Interesting, can't say I noticed this, but I probably wasn't looking closely enough. My suggestion is that you should commit your proposed patch as it improves the situation compared to the current situation. If there are additional
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
On 2010-09-08 02:52, pl bossart wrote: So what does this test mean? pavucontrol obviously affects the latency of the sink due to it's VI meters. This obviously increases the likelihood of a rewind being triggered.So, with this in mind what values do you suggest we pick? Pierre, is it your understanding that it is when DMA transfer collides with cpu-RAM transfer that makes the DMA stream to become broken permanently? Or exactly what is it that makes it break? The low-latency setting has nothing to do with rewinds. Indirectly it is - if the rewind margin is greater than the actual latency there won't be any rewind. It's when you actually change the volume that you will rewind and remix with the new stream volume. You could set a super low latency and rewind exactly once when the stream starts. There are two issues with the rewind. One is a race condition between the DMA and the ring buffer update. If you rewind and update the ring buffer, the DMA may actually have already fetched older data. To fetch the old data is not a problem IMO, that gives us just a ms more of the old data and we might miss a ms of the new data. That's not optimal, but not a complete disaster either. What is the big problem here, is when we get a persistent crackling (even when everything has straightened out after the rewind!), and if I understand you correctly, that has something to do with the DMA controller...? Could you elaborate on what conditions it is that cause the persistent crackling? The second is that you could experience underflows if you don't do the update fast enough. By enabling logs you should be able to find out if there are real underflows. Exactly. -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
On 2010-09-04 14:10, Colin Guthrie wrote: 'Twas brillig, and David Henningsson at 03/09/10 09:46 did gyre and gimble: 2010-09-02 16:06, pl bossart skrev: Agreed: You can pick those two patches, and then we add a third patch to both branches, which brings back the watermark for tsched devices and 20 ms for non-tsched. Assuming my suspicion is not disproved, of course. What does Pierre think of that? I don't want the watermark to be used for rewinds. The watermark is there for timer-based scheduling, so that you have enough time to wake-up from sleep and still refill the buffer. The rewinds happens when the processor is already awake, pulseaudio up and running and only the remix part needs to happen. Plus the watermark varies and the logic could really be improved. Also I think 20ms for rewinds is way too much. This will kill your actual latency. Imagine you have a low-latency app that starts, the first sample would be heard after at best 20ms. Not acceptable for speech or interactive sounds. But I agree that 256-bytes isn't fool-proof for heavy duty use cases such 8ch 192kHz 32-bit float. So how about we keep 256 bytes (1.33 ms for 48kHz) but add a 1.33 ms threshold to make sure we never rewind below. rewind_safeguard = max(256, pa_usec_to_bytes(1330)); This way you solve both the hardware issue (frequency independent) and leave enough headroom for the system to avoid underflows. Whether 1,33 ms or 20 ms is best - I assume your guess is as good as mine. Colin, feel free to go ahead with Pierre's suggestion - it's likely to be good enough. As for the watermark usage, I admit to not knowing enough of CPU scheduling and wake-up times to either prove Pierre right or wrong. OK, I've done this now. The patch is attached. It's based on stable-queue with the two previous patches cherry-picked first (and also Tanu's 0525807b63c11d3d71526cec553e8d80ad3f09cd which fixed a complier warning, but shouldn't get in the way) However, in testing this I had some problems. Likely this is due to me testing hard/more thoroughly than before. I found that using the attached patch fixed the chordtest.sh case for tsched=0, however, when running pavucontrol at the same time, everything started to go wrong pretty quickly (after two or three streams). When things when wrong, they generally stayed wrong. i.e. ctrl+c on the chordtest.sh kills all the streams, but if I rerun it, then the very first stream is generally cocked up. Interestingly a paplay seemed to work fine. So I changed the 1330 usec to 2 and tried again. This had slightly better results, but still broke the chordtest.sh case after three streams (fairly repeatable) when pavucontrol is running (unsurprisingly it also worked fine when pavucontrol was not running). The difference in this case however was the rerunning the test after an initial failure worked fine. The sound was back to normal on the first stream and only generally cocked up when it hit the third stream. I'm just guessing here, but I'm not sure you're experiencing the same problem then. It could be that you're having underruns from too high CPU usage instead, or something. And with tsched=0, we can't compensate by increasing the watermark. So what does this test mean? pavucontrol obviously affects the latency of the sink due to it's VI meters. This obviously increases the likelihood of a rewind being triggered.So, with this in mind what values do you suggest we pick? Pierre, is it your understanding that it is when DMA transfer collides with cpu-RAM transfer that makes the DMA stream to become broken permanently? Or exactly what is it that makes it break? I'd be interested as to whether anyone else can repeat this experiment and get similar results. Unfortunately I haven't got around to test it yet :-( -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
So what does this test mean? pavucontrol obviously affects the latency of the sink due to it's VI meters. This obviously increases the likelihood of a rewind being triggered.So, with this in mind what values do you suggest we pick? Pierre, is it your understanding that it is when DMA transfer collides with cpu-RAM transfer that makes the DMA stream to become broken permanently? Or exactly what is it that makes it break? The low-latency setting has nothing to do with rewinds. It's when you actually change the volume that you will rewind and remix with the new stream volume. You could set a super low latency and rewind exactly once when the stream starts. There are two issues with the rewind. One is a race condition between the DMA and the ring buffer update. If you rewind and update the ring buffer, the DMA may actually have already fetched older data. The second is that you could experience underflows if you don't do the update fast enough. By enabling logs you should be able to find out if there are real underflows. -Pierre ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
'Twas brillig, and David Henningsson at 03/09/10 09:46 did gyre and gimble: 2010-09-02 16:06, pl bossart skrev: Agreed: You can pick those two patches, and then we add a third patch to both branches, which brings back the watermark for tsched devices and 20 ms for non-tsched. Assuming my suspicion is not disproved, of course. What does Pierre think of that? I don't want the watermark to be used for rewinds. The watermark is there for timer-based scheduling, so that you have enough time to wake-up from sleep and still refill the buffer. The rewinds happens when the processor is already awake, pulseaudio up and running and only the remix part needs to happen. Plus the watermark varies and the logic could really be improved. Also I think 20ms for rewinds is way too much. This will kill your actual latency. Imagine you have a low-latency app that starts, the first sample would be heard after at best 20ms. Not acceptable for speech or interactive sounds. But I agree that 256-bytes isn't fool-proof for heavy duty use cases such 8ch 192kHz 32-bit float. So how about we keep 256 bytes (1.33 ms for 48kHz) but add a 1.33 ms threshold to make sure we never rewind below. rewind_safeguard = max(256, pa_usec_to_bytes(1330)); This way you solve both the hardware issue (frequency independent) and leave enough headroom for the system to avoid underflows. Whether 1,33 ms or 20 ms is best - I assume your guess is as good as mine. Colin, feel free to go ahead with Pierre's suggestion - it's likely to be good enough. As for the watermark usage, I admit to not knowing enough of CPU scheduling and wake-up times to either prove Pierre right or wrong. OK, I've done this now. The patch is attached. It's based on stable-queue with the two previous patches cherry-picked first (and also Tanu's 0525807b63c11d3d71526cec553e8d80ad3f09cd which fixed a complier warning, but shouldn't get in the way) However, in testing this I had some problems. Likely this is due to me testing hard/more thoroughly than before. I found that using the attached patch fixed the chordtest.sh case for tsched=0, however, when running pavucontrol at the same time, everything started to go wrong pretty quickly (after two or three streams). When things when wrong, they generally stayed wrong. i.e. ctrl+c on the chordtest.sh kills all the streams, but if I rerun it, then the very first stream is generally cocked up. Interestingly a paplay seemed to work fine. So I changed the 1330 usec to 2 and tried again. This had slightly better results, but still broke the chordtest.sh case after three streams (fairly repeatable) when pavucontrol is running (unsurprisingly it also worked fine when pavucontrol was not running). The difference in this case however was the rerunning the test after an initial failure worked fine. The sound was back to normal on the first stream and only generally cocked up when it hit the third stream. So what does this test mean? pavucontrol obviously affects the latency of the sink due to it's VI meters. This obviously increases the likelihood of a rewind being triggered. So, with this in mind what values do you suggest we pick? I'd be interested as to whether anyone else can repeat this experiment and get similar results. Do you guys get a broken chordtest too (it's on the RedHat bug I mentioned at the beginning of this thread)? Col -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited [http://www.tribalogic.net/] Open Source: Mandriva Linux Contributor [http://www.mandriva.com/] PulseAudio Hacker [http://www.pulseaudio.org/] Trac Hacker [http://trac.edgewall.org/] From a65825fa45a1c393899eeb1e4532236964ea3a2c Mon Sep 17 00:00:00 2001 From: Colin Guthrie cguth...@mandriva.org Date: Sat, 4 Sep 2010 11:58:05 +0100 Subject: [PATCH] alsa: Set the rewind safeguard proportionally to sample spec Currently when rewinding alsa, a fixed value of 256 bytes is used, which represents 1.33ms @ 48kHz (2ch, 16bit). This is typically fine and due to DMA constraints we would not want to rewind less than this. However with more demanding sample specs, (e.g. 8ch 192kHz 32bit) 256 bytes is likely not sufficient, so calculate what 1.33ms would be and use which ever value is bigger. Discussed with David Henningsson and Pierre-Louis Bossart here: http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/7286 --- src/modules/alsa/alsa-sink.c |5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c index 9e200bd..4a5fe3f 100644 --- a/src/modules/alsa/alsa-sink.c +++ b/src/modules/alsa/alsa-sink.c @@ -83,7 +83,8 @@ #define VOLUME_ACCURACY (PA_VOLUME_NORM/100) /* don't require volume adjustments to be perfectly correct. don't necessarily extend granularity in software unless the differences get greater than this level */ -#define DEFAULT_REWIND_SAFEGUARD_BYTES (256) /* 1.33ms
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
2010-09-02 16:06, pl bossart skrev: Agreed: You can pick those two patches, and then we add a third patch to both branches, which brings back the watermark for tsched devices and 20 ms for non-tsched. Assuming my suspicion is not disproved, of course. What does Pierre think of that? I don't want the watermark to be used for rewinds. The watermark is there for timer-based scheduling, so that you have enough time to wake-up from sleep and still refill the buffer. The rewinds happens when the processor is already awake, pulseaudio up and running and only the remix part needs to happen. Plus the watermark varies and the logic could really be improved. Also I think 20ms for rewinds is way too much. This will kill your actual latency. Imagine you have a low-latency app that starts, the first sample would be heard after at best 20ms. Not acceptable for speech or interactive sounds. But I agree that 256-bytes isn't fool-proof for heavy duty use cases such 8ch 192kHz 32-bit float. So how about we keep 256 bytes (1.33 ms for 48kHz) but add a 1.33 ms threshold to make sure we never rewind below. rewind_safeguard = max(256, pa_usec_to_bytes(1330)); This way you solve both the hardware issue (frequency independent) and leave enough headroom for the system to avoid underflows. Whether 1,33 ms or 20 ms is best - I assume your guess is as good as mine. Colin, feel free to go ahead with Pierre's suggestion - it's likely to be good enough. As for the watermark usage, I admit to not knowing enough of CPU scheduling and wake-up times to either prove Pierre right or wrong. -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
2010-09-01 20:06, pl bossart skrev:
Probably either one will work, but if we're about to release 0.9.22
(heard something from Lennart yet?), I suggest we go with my version for
0.9.22 as that one is the least invasive (only touches non-tsched
devices), and keep Pierre's version in master.
Sounds reasonable. Pierre, what's your take?
That would mean an additional post-release patch for tsched devices. I
am lazy and would prefer a one-stop fix. I don't care if this is
David's or mine, as long as the solution works for both cases.
The only real difference is the bytes/ms parameter. Although ms are
more intuitive, the bytes makes more sense from a hardware point of
view. If you pass a parameter in ms, there might be cases where the
actual number of bytes is lower than the DMA burst, it'll depend on
what frequency the sink operates at. There are some cases where
alsa-sink works at 8kHz (BT-SCO) or 48kHz (all other cases), a 6x
variability in the rewind behavior is difficult to handle.
Fair enough, how about the attached compromise (untested)? If you then
would like to turn the define of dma_rewind_margin_bytes into a
parameter, that should be fairly simple.
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
From 5ae6c880e20adfa0f372f2caf293e70253651c5a Mon Sep 17 00:00:00 2001
From: David Henningsson david.hennings...@canonical.com
Date: Thu, 2 Sep 2010 08:22:36 +0200
Subject: [PATCH] ALSA: Fix calculation of rewind margins
For non-tsched devices, use 20 ms as a default rewind margin.
Also make sure that we have at least 256 bytes as rewind margin for
all devices, to avoid problems with DMA controllers.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
src/modules/alsa/alsa-sink.c | 13 -
1 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
index a253cc5..097bb20 100644
--- a/src/modules/alsa/alsa-sink.c
+++ b/src/modules/alsa/alsa-sink.c
@@ -63,6 +63,9 @@
#define DEFAULT_DEVICE default
+#define NON_TSCHED_REWIND_MARGIN_USEC (20*PA_USEC_PER_MSEC)/* 20ms -- Default rewind margin for non-tsched devices */
+#define DMA_REWIND_MARGIN_BYTES (256) /* Make sure we're always have a rewind margin of at least 256 bytes - to avoid problems with some DMA controllers */
+
#define DEFAULT_TSCHED_BUFFER_USEC (2*PA_USEC_PER_SEC) /* 2s-- Overall buffer size */
#define DEFAULT_TSCHED_WATERMARK_USEC (20*PA_USEC_PER_MSEC)/* 20ms -- Fill up when only this much is left in the buffer */
@@ -1309,7 +1312,15 @@ static int process_rewind(struct userdata *u) {
return -1;
}
-unused_nbytes = u-tsched_watermark + (size_t) unused * u-frame_size;
+
+if (u-use_tsched)
+unused_nbytes = u-tsched_watermark;
+else
+unused_nbytes = pa_usec_to_bytes(NON_TSCHED_REWIND_MARGIN_USEC, u-sink-sample_spec);
+if (unused_nbytes DMA_REWIND_MARGIN_BYTES)
+unused_nbytes = DMA_REWIND_MARGIN_BYTES;
+
+unused_nbytes += (size_t) unused * u-frame_size;
if (u-hwbuf_size unused_nbytes)
limit_nbytes = u-hwbuf_size - unused_nbytes;
--
1.7.0.4
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
'Twas brillig, and David Henningsson at 02/09/10 07:29 did gyre and gimble: 2010-09-01 20:06, pl bossart skrev: Probably either one will work, but if we're about to release 0.9.22 (heard something from Lennart yet?), I suggest we go with my version for 0.9.22 as that one is the least invasive (only touches non-tsched devices), and keep Pierre's version in master. Sounds reasonable. Pierre, what's your take? That would mean an additional post-release patch for tsched devices. I am lazy and would prefer a one-stop fix. I don't care if this is David's or mine, as long as the solution works for both cases. The only real difference is the bytes/ms parameter. Although ms are more intuitive, the bytes makes more sense from a hardware point of view. If you pass a parameter in ms, there might be cases where the actual number of bytes is lower than the DMA burst, it'll depend on what frequency the sink operates at. There are some cases where alsa-sink works at 8kHz (BT-SCO) or 48kHz (all other cases), a 6x variability in the rewind behavior is difficult to handle. Fair enough, how about the attached compromise (untested)? If you then would like to turn the define of dma_rewind_margin_bytes into a parameter, that should be fairly simple. That patch works fine too with the chordtest test. In order to do more testing however, I also tried the following two cherry picks to stable-queue _instead_ of your patch: commit d11cd33e3aff14fdd66826b3252d90b1b0e38c48 Author: Lennart Poettering lenn...@poettering.net Date: Tue Feb 23 03:23:22 2010 +0100 alsa: don't make use of tsched related variables when tsched is disabled commit 4df443bbe682055a41e7c2248877dcc7682a69b8 Author: Pierre-Louis Bossart pierre-louis.boss...@intel.com Date: Thu Apr 29 10:48:11 2010 -0500 add rewind-safeguard parameter Rewinding the ring buffer completely causes audible issues with DMAs. Previous solution didn't work with tsched=0, and used tsched_watermark for guardband, which isn't linked to hardware and could become really high if underflows occurred. Added separate parameter that can be tuned to hardware limitations and size of DMA bursts. This approach also fixed the chordtest test case. Obviously to fix that test case, I'd rather cherry-pick those two patches than introduce a new patch only on stable-queue. David, as it's obvious that I'm not fully up-to-speed with how this alsa stuff works in any great depth, what do you think your patch adds on top of the above two? My primary concern is that with the current git master version, the u-tsched_watermark is not used at all to calculate unused_nbytes, whereas in your patch it is (the usage of u-tsched_watermark was removed in git master in Pierre's patch above). Now I can see that the general aim in Peirre's change was to prevent rewinding too far, but with your patch it seems to stop it rewinding too little (have I interpreted this right?) and there is no upper limit on the amount rewound (in tsched mode with a massive watermark at least). So is there still something missing in your patch? Can the rewind now be too much for the DMA controller (a problem alluded to in the comments in Pierre's fix). Right, I hope I've asked the right questions here. I'm aware that my not groking things fully isn't helping but I'm also keen to not cock things up :D In order to keep things sensible and prevent divergence, I think it's wise to cherry pick the above two patches to stable-queue and make any further change needed on top of that. This way the patch should apply to git master and stable-queue and I can prevent patch divergence and thus the strange reoccurances of problems post 0.9.22 when we ultimately release git master under whatever version it turns out to be. I think that makes sense. WDYT? Col -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited [http://www.tribalogic.net/] Open Source: Mandriva Linux Contributor [http://www.mandriva.com/] PulseAudio Hacker [http://www.pulseaudio.org/] Trac Hacker [http://trac.edgewall.org/] ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
2010-09-02 10:41, Colin Guthrie skrev: 'Twas brillig, and David Henningsson at 02/09/10 07:29 did gyre and gimble: 2010-09-01 20:06, pl bossart skrev: Probably either one will work, but if we're about to release 0.9.22 (heard something from Lennart yet?), I suggest we go with my version for 0.9.22 as that one is the least invasive (only touches non-tsched devices), and keep Pierre's version in master. Sounds reasonable. Pierre, what's your take? That would mean an additional post-release patch for tsched devices. I am lazy and would prefer a one-stop fix. I don't care if this is David's or mine, as long as the solution works for both cases. The only real difference is the bytes/ms parameter. Although ms are more intuitive, the bytes makes more sense from a hardware point of view. If you pass a parameter in ms, there might be cases where the actual number of bytes is lower than the DMA burst, it'll depend on what frequency the sink operates at. There are some cases where alsa-sink works at 8kHz (BT-SCO) or 48kHz (all other cases), a 6x variability in the rewind behavior is difficult to handle. Fair enough, how about the attached compromise (untested)? If you then would like to turn the define of dma_rewind_margin_bytes into a parameter, that should be fairly simple. That patch works fine too with the chordtest test. In order to do more testing however, I also tried the following two cherry picks to stable-queue _instead_ of your patch: commit d11cd33e3aff14fdd66826b3252d90b1b0e38c48 Author: Lennart Poettering lenn...@poettering.net Date: Tue Feb 23 03:23:22 2010 +0100 alsa: don't make use of tsched related variables when tsched is disabled commit 4df443bbe682055a41e7c2248877dcc7682a69b8 Author: Pierre-Louis Bossart pierre-louis.boss...@intel.com Date: Thu Apr 29 10:48:11 2010 -0500 add rewind-safeguard parameter Rewinding the ring buffer completely causes audible issues with DMAs. Previous solution didn't work with tsched=0, and used tsched_watermark for guardband, which isn't linked to hardware and could become really high if underflows occurred. Added separate parameter that can be tuned to hardware limitations and size of DMA bursts. This approach also fixed the chordtest test case. Obviously to fix that test case, I'd rather cherry-pick those two patches than introduce a new patch only on stable-queue. David, as it's obvious that I'm not fully up-to-speed with how this alsa stuff works in any great depth, what do you think your patch adds on top of the above two? So my only worry, but I might be wrong, is that a fixed limit of 256 bytes in combination with high sample rates (think 8 channels, 192 kHz, 32 bit float, that will make 256 bytes 0,04 ms only), will not be enough. We'll rewind back and before we have time to fill it up again we will get an underrun (and possibly DMA failure?). So that's why I want to see a ms based margin as well as a byte-based one. For tsched devices, the watermark is a dynamic ms-based margin that seems to have been working well (AFAIK), but for non-tsched devices, we have had no margin at all, causing failure. As a summary, Pierre is afraid a ms-based one will rewind too far if the sample rate is low, and I'm afraid a byte-based one will rewind too far if the sample rate is high. My primary concern is that with the current git master version, the u-tsched_watermark is not used at all to calculate unused_nbytes, whereas in your patch it is (the usage of u-tsched_watermark was removed in git master in Pierre's patch above). Now I can see that the general aim in Peirre's change was to prevent rewinding too far, but with your patch it seems to stop it rewinding too little (have I interpreted this right?) and there is no upper limit on the amount rewound (in tsched mode with a massive watermark at least). Both patches aim not to rewind too far, if my one isn't, something is fundamentally wrong with my version. So is there still something missing in your patch? Can the rewind now be too much for the DMA controller (a problem alluded to in the comments in Pierre's fix). Right, I hope I've asked the right questions here. I'm aware that my not groking things fully isn't helping but I'm also keen to not cock things up :D In order to keep things sensible and prevent divergence, I think it's wise to cherry pick the above two patches to stable-queue and make any further change needed on top of that. This way the patch should apply to git master and stable-queue and I can prevent patch divergence and thus the strange reoccurances of problems post 0.9.22 when we ultimately release git master under whatever version it turns out to be. I think that makes sense. WDYT? Agreed: You can pick those two patches, and then we add a third patch to both branches, which brings back the watermark for tsched devices and 20 ms for non-tsched. Assuming my
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
Agreed: You can pick those two patches, and then we add a third patch to both branches, which brings back the watermark for tsched devices and 20 ms for non-tsched. Assuming my suspicion is not disproved, of course. What does Pierre think of that? I don't want the watermark to be used for rewinds. The watermark is there for timer-based scheduling, so that you have enough time to wake-up from sleep and still refill the buffer. The rewinds happens when the processor is already awake, pulseaudio up and running and only the remix part needs to happen. Plus the watermark varies and the logic could really be improved. Also I think 20ms for rewinds is way too much. This will kill your actual latency. Imagine you have a low-latency app that starts, the first sample would be heard after at best 20ms. Not acceptable for speech or interactive sounds. But I agree that 256-bytes isn't fool-proof for heavy duty use cases such 8ch 192kHz 32-bit float. So how about we keep 256 bytes (1.33 ms for 48kHz) but add a 1.33 ms threshold to make sure we never rewind below. rewind_safeguard = max(256, pa_usec_to_bytes(1330)); This way you solve both the hardware issue (frequency independent) and leave enough headroom for the system to avoid underflows. -Pierre ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
On Wed, Sep 01, 2010 at 08:39:39AM +0100, Colin Guthrie wrote: What is the next stage here? Probably either one will work, but if we're about to release 0.9.22 (heard something from Lennart yet?), I suggest we go with my version for 0.9.22 as that one is the least invasive (only touches non-tsched devices), and keep Pierre's version in master. Sounds reasonable. Pierre, what's your take? Spoke briefly to to Lennart, I think he was generally up for some kind of release, but we'll wait for a couple weeks when F15 beta freeze hits and thus his systemd work will be paused for a while at least and he can focus some of his time back on PA :) Of course you meant F14 beta freeze (which is in two weeks). Fedora 15 beta freeze is over half a year away :) -- Tomasz TorczTo co nierealne -- tutaj jest normalne. xmpp: zdzich...@chrome.pl Ziomale na życie mają tu patenty specjalne. ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
'Twas brillig, and Tomasz Torcz at 01/09/10 09:17 did gyre and gimble: On Wed, Sep 01, 2010 at 08:39:39AM +0100, Colin Guthrie wrote: What is the next stage here? Probably either one will work, but if we're about to release 0.9.22 (heard something from Lennart yet?), I suggest we go with my version for 0.9.22 as that one is the least invasive (only touches non-tsched devices), and keep Pierre's version in master. Sounds reasonable. Pierre, what's your take? Spoke briefly to to Lennart, I think he was generally up for some kind of release, but we'll wait for a couple weeks when F15 beta freeze hits and thus his systemd work will be paused for a while at least and he can focus some of his time back on PA :) Of course you meant F14 beta freeze (which is in two weeks). Fedora 15 beta freeze is over half a year away :) Yeah I probably did mean that... could have sworn Lennart said F15, but then he's probably been up too long and needs a reboot... (wonder if he'll come back up without a panic...) :p -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited [http://www.tribalogic.net/] Open Source: Mandriva Linux Contributor [http://www.mandriva.com/] PulseAudio Hacker [http://www.pulseaudio.org/] Trac Hacker [http://trac.edgewall.org/] ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
Probably either one will work, but if we're about to release 0.9.22 (heard something from Lennart yet?), I suggest we go with my version for 0.9.22 as that one is the least invasive (only touches non-tsched devices), and keep Pierre's version in master. Sounds reasonable. Pierre, what's your take? That would mean an additional post-release patch for tsched devices. I am lazy and would prefer a one-stop fix. I don't care if this is David's or mine, as long as the solution works for both cases. The only real difference is the bytes/ms parameter. Although ms are more intuitive, the bytes makes more sense from a hardware point of view. If you pass a parameter in ms, there might be cases where the actual number of bytes is lower than the DMA burst, it'll depend on what frequency the sink operates at. There are some cases where alsa-sink works at 8kHz (BT-SCO) or 48kHz (all other cases), a 6x variability in the rewind behavior is difficult to handle. -Pierre ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
'Twas brillig, and pl bossart at 19/08/10 14:52 did gyre and gimble: Assuming your reasoning is correct (I'm not that deep into DMA yet), this should be fixed in the kernel - by not allowing rewinds further back than 128 (or 256) bytes ahead of actual position. You say HDA can transfer up to 128 bytes in advance, but what about all the other drivers? Could there be other drivers having a lot larger DMA fetches? What's the role of snd_pcm_rewindable()[1]? The documentation says Get safe count of frames which can be rewinded, which sounds to me like the function should always be called before snd_pcm_rewind(). Currently PA doesn't call _rewindable(). Yes it should be fixed in the kernel, but the DMA transfer size is not necessarily documented and known. Worse, it can vary depending on the mode. So until the kernel folks figure out a solution, and said solution works for all know drivers, this patch is mandatory on the PulseAudio side. Don't want to lose momentum on this patch, so just poking about. What is the next stage here? Should I test with a lower number (1.45ms) as originally suggested. If that turns out to work, what then? I'd like to get something that addresses this problem into stable queue sooner rather than later (i.e. before I forget about this thread and get distracted by something!). As I only pretend to know what's going on, I don't fully understand all the issues here so Pierre and David: if you could hash out the right approach between you, I'll commit and/or test the results (tho' not in that order :D) Cheers Col -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited [http://www.tribalogic.net/] Open Source: Mandriva Linux Contributor [http://www.mandriva.com/] PulseAudio Hacker [http://www.pulseaudio.org/] Trac Hacker [http://trac.edgewall.org/] ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
2010-08-19 07:32, Tanu Kaskinen skrev: On Wed, 2010-08-18 at 22:55 +0200, David Henningsson wrote: Yes the safeguard is needed in both cases, timer scheduling or good ol' audio interrupts. This comes from limitations of the snd_pcm_rewind() routine, you can rewind the appl_ptr all the way to the hardware pointer, but you may have DMA transfers in flight that cannot be rewound. For example HDaudio can fetch up to 128bytes, I added a default a safeguard of 256bytes to be super safe, but in theory this should be adjusted depending on how the DMA operates. Assuming your reasoning is correct (I'm not that deep into DMA yet), this should be fixed in the kernel - by not allowing rewinds further back than 128 (or 256) bytes ahead of actual position. You say HDA can transfer up to 128 bytes in advance, but what about all the other drivers? Could there be other drivers having a lot larger DMA fetches? What's the role of snd_pcm_rewindable()[1]? The documentation says Get safe count of frames which can be rewinded, which sounds to me like the function should always be called before snd_pcm_rewind(). Currently PA doesn't call _rewindable(). Good question. Looking at the code in alsa-lib, it seems like both snd_pcm_avail and snd_pcm_rewindable call snd_pcm_mmap_hw_avail, so in practice it probably doesn't matter, at least not for the common cases. That said, it would likely be a good thing to change it anyway, since the alsa-lib implementation might change. The DMA knows nothing about milliseconds, it behaves the same way no matter the sampling rate it, which is the reason why bytes make more sense, it's easier to correlate with the way the hardware works. So your idea is to prevent DMA transfers being modified, but I'm thinking of the maximum duration between the rewind and the point it gets filled up again by PA - all of that time we risc getting an XRUN because there is nothing in the buffer. And so I assume that the duration is never longer than 20 ms. I don't think it is much of a deal though. Rewind is not something used every second or so (or at least, shouldn't be). A comment on the last statement: I don't think the average frequency of rewinds is important, because there are cases where multiple rewinds do happen really quickly - for example when dragging a volume slider (IIRC pavucontrol ratelimits the volume change events to minimum of 100ms between each event sent to the daemon - that's still ten times a second). I don't remember hearing complaints about that the volume is crackling when changing volumes, but at least in theory this seems like a possible problem with the current implementation. Ah, are you saying we might have fixed that crackling as well? That would be even better :-) For volume slider changes I don't think there is a problem with waiting 20 ms before the volume change takes effect. -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
2010-08-17 21:10, Colin Guthrie skrev: 'Twas brillig, and David Henningsson at 17/08/10 19:20 did gyre and gimble: According to what you say in that bug, you could reproduce it yourself by setting tsched=0, so I'm eager to hear if this fix fixes your issue as well. Yeah I was able to reproduce it pretty easily with the chordtest.sh script attached to the bug. After the third tone, it started to sound terribly distored. After applying your patch, I tried several times and got a perfect run each time :) \o/ The patch is against stable-queue, and Raymond commented that git master has a slightly different way of solving the problem. To compare them: * Pierre-Louis Bossart's version in git master sets a fixed margin of 256 bytes, (configurable if you load the sink manually, i e not through module-udev-detect). * My version sets a fixed margin of 20 ms. * My version only changes non-tsched devices and keeps tsched having a margin of the current watermark, whereas Bossart's version changes both. I think a margin should be based on milliseconds rather than bytes (the amount of bytes varies with amount of channels, which means that we might get problems when people switch to surround output). I don't have an opinion on whether a fixed margin or watermark-based margin is better for tsched sinks. -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
'Twas brillig, and David Henningsson at 18/08/10 09:28 did gyre and gimble: 2010-08-17 21:10, Colin Guthrie skrev: 'Twas brillig, and David Henningsson at 17/08/10 19:20 did gyre and gimble: According to what you say in that bug, you could reproduce it yourself by setting tsched=0, so I'm eager to hear if this fix fixes your issue as well. Yeah I was able to reproduce it pretty easily with the chordtest.sh script attached to the bug. After the third tone, it started to sound terribly distored. After applying your patch, I tried several times and got a perfect run each time :) \o/ The patch is against stable-queue, and Raymond commented that git master has a slightly different way of solving the problem. To compare them: * Pierre-Louis Bossart's version in git master sets a fixed margin of 256 bytes, (configurable if you load the sink manually, i e not through module-udev-detect). * My version sets a fixed margin of 20 ms. * My version only changes non-tsched devices and keeps tsched having a margin of the current watermark, whereas Bossart's version changes both. I think a margin should be based on milliseconds rather than bytes (the amount of bytes varies with amount of channels, which means that we might get problems when people switch to surround output). I don't have an opinion on whether a fixed margin or watermark-based margin is better for tsched sinks. I presume Pierre-Louis will comment in due course. I'm sure he'll see this message. I'm annoyed I didn't appreciate that his fix would likely address the issue too when it was pushed, but such is life :D Col -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited [http://www.tribalogic.net/] Open Source: Mandriva Linux Contributor [http://www.mandriva.com/] PulseAudio Hacker [http://www.pulseaudio.org/] Trac Hacker [http://trac.edgewall.org/] ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
* Pierre-Louis Bossart's version in git master sets a fixed margin of 256 bytes, (configurable if you load the sink manually, i e not through module-udev-detect). * My version sets a fixed margin of 20 ms. * My version only changes non-tsched devices and keeps tsched having a margin of the current watermark, whereas Bossart's version changes both. I think a margin should be based on milliseconds rather than bytes (the amount of bytes varies with amount of channels, which means that we might get problems when people switch to surround output). I don't have an opinion on whether a fixed margin or watermark-based margin is better for tsched sinks. I presume Pierre-Louis will comment in due course. I'm sure he'll see this message. I'm annoyed I didn't appreciate that his fix would likely address the issue too when it was pushed, but such is life :D Well I didn't see the link between my patch (from April) and the problem David tried to fix either. Thanks Raymond for finding this out. Before making any conclusions, would the problem be solved with David's patch using the equivalent of 256 bytes, that is 1.45ms instead of 20ms? just want to make sure this is the same bug. Yes the safeguard is needed in both cases, timer scheduling or good ol' audio interrupts. This comes from limitations of the snd_pcm_rewind() routine, you can rewind the appl_ptr all the way to the hardware pointer, but you may have DMA transfers in flight that cannot be rewound. For example HDaudio can fetch up to 128bytes, I added a default a safeguard of 256bytes to be super safe, but in theory this should be adjusted depending on how the DMA operates. The DMA knows nothing about milliseconds, it behaves the same way no matter the sampling rate it, which is the reason why bytes make more sense, it's easier to correlate with the way the hardware works. -Pierre ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
2010-08-18 19:19, pl bossart skrev: * Pierre-Louis Bossart's version in git master sets a fixed margin of 256 bytes, (configurable if you load the sink manually, i e not through module-udev-detect). * My version sets a fixed margin of 20 ms. * My version only changes non-tsched devices and keeps tsched having a margin of the current watermark, whereas Bossart's version changes both. I think a margin should be based on milliseconds rather than bytes (the amount of bytes varies with amount of channels, which means that we might get problems when people switch to surround output). I don't have an opinion on whether a fixed margin or watermark-based margin is better for tsched sinks. I presume Pierre-Louis will comment in due course. I'm sure he'll see this message. I'm annoyed I didn't appreciate that his fix would likely address the issue too when it was pushed, but such is life :D Well I didn't see the link between my patch (from April) and the problem David tried to fix either. Thanks Raymond for finding this out. Before making any conclusions, would the problem be solved with David's patch using the equivalent of 256 bytes, that is 1.45ms instead of 20ms? just want to make sure this is the same bug. Haven't tested, but I feel quite certain that it's the same one. Yes the safeguard is needed in both cases, timer scheduling or good ol' audio interrupts. This comes from limitations of the snd_pcm_rewind() routine, you can rewind the appl_ptr all the way to the hardware pointer, but you may have DMA transfers in flight that cannot be rewound. For example HDaudio can fetch up to 128bytes, I added a default a safeguard of 256bytes to be super safe, but in theory this should be adjusted depending on how the DMA operates. Assuming your reasoning is correct (I'm not that deep into DMA yet), this should be fixed in the kernel - by not allowing rewinds further back than 128 (or 256) bytes ahead of actual position. You say HDA can transfer up to 128 bytes in advance, but what about all the other drivers? Could there be other drivers having a lot larger DMA fetches? The DMA knows nothing about milliseconds, it behaves the same way no matter the sampling rate it, which is the reason why bytes make more sense, it's easier to correlate with the way the hardware works. So your idea is to prevent DMA transfers being modified, but I'm thinking of the maximum duration between the rewind and the point it gets filled up again by PA - all of that time we risc getting an XRUN because there is nothing in the buffer. And so I assume that the duration is never longer than 20 ms. I don't think it is much of a deal though. Rewind is not something used every second or so (or at least, shouldn't be). -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
On Wed, 2010-08-18 at 22:55 +0200, David Henningsson wrote: Yes the safeguard is needed in both cases, timer scheduling or good ol' audio interrupts. This comes from limitations of the snd_pcm_rewind() routine, you can rewind the appl_ptr all the way to the hardware pointer, but you may have DMA transfers in flight that cannot be rewound. For example HDaudio can fetch up to 128bytes, I added a default a safeguard of 256bytes to be super safe, but in theory this should be adjusted depending on how the DMA operates. Assuming your reasoning is correct (I'm not that deep into DMA yet), this should be fixed in the kernel - by not allowing rewinds further back than 128 (or 256) bytes ahead of actual position. You say HDA can transfer up to 128 bytes in advance, but what about all the other drivers? Could there be other drivers having a lot larger DMA fetches? What's the role of snd_pcm_rewindable()[1]? The documentation says Get safe count of frames which can be rewinded, which sounds to me like the function should always be called before snd_pcm_rewind(). Currently PA doesn't call _rewindable(). The DMA knows nothing about milliseconds, it behaves the same way no matter the sampling rate it, which is the reason why bytes make more sense, it's easier to correlate with the way the hardware works. So your idea is to prevent DMA transfers being modified, but I'm thinking of the maximum duration between the rewind and the point it gets filled up again by PA - all of that time we risc getting an XRUN because there is nothing in the buffer. And so I assume that the duration is never longer than 20 ms. I don't think it is much of a deal though. Rewind is not something used every second or so (or at least, shouldn't be). A comment on the last statement: I don't think the average frequency of rewinds is important, because there are cases where multiple rewinds do happen really quickly - for example when dragging a volume slider (IIRC pavucontrol ratelimits the volume change events to minimum of 100ms between each event sent to the daemon - that's still ten times a second). I don't remember hearing complaints about that the volume is crackling when changing volumes, but at least in theory this seems like a possible problem with the current implementation. [1] http://www.alsa-project.org/alsa-doc/alsa-lib/group___p_c_m.html#g8f56faf60ea6b60839e131df88f080d7 -- Tanu Kaskinen ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
The tsched watermark variable was incorrectly used even for sinks
with timer scheduling disabled, causing XRUNs on every rewind. This
patch sets a fixed margin of 20 msec for such rewinds, thus avoiding
the underrun.
One could argue that the margin should be adjustable somehow (or based
on fragment-size, or something else), but this patch at least fixes the
immediate problem, causing crackling output on (at least) one machine.
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
From 87e8a04f029c18cdb5abebb4f3f3c2100d42107a Mon Sep 17 00:00:00 2001
From: David Henningsson david.hennings...@canonical.com
Date: Tue, 17 Aug 2010 19:37:51 +0200
Subject: [PATCH] Do not use tsched watermark if tsched is disabled
The tsched watermark variable was incorrectly used even for sinks
with timer scheduling disabled, causing XRUNs on every rewind. This
patch sets a fixed margin of 20 msec for such rewinds, thus avoiding
the underrun.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
src/modules/alsa/alsa-sink.c |8 +++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
index a253cc5..9202883 100644
--- a/src/modules/alsa/alsa-sink.c
+++ b/src/modules/alsa/alsa-sink.c
@@ -63,6 +63,8 @@
#define DEFAULT_DEVICE default
+#define NON_TSCHED_REWIND_MARGIN_USEC (20*PA_USEC_PER_MSEC)/* 20ms -- Rewind margin for non-tsched devices */
+
#define DEFAULT_TSCHED_BUFFER_USEC (2*PA_USEC_PER_SEC) /* 2s-- Overall buffer size */
#define DEFAULT_TSCHED_WATERMARK_USEC (20*PA_USEC_PER_MSEC)/* 20ms -- Fill up when only this much is left in the buffer */
@@ -1309,7 +1311,11 @@ static int process_rewind(struct userdata *u) {
return -1;
}
-unused_nbytes = u-tsched_watermark + (size_t) unused * u-frame_size;
+unused_nbytes = (size_t) unused * u-frame_size;
+if (u-use_tsched)
+unused_nbytes += u-tsched_watermark;
+else
+unused_nbytes += pa_usec_to_bytes(NON_TSCHED_REWIND_MARGIN_USEC, u-sink-sample_spec);
if (u-hwbuf_size unused_nbytes)
limit_nbytes = u-hwbuf_size - unused_nbytes;
--
1.7.0.4
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
'Twas brillig, and David Henningsson at 17/08/10 19:00 did gyre and gimble: The tsched watermark variable was incorrectly used even for sinks with timer scheduling disabled, causing XRUNs on every rewind. This patch sets a fixed margin of 20 msec for such rewinds, thus avoiding the underrun. One could argue that the margin should be adjustable somehow (or based on fragment-size, or something else), but this patch at least fixes the immediate problem, causing crackling output on (at least) one machine. /me wonders if this fixes https://bugzilla.redhat.com/show_bug.cgi?id=537378 -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited [http://www.tribalogic.net/] Open Source: Mandriva Linux Contributor [http://www.mandriva.com/] PulseAudio Hacker [http://www.pulseaudio.org/] Trac Hacker [http://trac.edgewall.org/] ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
'Twas brillig, and Colin Guthrie at 17/08/10 19:03 did gyre and gimble: 'Twas brillig, and David Henningsson at 17/08/10 19:00 did gyre and gimble: The tsched watermark variable was incorrectly used even for sinks with timer scheduling disabled, causing XRUNs on every rewind. This patch sets a fixed margin of 20 msec for such rewinds, thus avoiding the underrun. One could argue that the margin should be adjustable somehow (or based on fragment-size, or something else), but this patch at least fixes the immediate problem, causing crackling output on (at least) one machine. /me wonders if this fixes https://bugzilla.redhat.com/show_bug.cgi?id=537378 Hmm, it seems to in my tests :) Col -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited [http://www.tribalogic.net/] Open Source: Mandriva Linux Contributor [http://www.mandriva.com/] PulseAudio Hacker [http://www.pulseaudio.org/] Trac Hacker [http://trac.edgewall.org/] ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
2010-08-17 20:03, Colin Guthrie skrev: 'Twas brillig, and David Henningsson at 17/08/10 19:00 did gyre and gimble: The tsched watermark variable was incorrectly used even for sinks with timer scheduling disabled, causing XRUNs on every rewind. This patch sets a fixed margin of 20 msec for such rewinds, thus avoiding the underrun. One could argue that the margin should be adjustable somehow (or based on fragment-size, or something else), but this patch at least fixes the immediate problem, causing crackling output on (at least) one machine. /me wonders if this fixes https://bugzilla.redhat.com/show_bug.cgi?id=537378 It seems likely to me. I got a feeling that it isn't the whole story though, that there could be some additional bug in ALSA causing sound to remain distorted, if the XRUN is merely touched and afterwards immediately resolved by writing more data. So it could have been a combination of these two bugs causing the crackling sound, and by resolving the PA bug the problem does not appear anymore. According to what you say in that bug, you could reproduce it yourself by setting tsched=0, so I'm eager to hear if this fix fixes your issue as well. -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled
'Twas brillig, and David Henningsson at 17/08/10 19:20 did gyre and gimble: According to what you say in that bug, you could reproduce it yourself by setting tsched=0, so I'm eager to hear if this fix fixes your issue as well. Yeah I was able to reproduce it pretty easily with the chordtest.sh script attached to the bug. After the third tone, it started to sound terribly distored. After applying your patch, I tried several times and got a perfect run each time :) Col -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited [http://www.tribalogic.net/] Open Source: Mandriva Linux Contributor [http://www.mandriva.com/] PulseAudio Hacker [http://www.pulseaudio.org/] Trac Hacker [http://trac.edgewall.org/] ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
