Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1207353002:
Add new variance update option and unittests for intelligibility (Closed)
|
Created:
5 years, 6 months ago by ekm Modified:
5 years, 5 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, hlundin-webrtc, kwiberg-webrtc, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd new variance update option and unittests for intelligibility
- New option for computing variance that is more adaptive with lower complexity.
- Fixed related off-by-one errors.
- Added intelligibility unittests.
- Do not enhance if experiencing variance underflow.
R=andrew@webrtc.org, henrik.lundin@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/35b72fbceb09031cbd6039e0dbbd44ed24296509
Patch Set 1 #
Total comments: 29
Patch Set 2 : Addressed comments #Patch Set 3 : Renamed tests + minor changes #
Total comments: 78
Patch Set 4 : Addressed comments from hlundin #
Total comments: 20
Patch Set 5 : Simplified test data generation #
Total comments: 8
Patch Set 6 : Optimized search for lambda; fixed test data generation #Patch Set 7 : Addressed trybot errors #Patch Set 8 : Addressed trybot errors 2 #Patch Set 9 : Fixed floats for windows #Patch Set 10 : Added using std::round for windows #Patch Set 11 : Remove non-determinism by switching to vector #Patch Set 12 : Removed divide by 0 check, fixed headers #Patch Set 13 : free(temp_out_buffer_) #Patch Set 14 : Merge #Messages
Total messages: 41 (17 generated)
ekmeyerson@webrtc.org changed reviewers: + andrew@webrtc.org, turajs@google.com
turaj@webrtc.org changed reviewers: + turaj@webrtc.org
Please see my comments. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:257: gains_eq_[i] = 1.0f; Does this really mean that are not changing gains? What if we do nothing, then we will have same |gains_eq_| from the past and UpdateErbGains() will result in the same gains. It that is correct we even don't need to run UpdateErbGains() either. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:262: void VarianceArray::BlockBasedMovingAverage( There is a concern, which is not proven, that keeping track of sum of the elements in a circular buffer in the following fashion, i.e. subtract the oldest and add the newest, might diverge from the true sum, over time. Although it might not be the case for this application as a call typically is not so long. Could you please add a TODO that we add a counter so that after a while |running_mean_| and |running_mean_sq_| should be replaces with their "true" value by summing over all the element? https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/test/enhancer_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/enhancer_unittest.cc:108: void RunEnhancer(VarianceArray::StepType step_type) { Could this function use |enh_| instead? https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/enhancer_unittest.cc:131: TEST_F(EnhancerTest, TestPlumbing) { what is it that ids tested here? How could it fail? If you like to test pluming, you have to inject a mock of variance updater and expect certain calls to happen. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/enhancer_unittest.cc:173: float lambda = -0.001; I guess you need -0.001f otherwise Visual Studio complains. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/enhancer_unittest.cc:182: EXPECT_LT(error, kMaxTestError); I guess you can use EXPECT_NEAR(v1, v2, tolerance) here and later and on line 163. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/enhancer_unittest.cc:185: enh_.filtered_clear_var_[i] = static_cast<float>(i+1); 'i + 1' https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/test/utils_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/utils_unittest.cc:19: #include "webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc" Do you need to include .cc file? https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/utils_unittest.cc:58: EXPECT_GT(t.imag(), 0.f); I guess EXPECT_NE() is a better choice here. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/utils_unittest.cc:100: variance_array.Clear(); Can we move the for Clear() after following loop? Then we are sure it is doing the expected job. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/utils_unittest.cc:126: const int kWindowSize = 1; Does window size of one actually test the circular buffer of the keep track block mean and block mean of square? How about the window size of 2? https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/utils_unittest.cc:139: test_data[i][j].imag(0.f); Can we have non-zero imaginary part to be sure the complex computations are correct?
Patchset #2 (id:20001) has been deleted
https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:257: gains_eq_[i] = 1.0f; On 2015/06/26 00:32:57, turaj wrote: > Does this really mean that are not changing gains? What if we do nothing, then > we will have same |gains_eq_| from the past and UpdateErbGains() will result in > the same gains. It that is correct we even don't need to run UpdateErbGains() > either. Done. You're right. I was thinking that if the previous |gains_eq_| were high then it would be strange to boost a ~0 variance signal, but it results in ~0 signal either way, and this saves computation. When speech eventually comes back on, the previous |gains_eq_| will still apply for a window, but that should be ok. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:262: void VarianceArray::BlockBasedMovingAverage( On 2015/06/26 00:32:58, turaj wrote: > There is a concern, which is not proven, that keeping track of sum of the > elements in a circular buffer in the following fashion, i.e. subtract the oldest > and add the newest, might diverge from the true sum, over time. Although it > might not be the case for this application as a call typically is not so long. > > Could you please add a TODO that we add a counter so that after a while > |running_mean_| and |running_mean_sq_| should be replaces with their "true" > value by summing over all the element? Interesting. This is just do to floating point errors? and we can't expect the total error to converge to 0? https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/test/enhancer_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/enhancer_unittest.cc:108: void RunEnhancer(VarianceArray::StepType step_type) { On 2015/06/26 00:32:58, turaj wrote: > Could this function use |enh_| instead? That would be better, but I was having trouble modifying |step_type| after the first construction without using placement new, or reaching deep into VarianceArray. Is there a nice way to do this? Could do something like give enhancer a SetVarianceType method. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/enhancer_unittest.cc:131: TEST_F(EnhancerTest, TestPlumbing) { On 2015/06/26 00:32:58, turaj wrote: > what is it that ids tested here? How could it fail? If you like to test pluming, > you have to inject a mock of variance updater and expect certain calls to > happen. Before we switched to doing nothing in case of underflow, some of the variance array types were raising the error, and this helped to see when that happened. Maybe now this should test that ProcessRenderAudio modifies the render stream when we expect it to? The mock may be useful for checking when Step is called when we're using VAD, but I don't see what it'd be useful for now. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/enhancer_unittest.cc:173: float lambda = -0.001; On 2015/06/26 00:32:58, turaj wrote: > I guess you need -0.001f otherwise Visual Studio complains. Done. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/enhancer_unittest.cc:182: EXPECT_LT(error, kMaxTestError); On 2015/06/26 00:32:58, turaj wrote: > I guess you can use EXPECT_NEAR(v1, v2, tolerance) here and later and on line > 163. Done. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/enhancer_unittest.cc:185: enh_.filtered_clear_var_[i] = static_cast<float>(i+1); On 2015/06/26 00:32:58, turaj wrote: > 'i + 1' Done. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/test/utils_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/utils_unittest.cc:19: #include "webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc" On 2015/06/26 00:32:58, turaj wrote: > Do you need to include .cc file? This is to test functions from anonymous namespace. Is that ok since this is just a test, or would it be better to refactor .cc file to make testing easier? or not test these helper methods at all, since they are pretty simple? https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/utils_unittest.cc:58: EXPECT_GT(t.imag(), 0.f); On 2015/06/26 00:32:58, turaj wrote: > I guess EXPECT_NE() is a better choice here. Done. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/utils_unittest.cc:100: variance_array.Clear(); On 2015/06/26 00:32:58, turaj wrote: > Can we move the for Clear() after following loop? Then we are sure it is doing > the expected job. Done. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/utils_unittest.cc:126: const int kWindowSize = 1; On 2015/06/26 00:32:58, turaj wrote: > Does window size of one actually test the circular buffer of the keep track > block mean and block mean of square? How about the window size of 2? Done. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/utils_unittest.cc:139: test_data[i][j].imag(0.f); On 2015/06/26 00:32:58, turaj wrote: > Can we have non-zero imaginary part to be sure the complex computations are > correct? Done.
turaj@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
It looks pretty good. I don't have more comments. The only thing is that do you want to have a folder for the unittests? I guess enhancer_unittest.cc can be renamed to intelligibility_enhancer_unittest.cc and be in the same folder as the source it is testing, same for utils_unittest.cc. I guess we prefer to keep test folder, for instance, for an offline test of enhancer (like the one you have). Furthermore, Tina asked us to involve one of the Stockholm engineers in code reviews. So they know better about the details of work in case some tuning/changes needed in future. It is always easier to get to know the product as it develops. Therefore, I added Henrik Lundin to the review list. He always has great constructive comments which improves the quality of our code. Henrik, I hope we are not overloading you too much :) But you were the first who showed interest in this product :) The last comment, you can take advantage of C++11 features that is allowed in Chrome https://chromium-cpp.appspot.com/ for instance range-based for-loop which can replace some of the loops in the test. Thanks. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:262: void VarianceArray::BlockBasedMovingAverage( On 2015/06/26 19:07:09, ekm wrote: > On 2015/06/26 00:32:58, turaj wrote: > > There is a concern, which is not proven, that keeping track of sum of the > > elements in a circular buffer in the following fashion, i.e. subtract the > oldest > > and add the newest, might diverge from the true sum, over time. Although it > > might not be the case for this application as a call typically is not so long. > > > > > Could you please add a TODO that we add a counter so that after a while > > |running_mean_| and |running_mean_sq_| should be replaces with their "true" > > value by summing over all the element? > > Interesting. This is just do to floating point errors? and we can't expect the > total error to converge to 0? Something like that, but I'm not sure. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/test/enhancer_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/enhancer_unittest.cc:108: void RunEnhancer(VarianceArray::StepType step_type) { On 2015/06/26 19:07:09, ekm wrote: > On 2015/06/26 00:32:58, turaj wrote: > > Could this function use |enh_| instead? > > That would be better, but I was having trouble modifying |step_type| after the > first construction without using placement new, or reaching deep into > VarianceArray. Is there a nice way to do this? Could do something like give > enhancer a SetVarianceType method. Sorry, I didn't notice that |step_type| is an input. I agree that it does not worth to modify the code just for this test. One other way that the constructor takes an input, but I guess it is fine as it is. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/enhancer_unittest.cc:131: TEST_F(EnhancerTest, TestPlumbing) { On 2015/06/26 19:07:09, ekm wrote: > On 2015/06/26 00:32:58, turaj wrote: > > what is it that ids tested here? How could it fail? If you like to test > pluming, > > you have to inject a mock of variance updater and expect certain calls to > > happen. > > Before we switched to doing nothing in case of underflow, some of the variance > array types were raising the error, and this helped to see when that happened. > Maybe now this should test that ProcessRenderAudio modifies the render stream > when we expect it to? The mock may be useful for checking when Step is called > when we're using VAD, but I don't see what it'd be useful for now. Agreed, this is a better test. https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/test/utils_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/utils_unittest.cc:19: #include "webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc" On 2015/06/26 19:07:10, ekm wrote: > On 2015/06/26 00:32:58, turaj wrote: > > Do you need to include .cc file? > > This is to test functions from anonymous namespace. Is that ok since this is > just a test, or would it be better to refactor .cc file to make testing easier? > or not test these helper methods at all, since they are pretty simple? If you feel that they are simple and don't need test it is fine by me. Otherwise, like you suggested, we need a header for them. Although this is only a test but I don't think it is good idea to include .cc.
https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/test/utils_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/test/utils_unittest.cc:19: #include "webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc" On 2015/06/29 17:33:36, turaj wrote: > On 2015/06/26 19:07:10, ekm wrote: > > On 2015/06/26 00:32:58, turaj wrote: > > > Do you need to include .cc file? > > > > This is to test functions from anonymous namespace. Is that ok since this is > > just a test, or would it be better to refactor .cc file to make testing > easier? > > or not test these helper methods at all, since they are pretty simple? > > If you feel that they are simple and don't need test it is fine by me. > Otherwise, like you suggested, we need a header for them. Although this is only > a test but I don't think it is good idea to include .cc. Added them to the intelligibility namespace.
Nice. Mostly looks good, but please see my comments inline. I didn't get to the three last files yet. Will take a look tomorrow. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:44: Delete empty line. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:45: const float IntelligibilityEnhancer::kLambdaBot = -1.0; Are all of these constants only used locally in this .cc file? If so, you can move them out of the class and simply declare them in an anonymous namespace in this file. E.g., namespace { static const int kErbResolution = 2; ... } // namespace https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:245: float power_bot, power_top; Premature declaration of variables. Declare them when they are used: float power_top = DotProduct(...); https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:263: float lambda, power; Skip declaration of lambda and power here and declare them when used inside the loop. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:45: IntelligibilityEnhancer(int erb_resolution, This is a very complicated constructor call. Consider using a config struct instead. See for instance NetEq::Config in neteq.h. The config struct should typically have a constructor which populates the config with default values. A user will then only have to modify the values if they differ from the default. You can also add convenience method to the struct, for instance std::string ToString() const for logging and bool IsOk() const for validating a configuration before sending it to the class constructor. (An extra benefit of including an IsOk() method to the Config struct is that the class constructor can simply CHECK(config.IsOk()) and go down in a ball of fire if that fails.) https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:129: static const int kErbResolution; Static const data members should go before methods. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. 2015 And please check all other files that have been *added* during 2015. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:15: #include <cmath> math.h https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:29: const double kGaussianSamples[64] = {1689.1, 1437, -2251.1, 356.49, -850.24, Encapsulate all constants in an anonymous namespace. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:29: const double kGaussianSamples[64] = {1689.1, 1437, -2251.1, 356.49, -850.24, The constant declarations are a bit messy. I suggest you do the following: - Skip the array size; simply do "const double kGaussianSamples[] = { ... }; - Delete all constants that simply represent the number of elements in another const array. - Use arraysize from webrtc/base/arraysize.h whenever you need the length of a const array. Also, if your test code relies on the fact that an array is of a certain size, use static_assert to verify that at compile time. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:76: void GenerateConstantData(vector<float>& data, float constant) { Don't use non-const reference arguments; use pointers. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg... https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:82: void GenerateGaussianData(vector<float>& data) { Again, don't use non-const reference. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:83: static int count = 0; Static... <cringe>... https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:93: IntelligibilityEnhancer enh_; Declaration order is wrong. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:123: for (int i = 0; i < kSamples; i+= kFragmentSize) { Spaces around += https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:129: float updated = false; bool https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:132: updated = true; You can return true here, skip the bool variable, and return false after the loop (if that is reached). https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:146: for (VarianceArray::StepType step_type : step_types) { for (auto step_type : step_types) https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:147: GenerateConstantData(clear_data_, 0.0f); Do you have to regenerate the data on each loop? https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:167: kMaxTestError); Weird line wrapping. Run "git cl format" on the patch before submitting. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:175: ASSERT_EQ(enh_.start_freq_, 12); What is 12? And why is it crucial that start_freq_ is 12? https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:18: #include <cmath> math.h and string.h https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:28: // Return |current| changed towards |target|, with the change being at most Avoid copying the declaration comment to the implementation. They tend to deviate over time. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:257: // Recomputes variances from scratch each window based on previous window. *for* each window? https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h:44: std::complex<float> data, align https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h:47: void AddToMean(std::complex<float> data, int count, Function description missing. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h:48: std::complex<float>* mean); align
Last comments. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. 2015 https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:16: #include <cmath> math.h https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:44: EXPECT_EQ(intelligibility::UpdateFactor(0, 0, 0), 0); Switch the order of the parameters to EXPECT_EQ. Should be EXPECT_EQ(expected_value, tested_value). The reason is to get the printout correct when the test fails. It prints something like Error: blah failed Expected value: 17 Actual value: 4711 So, getting the order right matters. :) Note, this only applies to the equality tests (*_EQ, and maybe *_NEAR). The inequality tests are different. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:73: /*TEST(IntelligibilityUtilsTest, TestMeanUpdate) { Don't comment out tests. Instead, disable them so that they still compile, and don't rot over time. TEST(IntelligibilityUtilsTest, DISABLED_TestMeanUpdate) And, why is it disabled? https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:90: vector<VarianceArray::StepType> step_types = { const vector https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:96: for (VarianceArray::StepType step_type : step_types) { for (auto ...) https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:158: kMaxError); Wrong indentation.
Welcome Henrik, thanks for helping out :) https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:44: On 2015/06/30 14:00:52, hlundin-webrtc wrote: > Delete empty line. Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:45: const float IntelligibilityEnhancer::kLambdaBot = -1.0; On 2015/06/30 14:00:52, hlundin-webrtc wrote: > Are all of these constants only used locally in this .cc file? If so, you can > move them out of the class and simply declare them in an anonymous namespace in > this file. E.g., > > namespace { > static const int kErbResolution = 2; > ... > } // namespace Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:245: float power_bot, power_top; On 2015/06/30 14:00:51, hlundin-webrtc wrote: > Premature declaration of variables. Declare them when they are used: > float power_top = DotProduct(...); Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:263: float lambda, power; On 2015/06/30 14:00:52, hlundin-webrtc wrote: > Skip declaration of lambda and power here and declare them when used inside the > loop. Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:45: IntelligibilityEnhancer(int erb_resolution, On 2015/06/30 14:00:52, hlundin-webrtc wrote: > This is a very complicated constructor call. Consider using a config struct > instead. See for instance NetEq::Config in neteq.h. The config struct should > typically have a constructor which populates the config with default values. A > user will then only have to modify the values if they differ from the default. > You can also add convenience method to the struct, for instance > std::string ToString() const > for logging and > bool IsOk() const > for validating a configuration before sending it to the class constructor. (An > extra benefit of including an IsOk() method to the Config struct is that the > class constructor can simply CHECK(config.IsOk()) and go down in a ball of fire > if that fails.) You're right. To stay uniform with the other APM components, how about using Constructor + Intialize for creation and SetExtraOptions to modify defaults? Might this change be better to include in the next cl, which integrates into the apm pipeline? https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:129: static const int kErbResolution; On 2015/06/30 14:00:52, hlundin-webrtc wrote: > Static const data members should go before methods. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. On 2015/06/30 14:00:53, hlundin-webrtc wrote: > 2015 > And please check all other files that have been *added* during 2015. Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:15: #include <cmath> On 2015/06/30 14:00:53, hlundin-webrtc wrote: > math.h Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:29: const double kGaussianSamples[64] = {1689.1, 1437, -2251.1, 356.49, -850.24, On 2015/06/30 14:00:52, hlundin-webrtc wrote: > Encapsulate all constants in an anonymous namespace. Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:29: const double kGaussianSamples[64] = {1689.1, 1437, -2251.1, 356.49, -850.24, On 2015/06/30 14:00:53, hlundin-webrtc wrote: > The constant declarations are a bit messy. I suggest you do the following: > - Skip the array size; simply do "const double kGaussianSamples[] = { ... }; > - Delete all constants that simply represent the number of elements in another > const array. > - Use arraysize from webrtc/base/arraysize.h whenever you need the length of a > const array. > > Also, if your test code relies on the fact that an array is of a certain size, > use static_assert to verify that at compile time. Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:76: void GenerateConstantData(vector<float>& data, float constant) { On 2015/06/30 14:00:53, hlundin-webrtc wrote: > Don't use non-const reference arguments; use pointers. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg... Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:82: void GenerateGaussianData(vector<float>& data) { On 2015/06/30 14:00:53, hlundin-webrtc wrote: > Again, don't use non-const reference. Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:83: static int count = 0; On 2015/06/30 14:00:53, hlundin-webrtc wrote: > Static... <cringe>... Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:93: IntelligibilityEnhancer enh_; On 2015/06/30 14:00:53, hlundin-webrtc wrote: > Declaration order is wrong. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:123: for (int i = 0; i < kSamples; i+= kFragmentSize) { On 2015/06/30 14:00:53, hlundin-webrtc wrote: > Spaces around += Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:129: float updated = false; On 2015/06/30 14:00:52, hlundin-webrtc wrote: > bool Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:132: updated = true; On 2015/06/30 14:00:53, hlundin-webrtc wrote: > You can return true here, skip the bool variable, and return false after the > loop (if that is reached). Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:146: for (VarianceArray::StepType step_type : step_types) { On 2015/06/30 14:00:53, hlundin-webrtc wrote: > for (auto step_type : step_types) Done. Thanks; nice to know auto is supported in the style guide. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:147: GenerateConstantData(clear_data_, 0.0f); On 2015/06/30 14:00:52, hlundin-webrtc wrote: > Do you have to regenerate the data on each loop? Done. The clear data needs to be regenerated, but the noise does not. I've split each check into its own loop to reflect that. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:167: kMaxTestError); On 2015/06/30 14:00:52, hlundin-webrtc wrote: > Weird line wrapping. Run "git cl format" on the patch before submitting. Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:175: ASSERT_EQ(enh_.start_freq_, 12); On 2015/06/30 14:00:52, hlundin-webrtc wrote: > What is 12? And why is it crucial that start_freq_ is 12? Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:18: #include <cmath> On 2015/06/30 14:00:53, hlundin-webrtc wrote: > math.h and string.h Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:28: // Return |current| changed towards |target|, with the change being at most On 2015/06/30 14:00:53, hlundin-webrtc wrote: > Avoid copying the declaration comment to the implementation. They tend to > deviate over time. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... > Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:257: // Recomputes variances from scratch each window based on previous window. On 2015/06/30 14:00:53, hlundin-webrtc wrote: > *for* each window? Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h:44: std::complex<float> data, On 2015/06/30 14:00:54, hlundin-webrtc wrote: > align Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h:47: void AddToMean(std::complex<float> data, int count, On 2015/06/30 14:00:53, hlundin-webrtc wrote: > Function description missing. Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h:48: std::complex<float>* mean); On 2015/06/30 14:00:53, hlundin-webrtc wrote: > align Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. On 2015/07/01 09:09:27, hlundin-webrtc wrote: > 2015 Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:16: #include <cmath> On 2015/07/01 09:09:27, hlundin-webrtc wrote: > math.h Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:44: EXPECT_EQ(intelligibility::UpdateFactor(0, 0, 0), 0); On 2015/07/01 09:09:27, hlundin-webrtc wrote: > Switch the order of the parameters to EXPECT_EQ. Should be > EXPECT_EQ(expected_value, tested_value). The reason is to get the printout > correct when the test fails. It prints something like > > Error: blah failed > Expected value: 17 > Actual value: 4711 > > So, getting the order right matters. :) > Note, this only applies to the equality tests (*_EQ, and maybe *_NEAR). The > inequality tests are different. Done. It looks like for _NEAR order doesn't matter https://code.google.com/p/googletest/wiki/AdvancedGuide. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:73: /*TEST(IntelligibilityUtilsTest, TestMeanUpdate) { On 2015/07/01 09:09:27, hlundin-webrtc wrote: > Don't comment out tests. Instead, disable them so that they still compile, and > don't rot over time. > TEST(IntelligibilityUtilsTest, DISABLED_TestMeanUpdate) > > And, why is it disabled? Thanks for telling me about DISABLED_, that'll definitely be useful. This test should not be disabled; just forgot to uncomment :). https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:90: vector<VarianceArray::StepType> step_types = { On 2015/07/01 09:09:27, hlundin-webrtc wrote: > const vector Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:96: for (VarianceArray::StepType step_type : step_types) { On 2015/07/01 09:09:27, hlundin-webrtc wrote: > for (auto ...) Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:158: kMaxError); On 2015/07/01 09:09:27, hlundin-webrtc wrote: > Wrong indentation. Done.
https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:45: IntelligibilityEnhancer(int erb_resolution, On 2015/07/01 23:48:25, ekm wrote: > On 2015/06/30 14:00:52, hlundin-webrtc wrote: > > This is a very complicated constructor call. Consider using a config struct > > instead. See for instance NetEq::Config in neteq.h. The config struct should > > typically have a constructor which populates the config with default values. A > > user will then only have to modify the values if they differ from the default. > > You can also add convenience method to the struct, for instance > > std::string ToString() const > > for logging and > > bool IsOk() const > > for validating a configuration before sending it to the class constructor. (An > > extra benefit of including an IsOk() method to the Config struct is that the > > class constructor can simply CHECK(config.IsOk()) and go down in a ball of > fire > > if that fails.) > > You're right. To stay uniform with the other APM components, how about using > Constructor + Intialize for creation and SetExtraOptions to modify defaults? > Might this change be better to include in the next cl, which integrates into the > apm pipeline? Don't worry too much about uniformity with other components; some of the older ones may be less than ideal :) I think initializing everything in the constructor is fine, but is it really useful to expose all of these parameters? Do you intend to use them in a tool for optimizing them? If so, or if you want to keep some of them, an alternative is to use default arguments, which is recently permitted by the style guide in constructors. (Henrik's Config is a slick workaround for the ban on default arguments :) I'll send you an example off-review. But I'd make anything you're not intending to modify externally an internal const.
Didn't look that closely, but a few more comments. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:29: const double kGaussianSamples[64] = {1689.1, 1437, -2251.1, 356.49, -850.24, On 2015/06/30 14:00:52, hlundin-webrtc wrote: > Encapsulate all constants in an anonymous namespace. In fact, it's a good practice to wrap the entire test in an anonymous namespace. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:44: EXPECT_EQ(intelligibility::UpdateFactor(0, 0, 0), 0); On 2015/07/01 23:48:26, ekm wrote: > On 2015/07/01 09:09:27, hlundin-webrtc wrote: > > Switch the order of the parameters to EXPECT_EQ. Should be > > EXPECT_EQ(expected_value, tested_value). The reason is to get the printout > > correct when the test fails. It prints something like > > > > Error: blah failed > > Expected value: 17 > > Actual value: 4711 > > > > So, getting the order right matters. :) > > Note, this only applies to the equality tests (*_EQ, and maybe *_NEAR). The > > inequality tests are different. > > Done. It looks like for _NEAR order doesn't matter > https://code.google.com/p/googletest/wiki/AdvancedGuide. For consistency, I'd still use EXPECT_NEAR(expected, actual, abs_error); https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:158: kMaxError); On 2015/07/01 23:48:27, ekm wrote: > On 2015/07/01 09:09:27, hlundin-webrtc wrote: > > Wrong indentation. > > Done. Strongly suggest running "git cl format" on all your files just before landing. (Not earlier to avoid the diffs hampering review). It's good enough now that I can just ignore any formatting my editor doesn't do automatically, knowing it'll clean it up for me later. https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:30: namespace { Not a big deal, but normally this is still wrapped in the webrtc namespace. https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:215: float power_target = std::accumulate( const https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:239: float power_top = const https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:242: float power_bot = const https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:260: float lambda = lambda_bot + (lambda_top - lambda_bot) / 2.0f; const https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:262: float power = const https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:101: void GenerateConstantData(vector<float>* data, float constant) { Replace this function with: std::fill(data.begin(), data.end(), constant); at all callsites. https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:107: void GenerateGaussianData(vector<float>* data, int* count) { Does this need to be Gaussian distributed, or will uniform noise do? Consider just std::rand() with a seed to make it deterministic.
lgmt with mandatory comments. Well done! https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:45: IntelligibilityEnhancer(int erb_resolution, On 2015/07/02 02:24:49, andrew wrote: > On 2015/07/01 23:48:25, ekm wrote: > > On 2015/06/30 14:00:52, hlundin-webrtc wrote: > > > This is a very complicated constructor call. Consider using a config struct > > > instead. See for instance NetEq::Config in neteq.h. The config struct should > > > typically have a constructor which populates the config with default values. > A > > > user will then only have to modify the values if they differ from the > default. > > > You can also add convenience method to the struct, for instance > > > std::string ToString() const > > > for logging and > > > bool IsOk() const > > > for validating a configuration before sending it to the class constructor. > (An > > > extra benefit of including an IsOk() method to the Config struct is that the > > > class constructor can simply CHECK(config.IsOk()) and go down in a ball of > > fire > > > if that fails.) > > > > You're right. To stay uniform with the other APM components, how about using > > Constructor + Intialize for creation and SetExtraOptions to modify defaults? > > Might this change be better to include in the next cl, which integrates into > the > > apm pipeline? > > Don't worry too much about uniformity with other components; some of the older > ones may be less than ideal :) > > I think initializing everything in the constructor is fine, but is it really > useful to expose all of these parameters? Do you intend to use them in a tool > for optimizing them? If so, or if you want to keep some of them, an alternative > is to use default arguments, which is recently permitted by the style guide in > constructors. (Henrik's Config is a slick workaround for the ban on default > arguments :) > > I'll send you an example off-review. But I'd make anything you're not intending > to modify externally an internal const. I still think that a Config struct is a better option. Don't worry about the legacy code. Config structs are on the rise in the WebRTC code base, e.g., the new Call API and friends, new AudioEncoder classes. But, I'm fine with you doing that in a follow-up. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:73: /*TEST(IntelligibilityUtilsTest, TestMeanUpdate) { On 2015/07/01 23:48:26, ekm wrote: > On 2015/07/01 09:09:27, hlundin-webrtc wrote: > > Don't comment out tests. Instead, disable them so that they still compile, and > > don't rot over time. > > TEST(IntelligibilityUtilsTest, DISABLED_TestMeanUpdate) > > > > And, why is it disabled? > > Thanks for telling me about DISABLED_, that'll definitely be useful. This test > should not be disabled; just forgot to uncomment :). Acknowledged. https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h (right): https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. 2015 In fact, I believe all the files in webrtc/modules/audio_processing/intelligibility/ were created in 2015, and should be dated such. Please, fix. https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:37: You have a blank line here that is not present between the rest of the constants.
I don't always write lgmt, but when I do, I mean lgtm... With comments.
https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:45: IntelligibilityEnhancer(int erb_resolution, On 2015/07/02 10:53:13, hlundin-webrtc-VACATIONtoAUG3 wrote: > On 2015/07/02 02:24:49, andrew wrote: > > On 2015/07/01 23:48:25, ekm wrote: > > > On 2015/06/30 14:00:52, hlundin-webrtc wrote: > > > > This is a very complicated constructor call. Consider using a config > struct > > > > instead. See for instance NetEq::Config in neteq.h. The config struct > should > > > > typically have a constructor which populates the config with default > values. > > A > > > > user will then only have to modify the values if they differ from the > > default. > > > > You can also add convenience method to the struct, for instance > > > > std::string ToString() const > > > > for logging and > > > > bool IsOk() const > > > > for validating a configuration before sending it to the class constructor. > > (An > > > > extra benefit of including an IsOk() method to the Config struct is that > the > > > > class constructor can simply CHECK(config.IsOk()) and go down in a ball of > > > fire > > > > if that fails.) > > > > > > You're right. To stay uniform with the other APM components, how about using > > > Constructor + Intialize for creation and SetExtraOptions to modify defaults? > > > Might this change be better to include in the next cl, which integrates into > > the > > > apm pipeline? > > > > Don't worry too much about uniformity with other components; some of the older > > ones may be less than ideal :) > > > > I think initializing everything in the constructor is fine, but is it really > > useful to expose all of these parameters? Do you intend to use them in a tool > > for optimizing them? If so, or if you want to keep some of them, an > alternative > > is to use default arguments, which is recently permitted by the style guide in > > constructors. (Henrik's Config is a slick workaround for the ban on default > > arguments :) > > > > I'll send you an example off-review. But I'd make anything you're not > intending > > to modify externally an internal const. > > I still think that a Config struct is a better option. Don't worry about the > legacy code. Config structs are on the rise in the WebRTC code base, e.g., the > new Call API and friends, new AudioEncoder classes. > > But, I'm fine with you doing that in a follow-up. Acknowledged. Some of these params should not be exposed at all; a few I'd like to keep for optimizing in a tool. I'll go for the Config in the next cl. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:29: const double kGaussianSamples[64] = {1689.1, 1437, -2251.1, 356.49, -850.24, On 2015/07/02 02:46:47, andrew wrote: > On 2015/06/30 14:00:52, hlundin-webrtc wrote: > > Encapsulate all constants in an anonymous namespace. > > In fact, it's a good practice to wrap the entire test in an anonymous namespace. Got it. Leaving as is in this case since I'm using FRIEND_TEST. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:44: EXPECT_EQ(intelligibility::UpdateFactor(0, 0, 0), 0); On 2015/07/02 02:46:47, andrew wrote: > On 2015/07/01 23:48:26, ekm wrote: > > On 2015/07/01 09:09:27, hlundin-webrtc wrote: > > > Switch the order of the parameters to EXPECT_EQ. Should be > > > EXPECT_EQ(expected_value, tested_value). The reason is to get the printout > > > correct when the test fails. It prints something like > > > > > > Error: blah failed > > > Expected value: 17 > > > Actual value: 4711 > > > > > > So, getting the order right matters. :) > > > Note, this only applies to the equality tests (*_EQ, and maybe *_NEAR). The > > > inequality tests are different. > > > > Done. It looks like for _NEAR order doesn't matter > > https://code.google.com/p/googletest/wiki/AdvancedGuide. > > For consistency, I'd still use EXPECT_NEAR(expected, actual, abs_error); Done. https://codereview.webrtc.org/1207353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils_unittest.cc:158: kMaxError); On 2015/07/02 02:46:47, andrew wrote: > On 2015/07/01 23:48:27, ekm wrote: > > On 2015/07/01 09:09:27, hlundin-webrtc wrote: > > > Wrong indentation. > > > > Done. > > Strongly suggest running "git cl format" on all your files just before landing. > (Not earlier to avoid the diffs hampering review). > > It's good enough now that I can just ignore any formatting my editor doesn't do > automatically, knowing it'll clean it up for me later. Acknowledged. https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:30: namespace { On 2015/07/02 02:46:48, andrew wrote: > Not a big deal, but normally this is still wrapped in the webrtc namespace. Done. https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:215: float power_target = std::accumulate( On 2015/07/02 02:46:48, andrew wrote: > const Done. https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:239: float power_top = On 2015/07/02 02:46:48, andrew wrote: > const Done. https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:242: float power_bot = On 2015/07/02 02:46:48, andrew wrote: > const Done. https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:260: float lambda = lambda_bot + (lambda_top - lambda_bot) / 2.0f; On 2015/07/02 02:46:48, andrew wrote: > const Done. https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:262: float power = On 2015/07/02 02:46:47, andrew wrote: > const Done. https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h (right): https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. On 2015/07/02 10:53:13, hlundin-webrtc-VACATIONtoAUG3 wrote: > 2015 > > In fact, I believe all the files in > webrtc/modules/audio_processing/intelligibility/ were created in 2015, and > should be dated such. Please, fix. intelligibility_enhancer, _utils, and _proc existed in the old internal repo where they were added in 2014. Should I still change to 2015 since that's when they were added to the current repo? https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:37: On 2015/07/02 10:53:13, hlundin-webrtc-VACATIONtoAUG3 wrote: > You have a blank line here that is not present between the rest of the > constants. Done. https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:101: void GenerateConstantData(vector<float>* data, float constant) { On 2015/07/02 02:46:48, andrew wrote: > Replace this function with: > std::fill(data.begin(), data.end(), constant); > > at all callsites. Done. https://codereview.webrtc.org/1207353002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:107: void GenerateGaussianData(vector<float>* data, int* count) { On 2015/07/02 02:46:48, andrew wrote: > Does this need to be Gaussian distributed, or will uniform noise do? Consider > just std::rand() with a seed to make it deterministic. Done.
lgtm with a few things to fix. https://codereview.webrtc.org/1207353002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1207353002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:259: while (fabs(power_ratio - 1.0f) > kConvergeThresh && iters <= kMaxIters) { std::fabs https://codereview.webrtc.org/1207353002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:269: power_ratio = fabs(power / power_target); To be sure we avoid a divide, outside the loop: const float reciprocal_power_target = 1.f / power_target; ... power_rate = std::fabs(power * reciprocal_power_target); https://codereview.webrtc.org/1207353002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:136: vector<VarianceArray::StepType> step_types = { I don't think you should bother now, but just so you know there's a gtest facility for doing this kind of thing called a value-parameterized test. Example here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... https://codereview.webrtc.org/1207353002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:149: std::fill(noise_data_.begin(), noise_data_.end(), This isn't doing what you want. It's filling noise_data_ with a single randomly generated value. Instead: auto float_rand = []() { return std::rand() * 2.f / RAND_MAX - 1; } std::generate(noise_data_.begin(), noise_data_.end(), float_rand); (usually floating point audio values are given in the [-1, 1] range.) (and below with clear_data_)
The CQ bit was checked by ekmeyerson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, andrew@webrtc.org Link to the patchset: https://codereview.webrtc.org/1207353002/#ps120001 (title: "Optimized search for lambda; fixed test data generation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1207353002/120001
https://codereview.webrtc.org/1207353002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1207353002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:259: while (fabs(power_ratio - 1.0f) > kConvergeThresh && iters <= kMaxIters) { On 2015/07/09 03:22:46, andrew wrote: > std::fabs Done. https://codereview.webrtc.org/1207353002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:269: power_ratio = fabs(power / power_target); On 2015/07/09 03:22:46, andrew wrote: > To be sure we avoid a divide, outside the loop: > const float reciprocal_power_target = 1.f / power_target; > ... > power_rate = std::fabs(power * reciprocal_power_target); Done. Nice! https://codereview.webrtc.org/1207353002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1207353002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:136: vector<VarianceArray::StepType> step_types = { On 2015/07/09 03:22:47, andrew wrote: > I don't think you should bother now, but just so you know there's a gtest > facility for doing this kind of thing called a value-parameterized test. > > Example here: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... Acknowledged. Will use in future tests. https://codereview.webrtc.org/1207353002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:149: std::fill(noise_data_.begin(), noise_data_.end(), On 2015/07/09 03:22:47, andrew wrote: > This isn't doing what you want. It's filling noise_data_ with a single randomly > generated value. Instead: > > auto float_rand = []() { return std::rand() * 2.f / RAND_MAX - 1; } > std::generate(noise_data_.begin(), noise_data_.end(), float_rand); > > (usually floating point audio values are given in the [-1, 1] range.) > > (and below with clear_data_) Whoops. This is a very nice snippet, thanks. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_x64 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64/builds/2218) win on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win/builds/8584) (exceeded global retry quota)
The CQ bit was unchecked by ekmeyerson@webrtc.org
The CQ bit was checked by ekmeyerson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, andrew@webrtc.org Link to the patchset: https://codereview.webrtc.org/1207353002/#ps140001 (title: "Addressed trybot errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1207353002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_drmemory_light on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...) win_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/8474) win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/8275) (exceeded global retry quota)
The CQ bit was checked by ekmeyerson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, andrew@webrtc.org Link to the patchset: https://codereview.webrtc.org/1207353002/#ps220001 (title: "Remove non-determinism by switching to vector")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1207353002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_x64_gn on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn/builds/2976) win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/8290) (exceeded global retry quota)
The CQ bit was checked by ekmeyerson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, andrew@webrtc.org Link to the patchset: https://codereview.webrtc.org/1207353002/#ps240001 (title: "Removed divide by 0 check, fixed headers")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1207353002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/145)
Message was sent while issue was closed.
Committed patchset #14 (id:280001) manually as 35b72fbceb09031cbd6039e0dbbd44ed24296509 (presubmit successful). |
