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

[Georg Chini]

On 25.02.2015 03:25, Arun Raghavan wrote:
> On 23 February 2015 at 22:10, Alexander E. Patrakov <patrakov at gmail.com> wrote:
>> 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.
> The falce branch would only be called when recv_counter >
> send_counter, so you'd never actually have the zero case. AIUI, this
> all just accounting for any "in-flight" rewinds being propagated from
> the sink-input to the source-output.
>
> Also, overwriting the value as you do loses the latency values that
> were added in previous lines.
>
> -- Arun
> _______________________________________________
>
I cannot understand what is to discuss about that patch.
Calculating buffer = buffer + (send_counter - recv_counter)
in one branch and buffer = 2 * buffer - (recv_counter - send_counter)
looks very obviously wrong. What should be the reason for
doubling the buffer in the second case?