[pulseaudio-discuss] [PATCH 2/4] echo-cancel: Fix calc_diff for asymmetric sample specs

[Tanu Kaskinen]

On Tue, 2012-12-04 at 14:54 +0100, Peter Meerwald wrote:
> From: Stefan Huber <s.huber at bct-electronic.com>
> 
> In case that source and sink use different sample specs (e.g., different
> number of channels) the computation of the latency difference fails.
> To fix this, we obtain the corresponding latencies in terms of time using
> the respective sample specs instead of buffer sizes.
> 
> Signed-off-by: Stefan Huber <s.huber at bct-electronic.com>
> Acked-by: Peter Meerwald <p.meerwald at bct-electronic.com>
> ---
>  src/modules/echo-cancel/module-echo-cancel.c |   26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)

Disclaimer: I don't understand the code that is being changed, so all I
can do is to check that the conversion from bytes to usec looks
superficially correct.

> diff --git a/src/modules/echo-cancel/module-echo-cancel.c b/src/modules/echo-cancel/module-echo-cancel.c
> index 26ac30b..8d89d2d 100644
> --- a/src/modules/echo-cancel/module-echo-cancel.c
> +++ b/src/modules/echo-cancel/module-echo-cancel.c
> @@ -294,24 +294,28 @@ enum {
>  };
>  
>  static int64_t calc_diff(struct userdata *u, struct snapshot *snapshot) {
> -    int64_t buffer, diff_time, buffer_latency;
> +    int64_t diff_time, buffer_latency;
> +    int64_t plen, rlen, source_delay, sink_delay, recv_counter, send_counter;

I would prefer pa_usec_t for the variables that contain (non-negative)
time values.

>  
>      /* get the number of samples between capture and playback */

Isn't this comment now out of date? Instead of number of samples, it
should talk about latency or something like that.

> -    if (snapshot->plen > snapshot->rlen)
> -        buffer = snapshot->plen - snapshot->rlen;
> +    plen = pa_bytes_to_usec(snapshot->plen, &u->sink_input->sample_spec);
> +    rlen = pa_bytes_to_usec(snapshot->plen, &u->source_output->sample_spec);

In that last line, I guess the parameter to pa_bytes_to_usec() be
snapshot->rlen instead of snapshot->plen?

> +    if (plen > rlen)
> +        buffer_latency = plen-rlen;
>      else
> -        buffer = 0;
> +        buffer_latency = 0;
>  
> -    buffer += snapshot->source_delay + snapshot->sink_delay;
> +    source_delay = pa_bytes_to_usec(snapshot->source_delay, &u->source_output->sample_spec);
> +    sink_delay = pa_bytes_to_usec(snapshot->sink_delay, &u->sink_input->sample_spec);
> +    buffer_latency += source_delay + sink_delay;
>  
>      /* add the amount of samples not yet transferred to the source context */

Again, the comment talks about samples while the code works with time
units.

-- 
Tanu