[pulseaudio-discuss] [PATCHv3 4/5] echo-cancel: Enable different blocksizes for sink and source

[Tanu Kaskinen]

On Wed, 2012-12-19 at 13:08 +0100, Stefan Huber wrote:
> diff --git a/src/modules/echo-cancel/module-echo-cancel.c b/src/modules/echo-cancel/module-echo-cancel.c
> index 103aef0..6f85bd3 100644
> --- a/src/modules/echo-cancel/module-echo-cancel.c
> +++ b/src/modules/echo-cancel/module-echo-cancel.c
> @@ -211,7 +211,8 @@ struct userdata {
>      pa_bool_t save_aec;
>  
>      pa_echo_canceller *ec;
> -    uint32_t blocksize;
> +    uint32_t source_blocksize;
> +    uint32_t sink_blocksize;
>  
>      pa_bool_t need_realign;
>  
> @@ -293,6 +294,7 @@ enum {
>      ECHO_CANCELLER_MESSAGE_SET_VOLUME,
>  };
>  
> +

Extra empty line added.

>  static int64_t calc_diff(struct userdata *u, struct snapshot *snapshot) {
>      int64_t diff_time, buffer_latency;
>      pa_usec_t plen, rlen, source_delay, sink_delay, recv_counter, send_counter;
> @@ -417,7 +419,7 @@ static int source_process_msg_cb(pa_msgobject *o, int code, void *data, int64_t
>                  /* Add the latency internal to our source output on top */
>                  pa_bytes_to_usec(pa_memblockq_get_length(u->source_output->thread_info.delay_memblockq), &u->source_output->source->sample_spec) +
>                  /* and the buffering we do on the source */
> -                pa_bytes_to_usec(u->blocksize, &u->source_output->source->sample_spec);
> +                pa_bytes_to_usec( u->source_blocksize, &u->source_output->source->sample_spec);

Extra space added

>  
>              return 0;
>  
> @@ -734,9 +736,11 @@ static void do_push_drift_comp(struct userdata *u) {
>       * samples left from the last iteration (to avoid double counting
>       * those remainder samples.
>       */
> +
> +

We don't use double empty lines. One is enough. And in this case, in my
opinion the original of zero empty lines is best, because the comment
applies only to the drift calculation, not anything after that (an empty
line would be appropriate if the comment would apply to more than only
the immediately following group of code).

>      drift = ((float)(plen - u->sink_rem) - (rlen - u->source_rem)) / ((float)(rlen - u->source_rem));
> -    u->sink_rem = plen % u->blocksize;
> -    u->source_rem = rlen % u->blocksize;
> +    u->sink_rem = plen % u->sink_blocksize;
> +    u->source_rem = rlen % u->source_blocksize;
>  
>      /* Now let the canceller work its drift compensation magic */
>      u->ec->set_drift(u->ec, drift);

> @@ -936,9 +940,9 @@ static void source_output_push_cb(pa_source_output *o, const pa_memchunk *chunk)
>              u->source_skip -= to_skip;
>          }
>  
> -        if (rlen && u->source_skip % u->blocksize) {
> -            u->sink_skip += u->blocksize - (u->source_skip % u->blocksize);
> -            u->source_skip -= (u->source_skip % u->blocksize);
> +        if (rlen && u->source_skip % u->source_blocksize) {
> +            u->sink_skip += (u->source_blocksize - (u->source_skip % u->source_blocksize)) * u->sink_blocksize / u->source_blocksize;

Is the multiplication totally safe? It doesn't seem unconceivable that
multiplying two blocksizes could some day overflow the 32-bit space. A
cast to uint64_t should probably be added.

-- 
Tanu