[pulseaudio-discuss] [PATCH 3/6] source, sink, core: Setting latency offsets of individual sources and sinks.

[Tanu Kaskinen]

On Sat, 2016-01-23 at 12:31 +1100, Chris Billington wrote:

> Some sinks and sources do not have ports associated with them, such as a null
> sink. Nonetheless sources and sinks with no ports can be used for sound
> input/output with an associated latency, for example by capturing output from
> a null sink and directing over a network, which is how some implementations of
> network audio streaming protocols interact with PulseAudio.
> 
> To keep graphics in sync with audio in these cases, the application/plugin must
> be able to report the latency due to network transport/buffering to PulseAudio
> and have it included in the total latency for that source or sink.
> 
> Previously, only the latency offset of a whole port could be set.
> You could call pa_sink_set_latency_offset(), but the value you passed in would
> be overwritten on change to a new port - that value was really just a cached
> value of the port's latency offset. So those variables and functions have since
> (in the previous commit) been renamed pa_sink_set_port_latency_offset() etc, to
> distinguish them from the ones introduced in this commit.
> 
> This patch adds set_sink_latency_offset() and set_source_latency offset(),
> and the corresponding members of the sink and source structs.
> 
> It also adds corresponding hooks PA_CORE_HOOK_SINK_LATENCY_OFFSET_CHANGED
> and PA_CORE_HOOK_SOURCE_LATENCY_OFFSET_CHANGED.
> 
> The total latency reported by a source or sink now includes both the port
> latency offset (if any) as well as the source or sink's own latency offset.

Thanks, looks mostly good. I'll point out a few small issues below.

Since it's been already many months since you submitted these patches,
please let me know if during that time you have lost interest to finish
off this work (it would be understandable).

> @@ -125,6 +125,8 @@ typedef enum pa_core_hook {
>      PA_CORE_HOOK_CARD_PROFILE_AVAILABLE_CHANGED,
>      PA_CORE_HOOK_PORT_AVAILABLE_CHANGED,
>      PA_CORE_HOOK_PORT_LATENCY_OFFSET_CHANGED,
> +    PA_CORE_HOOK_SINK_LATENCY_OFFSET_CHANGED,
> +    PA_CORE_HOOK_SOURCE_LATENCY_OFFSET_CHANGED,

A cosmetic note about the hook list ordering: I prefer different
grouping - e.g. all sink hooks should be together.

> @@ -314,6 +319,11 @@ pa_sink* pa_sink_new(
>      else
>          s->port_latency_offset = 0;
>  
> +    if (data->latency_offset)
> +        s->latency_offset = data->latency_offset;
> +    else
> +        s->latency_offset = 0;

This can be replaced with just

    s->latency_offset = data->latency_offset;

without any changes in behaviour. The same applies to the code in
source.c.

> @@ -1512,6 +1525,12 @@ pa_usec_t pa_sink_get_latency(pa_sink *s) {
>      else
>          usec = 0;
>  
> +    /* Similarly checking for underflow */
> +    if (-s->latency_offset <= (int64_t) usec)
> +        usec += s->latency_offset;
> +    else
> +        usec = 0;

The two offsets should be summed together and then added to usec,
rather than adding them to usec one by one. The code in the patch
doesn't work correctly if, for example, the initial usec is 1000, the
port latency offset -2000 and the sink latency offset is 2000. The
final usec should be unchanged (1000), because the two offsets undo
each other, but this code results in the final usec being 2000.

This applies also to pa_sink_get_latency_within_thread() and the
corresponding functions in source.c.

> @@ -1096,6 +1107,8 @@ pa_usec_t pa_source_get_latency(pa_source *s) {
>  
>      pa_assert_se(pa_asyncmsgq_send(s->asyncmsgq, PA_MSGOBJECT(s), PA_SOURCE_MESSAGE_GET_LATENCY, &usec, 0, NULL) == 0);
>  
> +    /* Total latency offset is the sum of the port latency offset and the sink latency offset */

Copy-paste mistake: s/sink/source/

> @@ -1130,6 +1149,8 @@ pa_usec_t pa_source_get_latency_within_thread(pa_source *s) {
>      if (o->process_msg(o, PA_SOURCE_MESSAGE_GET_LATENCY, &usec, 0, NULL) < 0)
>          return -1;
>  
> +    /* Total latency offset is the sum of the port latency offset and the sink latency offset */

The same copy-paste error here too.

-- 
Tanu