[pulseaudio-discuss] [PATCH v6 14/25] source.c, sink.c: Implement pa_{source, sink}_get_raw_latency_within_thread() calls

[Tanu Kaskinen]

On Sun, 2016-06-05 at 21:05 +0200, Georg Chini wrote:
> In certain situations, the reported sink or source latency may become
> negative.

What are those certain situations?

> This does not indicate that the latency is indeed negative but can be considered
> merely an offset error. The current get_latency() calls truncate negative latencies
> because they do not make sense from a physical point of view.
> In the context of module loopback, negative source or sink latencies are acceptable
> because the overall latency never becomes negative. Truncating negative values leads
> to discontinuities in the latency reports, therefore this patch implements functions
> in source.c and sink.c which can return the raw value without truncation.

What's the practical problem with the discontinuities? A negative
latency report is always incorrect, so changing it to 0 can only make
the latency report closer to truth. The explanation isn't sufficient
for me to understand why that is such a big problem that it warrants
adding this stuff to the core.

If this stuff makes sense, there should be a comment somewhere
explaining what the "raw latency" means and why we care about it.

> The corresponding PA_SINK_MESSAGE_GET_RAW_LATENCY needs to be handled by the driver.
> 
> ---
>  src/pulsecore/sink.c   | 30 ++++++++++++++++++++++++++++++
>  src/pulsecore/sink.h   |  2 ++
>  src/pulsecore/source.c | 30 ++++++++++++++++++++++++++++++
>  src/pulsecore/source.h |  2 ++
>  4 files changed, 64 insertions(+)
> 
> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> index 3f1ef72..2b72279 100644
> --- a/src/pulsecore/sink.c
> +++ b/src/pulsecore/sink.c
> @@ -1550,6 +1550,35 @@ pa_usec_t pa_sink_get_latency_within_thread(pa_sink *s) {
>      return usec;
>  }
>  
> +/* Called from IO thread */
> +int64_t pa_sink_get_raw_latency_within_thread(pa_sink *s) {
> +    int64_t usec = 0;
> +    pa_msgobject *o;
> +
> +    pa_sink_assert_ref(s);
> +    pa_sink_assert_io_context(s);
> +    pa_assert(PA_SINK_IS_LINKED(s->thread_info.state));
> +
> +    /* The returned value is supposed to be in the time domain of the sound card! */
> +
> +    if (s->thread_info.state == PA_SINK_SUSPENDED)
> +        return 0;
> +
> +    if (!(s->flags & PA_SINK_LATENCY))
> +        return 0;
> +
> +    o = PA_MSGOBJECT(s);
> +
> +    /* FIXME: We probably should make this a proper vtable callback instead of going through process_msg() */
> +
> +    if (o->process_msg(o, PA_SINK_MESSAGE_GET_RAW_LATENCY, &usec, 0, NULL) < 0) {
> +       if (o->process_msg(o, PA_SINK_MESSAGE_GET_LATENCY, &usec, 0, NULL) < 0)
> +          return 0;

This code path ignores the latency offset. I don't think it makes sense
to return here, the failure can just be ignored.

-- 
Tanu