[pulseaudio-discuss] [RFC] [PATCH] alsa-sink: Reduce unnecessarily large max rewind

[David Henningsson]

Ambivalent about this one - I think I agree with the general concept, 
but OTOH it somehow feels like the big rewind is there for a reason too.

Maybe if you dig deeper into the problem about filter sinks that you 
noticed, maybe that can give us some hint to whether or not this is a 
correct patch?

I'm marking it as "changes requested" in patchwork for the time being.

On 2015-11-17 08:40, arun at accosted.net wrote:
> From: Arun Raghavan <git at arunraghavan.net>
>
> This makes us not report more than the hardware buffer size being used
> as the max rewind that sink inputs need to support. The rationale is
> that we won't ever rewind more than the length of the buffer that we are
> currently using, so we don't need to keep the extra data around. On
> large HDA buffers, this should save some memory for each sink input.
>
> However, I do notice a problem -- when there is a filter sink attached,
> the first stream after a resume from suspend playse fine, but the second
> stream has a lag that seems to be in the order of the buffer size. I'm
> not sure where the problem lies.
>
> I'll try to take a look at what's going wrong, but if anyone has any
> ideas, I'm all ears.
> ---
>   src/modules/alsa/alsa-sink.c | 39 ++++++++++++++++++++-------------------
>   1 file changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
> index c5a72c3..94aeff0 100644
> --- a/src/modules/alsa/alsa-sink.c
> +++ b/src/modules/alsa/alsa-sink.c
> @@ -956,12 +956,14 @@ 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 rewind) {
>       snd_pcm_uframes_t avail_min;
> +    size_t before;
>       int err;
>
>       pa_assert(u);
>
> +    before = u->hwbuf_unused;
>       /* Use the full buffer if no one asked us for anything specific */
>       u->hwbuf_unused = 0;
>
> @@ -1006,9 +1008,21 @@ static int update_sw_params(struct userdata *u) {
>           return err;
>       }
>
> +    if (rewind) {
> +        /* 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(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);
> @@ -1109,7 +1123,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)
> @@ -1505,7 +1519,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
> @@ -1514,19 +1527,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) {
> @@ -2360,7 +2361,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) {
>

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic