Using buffered signal to calculate the level of echo cancellation.

Using buffered signal to calculate the level of echo cancellation.

The level of the error signal after linear echo cancellation was based on non-buffered signal while that of the near-end and far-end signal based on buffered signal. This discrepancy made the comparison of them unfair.

This CL is to make calculating the error level rely on the same buffering.

BUG=

Committed: https://crrev.com/691b8369ff1a77144d715b18675f610dfd3d60e3
Cr-Commit-Position: refs/heads/master@{#11408}

[minyue-webrtc, 2016-01-22 14:37:27]

Patchset #1 (id:1) has been deleted

[minyue-webrtc, 2016-01-22 14:37:33]

Patchset #1 (id:20001) has been deleted

[minyue-webrtc, 2016-01-22 14:43:49]

Description was changed from

==========
using buffered error signal to calculate linoutlevel


Removing test codes


Merge branch 'ace_farend_timedomain' into UpdateLevelInTime


patch finished


should get time domain signal

BUG=
==========

to

==========
Using buffered signal to calculate the level of echo cancellation.

The level of the error signal after linear echo cancellation was based on
non-buffered signal while that of the near-end and far-end signal based on
buffered signal. This discrepancy made the comparison of them unfair.

This CL is to make calculating the error level rely on the same buffering (which
was already there).

BUG=
==========

[minyue-webrtc, 2016-01-22 14:43:49]

minyue@webrtc.org changed reviewers:
+ peah@webrtc.org

[minyue-webrtc, 2016-01-22 14:44:57]

Hi Per,

This is another change in the AEC metric calculation. Would you help review?
Thanks!

[minyue-webrtc, 2016-01-25 11:37:55]

Hi Per,

If you have not reviewed this yet, skip patch set 1. There is a bug.

Also to add:

We have unittest that checks the metrics. Why don't they fail?

Due to a different buffering mechanism for the error level calculation, the
level updating receives a different value each time (I've checked this).
However, the metric is based on an averaged value and, therefore gives a similar
value to the old buffering mechanism.

In addition, the metrics are rounded to the integers and therefore are
insensitive to tiny differences.

[peah-webrtc, 2016-01-25 12:23:02]

https://codereview.webrtc.org/1510873004/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1510873004/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:1372: // Update eBuf with echo
subtractor output.
This may be affected by changes in the future. As it is now, linoutLevel is
always the level of the linear filter output. Currently, that is equivalent to
echo_subtractor_output but that will change. For instance, during filter
divergence, echo_subtractor_output should be equal to the microphone signal
rather than the linear filter output.

That change may be fine though, depending upon how the metric should be used. If
it is a metric over the ERLE achieved by the EchoSubtraction function call, that
is fine, but if it is a metric of the ERLE currently achieved by the adaptive
filter, that will in that case no longer be correct.

 Just wanted to give a hea

[minyue-webrtc, 2016-01-25 14:26:55]

https://codereview.webrtc.org/1510873004/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1510873004/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:1372: // Update eBuf with echo
subtractor output.
On 2016/01/25 12:23:02, peah-webrtc wrote:
> This may be affected by changes in the future. As it is now, linoutLevel is
> always the level of the linear filter output. Currently, that is equivalent to
> echo_subtractor_output but that will change. For instance, during filter
> divergence, echo_subtractor_output should be equal to the microphone signal
> rather than the linear filter output.
> 
> That change may be fine though, depending upon how the metric should be used.
If
> it is a metric over the ERLE achieved by the EchoSubtraction function call,
that
> is fine, but if it is a metric of the ERLE currently achieved by the adaptive
> filter, that will in that case no longer be correct.
> 
>  Just wanted to give a hea

Good point. It is true that when the linear filter diverges, we bypass it and
the performance of the linear filter does not correspond to any echo
cancellation performance. However, the divergence status can still be used since
1. it works as a double-talk detector
2. it should not toggle too often

Regarding ERLE, this change does not affect it. It affects only A_NLP.

[peah-webrtc, 2016-01-25 16:28:16]

lgtm

However,
Would it be possible to compute the levels for all the metrics over PART_LEN
samples instead of PART_LEN2 samples? It feels more natural to do these kind of
computations using the block length rather than the double block length.
Furthermore, that would be cheaper (half the complexity, as currently the power
of each block is computed twice (over two following frames).
It would make sense to do such a transition while you are anyway doing other
changes to the metrics.

[minyue-webrtc, 2016-01-26 15:58:26]

On 2016/01/25 16:28:16, peah-webrtc wrote:
> lgtm
> 
> However,
> Would it be possible to compute the levels for all the metrics over PART_LEN
> samples instead of PART_LEN2 samples? It feels more natural to do these kind
of
> computations using the block length rather than the double block length.
> Furthermore, that would be cheaper (half the complexity, as currently the
power
> of each block is computed twice (over two following frames).
> It would make sense to do such a transition while you are anyway doing other
> changes to the metrics.

I like this idea. I will try.

[minyue-webrtc, 2016-01-26 16:42:01]

Description was changed from

==========
Using buffered signal to calculate the level of echo cancellation.

The level of the error signal after linear echo cancellation was based on
non-buffered signal while that of the near-end and far-end signal based on
buffered signal. This discrepancy made the comparison of them unfair.

This CL is to make calculating the error level rely on the same buffering (which
was already there).

BUG=
==========

to

==========
Using buffered signal to calculate the level of echo cancellation.

The level of the error signal after linear echo cancellation was based on
non-buffered signal while that of the near-end and far-end signal based on
buffered signal. This discrepancy made the comparison of them unfair.

This CL is to make calculating the error level rely on the same buffering.

BUG=
==========

[minyue-webrtc, 2016-01-26 16:46:47]

I changed the implementation. Now the unittest has to change a bit, the new
result differ with the old one by at the most a rounding error.

https://codereview.webrtc.org/1510873004/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1510873004/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:1271: if (aec->metricsMode == 1)
{
I reduced the buffer sizes for calculating farlevel and nearlevel. I also moved
the code a bit to make it read more nicely.

[peah-webrtc, 2016-01-27 22:27:19]

On 2016/01/26 16:46:47, minyue-webrtc wrote:
> I changed the implementation. Now the unittest has to change a bit, the new
> result differ with the old one by at the most a rounding error.
> 
>
https://codereview.webrtc.org/1510873004/diff/80001/webrtc/modules/audio_proc...
> File webrtc/modules/audio_processing/aec/aec_core.c (right):
> 
>
https://codereview.webrtc.org/1510873004/diff/80001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/aec/aec_core.c:1271: if (aec->metricsMode ==
1)
> {
> I reduced the buffer sizes for calculating farlevel and nearlevel. I also
moved
> the code a bit to make it read more nicely.

Nice!!!
lgtm

[minyue-webrtc, 2016-01-27 22:29:18]

The CQ bit was checked by minyue@webrtc.org

[commit-bot: I haz the power, 2016-01-27 22:29:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1510873004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1510873004/80001

[commit-bot: I haz the power, 2016-01-27 23:44:57]

Description was changed from

==========
Using buffered signal to calculate the level of echo cancellation.

The level of the error signal after linear echo cancellation was based on
non-buffered signal while that of the near-end and far-end signal based on
buffered signal. This discrepancy made the comparison of them unfair.

This CL is to make calculating the error level rely on the same buffering.

BUG=
==========

to

==========
Using buffered signal to calculate the level of echo cancellation.

The level of the error signal after linear echo cancellation was based on
non-buffered signal while that of the near-end and far-end signal based on
buffered signal. This discrepancy made the comparison of them unfair.

This CL is to make calculating the error level rely on the same buffering.

BUG=
==========

[commit-bot: I haz the power, 2016-01-27 23:44:58]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-01-27 23:45:06]

Description was changed from

==========
Using buffered signal to calculate the level of echo cancellation.

The level of the error signal after linear echo cancellation was based on
non-buffered signal while that of the near-end and far-end signal based on
buffered signal. This discrepancy made the comparison of them unfair.

This CL is to make calculating the error level rely on the same buffering.

BUG=
==========

to

==========
Using buffered signal to calculate the level of echo cancellation.

The level of the error signal after linear echo cancellation was based on
non-buffered signal while that of the near-end and far-end signal based on
buffered signal. This discrepancy made the comparison of them unfair.

This CL is to make calculating the error level rely on the same buffering.

BUG=

Committed: https://crrev.com/691b8369ff1a77144d715b18675f610dfd3d60e3
Cr-Commit-Position: refs/heads/master@{#11408}
==========

[commit-bot: I haz the power, 2016-01-27 23:45:07]

Patchset 3 (id:??) landed as
https://crrev.com/691b8369ff1a77144d715b18675f610dfd3d60e3
Cr-Commit-Position: refs/heads/master@{#11408}