[pulseaudio-discuss] [PATCH 1/2] alsa-sink: update max_rewind when updating the latency

[Tanu Kaskinen]

Previously max_rewind was always set to the full hw buffer size, but
the actual maximum rewind amount is limited to the part of the hw buffer
that is in use.

The rewind request that was done when lowering the sink latency had to
be moved to happen before updating max_rewind.

The practical benefit of this change: When using a filter source on a
monitor source, the filter source latency is increased by max_rewind.
Without this change the max_rewind of an alsa sink is often
unnecessarily high, which leads to unnecessarily high latency with
filter sources.

Monitor sources themselves don't suffer from the latency issue, because
they use the current sink latency instead of max_rewind for the extra
buffer that they keep to deal with rewinds. It would be good to do the
same with filter sources that are on top of monitor sources, but it's
not obvious to me how to implement that.
---
 src/modules/alsa/alsa-sink.c  | 39 ++++++++++++++++++++-------------------
 src/pulsecore/source-output.c |  6 ++++++
 2 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
index 96fd02aec..7936cfaca 100644
--- a/src/modules/alsa/alsa-sink.c
+++ b/src/modules/alsa/alsa-sink.c
@@ -969,13 +969,15 @@ static int suspend(struct userdata *u) {
 }
 
 /* Called from IO context */
-static int update_sw_params(struct userdata *u) {
+static int update_sw_params(struct userdata *u, bool may_need_rewind) {
+    size_t old_unused;
     snd_pcm_uframes_t avail_min;
     int err;
 
     pa_assert(u);
 
     /* Use the full buffer if no one asked us for anything specific */
+    old_unused = u->hwbuf_unused;
     u->hwbuf_unused = 0;
 
     if (u->use_tsched) {
@@ -1019,9 +1021,21 @@ static int update_sw_params(struct userdata *u) {
         return err;
     }
 
+    /* If we're lowering the latency, we need to do a rewind, because otherwise
+     * we might end up in a situation where the hw buffer contains more data
+     * than the new configured latency. The rewind has to be requested before
+     * updating max_rewind, because the rewind amount is limited to max_rewind.
+     *
+     * If may_need_rewind is false, it means that we're just starting playback,
+     * and rewinding is never needed in that situation. */
+    if (may_need_rewind && u->hwbuf_unused > old_unused) {
+        pa_log_debug("Requesting rewind due to latency change.");
+        pa_sink_request_rewind(u->sink, (size_t) -1);
+    }
+
     pa_sink_set_max_request_within_thread(u->sink, u->hwbuf_size - u->hwbuf_unused);
-     if (pa_alsa_pcm_is_hw(u->pcm_handle))
-         pa_sink_set_max_rewind_within_thread(u->sink, u->hwbuf_size);
+    if (pa_alsa_pcm_is_hw(u->pcm_handle))
+        pa_sink_set_max_rewind_within_thread(u->sink, u->hwbuf_size - u->hwbuf_unused);
     else {
         pa_log_info("Disabling rewind_within_thread for device %s", u->device_name);
         pa_sink_set_max_rewind_within_thread(u->sink, 0);
@@ -1122,7 +1136,7 @@ static int unsuspend(struct userdata *u) {
         goto fail;
     }
 
-    if (update_sw_params(u) < 0)
+    if (update_sw_params(u, false) < 0)
         goto fail;
 
     if (build_pollfd(u) < 0)
@@ -1518,7 +1532,6 @@ static int sink_set_port_cb(pa_sink *s, pa_device_port *p) {
 
 static void sink_update_requested_latency_cb(pa_sink *s) {
     struct userdata *u = s->userdata;
-    size_t before;
     pa_assert(u);
     pa_assert(u->use_tsched); /* only when timer scheduling is used
                                * we can dynamically adjust the
@@ -1527,19 +1540,7 @@ static void sink_update_requested_latency_cb(pa_sink *s) {
     if (!u->pcm_handle)
         return;
 
-    before = u->hwbuf_unused;
-    update_sw_params(u);
-
-    /* Let's check whether we now use only a smaller part of the
-    buffer then before. If so, we need to make sure that subsequent
-    rewinds are relative to the new maximum fill level and not to the
-    current fill level. Thus, let's do a full rewind once, to clear
-    things up. */
-
-    if (u->hwbuf_unused > before) {
-        pa_log_debug("Requesting rewind due to latency change.");
-        pa_sink_request_rewind(s, (size_t) -1);
-    }
+    update_sw_params(u, true);
 }
 
 static pa_idxset* sink_get_formats(pa_sink *s) {
@@ -2396,7 +2397,7 @@ pa_sink *pa_alsa_sink_new(pa_module *m, pa_modargs *ma, const char*driver, pa_ca
 
     reserve_update(u);
 
-    if (update_sw_params(u) < 0)
+    if (update_sw_params(u, false) < 0)
         goto fail;
 
     if (u->ucm_context) {
diff --git a/src/pulsecore/source-output.c b/src/pulsecore/source-output.c
index f8f4e0ef0..a66dc2d8a 100644
--- a/src/pulsecore/source-output.c
+++ b/src/pulsecore/source-output.c
@@ -758,6 +758,12 @@ void pa_source_output_push(pa_source_output *o, const pa_memchunk *chunk) {
          * of the queued data is actually still changeable. Hence
          * FIXME! */
 
+        /* Another FIXME: since we only check o->source->monitor_of, this
+         * doesn't work when the source is a filter on top of a monitor source.
+         * The result is that the limit is set to max_rewind instead of the
+         * current latency of the monitored sink, which means that the delay
+         * queue and hence latency become larger than necessary. */
+
         latency = pa_sink_get_latency_within_thread(o->source->monitor_of, false);
 
         n = pa_usec_to_bytes(latency, &o->source->sample_spec);
-- 
2.14.2