[pulseaudio-discuss] [PATCH 2/4] sink: Add a latency offset which is inherited from the port

[Tanu Kaskinen]

On Sun, 2012-06-17 at 14:47 +0200, poljar (Damir Jelic) wrote:
> From: poljar <poljarinho at gmail.com>
> 
> A latency offset variable was added to the sink struct.
> 
> This variable gets automatically populated with the latency from the
> currently active port.
> ---
>  src/pulsecore/sink.c |   24 ++++++++++++++++++++++++
>  src/pulsecore/sink.h |    7 +++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> index e4c343d..a39d34d 100644
> --- a/src/pulsecore/sink.c
> +++ b/src/pulsecore/sink.c
> @@ -308,6 +308,11 @@ pa_sink* pa_sink_new(
>                  s->active_port = p;
>      }
>  
> +    if (s->active_port)
> +        pa_sink_set_latency_offset(s, pa_device_port_get_latency_offset(s->active_port));
> +    else
> +        pa_sink_set_latency_offset(s, 0);
> +
>      s->save_volume = data->save_volume;
>      s->save_muted = data->save_muted;
>  
> @@ -338,6 +343,7 @@ pa_sink* pa_sink_new(
>      pa_sw_cvolume_multiply(&s->thread_info.current_hw_volume, &s->soft_volume, &s->real_volume);
>      s->thread_info.volume_change_safety_margin = core->deferred_volume_safety_margin_usec;
>      s->thread_info.volume_change_extra_delay = core->deferred_volume_extra_delay_usec;
> +    s->thread_info.latency_offset_copy = &s->latency_offset;
>  
>      /* FIXME: This should probably be moved to pa_sink_put() */
>      pa_assert_se(pa_idxset_put(core->sinks, s, &s->index) >= 0);
> @@ -1422,6 +1428,8 @@ pa_usec_t pa_sink_get_latency(pa_sink *s) {
>  
>      pa_assert_se(pa_asyncmsgq_send(s->asyncmsgq, PA_MSGOBJECT(s), PA_SINK_MESSAGE_GET_LATENCY, &usec, 0, NULL) == 0);
>  
> +    usec += pa_sink_get_latency_offset(s);
> +
>      return usec;
>  }
>  
> @@ -1449,6 +1457,8 @@ pa_usec_t pa_sink_get_latency_within_thread(pa_sink *s) {
>      if (o->process_msg(o, PA_SINK_MESSAGE_GET_LATENCY, &usec, 0, NULL) < 0)
>          return -1;
>  
> +    usec += pa_sink_get_latency_offset(s);
> +
>      return usec;
>  }
>  
> @@ -3223,6 +3233,18 @@ void pa_sink_set_fixed_latency_within_thread(pa_sink *s, pa_usec_t latency) {
>      pa_source_set_fixed_latency_within_thread(s->monitor_source, latency);
>  }
>  
> +void pa_sink_set_latency_offset(pa_sink *s, pa_usec_t latency) {

All functions in sink.c should have a comment about from which thread
they are supposed to be called.

> +    pa_sink_assert_ref(s);
> +
> +    s->latency_offset = latency;
> +}
> +
> +pa_usec_t pa_sink_get_latency_offset(pa_sink *s) {
> +    pa_sink_assert_ref(s);
> +
> +    return s->latency_offset;
> +}

The comment about trivial getter functions applies here too.

> +
>  /* Called from main context */
>  size_t pa_sink_get_max_rewind(pa_sink *s) {
>      size_t r;
> @@ -3293,6 +3315,8 @@ int pa_sink_set_port(pa_sink *s, const char *name, pa_bool_t save) {
>      s->active_port = port;
>      s->save_port = save;
>  
> +    pa_sink_set_latency_offset(s, pa_device_port_get_latency_offset(port));
> +
>      pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SINK_PORT_CHANGED], s);
>  
>      return 0;
> diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
> index 0b5048a..cea988e 100644
> --- a/src/pulsecore/sink.h
> +++ b/src/pulsecore/sink.h
> @@ -109,8 +109,10 @@ struct pa_sink {
>  
>      pa_memchunk silence;
>  
> +    /* The latency offset is inherited from the currently active port */
>      pa_hashmap *ports;
>      pa_device_port *active_port;
> +    pa_usec_t latency_offset;
>      pa_atomic_t mixer_dirty;
>  
>      unsigned priority;
> @@ -268,6 +270,9 @@ struct pa_sink {
>           * in changing it */
>          pa_usec_t fixed_latency; /* for sinks with PA_SINK_DYNAMIC_LATENCY this is 0 */
>  
> +        /* This points to sink->latency_offset */
> +        pa_usec_t *latency_offset_copy;

It's not a copy if it points to the same memory location that is
supposed to be copied. This needs to be a real copy. The way to update
this is to send a message (PA_SINK_MESSAGE_SET_LATENCY_OFFSET) to the IO
thread when pa_sink.latency_offset is changed. The message handler can
then safely copy the value from pa_sink.latency_offset to
pa_sink.thread_info.latency_offset, because when the main thread sends
the message with pa_asyncmsgq_send() to the IO thread, the main thread
will stop until the IO thread has finished handling the message.

(Sidenote: the difference between pa_asyncmsgq_send() and
pa_asyncmsgq_post() is that send() is synchronous, i.e. the main thread
waits until the message is processed, and post() is asynchronous, i.e.
the function returns before the message is handled. Usually when the
main thread sends something to an IO thread, send() is used, because
it's much simpler to reason about synchronous calls. Usually when an IO
thread sends something to the main thread, post() is used, because the
IO thread (realtime thread) can not afford to wait for the main thread
(non-realtime thread).)

I'd drop the "_copy" part from the variable name. There are also other
copied values in thread_info, and they don't have a "_copy" suffix
either.

-- 
Tanu