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
