|
|
Created:
5 years ago by minyue-webrtc Modified:
4 years, 10 months ago Reviewers:
peah-webrtc CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, the sun, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUsing 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}
Patch Set 1 : #Patch Set 2 : correcting #
Total comments: 2
Patch Set 3 : reducing buffer for nearLevel and farLevel #
Total comments: 1
Messages
Total messages: 19 (8 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
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.org changed reviewers: + peah@webrtc.org
Hi Per, This is another change in the AEC metric calculation. Would you help review? Thanks!
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.
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
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.
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.
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.
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= ==========
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.
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
The CQ bit was checked by minyue@webrtc.org
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
Message was sent while issue was closed.
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= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/691b8369ff1a77144d715b18675f610dfd3d60e3 Cr-Commit-Position: refs/heads/master@{#11408} |