[pulseaudio-discuss] [PATCH] loopback: Calculate and track minimum possible latency

[Tanu Kaskinen]

On Wed, 2017-03-29 at 10:36 +0200, Georg Chini wrote:
> With the current code, the user can request any end-to-end latency. Because there
> is no protection against underruns, setting the latency too small will result in
> repetitive underruns.
> 
> This patch tries to mitigate the problem by calculating the minimum possible latency
> for the current combination of source and sink. The actual calculation has been put
> in a separate function so it can easily be changed. To keep the values up to date,
> changes in the latency ranges have to be tracked.
> 
> The calculated minimum latency is used to limit the configured latency.
> The minimum latency is only a "best guess", so the actual minimum may be much
> larger (for example for USB devices) or much smaller than the calculated value.
> 
> Port latency offsets are not yet handled properly, this will be done in a separate
> patch.

In what way are they not handled properly? Does it make sense to have
any code related to latency offsets in this patch if it needs to be
fixed later?

> ---
>  src/modules/module-loopback.c | 199 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 191 insertions(+), 8 deletions(-)
> 
> diff --git a/src/modules/module-loopback.c b/src/modules/module-loopback.c
> index 9b4ea49..e2cd986 100644
> --- a/src/modules/module-loopback.c
> +++ b/src/modules/module-loopback.c
> @@ -65,10 +65,14 @@ PA_MODULE_USAGE(
>  
>  #define DEFAULT_ADJUST_TIME_USEC (10*PA_USEC_PER_SEC)
>  
> +typedef struct loopback_msg loopback_msg;

https://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/Developer/CodingStyle/

"Exported structs should be typedef'ed so they can be used without
typing "struct" each time. Structs that are used only inside a single
source file should not be typedef'ed that way."

> +enum {
> +    LOOPBACK_MESSAGE_SOURCE_LATENCY_CHANGED,
> +    LOOPBACK_MESSAGE_SINK_LATENCY_CHANGED

I'd name these using "LATENCY_RANGE" instead of just "LATENCY", for
clarity.

> @@ -751,6 +860,12 @@ static int sink_input_process_msg_cb(pa_msgobject *obj, int code, void *data, in
>              u->output_thread_info.effective_source_latency = (pa_usec_t)offset;
>  
>              return 0;
> +
> +        case SINK_INPUT_MESSAGE_UPDATE_MIN_LATENCY:
> +
> +            u->output_thread_info.minimum_latency = (pa_usec_t)offset;

If the minimum latency grows, and the target latency grows too because
of that, I think we should push some silence to the memblockq to
account for the change in the target latency. Otherwise I think there's
 a risk of getting many underruns before the latency controller adjusts
to the new target.

> +/* Called from main context */
> +static int loopback_process_msg_cb(pa_msgobject *o, int code, void *userdata, int64_t offset, pa_memchunk *chunk) {
> +    struct loopback_msg *msg;
> +    struct userdata *u;
> +    pa_usec_t current_latency;
> +
> +    pa_assert(o);
> +    pa_assert_ctl_context();
> +
> +    msg = LOOPBACK_MSG(o);
> +    pa_assert_se(u = msg->userdata);
> +
> +    switch (code) {
> +
> +        case LOOPBACK_MESSAGE_SOURCE_LATENCY_CHANGED:
> +
> +            update_effective_source_latency(u, u->source_output->source, u->sink_input->sink);
> +            current_latency = pa_source_get_requested_latency(u->source_output->source);
> +            if (current_latency > u->configured_source_latency) {
> +                /* The minimum latency has changed to a value larger than the configured latency. so
> +                 * the source latency has been increased. The case that the minimum latency changes
> +                 * back to a smaller value is not handled because this is currently not implemented */

At the end of the comment, I think "because that never happens with the
current source implementations" would be clearer than "because this is
currently not implemented".

> +                pa_log_warn("Source minimum latency increased to %0.2f ms", (double)current_latency / PA_USEC_PER_MSEC);
> +                u->configured_source_latency = current_latency;
> +                update_latency_boundaries(u, u->source_output->source, u->sink_input->sink, false);
> +            }
> +
> +            return 0;
> +
> +        case LOOPBACK_MESSAGE_SINK_LATENCY_CHANGED:
> +
> +            current_latency = pa_sink_get_requested_latency(u->sink_input->sink);
> +            if (current_latency > u->configured_sink_latency) {
> +                /* The minimum latency has changed to a value larger than the configured latency, so
> +                 * the sink latency has been increased. The case that the minimum latency changes back
> +                 * to a smaller value is not handled because this is currently not implemented */

Same comment as above.

-- 
Tanu

https://www.patreon.com/tanuk