[pulseaudio-discuss] [PATCH] echo-cancel: fix the obviously-wrong "buffer+=buffer" logic

[Alexander E. Patrakov]

23.02.2015 21:12, Peter Meerwald wrote:
> Hi Alexander,
>
>> Same bug as in module-loopback, pointed out by Georg Chini in a private
>> email.
>
> can you please elaborate? I fail to see the obviousness

The recv_counter == send_counter case looks like a neutral case here 
(although I might be wrong), with no inherent discontinuity at this 
point. So, any branch of the "if" should give the same result in this 
corner case, and the whole point of the "if"/"PA_CLIP_SUB" construction, 
as I see it, is to avoid producing negative numbers by mistake.

The true branch simplifies to: buffer_latency += 0

The original false branch simplifies to: buffer_latency += 
PA_CLIP_SUB(buffer_latency, 0), which, for buffer_latency > 0, means 
buffer_latency += buffer_latency - 0. I.e. doubling the latency (as 
opposed to not changing it), which looks strange even by itself.

>
> p.
>
>>
>> Signed-off-by: Alexander E. Patrakov <patrakov at gmail.com>
>> ---
>>   src/modules/echo-cancel/module-echo-cancel.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/modules/echo-cancel/module-echo-cancel.c b/src/modules/echo-cancel/module-echo-cancel.c
>> index b95a965..639cd41 100644
>> --- a/src/modules/echo-cancel/module-echo-cancel.c
>> +++ b/src/modules/echo-cancel/module-echo-cancel.c
>> @@ -315,7 +315,7 @@ static int64_t calc_diff(struct userdata *u, struct snapshot *snapshot) {
>>       if (recv_counter <= send_counter)
>>           buffer_latency += (int64_t) (send_counter - recv_counter);
>>       else
>> -        buffer_latency += PA_CLIP_SUB(buffer_latency, (int64_t) (recv_counter - send_counter));
>> +        buffer_latency = PA_CLIP_SUB(buffer_latency, (int64_t) (recv_counter - send_counter));
>>
>>       /* capture and playback are perfectly aligned when diff_time is 0 */
>>       diff_time = (snapshot->sink_now + snapshot->sink_latency - buffer_latency) -
>> --
>> 2.2.1
>>
>> _______________________________________________
>> pulseaudio-discuss mailing list
>> pulseaudio-discuss at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
>>
>

-- 
Alexander E. Patrakov