Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1887003002:
Added support in the AEC for refined filter adaptation. (Closed)
|
Created:
4 years, 8 months ago by peah-webrtc Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, yujie_mao (webrtc), kwiberg-webrtc, Andrew MacDonald, hlundin-webrtc, tterriberry_mozilla.com, audio-team_agora.io, qiang.lu, niklas.enbom, peah-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. |
DescriptionAdded support in the AEC for refined filter adaptation.
The following algorithmic functionality was added:
-Add support for an exact regressor power to be computed
which avoids the issue with the updating of the filter
sometimes being unstable.
-Lowered the fixed step size of the adaptive filter to 0.05
which significantly reduces the sensitivity of the
adaptive filter to near-end noise, nonlinearities,
doubletalk and the unmodelled echo path tail. It also
reduces the tracking speed of the adaptive filter but the
chosen value proved to give a sufficient tradeoff for the
requirements on the adaptive filter.
To allow the new functionality to be selectively applied the following was done:
-A new Config was added for selectively activating the functionality.
-Functionality was added in the audioprocessing and echocancellationimpl classes
for passing the activation of the functionality down to the AEC algorithms.
To make the code for the introduction of the functionality clean,
the following refactoring was done:
-The selection of the step size was moved to a single place.
-The constant for the step size of the adaptive filter in extended filter mode was
made local.
-The state variable storing the step-size was renamed to a more describing name.
When the new functionality is not activated, the changes
have been tested for bitexactness on Linux.
TBR=minyue@webrtc.org
BUG=webrtc:5778, webrtc:5777
Committed: https://crrev.com/0332c2db39d6f5c780ce9e92b850bcb57e24e7f8
Cr-Commit-Position: refs/heads/master@{#12384}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Changes in response to reviewer comments #Patch Set 3 : Preinitialized the sample rate to avoid the possiblitily of access to uninitialized value #
Total comments: 6
Patch Set 4 : Corrected the issue with the missing const specifier in the header file #
Total comments: 1
Patch Set 5 : Rebase with latest master #Patch Set 6 : Added experiment string for the refined adaptive filter experiment in the aecdump #
Total comments: 8
Patch Set 7 : Changes in response to reviewer comments. #Messages
Total messages: 55 (26 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Added support in the AEC for refined filter adaptation. The following algorithmic functionality was added: -Add support for an exact regressor power to be computed. -Lowered the fixed step size of the adaptive filter to 0.05. To allow the new functionality to be selectively applied the following was done: -A new Config was added for selectively activating the functionality. -Functionality was added in the audioprocessing and echocancellationimpl classes for passing the activation of the functionality down to the AEC algorithms. To make the code for the introduction of the functionality clean, the following refactoring was done: -The selection of the step size was moved to a single place. -The constant for the step size of the adaptive filter in extended filter mode was made local. -The state variable storing the step-size was renamed to a more describing name. BUG=webrtc:5778, webrtc:5777 ========== to ========== Added support in the AEC for refined filter adaptation. The following algorithmic functionality was added: -Add support for an exact regressor power to be computed. -Lowered the fixed step size of the adaptive filter to 0.05. To allow the new functionality to be selectively applied the following was done: -A new Config was added for selectively activating the functionality. -Functionality was added in the audioprocessing and echocancellationimpl classes for passing the activation of the functionality down to the AEC algorithms. To make the code for the introduction of the functionality clean, the following refactoring was done: -The selection of the step size was moved to a single place. -The constant for the step size of the adaptive filter in extended filter mode was made local. -The state variable storing the step-size was renamed to a more describing name. When the new functionality is not activated, the changes have been tested for bitexactness on Linux. BUG=webrtc:5778, webrtc:5777 ==========
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, tina.legrand@webrtc.org
LG, with just a few minor comments and questions? https://codereview.webrtc.org/1887003002/diff/1/webrtc/common.h File webrtc/common.h (right): https://codereview.webrtc.org/1887003002/diff/1/webrtc/common.h#newcode36 webrtc/common.h:36: kRefinedAdaptiveFilter I think kAecRefinedAdaptiveFilter is better, since this file is for all of WebRTC and there may be adaptive filters in many places. Note that I do not think this clarification is needed for the named variables and functions inside AEC, since the context is already clear there. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:960: x_fft_buf_position = 0; Can you DCHECK what you expect x_fft_buf_position to be before setting it to 0? https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:1597: aec->filter_step_size = 0.05f; The old code seems to vary the step size depending on sample rate. Is this motivated for the new code too? https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:1998: bool WebRtcAec_refined_adaptive_filter_enabled(AecCore* self) { const AecCore* https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.h:105: bool refined_adaptive_filter_enabled_ GUARDED_BY(crit_capture_); Any reason not to start using class member initialization? That is, set the member to false already here and skip it in the ctor.
I want to take one more look at the RegressorPower function, to make sure I understand. Sending you the rest of my comments. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:242: static void ScaleErrorSignal(int extended_filter_enabled, This function don't really need the first parameter, if you instead call it with normal_error_threshold = kExtendedErrorThreshold (and rename normal_error_threshold to simply error_threshold). https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:243: float mu, With removal of mu = kExtendedMu if extended filter is enabled, is the function now called in a different way? Otherwise, you have changed behavior. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core_mips.cc (right): https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core_mips.cc:714: float ef[2][PART_LEN1]) { Same comments as for the main function. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core_neon.cc (right): https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core_neon.cc:134: float ef[2][PART_LEN1]) { Same comments as for the main function. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core_sse2.cc (right): https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core_sse2.cc:88: const __m128 kMu = _mm_set1_ps(mu); Same comments as for the main function.
One more comment that must be fixed. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:243: float mu, On 2016/04/14 14:32:42, tlegrand-webrtc wrote: > With removal of mu = kExtendedMu if extended filter is enabled, is the function > now called in a different way? Otherwise, you have changed behavior. It is now called (through EchoSubtraction) with aec->filter_step_size instead of aec->norma_mu. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:2007: } You must call SetAdaptiveFilterStepSize here too.
https://codereview.webrtc.org/1887003002/diff/1/webrtc/common.h File webrtc/common.h (right): https://codereview.webrtc.org/1887003002/diff/1/webrtc/common.h#newcode36 webrtc/common.h:36: kRefinedAdaptiveFilter On 2016/04/14 14:02:26, hlundin-webrtc wrote: > I think kAecRefinedAdaptiveFilter is better, since this file is for all of > WebRTC and there may be adaptive filters in many places. Note that I do not > think this clarification is needed for the named variables and functions inside > AEC, since the context is already clear there. Nice! Good point! Done. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:242: static void ScaleErrorSignal(int extended_filter_enabled, On 2016/04/14 14:32:42, tlegrand-webrtc wrote: > This function don't really need the first parameter, if you instead call it with > normal_error_threshold = kExtendedErrorThreshold (and rename > normal_error_threshold to simply error_threshold). That is definitely a good change. I did not want to do it in this CL, as it is not really related to the changes being done. But I now grab the opportunity and do the change :-) Done. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:243: float mu, On 2016/04/14 14:47:46, hlundin-webrtc wrote: > On 2016/04/14 14:32:42, tlegrand-webrtc wrote: > > With removal of mu = kExtendedMu if extended filter is enabled, is the > function > > now called in a different way? Otherwise, you have changed behavior. > > It is now called (through EchoSubtraction) with aec->filter_step_size instead of > aec->norma_mu. Fully true. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:243: float mu, On 2016/04/14 14:32:42, tlegrand-webrtc wrote: > With removal of mu = kExtendedMu if extended filter is enabled, is the function > now called in a different way? Otherwise, you have changed behavior. It is now called in a different way but with a mu parameter that is updated in another place. As it was, mu was selected in several places which did not make sense. Now the decision is done in one place, in such a way that it is identical to what it was before (if not the new functionality in this CL is active as the modification of mu is part of that). https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:960: x_fft_buf_position = 0; On 2016/04/14 14:02:26, hlundin-webrtc wrote: > Can you DCHECK what you expect x_fft_buf_position to be before setting it to 0? Good point! Done. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:1597: aec->filter_step_size = 0.05f; On 2016/04/14 14:02:26, hlundin-webrtc wrote: > The old code seems to vary the step size depending on sample rate. Is this > motivated for the new code too? Maybe, The only way the sample rate affects the adaptive filter is the way that the echo path varies. And that variability could motivate both using a higher and lower rate. To me the sample-rate dependency would be better handled by having the filter length (measured in samples) be proportional to the sample rate. I have no idea how the decision to have the different adaptation step sizes for the different sample rates were made. Both step sizes are very high. Probably the step-size could be tuned differently for different sample rates, but that tuning would require a lot of work and be very specific to specific AEC implementation at hand. I'd rather limit the use to the new single step-size value. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:1998: bool WebRtcAec_refined_adaptive_filter_enabled(AecCore* self) { On 2016/04/14 14:02:27, hlundin-webrtc wrote: > const AecCore* Done. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:2007: } On 2016/04/14 14:47:46, hlundin-webrtc wrote: > You must call SetAdaptiveFilterStepSize here too. Great find! Done. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core_mips.cc (right): https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core_mips.cc:714: float ef[2][PART_LEN1]) { On 2016/04/14 14:32:42, tlegrand-webrtc wrote: > Same comments as for the main function. Done. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core_neon.cc (right): https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core_neon.cc:134: float ef[2][PART_LEN1]) { On 2016/04/14 14:32:42, tlegrand-webrtc wrote: > Same comments as for the main function. Done. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core_sse2.cc (right): https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core_sse2.cc:88: const __m128 kMu = _mm_set1_ps(mu); On 2016/04/14 14:32:42, tlegrand-webrtc wrote: > Same comments as for the main function. Done. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.h:105: bool refined_adaptive_filter_enabled_ GUARDED_BY(crit_capture_); On 2016/04/14 14:02:27, hlundin-webrtc wrote: > Any reason not to start using class member initialization? That is, set the > member to false already here and skip it in the ctor. No, none at all. Done.
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/1887003002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887003002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_msan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/9275)
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/1887003002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887003002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A few more comments. https://codereview.webrtc.org/1887003002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1887003002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:964: int extended_filter_enabled, Now this is unused... https://codereview.webrtc.org/1887003002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1000: if (!aec->extended_filter_enabled && aec->extreme_filter_divergence) { ... since you are getting the value from the aec state directly, and not through the extended_filter_enabled input parameter. https://codereview.webrtc.org/1887003002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:2010: bool WebRtcAec_refined_adaptive_filter_enabled(const AecCore* self) { You must do the same in the h file too.
https://codereview.webrtc.org/1887003002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1887003002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:964: int extended_filter_enabled, On 2016/04/15 06:39:23, hlundin-webrtc wrote: > Now this is unused... Acknowledged. https://codereview.webrtc.org/1887003002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1000: if (!aec->extended_filter_enabled && aec->extreme_filter_divergence) { On 2016/04/15 06:39:24, hlundin-webrtc wrote: > ... since you are getting the value from the aec state directly, and not through > the extended_filter_enabled input parameter. True! Good find! After offline discussions, we've agreed to address this in a follow-up CL. https://codereview.webrtc.org/1887003002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:2010: bool WebRtcAec_refined_adaptive_filter_enabled(const AecCore* self) { On 2016/04/15 06:39:23, hlundin-webrtc wrote: > You must do the same in the h file too. Done.
LGTM with the promise that you clean up the input parameters in EchoSubtraction() in a follow-up. https://codereview.webrtc.org/1887003002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1887003002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:962: static void EchoSubtraction(AecCore* aec, The parameter list has been somewhat messed up by an earlier change. In offline discussion we agreed to fix it in a follow-up CL.
Patchset #6 (id:120001) has been deleted
Hi, I did a rebase to the latest changes in master, and then added a string to the experiments description field of the aecdump that specifies that the experiment is active. I also added some tests to verify the correctness of the stored string. PTAL.
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/1887003002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887003002/140001
LGTM https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:242: static void ScaleErrorSignal(int extended_filter_enabled, On 2016/04/14 22:12:41, peah-webrtc wrote: > On 2016/04/14 14:32:42, tlegrand-webrtc wrote: > > This function don't really need the first parameter, if you instead call it > with > > normal_error_threshold = kExtendedErrorThreshold (and rename > > normal_error_threshold to simply error_threshold). > > That is definitely a good change. I did not want to do it in this CL, as it is > not really related to the changes being done. But I now grab the opportunity and > do the change :-) > Done. Great! I think it's fine, since the CL isn't that big, and it makes the code easier to follow. https://codereview.webrtc.org/1887003002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:243: float mu, On 2016/04/14 22:12:41, peah-webrtc wrote: > On 2016/04/14 14:32:42, tlegrand-webrtc wrote: > > With removal of mu = kExtendedMu if extended filter is enabled, is the > function > > now called in a different way? Otherwise, you have changed behavior. > > It is now called in a different way but with a mu parameter that is updated in > another place. As it was, mu was selected in several places which did not make > sense. Now the decision is done in one place, in such a way that it is identical > to what it was before (if not the new functionality in this CL is active as the > modification of mu is part of that). Acknowledged.
The CQ bit was unchecked by peah@webrtc.org
LGTM, but with a question and an optional comment. https://codereview.webrtc.org/1887003002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1887003002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:361: EXPECT_TRUE(msg->has_experiments_description()); What happens with the call to msg->experiments_description() if msg->has_experiments_description() is false? Should you ASSERT_TRUE here instead? https://codereview.webrtc.org/1887003002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:362: EXPECT_NE(std::string::npos, I believe you can use the following pattern instead: EXPECT_PRED_FORMAT2(testing::IsSubstring, "RefinedAdaptiveFilter", msg->experiments_description().c_str()); I'm not sure it renders more readable code, though. I leave it up to you to decide.
minyue@webrtc.org changed reviewers: + minyue@webrtc.org
great work! I added a couple of drive-in comments. https://codereview.webrtc.org/1887003002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1887003002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:1356: // Calculate absolute spectra maybe fix the comment. https://codereview.webrtc.org/1887003002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:1593: aec->filter_step_size = 0.05f; This seems a big change from the old value. I believe that you have a strong reason for it. You may add a sentence for people to understand this CL better.
https://codereview.webrtc.org/1887003002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1887003002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:1356: // Calculate absolute spectra On 2016/04/15 09:51:13, minyue-webrtc wrote: > maybe fix the comment. Done. https://codereview.webrtc.org/1887003002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:1593: aec->filter_step_size = 0.05f; On 2016/04/15 09:51:13, minyue-webrtc wrote: > This seems a big change from the old value. I believe that you have a strong > reason for it. You may add a sentence for people to understand this CL better. Good point. Will do that in the description of the CL. Done. https://codereview.webrtc.org/1887003002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1887003002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:361: EXPECT_TRUE(msg->has_experiments_description()); On 2016/04/15 09:24:02, hlundin-webrtc wrote: > What happens with the call to msg->experiments_description() if > msg->has_experiments_description() is false? Should you ASSERT_TRUE here > instead? Good point! Done. https://codereview.webrtc.org/1887003002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:362: EXPECT_NE(std::string::npos, On 2016/04/15 09:24:02, hlundin-webrtc wrote: > I believe you can use the following pattern instead: > EXPECT_PRED_FORMAT2(testing::IsSubstring, "RefinedAdaptiveFilter", > msg->experiments_description().c_str()); > I'm not sure it renders more readable code, though. I leave it up to you to > decide. Thanks! That change gave much nicer output! Done.
Description was changed from ========== Added support in the AEC for refined filter adaptation. The following algorithmic functionality was added: -Add support for an exact regressor power to be computed. -Lowered the fixed step size of the adaptive filter to 0.05. To allow the new functionality to be selectively applied the following was done: -A new Config was added for selectively activating the functionality. -Functionality was added in the audioprocessing and echocancellationimpl classes for passing the activation of the functionality down to the AEC algorithms. To make the code for the introduction of the functionality clean, the following refactoring was done: -The selection of the step size was moved to a single place. -The constant for the step size of the adaptive filter in extended filter mode was made local. -The state variable storing the step-size was renamed to a more describing name. When the new functionality is not activated, the changes have been tested for bitexactness on Linux. BUG=webrtc:5778, webrtc:5777 ========== to ========== Added support in the AEC for refined filter adaptation. The following algorithmic functionality was added: -Add support for an exact regressor power to be computed. -Lowered the fixed step size of the adaptive filter to 0.05 which significantly reduces the sensitivity of the adaptive filter to near-end noise, nonlinearities, doubletalk and the unmodelled echo path tail. It also reduces the tracking speed of the adaptive filter but the chosen value proved to give a sufficient tradeoff for the requirements on the adaptive filter. To allow the new functionality to be selectively applied the following was done: -A new Config was added for selectively activating the functionality. -Functionality was added in the audioprocessing and echocancellationimpl classes for passing the activation of the functionality down to the AEC algorithms. To make the code for the introduction of the functionality clean, the following refactoring was done: -The selection of the step size was moved to a single place. -The constant for the step size of the adaptive filter in extended filter mode was made local. -The state variable storing the step-size was renamed to a more describing name. When the new functionality is not activated, the changes have been tested for bitexactness on Linux. BUG=webrtc:5778, webrtc:5777 ==========
Description was changed from ========== Added support in the AEC for refined filter adaptation. The following algorithmic functionality was added: -Add support for an exact regressor power to be computed. -Lowered the fixed step size of the adaptive filter to 0.05 which significantly reduces the sensitivity of the adaptive filter to near-end noise, nonlinearities, doubletalk and the unmodelled echo path tail. It also reduces the tracking speed of the adaptive filter but the chosen value proved to give a sufficient tradeoff for the requirements on the adaptive filter. To allow the new functionality to be selectively applied the following was done: -A new Config was added for selectively activating the functionality. -Functionality was added in the audioprocessing and echocancellationimpl classes for passing the activation of the functionality down to the AEC algorithms. To make the code for the introduction of the functionality clean, the following refactoring was done: -The selection of the step size was moved to a single place. -The constant for the step size of the adaptive filter in extended filter mode was made local. -The state variable storing the step-size was renamed to a more describing name. When the new functionality is not activated, the changes have been tested for bitexactness on Linux. BUG=webrtc:5778, webrtc:5777 ========== to ========== Added support in the AEC for refined filter adaptation. The following algorithmic functionality was added: -Add support for an exact regressor power to be computed. -Lowered the fixed step size of the adaptive filter to 0.05 which significantly reduces the sensitivity of the adaptive filter to near-end noise, nonlinearities, doubletalk and the unmodelled echo path tail. It also reduces the tracking speed of the adaptive filter but the chosen value proved to give a sufficient tradeoff for the requirements on the adaptive filter. To allow the new functionality to be selectively applied the following was done: -A new Config was added for selectively activating the functionality. -Functionality was added in the audioprocessing and echocancellationimpl classes for passing the activation of the functionality down to the AEC algorithms. To make the code for the introduction of the functionality clean, the following refactoring was done: -The selection of the step size was moved to a single place. -The constant for the step size of the adaptive filter in extended filter mode was made local. -The state variable storing the step-size was renamed to a more describing name. When the new functionality is not activated, the changes have been tested for bitexactness on Linux. BUG=webrtc:5778, webrtc:5777 ==========
Description was changed from ========== Added support in the AEC for refined filter adaptation. The following algorithmic functionality was added: -Add support for an exact regressor power to be computed. -Lowered the fixed step size of the adaptive filter to 0.05 which significantly reduces the sensitivity of the adaptive filter to near-end noise, nonlinearities, doubletalk and the unmodelled echo path tail. It also reduces the tracking speed of the adaptive filter but the chosen value proved to give a sufficient tradeoff for the requirements on the adaptive filter. To allow the new functionality to be selectively applied the following was done: -A new Config was added for selectively activating the functionality. -Functionality was added in the audioprocessing and echocancellationimpl classes for passing the activation of the functionality down to the AEC algorithms. To make the code for the introduction of the functionality clean, the following refactoring was done: -The selection of the step size was moved to a single place. -The constant for the step size of the adaptive filter in extended filter mode was made local. -The state variable storing the step-size was renamed to a more describing name. When the new functionality is not activated, the changes have been tested for bitexactness on Linux. BUG=webrtc:5778, webrtc:5777 ========== to ========== Added support in the AEC for refined filter adaptation. The following algorithmic functionality was added: -Add support for an exact regressor power to be computed which avoids the issue with the updating of the filter sometimes being unstable. -Lowered the fixed step size of the adaptive filter to 0.05 which significantly reduces the sensitivity of the adaptive filter to near-end noise, nonlinearities, doubletalk and the unmodelled echo path tail. It also reduces the tracking speed of the adaptive filter but the chosen value proved to give a sufficient tradeoff for the requirements on the adaptive filter. To allow the new functionality to be selectively applied the following was done: -A new Config was added for selectively activating the functionality. -Functionality was added in the audioprocessing and echocancellationimpl classes for passing the activation of the functionality down to the AEC algorithms. To make the code for the introduction of the functionality clean, the following refactoring was done: -The selection of the step size was moved to a single place. -The constant for the step size of the adaptive filter in extended filter mode was made local. -The state variable storing the step-size was renamed to a more describing name. When the new functionality is not activated, the changes have been tested for bitexactness on Linux. BUG=webrtc:5778, webrtc:5777 ==========
Description was changed from ========== Added support in the AEC for refined filter adaptation. The following algorithmic functionality was added: -Add support for an exact regressor power to be computed which avoids the issue with the updating of the filter sometimes being unstable. -Lowered the fixed step size of the adaptive filter to 0.05 which significantly reduces the sensitivity of the adaptive filter to near-end noise, nonlinearities, doubletalk and the unmodelled echo path tail. It also reduces the tracking speed of the adaptive filter but the chosen value proved to give a sufficient tradeoff for the requirements on the adaptive filter. To allow the new functionality to be selectively applied the following was done: -A new Config was added for selectively activating the functionality. -Functionality was added in the audioprocessing and echocancellationimpl classes for passing the activation of the functionality down to the AEC algorithms. To make the code for the introduction of the functionality clean, the following refactoring was done: -The selection of the step size was moved to a single place. -The constant for the step size of the adaptive filter in extended filter mode was made local. -The state variable storing the step-size was renamed to a more describing name. When the new functionality is not activated, the changes have been tested for bitexactness on Linux. BUG=webrtc:5778, webrtc:5777 ========== to ========== Added support in the AEC for refined filter adaptation. The following algorithmic functionality was added: -Add support for an exact regressor power to be computed which avoids the issue with the updating of the filter sometimes being unstable. -Lowered the fixed step size of the adaptive filter to 0.05 which significantly reduces the sensitivity of the adaptive filter to near-end noise, nonlinearities, doubletalk and the unmodelled echo path tail. It also reduces the tracking speed of the adaptive filter but the chosen value proved to give a sufficient tradeoff for the requirements on the adaptive filter. To allow the new functionality to be selectively applied the following was done: -A new Config was added for selectively activating the functionality. -Functionality was added in the audioprocessing and echocancellationimpl classes for passing the activation of the functionality down to the AEC algorithms. To make the code for the introduction of the functionality clean, the following refactoring was done: -The selection of the step size was moved to a single place. -The constant for the step size of the adaptive filter in extended filter mode was made local. -The state variable storing the step-size was renamed to a more describing name. When the new functionality is not activated, the changes have been tested for bitexactness on Linux. TBD=minyue@webrtc.org BUG=webrtc:5778, webrtc:5777 ==========
Description was changed from ========== Added support in the AEC for refined filter adaptation. The following algorithmic functionality was added: -Add support for an exact regressor power to be computed which avoids the issue with the updating of the filter sometimes being unstable. -Lowered the fixed step size of the adaptive filter to 0.05 which significantly reduces the sensitivity of the adaptive filter to near-end noise, nonlinearities, doubletalk and the unmodelled echo path tail. It also reduces the tracking speed of the adaptive filter but the chosen value proved to give a sufficient tradeoff for the requirements on the adaptive filter. To allow the new functionality to be selectively applied the following was done: -A new Config was added for selectively activating the functionality. -Functionality was added in the audioprocessing and echocancellationimpl classes for passing the activation of the functionality down to the AEC algorithms. To make the code for the introduction of the functionality clean, the following refactoring was done: -The selection of the step size was moved to a single place. -The constant for the step size of the adaptive filter in extended filter mode was made local. -The state variable storing the step-size was renamed to a more describing name. When the new functionality is not activated, the changes have been tested for bitexactness on Linux. TBD=minyue@webrtc.org BUG=webrtc:5778, webrtc:5777 ========== to ========== Added support in the AEC for refined filter adaptation. The following algorithmic functionality was added: -Add support for an exact regressor power to be computed which avoids the issue with the updating of the filter sometimes being unstable. -Lowered the fixed step size of the adaptive filter to 0.05 which significantly reduces the sensitivity of the adaptive filter to near-end noise, nonlinearities, doubletalk and the unmodelled echo path tail. It also reduces the tracking speed of the adaptive filter but the chosen value proved to give a sufficient tradeoff for the requirements on the adaptive filter. To allow the new functionality to be selectively applied the following was done: -A new Config was added for selectively activating the functionality. -Functionality was added in the audioprocessing and echocancellationimpl classes for passing the activation of the functionality down to the AEC algorithms. To make the code for the introduction of the functionality clean, the following refactoring was done: -The selection of the step size was moved to a single place. -The constant for the step size of the adaptive filter in extended filter mode was made local. -The state variable storing the step-size was renamed to a more describing name. When the new functionality is not activated, the changes have been tested for bitexactness on Linux. TBR=minyue@webrtc.org BUG=webrtc:5778, webrtc:5777 ==========
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, tina.legrand@webrtc.org Link to the patchset: https://codereview.webrtc.org/1887003002/#ps160001 (title: "Changes in response to reviewer comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1887003002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887003002/160001
lgtm
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1887003002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887003002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1887003002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887003002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1887003002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887003002/160001
Message was sent while issue was closed.
Description was changed from ========== Added support in the AEC for refined filter adaptation. The following algorithmic functionality was added: -Add support for an exact regressor power to be computed which avoids the issue with the updating of the filter sometimes being unstable. -Lowered the fixed step size of the adaptive filter to 0.05 which significantly reduces the sensitivity of the adaptive filter to near-end noise, nonlinearities, doubletalk and the unmodelled echo path tail. It also reduces the tracking speed of the adaptive filter but the chosen value proved to give a sufficient tradeoff for the requirements on the adaptive filter. To allow the new functionality to be selectively applied the following was done: -A new Config was added for selectively activating the functionality. -Functionality was added in the audioprocessing and echocancellationimpl classes for passing the activation of the functionality down to the AEC algorithms. To make the code for the introduction of the functionality clean, the following refactoring was done: -The selection of the step size was moved to a single place. -The constant for the step size of the adaptive filter in extended filter mode was made local. -The state variable storing the step-size was renamed to a more describing name. When the new functionality is not activated, the changes have been tested for bitexactness on Linux. TBR=minyue@webrtc.org BUG=webrtc:5778, webrtc:5777 ========== to ========== Added support in the AEC for refined filter adaptation. The following algorithmic functionality was added: -Add support for an exact regressor power to be computed which avoids the issue with the updating of the filter sometimes being unstable. -Lowered the fixed step size of the adaptive filter to 0.05 which significantly reduces the sensitivity of the adaptive filter to near-end noise, nonlinearities, doubletalk and the unmodelled echo path tail. It also reduces the tracking speed of the adaptive filter but the chosen value proved to give a sufficient tradeoff for the requirements on the adaptive filter. To allow the new functionality to be selectively applied the following was done: -A new Config was added for selectively activating the functionality. -Functionality was added in the audioprocessing and echocancellationimpl classes for passing the activation of the functionality down to the AEC algorithms. To make the code for the introduction of the functionality clean, the following refactoring was done: -The selection of the step size was moved to a single place. -The constant for the step size of the adaptive filter in extended filter mode was made local. -The state variable storing the step-size was renamed to a more describing name. When the new functionality is not activated, the changes have been tested for bitexactness on Linux. TBR=minyue@webrtc.org BUG=webrtc:5778, webrtc:5777 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Added support in the AEC for refined filter adaptation. The following algorithmic functionality was added: -Add support for an exact regressor power to be computed which avoids the issue with the updating of the filter sometimes being unstable. -Lowered the fixed step size of the adaptive filter to 0.05 which significantly reduces the sensitivity of the adaptive filter to near-end noise, nonlinearities, doubletalk and the unmodelled echo path tail. It also reduces the tracking speed of the adaptive filter but the chosen value proved to give a sufficient tradeoff for the requirements on the adaptive filter. To allow the new functionality to be selectively applied the following was done: -A new Config was added for selectively activating the functionality. -Functionality was added in the audioprocessing and echocancellationimpl classes for passing the activation of the functionality down to the AEC algorithms. To make the code for the introduction of the functionality clean, the following refactoring was done: -The selection of the step size was moved to a single place. -The constant for the step size of the adaptive filter in extended filter mode was made local. -The state variable storing the step-size was renamed to a more describing name. When the new functionality is not activated, the changes have been tested for bitexactness on Linux. TBR=minyue@webrtc.org BUG=webrtc:5778, webrtc:5777 ========== to ========== Added support in the AEC for refined filter adaptation. The following algorithmic functionality was added: -Add support for an exact regressor power to be computed which avoids the issue with the updating of the filter sometimes being unstable. -Lowered the fixed step size of the adaptive filter to 0.05 which significantly reduces the sensitivity of the adaptive filter to near-end noise, nonlinearities, doubletalk and the unmodelled echo path tail. It also reduces the tracking speed of the adaptive filter but the chosen value proved to give a sufficient tradeoff for the requirements on the adaptive filter. To allow the new functionality to be selectively applied the following was done: -A new Config was added for selectively activating the functionality. -Functionality was added in the audioprocessing and echocancellationimpl classes for passing the activation of the functionality down to the AEC algorithms. To make the code for the introduction of the functionality clean, the following refactoring was done: -The selection of the step size was moved to a single place. -The constant for the step size of the adaptive filter in extended filter mode was made local. -The state variable storing the step-size was renamed to a more describing name. When the new functionality is not activated, the changes have been tested for bitexactness on Linux. TBR=minyue@webrtc.org BUG=webrtc:5778, webrtc:5777 Committed: https://crrev.com/0332c2db39d6f5c780ce9e92b850bcb57e24e7f8 Cr-Commit-Position: refs/heads/master@{#12384} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0332c2db39d6f5c780ce9e92b850bcb57e24e7f8 Cr-Commit-Position: refs/heads/master@{#12384} |
