Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1967033002:
Corrected the delay agnostic AEC behavior during periods of silent farend signal. (Closed)
|
Created:
4 years, 7 months ago by peah-webrtc Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-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. |
DescriptionCorrected the delay agnostic AEC behavior during periods of silent farend signal.
Added conditional updating of the statistics and the delay estimate so that
updates are only done when the farend is non-stationary.
The reason for this is that all the values that go into the updating of the
statistics, and that in turn are also used to update the delay, are frozen
when the farend signal is non-stationary. Therefore, when the farend signal
is silent (stationary), the last estimates present before the silent (stationary)
period began are used to continue to update the statistics. This is a problem as
the updating is done in a manner that assumes that the estimates continue
to be updated.
This CL conditions the updating based on stationarity instead of silence
as both are treated in the same manner in the delay agnostic AEC.
This makes sense theoretically as the delay agnostic AEC operates on
analyzing power deviations (in bands) from a slowly updated average power and
therefore for a stationary signal will have no such deviations to base its analysis
on.
BUG=webrtc:5875, chromium:576624
NOTRY=True
Committed: https://crrev.com/b1fc54d33e109b9bab10114b3aa0d47eb90b799d
Cr-Commit-Position: refs/heads/master@{#12700}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Changes according to reviewer comments #
Total comments: 2
Patch Set 3 : Corrected the order of includes #Patch Set 4 : Updated the unittest reference file with a new standard deviation value obtained from APM with the … #Patch Set 5 : Updated the unittest reference file for mac with a new standard deviation value obtained from APM w… #
Messages
Total messages: 33 (17 generated)
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Nice! I have two comments in the code, and another two for the commit message: "freezed" -> "frozen" "as the both are treated" -> drop "the" https://codereview.webrtc.org/1967033002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/utility/delay_estimator.cc (right): https://codereview.webrtc.org/1967033002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/utility/delay_estimator.cc:622: bool non_stationary_farend = false; You are traversing the array from end to beginning. If this is not important, I suggest a one-liner instead: const bool non_stationary_farend = std::any_of(self->farend->far_bit_counts, self->farend->far_bit_counts + self->history_size, [](int a){ return a > 0; }); I don't think it works to use any_of to backwards-traverse a raw array. One option is to wrap the array (just locally) in an ArrayView and get iterators out of it, but ArrayView currently has no reverse-iterators implemented. https://codereview.webrtc.org/1967033002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/utility/delay_estimator.cc:632: // as the underlying estimates are otherwise freezed. frozen
Description was changed from ========== Corrected the delay agnostic AEC behavior during periods of silent farend signal. Added conditional updating of the statistics and the delay estimate so that updates are only done when the farend is non-stationary. The reason for this is that all the values that go into the updating of the statistics, and that in turn are also used to update the delay, are freezed when the farend signal is non-stationary. Therefore, when the farend signal is silent (stationary), the last estimates present before the silent (stationary) period began are used to continue to update the statistics. This is a problem as the updating is done in a manner that assumes that the estimates continue to be updated. This CL conditions the updating based on stationarity instead of silence as the both are treated in the same manner in the delay agnostic AEC. This makes sense theoretically as the delay agnostic AEC operates on analyzing power deviations (in bands) from a slowly updated average power and therefore for a stationary signal will have no such deviations to base its analysis on. BUG=webrtc:5875, chromium:576624 ========== to ========== Corrected the delay agnostic AEC behavior during periods of silent farend signal. Added conditional updating of the statistics and the delay estimate so that updates are only done when the farend is non-stationary. The reason for this is that all the values that go into the updating of the statistics, and that in turn are also used to update the delay, are frozen when the farend signal is non-stationary. Therefore, when the farend signal is silent (stationary), the last estimates present before the silent (stationary) period began are used to continue to update the statistics. This is a problem as the updating is done in a manner that assumes that the estimates continue to be updated. This CL conditions the updating based on stationarity instead of silence as the both are treated in the same manner in the delay agnostic AEC. This makes sense theoretically as the delay agnostic AEC operates on analyzing power deviations (in bands) from a slowly updated average power and therefore for a stationary signal will have no such deviations to base its analysis on. BUG=webrtc:5875, chromium:576624 ==========
Description was changed from ========== Corrected the delay agnostic AEC behavior during periods of silent farend signal. Added conditional updating of the statistics and the delay estimate so that updates are only done when the farend is non-stationary. The reason for this is that all the values that go into the updating of the statistics, and that in turn are also used to update the delay, are frozen when the farend signal is non-stationary. Therefore, when the farend signal is silent (stationary), the last estimates present before the silent (stationary) period began are used to continue to update the statistics. This is a problem as the updating is done in a manner that assumes that the estimates continue to be updated. This CL conditions the updating based on stationarity instead of silence as the both are treated in the same manner in the delay agnostic AEC. This makes sense theoretically as the delay agnostic AEC operates on analyzing power deviations (in bands) from a slowly updated average power and therefore for a stationary signal will have no such deviations to base its analysis on. BUG=webrtc:5875, chromium:576624 ========== to ========== Corrected the delay agnostic AEC behavior during periods of silent farend signal. Added conditional updating of the statistics and the delay estimate so that updates are only done when the farend is non-stationary. The reason for this is that all the values that go into the updating of the statistics, and that in turn are also used to update the delay, are frozen when the farend signal is non-stationary. Therefore, when the farend signal is silent (stationary), the last estimates present before the silent (stationary) period began are used to continue to update the statistics. This is a problem as the updating is done in a manner that assumes that the estimates continue to be updated. This CL conditions the updating based on stationarity instead of silence as both are treated in the same manner in the delay agnostic AEC. This makes sense theoretically as the delay agnostic AEC operates on analyzing power deviations (in bands) from a slowly updated average power and therefore for a stationary signal will have no such deviations to base its analysis on. BUG=webrtc:5875, chromium:576624 ==========
https://codereview.webrtc.org/1967033002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/utility/delay_estimator.cc (right): https://codereview.webrtc.org/1967033002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/utility/delay_estimator.cc:622: bool non_stationary_farend = false; On 2016/05/11 12:39:48, hlundin-webrtc wrote: > You are traversing the array from end to beginning. If this is not important, I > suggest a one-liner instead: > > const bool non_stationary_farend = std::any_of(self->farend->far_bit_counts, > self->farend->far_bit_counts + self->history_size, [](int a){ return a > 0; }); > > I don't think it works to use any_of to backwards-traverse a raw array. One > option is to wrap the array (just locally) in an ArrayView and get iterators out > of it, but ArrayView currently has no reverse-iterators implemented. Wow! That was great!! And works splendidly. Thanks! Done. https://codereview.webrtc.org/1967033002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/utility/delay_estimator.cc:632: // as the underlying estimates are otherwise freezed. On 2016/05/11 12:39:48, hlundin-webrtc wrote: > frozen Done.
lgtm with one nit. https://codereview.webrtc.org/1967033002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/utility/delay_estimator.cc (right): https://codereview.webrtc.org/1967033002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/utility/delay_estimator.cc:13: #include <algorithm> C system files go before C++ system files.
https://codereview.webrtc.org/1967033002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/utility/delay_estimator.cc (right): https://codereview.webrtc.org/1967033002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/utility/delay_estimator.cc:13: #include <algorithm> On 2016/05/11 13:06:19, hlundin-webrtc wrote: > C system files go before C++ system files. Good catch! Done.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1967033002/#ps40001 (title: "Corrected the order of includes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967033002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967033002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/14930)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1967033002/#ps60001 (title: "Updated the unittest reference file with a new standard deviation value obtained from APM with the …")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967033002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967033002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/13216)
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967033002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967033002/80001
peah@webrtc.org changed reviewers: + tina.legrand@webrtc.org
+@tina.legrand as an OWNER of the unittest reference files in data/audio_processing
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/13221)
lgtm
Description was changed from ========== Corrected the delay agnostic AEC behavior during periods of silent farend signal. Added conditional updating of the statistics and the delay estimate so that updates are only done when the farend is non-stationary. The reason for this is that all the values that go into the updating of the statistics, and that in turn are also used to update the delay, are frozen when the farend signal is non-stationary. Therefore, when the farend signal is silent (stationary), the last estimates present before the silent (stationary) period began are used to continue to update the statistics. This is a problem as the updating is done in a manner that assumes that the estimates continue to be updated. This CL conditions the updating based on stationarity instead of silence as both are treated in the same manner in the delay agnostic AEC. This makes sense theoretically as the delay agnostic AEC operates on analyzing power deviations (in bands) from a slowly updated average power and therefore for a stationary signal will have no such deviations to base its analysis on. BUG=webrtc:5875, chromium:576624 ========== to ========== Corrected the delay agnostic AEC behavior during periods of silent farend signal. Added conditional updating of the statistics and the delay estimate so that updates are only done when the farend is non-stationary. The reason for this is that all the values that go into the updating of the statistics, and that in turn are also used to update the delay, are frozen when the farend signal is non-stationary. Therefore, when the farend signal is silent (stationary), the last estimates present before the silent (stationary) period began are used to continue to update the statistics. This is a problem as the updating is done in a manner that assumes that the estimates continue to be updated. This CL conditions the updating based on stationarity instead of silence as both are treated in the same manner in the delay agnostic AEC. This makes sense theoretically as the delay agnostic AEC operates on analyzing power deviations (in bands) from a slowly updated average power and therefore for a stationary signal will have no such deviations to base its analysis on. BUG=webrtc:5875, chromium:576624 NOTRY=True ==========
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1967033002/#ps80001 (title: "Updated the unittest reference file for mac with a new standard deviation value obtained from APM w…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967033002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967033002/80001
Message was sent while issue was closed.
Description was changed from ========== Corrected the delay agnostic AEC behavior during periods of silent farend signal. Added conditional updating of the statistics and the delay estimate so that updates are only done when the farend is non-stationary. The reason for this is that all the values that go into the updating of the statistics, and that in turn are also used to update the delay, are frozen when the farend signal is non-stationary. Therefore, when the farend signal is silent (stationary), the last estimates present before the silent (stationary) period began are used to continue to update the statistics. This is a problem as the updating is done in a manner that assumes that the estimates continue to be updated. This CL conditions the updating based on stationarity instead of silence as both are treated in the same manner in the delay agnostic AEC. This makes sense theoretically as the delay agnostic AEC operates on analyzing power deviations (in bands) from a slowly updated average power and therefore for a stationary signal will have no such deviations to base its analysis on. BUG=webrtc:5875, chromium:576624 NOTRY=True ========== to ========== Corrected the delay agnostic AEC behavior during periods of silent farend signal. Added conditional updating of the statistics and the delay estimate so that updates are only done when the farend is non-stationary. The reason for this is that all the values that go into the updating of the statistics, and that in turn are also used to update the delay, are frozen when the farend signal is non-stationary. Therefore, when the farend signal is silent (stationary), the last estimates present before the silent (stationary) period began are used to continue to update the statistics. This is a problem as the updating is done in a manner that assumes that the estimates continue to be updated. This CL conditions the updating based on stationarity instead of silence as both are treated in the same manner in the delay agnostic AEC. This makes sense theoretically as the delay agnostic AEC operates on analyzing power deviations (in bands) from a slowly updated average power and therefore for a stationary signal will have no such deviations to base its analysis on. BUG=webrtc:5875, chromium:576624 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Corrected the delay agnostic AEC behavior during periods of silent farend signal. Added conditional updating of the statistics and the delay estimate so that updates are only done when the farend is non-stationary. The reason for this is that all the values that go into the updating of the statistics, and that in turn are also used to update the delay, are frozen when the farend signal is non-stationary. Therefore, when the farend signal is silent (stationary), the last estimates present before the silent (stationary) period began are used to continue to update the statistics. This is a problem as the updating is done in a manner that assumes that the estimates continue to be updated. This CL conditions the updating based on stationarity instead of silence as both are treated in the same manner in the delay agnostic AEC. This makes sense theoretically as the delay agnostic AEC operates on analyzing power deviations (in bands) from a slowly updated average power and therefore for a stationary signal will have no such deviations to base its analysis on. BUG=webrtc:5875, chromium:576624 NOTRY=True ========== to ========== Corrected the delay agnostic AEC behavior during periods of silent farend signal. Added conditional updating of the statistics and the delay estimate so that updates are only done when the farend is non-stationary. The reason for this is that all the values that go into the updating of the statistics, and that in turn are also used to update the delay, are frozen when the farend signal is non-stationary. Therefore, when the farend signal is silent (stationary), the last estimates present before the silent (stationary) period began are used to continue to update the statistics. This is a problem as the updating is done in a manner that assumes that the estimates continue to be updated. This CL conditions the updating based on stationarity instead of silence as both are treated in the same manner in the delay agnostic AEC. This makes sense theoretically as the delay agnostic AEC operates on analyzing power deviations (in bands) from a slowly updated average power and therefore for a stationary signal will have no such deviations to base its analysis on. BUG=webrtc:5875, chromium:576624 NOTRY=True Committed: https://crrev.com/b1fc54d33e109b9bab10114b3aa0d47eb90b799d Cr-Commit-Position: refs/heads/master@{#12700} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b1fc54d33e109b9bab10114b3aa0d47eb90b799d Cr-Commit-Position: refs/heads/master@{#12700} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
