[pulseaudio-discuss] [PATCH 1/4] device-port: Add a latency variable to the port struct

[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 port struct and functions to
> for handling a custom latency.
> 
> We can now attach a latency to the port which will be added to the
> sink/source latency if the port is active.
> ---
>  src/pulsecore/device-port.c |   34 ++++++++++++++++++++++++++++++++++
>  src/pulsecore/device-port.h |    6 ++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/src/pulsecore/device-port.c b/src/pulsecore/device-port.c
> index 5870913..c5639b8 100644
> --- a/src/pulsecore/device-port.c
> +++ b/src/pulsecore/device-port.c
> @@ -97,6 +97,7 @@ pa_device_port *pa_device_port_new(pa_core *c, const char *name, const char *des
>      p->profiles = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
>      p->is_input = FALSE;
>      p->is_output = FALSE;
> +    p->latency_offset = 0;
>      p->proplist = pa_proplist_new();
>  
>      return p;
> @@ -112,3 +113,36 @@ void pa_device_port_hashmap_free(pa_hashmap *h) {
>  
>      pa_hashmap_free(h, NULL, NULL);
>  }
> +
> +void pa_device_port_set_latency_offset(pa_device_port *p, pa_usec_t latency) {

I think "offset" would be a more descriptive parameter name than
"latency", because the offset is not really a stand-alone latency value.

> +    uint32_t state;
> +    union {
> +        pa_sink *sink;
> +        pa_source *source;
> +    } data;
> +
> +    pa_assert(p);
> +
> +    p->latency_offset = latency;
> +
> +    if (p->is_output) {

The sink pointer could be declared here, because it's not used outside
this if block. That would avoid the need for the union.

> +        PA_IDXSET_FOREACH(data.sink, p->core->sinks, state)
> +            if (data.sink->active_port == p) {
> +                pa_sink_set_latency_offset(data.sink, p->latency_offset);

It would be good to keep the code compilable in each patch. Here
pa_sink_set_latency_offset() is used before it exists, so the
compilation breaks. This matters at least when doing bisecting.

Maybe the most straight-forward way of fixing this would be to squash
the three first patches together.

> +                break;
> +            }
> +
> +    } else {
> +        PA_IDXSET_FOREACH(data.source, p->core->sources, state)
> +            if (data.source->active_port == p) {
> +                pa_source_set_latency_offset(data.source, p->latency_offset);
> +                break;
> +        }

Misaligned closing bracket.

> +    }
> +}
> +
> +pa_usec_t pa_device_port_get_latency_offset(pa_device_port *p) {
> +    pa_assert(p);
> +
> +    return p->latency_offset;
> +}

We tend not to have trivial getter functions in the server-side APIs,
unless the struct definition is hidden (pa_device_port definition is not
hidden). Trivial setter functions are fine, at least in my opinion, and
I even recommend using them (I don't like when component state is
directly changed from code outside that component).

-- 
Tanu