Re: [pulseaudio-discuss] Yet another attempt to fight rewinds
On Tue, Sep 20, 2011 at 04:25:10PM +0800, David Henningsson wrote: [...] > Hi Lu, > > This is interesting. If there was something in the kernel that makes > Atom HDA controllers (?) report the wrong timing info, and this was > later fixed, could you point me to that patch? Hi David, We're using Moorestown as our platform, http://en.wikipedia.org/wiki/Moorestown_(computing_platform) It don't use HDA controller, instead, it uses a kind of PMIC as its audio codec. And the patch was deep in the firmware, so it doesn't help your bug very much I'm afraid... Sorry about that... -- guanqun ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Yet another attempt to fight rewinds
On 09/20/2011 10:21 AM, Lu Guanqun wrote: On Mon, Sep 19, 2011 at 11:49:03PM +0800, David Henningsson wrote: A few Atom users have complained about enternal rewinds since they upgraded to 0.99.x, see https://bugs.launchpad.net/ubuntu/+source/pulseaudio/+bug/825709 So here's just an idea. As a "last resort", ratelimit the number of rewinds. If there are more than 10 rewinds in 200 ms, go to sleep for 200 ms. The idea is that during those 200 ms, the client application will produce enough packets to fill up the buffer enough. Those packets will then be merged into one, due to an earlier rewind patch that is already in. The 200 ms sleep might cause a noticable glitch, but hopefully we get that one glitch only instead of complete brokenness. But I don't have any such setup here currently, so maybe any of you could check this patch and see if it works as intended, and has real effect? Hi David, Thanks for the patch, rate limiting the rewinding seems a good idea, however, I don't have Atom machine right on my hand. I met the flood of rewinds before, but that was later root caused to the wrong report of timing info from underlying device. After this was fixed, no flood of rewinds were seen. Hi Lu, This is interesting. If there was something in the kernel that makes Atom HDA controllers (?) report the wrong timing info, and this was later fixed, could you point me to that patch? Thanks in advance! -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Yet another attempt to fight rewinds
On Mon, Sep 19, 2011 at 11:49:03PM +0800, David Henningsson wrote: > A few Atom users have complained about enternal rewinds since they > upgraded to 0.99.x, see > https://bugs.launchpad.net/ubuntu/+source/pulseaudio/+bug/825709 > > So here's just an idea. As a "last resort", ratelimit the number of > rewinds. If there are more than 10 rewinds in 200 ms, go to sleep for > 200 ms. The idea is that during those 200 ms, the client application > will produce enough packets to fill up the buffer enough. Those packets > will then be merged into one, due to an earlier rewind patch that is > already in. The 200 ms sleep might cause a noticable glitch, but > hopefully we get that one glitch only instead of complete brokenness. > > But I don't have any such setup here currently, so maybe any of you > could check this patch and see if it works as intended, and has real effect? Hi David, Thanks for the patch, rate limiting the rewinding seems a good idea, however, I don't have Atom machine right on my hand. I met the flood of rewinds before, but that was later root caused to the wrong report of timing info from underlying device. After this was fixed, no flood of rewinds were seen. -- guanqun ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Yet another attempt to fight rewinds
On 09/19/2011 07:14 PM, Tanu Kaskinen wrote:
On Mon, 2011-09-19 at 17:49 +0200, David Henningsson wrote:
+if (!pa_ratelimit_test(&s->thread_info.rewind_limit, PA_LOG_DEBUG)) {
+pa_log_warn("Okay, I'm sick and tired of all this rewinding. I'm going to
sleep for %lu ms!",
+s->thread_info.rewind_limit.interval / PA_USEC_PER_MSEC);
+usleep(s->thread_info.rewind_limit.interval);
+pa_log_debug("Waking up.");
+}
The patch seems otherwise ok to me, but I'd prefer the log prints to
include the sink name. When a system has e.g. 6 sinks, all of which may
be in active use at the same time, I really don't like the sink.c
messages that don't tell which sink is in question... Also, instead of
just "waking up", maybe it would be useful to have "waking up; not so
sick and tired anymore" or something else that clearly connects the
wakeup message to the preceding warning.
Sure. I don't mind changing the log messages. (As for sink names, I
think there was a plan to add that to the thread name in the log?)
I'm more concerned about what happens if we wake up and that sleep has
caused an underrun on the ALSA level, what that will do to the
watermark, etc.
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Yet another attempt to fight rewinds
On Mon, 2011-09-19 at 17:49 +0200, David Henningsson wrote:
> +if (!pa_ratelimit_test(&s->thread_info.rewind_limit, PA_LOG_DEBUG)) {
> +pa_log_warn("Okay, I'm sick and tired of all this rewinding. I'm
> going to sleep for %lu ms!",
> +s->thread_info.rewind_limit.interval / PA_USEC_PER_MSEC);
> +usleep(s->thread_info.rewind_limit.interval);
> +pa_log_debug("Waking up.");
> +}
The patch seems otherwise ok to me, but I'd prefer the log prints to
include the sink name. When a system has e.g. 6 sinks, all of which may
be in active use at the same time, I really don't like the sink.c
messages that don't tell which sink is in question... Also, instead of
just "waking up", maybe it would be useful to have "waking up; not so
sick and tired anymore" or something else that clearly connects the
wakeup message to the preceding warning.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] Yet another attempt to fight rewinds
A few Atom users have complained about enternal rewinds since they
upgraded to 0.99.x, see
https://bugs.launchpad.net/ubuntu/+source/pulseaudio/+bug/825709
So here's just an idea. As a "last resort", ratelimit the number of
rewinds. If there are more than 10 rewinds in 200 ms, go to sleep for
200 ms. The idea is that during those 200 ms, the client application
will produce enough packets to fill up the buffer enough. Those packets
will then be merged into one, due to an earlier rewind patch that is
already in. The 200 ms sleep might cause a noticable glitch, but
hopefully we get that one glitch only instead of complete brokenness.
But I don't have any such setup here currently, so maybe any of you
could check this patch and see if it works as intended, and has real effect?
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
>From e1a11d045c898b1875532b9a1497c138968bd2ea Mon Sep 17 00:00:00 2001
From: David Henningsson
Date: Mon, 19 Sep 2011 15:54:58 +0200
Subject: [PATCH] Ratelimit rewinds of sinks
Allow max 10 rewinds in 200 ms in order to try to counteract the
eternal rewind issues still seen on low-end CPUs (e g Atom)
BugLink: http://bugs.launchpad.net/bugs/825709
Signed-off-by: David Henningsson
---
src/pulsecore/sink.c |8
src/pulsecore/sink.h |2 ++
2 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
index d97fb7e..2d006a5 100644
--- a/src/pulsecore/sink.c
+++ b/src/pulsecore/sink.c
@@ -332,6 +332,7 @@ pa_sink* pa_sink_new(
s->thread_info.state = s->state;
s->thread_info.rewind_nbytes = 0;
s->thread_info.rewind_requested = FALSE;
+PA_INIT_RATELIMIT(s->thread_info.rewind_limit, PA_USEC_PER_MSEC * 200, 10); /* max 10 rewinds in 200 ms */
s->thread_info.max_rewind = 0;
s->thread_info.max_request = 0;
s->thread_info.requested_latency_valid = FALSE;
@@ -932,6 +933,13 @@ void pa_sink_process_rewind(pa_sink *s, size_t nbytes) {
if (s->monitor_source && PA_SOURCE_IS_LINKED(s->monitor_source->thread_info.state))
pa_source_process_rewind(s->monitor_source, nbytes);
}
+
+if (!pa_ratelimit_test(&s->thread_info.rewind_limit, PA_LOG_DEBUG)) {
+pa_log_warn("Okay, I'm sick and tired of all this rewinding. I'm going to sleep for %lu ms!",
+s->thread_info.rewind_limit.interval / PA_USEC_PER_MSEC);
+usleep(s->thread_info.rewind_limit.interval);
+pa_log_debug("Waking up.");
+}
}
/* Called from IO thread context */
diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
index 0642dda..8c998e8 100644
--- a/src/pulsecore/sink.h
+++ b/src/pulsecore/sink.h
@@ -47,6 +47,7 @@ typedef struct pa_sink_volume_change pa_sink_volume_change;
#include
#include
#include
+#include
#define PA_MAX_INPUTS_PER_SINK 32
@@ -261,6 +262,7 @@ struct pa_sink {
/* Maximum of what clients requested to rewind in this cycle */
size_t rewind_nbytes;
pa_bool_t rewind_requested;
+pa_ratelimit rewind_limit;
/* Both dynamic and fixed latencies will be clamped to this
* range. */
--
1.7.5.4
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
