[pulseaudio-discuss] [PATCH] module-virtual-sink: Speed up draining

[Alexander E. Patrakov]

02.10.2014 19:58, David Henningsson wrote:
> The drain speed increase done a while ago, only worked when directly connected
> to ALSA sinks. Playing through an module-virtual-sink would cause slower drains.
>
> This fix has two parts:
>   1) We now refuse to process handed out silence. This has the effect
>      that it tells ALSAs sink input that we're draining, and that we want to
>      be woken up exactly at this sample based point in time.
>   2) When we're being called to process input underruns, we know this has
>      happened, so we can just forward the call up the sink chain to get
>      to protocol-native, which can acknowledge that the drain has completed.
>
> This fix is incomplete, because if more than one sink input is playing back
> into the sink, only the last one will be drained in time. But it's better
> than nothing.
>
> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> ---
>
> Happy for testing of this patch - Ubuntu runs HDA-intel with 64K buffers instead of the 2M that
> some distros use, so I'm not seeing that much difference that you do anyway.
>
> Also, when I was testing, I also noticed a bigger startup latency when playing
> back through a virtual sink. This is nothing I have looked deeper at.

Hello David,

I have looked at the patch, but came to the conclusion that I cannot 
provide a good review now. Please find some criticisms below, but they 
are not serious. In fact, I will accept any of the following responses 
to the criticisms:

1. Let's ignore the issue.

2. Let's apply a big hammer and just disallow high (or, for that matter, 
dynamic) latencies on virtual sinks, effectively masking the issue.

3. (a fix)

In fact, I was going to suggest (2) at the miniconf, together with 
punting on rewind support for virtual sinks. In too many cases, this is 
just impossible to implement due to external reasons. But that's a 
rather controversial decision that would need to be discussed.

>   src/modules/module-virtual-sink.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
>
> diff --git a/src/modules/module-virtual-sink.c b/src/modules/module-virtual-sink.c
> index 66fd8a9..c1e5969 100644
> --- a/src/modules/module-virtual-sink.c
> +++ b/src/modules/module-virtual-sink.c
> @@ -216,6 +216,10 @@ static int sink_input_pop_cb(pa_sink_input *i, size_t nbytes, pa_memchunk *chunk
>           pa_memchunk nchunk;
>
>           pa_sink_render(u->sink, nbytes, &nchunk);
> +        if (nchunk.memblock == u->sink->silence.memblock) {
> +            pa_memblock_unref(nchunk.memblock);
> +            return -1;
> +        }

This may be valid for module-virtual-sink, but is definitely invalid for 
any sink that does non-trivial processing. Consider an imaginary virtual 
sink that adds some reverb to the sounds that come through it: the 
output does not end when the input ends. I guess we need to define first 
what "drain" really means in such cases, or agree to ignore the issue 
(which is fine if there is a comment about that).

I'd prefer the definition when "drained" means "the first output sample 
affected by the last valid input sample has been crossed by the hardware 
pointer of the real sink", i.e. so that it does not include the tail of 
the reverb. But this may be technically too complex to implement.

>           pa_memblockq_push(u->memblockq, &nchunk);
>           pa_memblock_unref(nchunk.memblock);
>       }
> @@ -265,6 +269,23 @@ static int sink_input_pop_cb(pa_sink_input *i, size_t nbytes, pa_memchunk *chunk
>       return 0;
>   }
>
> +/* Called from thread context */
> +
> +static bool sink_input_process_underrun_cb(pa_sink_input *i) {
> +    struct userdata *u;
> +
> +    pa_sink_input_assert_ref(i);
> +    pa_assert_se(u = i->userdata);
> +
> +    if (!PA_SINK_IS_LINKED(u->sink->thread_info.state) ||
> +        !PA_SINK_INPUT_IS_LINKED(u->sink_input->thread_info.state))
> +        return false;
> +
> +    pa_log_debug("sink_input_process_underrun_cb for virtual sink");
> +    return pa_sink_process_input_underruns(u->sink, 0);
> +}
> +
> +

This, again, relies on the generally-false assumption that the original 
and processed streams underrun simultaneously.

>   /* Called from I/O thread context */
>   static void sink_input_process_rewind_cb(pa_sink_input *i, size_t nbytes) {
>       struct userdata *u;
> @@ -586,6 +607,7 @@ int pa__init(pa_module*m) {
>           goto fail;
>
>       u->sink_input->pop = sink_input_pop_cb;
> +    u->sink_input->process_underrun = sink_input_process_underrun_cb;
>       u->sink_input->process_rewind = sink_input_process_rewind_cb;
>       u->sink_input->update_max_rewind = sink_input_update_max_rewind_cb;
>       u->sink_input->update_max_request = sink_input_update_max_request_cb;
>

-- 
Alexander E. Patrakov