Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1394803002:
Unittest for the locking in APM (Closed)
|
Created:
5 years, 2 months ago by peah-webrtc Modified:
5 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, hlundin-webrtc, aluebs-webrtc, bjornv1, ivoc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded unittest of the locking functionality in the audio processing module
The test is currently disabled as it takes too long to run in a coffe-cup manner
BUG=webrtc:5099
Committed: https://crrev.com/1f1912d1f0432c5ddc208b9f7f2b1c1bf210b79a
Cr-Commit-Position: refs/heads/master@{#10560}
Patch Set 1 #
Total comments: 56
Patch Set 2 : Updates in response to reviewer comments #Patch Set 3 : Added missing test disabling by default #Patch Set 4 : Added a brief level of unittest #Patch Set 5 : Added threadsafe random value generator and redesigned the shared to local state-variable copy #Patch Set 6 : Changed the random generator name and description #Patch Set 7 : Changed name of the random number generator #
Total comments: 70
Patch Set 8 : Various fixes in response to reviewer comments #
Total comments: 12
Patch Set 9 : Moved initialization to struct constructors, moved testcase generators to the TestCase class, fixed… #Patch Set 10 : Split the code for the different threads into three classes. Added some refactoring of the test cas… #Patch Set 11 : Fixed an erroneous thruth check" #
Total comments: 18
Patch Set 12 : Changes in rensponse to reviewer comments. #
Total comments: 5
Patch Set 13 : Fixed bug returning the wrong counter #Patch Set 14 : Merge #Patch Set 15 : Removed failing and non-possible output check #Patch Set 16 : Latest merged code #Patch Set 17 : Replaced asserts by FAIL #
Total comments: 62
Patch Set 18 : Response to reviewer comments #
Total comments: 12
Patch Set 19 : Changes in response to lastest reviewer comments #
Total comments: 4
Patch Set 20 : Changes according to latest excellent reviewer comments #Patch Set 21 : Corrected the use of rtc::Random to be on par with latest changes in master #Patch Set 22 : Correction of the previous patch set #Patch Set 23 : Removed keyboard from the channel data specification that crashed test #Patch Set 24 : Fixed include path from merge that broke build. #Patch Set 25 : Disabled the brief lock test as it does not yet pass through memory sanitizer (until the new lock s… #
Dependent Patchsets: Messages
Total messages: 59 (12 generated)
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org, solenberg@google.com, solenberg@webrtc.org
I added an extensive unittest for the locking scheme in the APM. It is disabled because it takes too long to run. The intention is that this unittest should work for any locking scheme used in APM.
On 2015/10/08 11:40:08, peah-webrtc wrote: > I added an extensive unittest for the locking scheme in the APM. It is disabled > because it takes too long to run. The intention is that this unittest should > work for any locking scheme used in APM. This is a good tool, and should be checked into the codebase. But do not put it in modules_unittest. Make it a separate target (executable).
https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:247: int max_absolute_value, amplitude? https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:251: for (int ch = 0; ch < num_channels; ch++) Please, always use braces. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:268: // Store random 16 bit quantized float number between -1 and 1. Assert on the range of max_absolute_value, plus change name to "amplitude". https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:271: (max_absolute_value + 1)); This computation is not correct. Say that max_absolute_value is 32768: If we after the modulo have the max, 65536, then subtracting (32768+1) yields 32767; fine. If we after the module have the min, 0, then subtracting (32768+1) yields -32769; not so fine, if we intend to stick it in an int16_t.
On 2015/10/08 12:02:20, minyue-webrtc wrote: > On 2015/10/08 11:40:08, peah-webrtc wrote: > > I added an extensive unittest for the locking scheme in the APM. It is > disabled > > because it takes too long to run. The intention is that this unittest should > > work for any locking scheme used in APM. > > This is a good tool, and should be checked into the codebase. But do not put it > in modules_unittest. Make it a separate target (executable). Yes, but it would be even better if it could always run on try/build bots. A major point of testing locking should be to test on *all available platforms*. Do we really need to test all permutations? I have a problem with understanding the boundaries for this test. Why are different flavors of AEC tested, but not different settings for the AGC? If we're black-box testing the whole APM, shouldn't we test *all* combinations? Or rather, are we testing too many, as it is now?
+1 to Minyue's idea about putting it in a separate binary. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. Use the present year without rounding down to the nearest integer multiple of 53. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:47: // Holds the configuration for the test to use. You can probably drop the "Holds the" in all these descriptions. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:109: delete[] render_output_frame_; Use scoped_ptrs to hold these. Or even better: since the sizes are known at compile time, don't allocate them separately. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:123: bool use_config = false; Move line 123 to 144, to reduce the scope of use_config. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:243: const int kCaptureInputFixLevel = 1024; static const for all of these? https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:271: (max_absolute_value + 1)); I recognize this from 15 lines ago. Subroutine? https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:298: return false; Just return (render_count_ > kMinNumCalls) && (capture_count_ > kMinNumCalls); https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:301: // Sleeps a random time. Time unit? https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:523: } If you want, you can write it like this: const int render_count_local = [&]{ rtc::CritScope cs(&crit_); return render_count_; }(); This is easier to read, since you never have an uninitialized variable (and can even make it const). https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:530: else if ((render_count_local % 47) == 0) Drop the extra parentheses. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:537: render_input_sample_rate_hz_ = 8000; Where do all these numbers come from? https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:895: int render_output_samples_per_channel_; This is a large pile of member variables. Any chance they'll neatly split into, say, one private object per thread (with no locking) plus one shared object?
On 2015/10/08 12:40:39, the sun wrote: > On 2015/10/08 12:02:20, minyue-webrtc wrote: > > On 2015/10/08 11:40:08, peah-webrtc wrote: > > > I added an extensive unittest for the locking scheme in the APM. It is > > disabled > > > because it takes too long to run. The intention is that this unittest should > > > work for any locking scheme used in APM. > > > > This is a good tool, and should be checked into the codebase. But do not put > it > > in modules_unittest. Make it a separate target (executable). > > Yes, but it would be even better if it could always run on try/build bots. A > major point of testing locking should be to test on *all available platforms*. > > Do we really need to test all permutations? Yes, I agree. This exhaustive test can be good anyway. In additional to it, it would be good to have some dedicated tests that secure some important cases, e.g., a test that fails if a tricky locking in the src is mistakenly removed. > > I have a problem with understanding the boundaries for this test. Why are > different flavors of AEC tested, but not different settings for the AGC? If > we're black-box testing the whole APM, shouldn't we test *all* combinations? Or > rather, are we testing too many, as it is now?
ivoc@webrtc.org changed reviewers: + ivoc@webrtc.org
https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:19: #include "webrtc/base/criticalsection.h" These should be in alphabetical order, I think. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:44: enum class RuntimeParameterSettingScheme { Scheme1, Scheme2, Scheme3, Scheme4 }; I think these enums are a bit cryptic. Is it possible to give descriptive names or at least add some comments explaining the different schemes (this also applies to the previous 2 enums). https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:109: delete[] render_output_frame_; On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > Use scoped_ptrs to hold these. Or even better: since the sizes are known at > compile time, don't allocate them separately. vectors are another option. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:256: static_cast<float>((rand_r(seed) % (32768 + 32768 + 1)) - 32768) / I don't understand the "+ 1" here. A 16 bit value can have 2^16 different values right? This value can have 2^16 + 1 values (normally the max value a 16 bit unsigned value can take is 2^16 - 1). IMO, a better way would be to do: (2*(rand_r(seed) % 65536) / 65535.f)-1.f https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:271: (max_absolute_value + 1)); On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > I recognize this from 15 lines ago. Subroutine? Not exactly the same, there's no division and conversion to float here. but I agree that this function doesn't look correct. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:389: (void)apm_->set_stream_delay_ms(30); What does this (void) thing do? https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:410: assert(false); Shouldn't this be something like ASSERT_TRUE(false), so we mark the test as failed instead of crashing the executable? https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:537: render_input_sample_rate_hz_ = 8000; On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > Where do all these numbers come from? Looks very confusing indeed, needs some comments to explain what's going on.
Thanks for all the great review comments! I have made changes in response to the >This is a good tool, and should be checked into the codebase. But do not put it >in modules_unittest. Make it a separate target (executable). That makes sense. But I also agree with the other reviewers that we'd like this to be run on many platforms. And Ideally I'd like it to be run on the bots as that ensures that it is run on all commits. >Yes, but it would be even better if it could always run on try/build bots. A >major point of testing locking should be to test on *all available platforms*. >Do we really need to test all permutations? >I have a problem with understanding the boundaries for this test. Why are >different flavors of AEC tested, but not different settings for the AGC? If >we're black-box testing the whole APM, shouldn't we test *all* combinations? Or >rather, are we testing too many, as it is now? You are right that the boundaries of the test are quite fuzzy but I tried to make them such that the codepaths that could cause lock problems (i.e., the ones that use both render and capture) are tested. Regarding the AGC, both the render and capture sides are always active. For the AEC, it is a bit different as the different settings use different codepaths. I tried to test as many as possible of them, but some are probably redundant and some are probably missing. Regarding the need to test all permutations, I included a brief test parameter set that takes 1 sec https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > Use the present year without rounding down to the nearest integer multiple of > 53. Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:19: #include "webrtc/base/criticalsection.h" On 2015/10/09 15:47:15, ivoc wrote: > These should be in alphabetical order, I think. Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:44: enum class RuntimeParameterSettingScheme { Scheme1, Scheme2, Scheme3, Scheme4 }; On 2015/10/09 15:47:15, ivoc wrote: > I think these enums are a bit cryptic. Is it possible to give descriptive names > or at least add some comments explaining the different schemes (this also > applies to the previous 2 enums). Good point! Should be better now! https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:44: enum class RuntimeParameterSettingScheme { Scheme1, Scheme2, Scheme3, Scheme4 }; On 2015/10/09 15:47:15, ivoc wrote: > I think these enums are a bit cryptic. Is it possible to give descriptive names > or at least add some comments explaining the different schemes (this also > applies to the previous 2 enums). Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:47: // Holds the configuration for the test to use. On 2015/10/08 13:25:21, kwiberg-webrtc wrote: > You can probably drop the "Holds the" in all these descriptions. Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:109: delete[] render_output_frame_; On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > Use scoped_ptrs to hold these. Or even better: since the sizes are known at > compile time, don't allocate them separately. Good point! Now I changed to a scheme using vectors. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:109: delete[] render_output_frame_; On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > Use scoped_ptrs to hold these. Or even better: since the sizes are known at > compile time, don't allocate them separately. Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:109: delete[] render_output_frame_; On 2015/10/09 15:47:15, ivoc wrote: > On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > > Use scoped_ptrs to hold these. Or even better: since the sizes are known at > > compile time, don't allocate them separately. > > vectors are another option. Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:123: bool use_config = false; On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > Move line 123 to 144, to reduce the scope of use_config. Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:243: const int kCaptureInputFixLevel = 1024; On 2015/10/08 13:25:21, kwiberg-webrtc wrote: > static const for all of these? Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:247: int max_absolute_value, On 2015/10/08 12:38:10, the sun wrote: > amplitude? Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:251: for (int ch = 0; ch < num_channels; ch++) On 2015/10/08 12:38:10, the sun wrote: > Please, always use braces. Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:268: // Store random 16 bit quantized float number between -1 and 1. On 2015/10/08 12:38:09, the sun wrote: > Assert on the range of max_absolute_value, plus change name to "amplitude". Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:271: (max_absolute_value + 1)); On 2015/10/08 12:38:09, the sun wrote: > This computation is not correct. Say that max_absolute_value is 32768: > > If we after the modulo have the max, 65536, then subtracting (32768+1) yields > 32767; fine. > If we after the module have the min, 0, then subtracting (32768+1) yields > -32769; not so fine, if we intend to stick it in an int16_t. You are definitely correct. I now limited the range of amplitude by asserts so that it is between 0 and 32767. Then the result should fit in an int16_t. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:271: (max_absolute_value + 1)); On 2015/10/08 12:38:09, the sun wrote: > This computation is not correct. Say that max_absolute_value is 32768: > > If we after the modulo have the max, 65536, then subtracting (32768+1) yields > 32767; fine. > If we after the module have the min, 0, then subtracting (32768+1) yields > -32769; not so fine, if we intend to stick it in an int16_t. Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:271: (max_absolute_value + 1)); On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > I recognize this from 15 lines ago. Subroutine? Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:271: (max_absolute_value + 1)); On 2015/10/09 15:47:15, ivoc wrote: > On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > > I recognize this from 15 lines ago. Subroutine? > > Not exactly the same, there's no division and conversion to float here. but I > agree that this function doesn't look correct. I think it should be correct now. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:271: (max_absolute_value + 1)); On 2015/10/09 15:47:15, ivoc wrote: > On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > > I recognize this from 15 lines ago. Subroutine? > > Not exactly the same, there's no division and conversion to float here. but I > agree that this function doesn't look correct. Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:298: return false; On 2015/10/08 13:25:21, kwiberg-webrtc wrote: > Just > > return (render_count_ > kMinNumCalls) && (capture_count_ > kMinNumCalls); Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:301: // Sleeps a random time. On 2015/10/08 13:25:21, kwiberg-webrtc wrote: > Time unit? Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:389: (void)apm_->set_stream_delay_ms(30); On 2015/10/09 15:47:15, ivoc wrote: > What does this (void) thing do? It explicitly discards the output of the function, thereby avoiding warnings for not catching the output. However, one of these were wrong so I added a check for that output. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:389: (void)apm_->set_stream_delay_ms(30); On 2015/10/09 15:47:15, ivoc wrote: > What does this (void) thing do? Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:410: assert(false); On 2015/10/09 15:47:15, ivoc wrote: > Shouldn't this be something like ASSERT_TRUE(false), so we mark the test as > failed instead of crashing the executable? Good point! Added that! https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:410: assert(false); On 2015/10/09 15:47:15, ivoc wrote: > Shouldn't this be something like ASSERT_TRUE(false), so we mark the test as > failed instead of crashing the executable? Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:523: } On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > If you want, you can write it like this: > > const int render_count_local = [&]{ > rtc::CritScope cs(&crit_); > return render_count_; > }(); > > This is easier to read, since you never have an uninitialized variable (and can > even make it const). That looks awesome! Unfortunately it seems that we are not yet allowed to use that feature so I reverted back to the old implementation. ("Default lambda captures are an unapproved C++ feature. [build/c++11] [4]"). https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:530: else if ((render_count_local % 47) == 0) On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > Drop the extra parentheses. Done. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:537: render_input_sample_rate_hz_ = 8000; On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > Where do all these numbers come from? They are prime numbers that are chosen in order to produce a varying permutation scheme of the settings. I added a comment about that. https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:537: render_input_sample_rate_hz_ = 8000; On 2015/10/09 15:47:15, ivoc wrote: > On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > > Where do all these numbers come from? > > Looks very confusing indeed, needs some comments to explain what's going on. Please let me know if the comment is sufficient! https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:895: int render_output_samples_per_channel_; On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > This is a large pile of member variables. Any chance they'll neatly split into, > say, one private object per thread (with no locking) plus one shared object? Done.
Missed part of the text in the previous message, therefore I send the message again: Thanks for all the great review comments! I have made changes in response to the >This is a good tool, and should be checked into the codebase. But do not put it >in modules_unittest. Make it a separate target (executable). That makes sense. But I also agree with the other reviewers that we'd like this to be run on many platforms. And Ideally I'd like it to be run on the bots as that ensures that it is run on all commits. >Yes, but it would be even better if it could always run on try/build bots. A >major point of testing locking should be to test on *all available platforms*. >Do we really need to test all permutations? >I have a problem with understanding the boundaries for this test. Why are >different flavors of AEC tested, but not different settings for the AGC? If >we're black-box testing the whole APM, shouldn't we test *all* combinations? Or >rather, are we testing too many, as it is now? You are right that the boundaries of the test are quite fuzzy but I tried to make them such that the codepaths that could cause lock problems (i.e., the ones that use both render and capture) are tested. Regarding the AGC, both the render and capture sides are always active. For the AEC, it is a bit different as the different settings use different codepaths. I tried to test as many as possible of them, but some are probably redundant and some are probably missing. Regarding the need to test all permutations, I included a brief test parameter set that takes 1 second to run and that is always activated. Would it be ok to have that one alway activated, and the other one deactivated by default but kept for the occasion when we want to run these tests?
https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:523: } On 2015/10/13 06:58:39, peah-webrtc wrote: > On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > > If you want, you can write it like this: > > > > const int render_count_local = [&]{ > > rtc::CritScope cs(&crit_); > > return render_count_; > > }(); > > > > This is easier to read, since you never have an uninitialized variable (and > can > > even make it const). > > That looks awesome! Unfortunately it seems that we are not yet allowed to use > that feature so I reverted back to the old implementation. ("Default lambda > captures are an unapproved C++ feature. [build/c++11] [4]"). It looks like that rule is going to get the obvious exception for lambdas that can't escape the scope where they're defined, but in the meantime you could capture this explicitly if you want: const int render_count_local = [this]{ rtc::CritScope cs(&crit_); return render_count_; }();
Added threadsafe random value generator and redesigned the shared to local state-variable copy https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:523: } On 2015/10/13 09:35:12, kwiberg-webrtc wrote: > On 2015/10/13 06:58:39, peah-webrtc wrote: > > On 2015/10/08 13:25:22, kwiberg-webrtc wrote: > > > If you want, you can write it like this: > > > > > > const int render_count_local = [&]{ > > > rtc::CritScope cs(&crit_); > > > return render_count_; > > > }(); > > > > > > This is easier to read, since you never have an uninitialized variable (and > > can > > > even make it const). > > > > That looks awesome! Unfortunately it seems that we are not yet allowed to use > > that feature so I reverted back to the old implementation. ("Default lambda > > captures are an unapproved C++ feature. [build/c++11] [4]"). > > It looks like that rule is going to get the obvious exception for lambdas that > can't escape the scope where they're defined, but in the meantime you could > capture this explicitly if you want: > > const int render_count_local = [this]{ > rtc::CritScope cs(&crit_); > return render_count_; > }(); Great! That worked super!
solenberg@webrtc.org changed reviewers: - solenberg@google.com
Description was changed from ========== Added unittest of the locking functionality in the audio processing module The test is currently disabled as it takes too long to run in a coffe-cup manner BUG= ========== to ========== Added unittest of the locking functionality in the audio processing module The test is currently disabled as it takes too long to run in a coffe-cup manner BUG=webrtc:5099 ==========
Any more comments on this? I actually realized that the huge (disabled) test causes the APM to fail in a nonconsistent manner. It is thread related and I've not been able to find where it happens. This does not happen with the new locking scheme implemented so I keep the test as it is for now if ok with everyone.
https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:256: static_cast<float>((rand_r(seed) % (32768 + 32768 + 1)) - 32768) / On 2015/10/09 15:47:15, ivoc wrote: > I don't understand the "+ 1" here. A 16 bit value can have 2^16 different values > right? This value can have 2^16 + 1 values (normally the max value a 16 bit > unsigned value can take is 2^16 - 1). > IMO, a better way would be to do: (2*(rand_r(seed) % 65536) / 65535.f)-1.f You are totally correct in that! It is now rewritten.
I like the effect of separating each thread's private variables from the shared ones. But now the variable names take up half a line each, so I have another suggestion: Split AudioProcessingImpLockTest into several smaller classes, according to how you've already split the member variables. That way, you'll also end up grouping the methods by the data they operate on, which will be welcome in this 990-line class. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:73: explicit ThreadSafeRandomNumberGenerator(int seed) { srand(seed); } Since the sequence of random numbers observed by a given thread will depend on the thread scheduling and thus be unpredictable anyway, providing a constructor with a seed argument is probably just misleading. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:336: assert(amplitude > 0); ASSERT_GT, or maybe EXPECT_GT https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:337: assert(amplitude <= 32767); Same, if you want to keep this no-op check. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:375: void SleepRandomTime(int max_sleep) const { SleepRandomMs?
See comments inline. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:53: BasicWebRtcAecSettingsWithExtentedFilter, I gather that BasicWebRtcAecSettingsWith* alters the configuration from the default one received from BasicWebRtcAecSettings. That is, the default setting does not enable extended filter mode, for instance, but the extra test case does test with that. What if the default config is changed? Can you somehow detect that and throw and error, so that the one doing that change can also flip those tests? https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:63: int initial_sample_rate; Unit should be included in variable name, e.g., initial_sample_rate_hz. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:68: // Provides thread_safe random numbers. thread_safe -> thread-safe https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:73: explicit ThreadSafeRandomNumberGenerator(int seed) { srand(seed); } On 2015/10/26 14:43:14, kwiberg-webrtc wrote: > Since the sequence of random numbers observed by a given thread will depend on > the thread scheduling and thus be unpredictable anyway, providing a constructor > with a seed argument is probably just misleading. Agree. But if you do keep this constructor, the argument should be an unsigned, not an int, to match the argument type of srand(unsigned). https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:95: // Setup the two-dimensional arrays needed for the APM API calls. Nit: Setup -> Set up The former is a noun or adjective, the latter is the verb you are looking for. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:131: virtual void SetUp() { override, and not virtual. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:135: EXPECT_EQ(apm_->kNoError, apm_->level_estimator()->Enable(true)); Does it make sense to continue with the test if any of the EXPECT_EQs in this method fails? If not, change them to ASSERT_EQ instead. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:181: virtual void TearDown() { override, and not virtual. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:220: // Return the created test configurations End with . https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:225: // tests End with . https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:228: // Loop over all possible test configurations End with . https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:281: // parameter setting schemes End with . https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:301: // Return the created test configurations End with . https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:308: static constexpr float kRenderInputFloatLevel = 0.5f; constexpr is still banned in Chromium. See https://chromium-cpp.appspot.com/. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:309: static constexpr float kCaptureInputFloatLevel = 0.03125f; And here. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:323: for (int ch = 0; ch < num_channels; ch++) { Nit: ch and k should probably both be size_t, being indices into an array. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:336: assert(amplitude > 0); On 2015/10/26 14:43:14, kwiberg-webrtc wrote: > ASSERT_GT, or maybe EXPECT_GT Agree. Never use assert, CHECK or DCHECK in unit test code. If they trigger, they'll bring the whole test binary down; hundreds of test targets will die in vain. On the other hand, it is good to know that ASSERT_* doesn't necessarily stop that test target. It only aborts the current function. See https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai.... https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:385: (void)apm_->echo_cancellation()->is_enabled(); Why do you need the C-style (void) cast? https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:437: // End the test early if a fatal failure (ASSERT_*) has occurred. Do you have to test this twice in the same function? https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:447: // End the test early if a fatal failure (ASSERT_*) has occurred. Thrice...?! https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:887: ASSERT_TRUE(false); Simply FAIL(); https://code.google.com/p/googletest/wiki/AdvancedGuide#Explicit_Success_and_... https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:940: // End the test early if a fatal failure (ASSERT_*) has occurred. Same comment as earlier: do you really have to check for fatal failures three times in this function? Especially in the light of the test only having a single fatal failure point after thread start-up. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:1035: std::vector<float*> capture_output_frame; You can make most of the member variable names shorter by dropping the capture_ prefix. When you address them in the code, it is always something like "capture_thread_only_state_.capture_input_samples_per_channel", which mentions capture twice. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:1049: } capture_thread_only_state_; You can make many lines shorter by abbreviating this struct's name to capture_state_. I don't think it makes the code any harder to understand, but definitely easier to read. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:1056: AudioFrame render_frame; Same as above: drop the render_ prefix. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:1071: } render_thread_only_state_; Same as above: name it render_state_.
https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:69: class ThreadSafeRandomNumberGenerator { Please use webrtc/test/random.h instead. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:85: class AudioProcessingImpLockTest : public ::testing::TestWithParam<TestConfig> { nit: final https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:347: static bool CbRenderThread(void* context) { Drop the "Cb", here and below. I think it is confusing to call a thread function a callback. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:349: ->CbRenderImpl(); CbRenderImpl -> RenderThreadImpl or simply RenderThread. And below. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:385: (void)apm_->echo_cancellation()->is_enabled(); Why do you need to call these at all? If you do, don't you want to EXPECT_EQ() the results?
https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:53: BasicWebRtcAecSettingsWithExtentedFilter, On 2015/10/27 10:54:24, hlundin-webrtc wrote: > I gather that BasicWebRtcAecSettingsWith* alters the configuration from the > default one received from BasicWebRtcAecSettings. That is, the default setting > does not enable extended filter mode, for instance, but the extra test case does > test with that. What if the default config is changed? Can you somehow detect > that and throw and error, so that the one doing that change can also flip those > tests? Great point! Unfortunately the APM api does not allow for that to be checked. I instead revised the code to avoid relying on the default values for those. Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:63: int initial_sample_rate; On 2015/10/27 10:54:25, hlundin-webrtc wrote: > Unit should be included in variable name, e.g., initial_sample_rate_hz. Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:68: // Provides thread_safe random numbers. On 2015/10/27 10:54:25, hlundin-webrtc wrote: > thread_safe -> thread-safe Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:69: class ThreadSafeRandomNumberGenerator { On 2015/10/27 11:02:15, the sun wrote: > Please use webrtc/test/random.h instead. Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:73: explicit ThreadSafeRandomNumberGenerator(int seed) { srand(seed); } On 2015/10/26 14:43:14, kwiberg-webrtc wrote: > Since the sequence of random numbers observed by a given thread will depend on > the thread scheduling and thus be unpredictable anyway, providing a constructor > with a seed argument is probably just misleading. Good point. This is now changed to use the random number generator in random.h instead. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:73: explicit ThreadSafeRandomNumberGenerator(int seed) { srand(seed); } On 2015/10/27 10:54:25, hlundin-webrtc wrote: > On 2015/10/26 14:43:14, kwiberg-webrtc wrote: > > Since the sequence of random numbers observed by a given thread will depend on > > the thread scheduling and thus be unpredictable anyway, providing a > constructor > > with a seed argument is probably just misleading. > > Agree. But if you do keep this constructor, the argument should be an unsigned, > not an int, to match the argument type of srand(unsigned). Another good point! This is now, however changed to use the random number generator in random.h instead. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:85: class AudioProcessingImpLockTest : public ::testing::TestWithParam<TestConfig> { On 2015/10/27 11:02:15, the sun wrote: > nit: final Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:95: // Setup the two-dimensional arrays needed for the APM API calls. On 2015/10/27 10:54:25, hlundin-webrtc wrote: > Nit: Setup -> Set up > The former is a noun or adjective, the latter is the verb you are looking for. Great catch! Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:131: virtual void SetUp() { On 2015/10/27 10:54:25, hlundin-webrtc wrote: > override, and not virtual. Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:135: EXPECT_EQ(apm_->kNoError, apm_->level_estimator()->Enable(true)); On 2015/10/27 10:54:25, hlundin-webrtc wrote: > Does it make sense to continue with the test if any of the EXPECT_EQs in this > method fails? If not, change them to ASSERT_EQ instead. Good point! Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:181: virtual void TearDown() { On 2015/10/27 10:54:25, hlundin-webrtc wrote: > override, and not virtual. Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:220: // Return the created test configurations On 2015/10/27 10:54:25, hlundin-webrtc wrote: > End with . Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:225: // tests On 2015/10/27 10:54:25, hlundin-webrtc wrote: > End with . Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:228: // Loop over all possible test configurations On 2015/10/27 10:54:24, hlundin-webrtc wrote: > End with . Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:281: // parameter setting schemes On 2015/10/27 10:54:25, hlundin-webrtc wrote: > End with . Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:301: // Return the created test configurations On 2015/10/27 10:54:25, hlundin-webrtc wrote: > End with . Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:308: static constexpr float kRenderInputFloatLevel = 0.5f; On 2015/10/27 10:54:24, hlundin-webrtc wrote: > constexpr is still banned in Chromium. See https://chromium-cpp.appspot.com/. Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:309: static constexpr float kCaptureInputFloatLevel = 0.03125f; On 2015/10/27 10:54:25, hlundin-webrtc wrote: > And here. Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:323: for (int ch = 0; ch < num_channels; ch++) { On 2015/10/27 10:54:25, hlundin-webrtc wrote: > Nit: ch and k should probably both be size_t, being indices into an array. Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:336: assert(amplitude > 0); On 2015/10/27 10:54:24, hlundin-webrtc wrote: > On 2015/10/26 14:43:14, kwiberg-webrtc wrote: > > ASSERT_GT, or maybe EXPECT_GT > > Agree. Never use assert, CHECK or DCHECK in unit test code. If they trigger, > they'll bring the whole test binary down; hundreds of test targets will die in > vain. > > On the other hand, it is good to know that ASSERT_* doesn't necessarily stop > that test target. It only aborts the current function. See > https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai.... Thanks for the explanation! That is a great point and I guess that ASSERT_* should always be the desired behavior! Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:336: assert(amplitude > 0); On 2015/10/26 14:43:14, kwiberg-webrtc wrote: > ASSERT_GT, or maybe EXPECT_GT Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:337: assert(amplitude <= 32767); On 2015/10/26 14:43:14, kwiberg-webrtc wrote: > Same, if you want to keep this no-op check. Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:347: static bool CbRenderThread(void* context) { On 2015/10/27 11:02:15, the sun wrote: > Drop the "Cb", here and below. I think it is confusing to call a thread function > a callback. Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:349: ->CbRenderImpl(); On 2015/10/27 11:02:15, the sun wrote: > CbRenderImpl -> RenderThreadImpl or simply RenderThread. And below. Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:375: void SleepRandomTime(int max_sleep) const { On 2015/10/26 14:43:14, kwiberg-webrtc wrote: > SleepRandomMs? Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:385: (void)apm_->echo_cancellation()->is_enabled(); On 2015/10/27 10:54:24, hlundin-webrtc wrote: > Why do you need the C-style (void) cast? Ouch! Old habit! I'm used to having build errors when not catching function outputs. Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:385: (void)apm_->echo_cancellation()->is_enabled(); On 2015/10/27 11:02:15, the sun wrote: > Why do you need to call these at all? If you do, don't you want to EXPECT_EQ() > the results? I'm interested in catching any side-effects present in the functions. Some are for sure at the moment trivial getters but for instance the speech_probablitity() getter is more advanced. It is not that easy to sanity check the outputs of these properly in all cases but I tried to add some basic checks now where it is possible. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:437: // End the test early if a fatal failure (ASSERT_*) has occurred. On 2015/10/27 10:54:25, hlundin-webrtc wrote: > Do you have to test this twice in the same function? Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:447: // End the test early if a fatal failure (ASSERT_*) has occurred. On 2015/10/27 10:54:25, hlundin-webrtc wrote: > Thrice...?! Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:887: ASSERT_TRUE(false); On 2015/10/27 10:54:24, hlundin-webrtc wrote: > Simply FAIL(); > https://code.google.com/p/googletest/wiki/AdvancedGuide#Explicit_Success_and_... Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:940: // End the test early if a fatal failure (ASSERT_*) has occurred. On 2015/10/27 10:54:24, hlundin-webrtc wrote: > Same comment as earlier: do you really have to check for fatal failures three > times in this function? Especially in the light of the test only having a single > fatal failure point after thread start-up. Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:1035: std::vector<float*> capture_output_frame; On 2015/10/27 10:54:25, hlundin-webrtc wrote: > You can make most of the member variable names shorter by dropping the capture_ > prefix. When you address them in the code, it is always something like > "capture_thread_only_state_.capture_input_samples_per_channel", which mentions > capture twice. Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:1049: } capture_thread_only_state_; On 2015/10/27 10:54:24, hlundin-webrtc wrote: > You can make many lines shorter by abbreviating this struct's name to > capture_state_. I don't think it makes the code any harder to understand, but > definitely easier to read. Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:1056: AudioFrame render_frame; On 2015/10/27 10:54:25, hlundin-webrtc wrote: > Same as above: drop the render_ prefix. Done. https://codereview.webrtc.org/1394803002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:1071: } render_thread_only_state_; On 2015/10/27 10:54:24, hlundin-webrtc wrote: > Same as above: name it render_state_. Done.
In order to be able to use the functionality in random.h I needed to do a merge from master, hence the merge. However, the unittest file is unaffected by the merge and that is the only one that needs reviewing.
LGTM with one nit. https://codereview.webrtc.org/1394803002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:296: return rand_gen_.Rand(-amplitude, amplitude); Does this really generate "random number between -(amplitude+1) and amplitude"?
https://codereview.webrtc.org/1394803002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:37: }; nit: space between the enum declarations https://codereview.webrtc.org/1394803002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:80: // Set up the two-dimensional arrays needed for the APM API calls. Why doesn't this setting up go in the c-tors of these structs? https://codereview.webrtc.org/1394803002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:81: capture_thread_state_.input_framechannels_.resize(2 * 480); nit: Use a constant for 480 https://codereview.webrtc.org/1394803002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:169: static std::vector<TestConfig> GenerateBriefTestConfigs() { Move this function out of this class. Maybe put it in TestConfig instead? https://codereview.webrtc.org/1394803002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:207: static std::vector<TestConfig> GenerateExtensiveTestConfigs() { Move out of this class. Into TestConfig?
https://codereview.webrtc.org/1394803002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:37: }; On 2015/10/29 10:31:30, the sun wrote: > nit: space between the enum declarations Done. https://codereview.webrtc.org/1394803002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:80: // Set up the two-dimensional arrays needed for the APM API calls. On 2015/10/29 10:31:30, the sun wrote: > Why doesn't this setting up go in the c-tors of these structs? Done. https://codereview.webrtc.org/1394803002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:81: capture_thread_state_.input_framechannels_.resize(2 * 480); On 2015/10/29 10:31:30, the sun wrote: > nit: Use a constant for 480 Done. https://codereview.webrtc.org/1394803002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:169: static std::vector<TestConfig> GenerateBriefTestConfigs() { On 2015/10/29 10:31:30, the sun wrote: > Move this function out of this class. Maybe put it in TestConfig instead? Done. https://codereview.webrtc.org/1394803002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:207: static std::vector<TestConfig> GenerateExtensiveTestConfigs() { On 2015/10/29 10:31:30, the sun wrote: > Move out of this class. Into TestConfig? Done. https://codereview.webrtc.org/1394803002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:296: return rand_gen_.Rand(-amplitude, amplitude); On 2015/10/29 09:13:22, hlundin-webrtc wrote: > Does this really generate "random number between -(amplitude+1) and amplitude"? Good find! Fixed! Done.
Quite extensive code changes: -Split the code for the different threads into three separate classes in response to reviewer comments. -Refactored the function for generating extensive testcases to be easier to follow. -Did some additional refactoring motivated from the new code structure.
Looks *much* better now. My remaining main concern is that even in this componentized state, the test is still extremely hard to verify as correct simply because of the number of lines of code. If a plane might crash if this test didn't do its job, I'd want tests for the various test components... But given the requirements we actually have, I'll settle for getting the stuff below adressed. https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:64: // Store random 16 bit quantized float number between -1 and 1. This comment isn't consistent with amplitude being a parameter, is it? https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:76: }; Suggestion: "Implementation" -> "Impl" and "Function" -> "". That ought to make these names a bunch shorter without making them any harder to understand. https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:134: // The configuration for the test to use. Remove "to use". https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:198: std::vector<TestConfig> tmp_test_configs; I don't quite get how you use these. You repeatedly clear and fill up one, then swap. And at the end, you return just one of them. Doesn't that throw away almost all the work you're doing in this function? Wouldn't a single vector that you push_back onto do the job? https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:311: class FrameCounter { Nit: plural? https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:557: apm_->gain_control()->set_mode(GainControl::kFixedDigital)); You enable gain control twice, each time setting a different mode afterwards. Does this merit a comment? https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:567: if (test_config_.aec_type == AecType::BasicWebRtcAecSettingsWithAecMobile) { if (a) { ... } else { if (b) { ... } else { ... } } is better written as if (a) { ... } else if (b) { ... } else { ... } https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:767: if ((capture_count_local % 2) == 0) { Nit: use fewer parentheses.
Regarding: <Looks *much* better now. My remaining main concern is that <even in this <componentized state, the test is still extremely hard to <verify as correct <simply because of the number of lines of code. If a plane <might crash if this <test didn't do its job, I'd want tests for the various test <components... <But given the requirements we actually have, I'll settle for <getting the stuffb elow adressed. Totally agree. But this test is not a functional test and more of a test to find potential thread or lock issues. The APM api is quite extensive and in order to find as many of those as possible the test must be broad and and trigger as many allowed parallel operation schemes as possible. My concern is rather that the test is too limited, as it does just to a limited amount test concurrent access to the apm submodules at the same time as the apm is accessed. https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:64: // Store random 16 bit quantized float number between -1 and 1. On 2015/11/04 07:08:25, kwiberg-webrtc wrote: > This comment isn't consistent with amplitude being a parameter, is it? Done. https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:76: }; On 2015/11/04 07:08:25, kwiberg-webrtc wrote: > Suggestion: "Implementation" -> "Impl" and "Function" -> "". That ought to make > these names a bunch shorter without making them any harder to understand. Done. https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:134: // The configuration for the test to use. On 2015/11/04 07:08:25, kwiberg-webrtc wrote: > Remove "to use". Done. https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:198: std::vector<TestConfig> tmp_test_configs; On 2015/11/04 07:08:25, kwiberg-webrtc wrote: > I don't quite get how you use these. You repeatedly clear and fill up one, then > swap. And at the end, you return just one of them. Doesn't that throw away > almost all the work you're doing in this function? > > Wouldn't a single vector that you push_back onto do the job? The idea is to use this pattern to achieve a test set of all combinations of the test parameters. The key is that there is a copy done at each puch_back(). So after the first for-loop there are 4 test_configs created where only the render_api_function variable is specified. Later, in +for (TestConfig test_config : tmp_test_configs) { + for (int capture = static_cast<int>( + CaptureApiFunctionImplementation::ProcessStreamImplementation1); + capture <= + static_cast<int>( + CaptureApiFunctionImplementation::ProcessStreamImplementation3); + capture++) { + test_config.capture_api_function = + static_cast<CaptureApiFunctionImplementation>(capture); + test_configs.push_back(test_config); + } + } each of these 4 test_configs gets their capture_api_function variable re-set to the 3 different values to test for this, and each time a new value is set for this, a copy is made into the test_configs vector. In order to avoid building up the sets with partially empty test_configs, the swap, and clear pattern is needed. Not sure if that made it more clear :-). https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:311: class FrameCounter { On 2015/11/04 07:08:25, kwiberg-webrtc wrote: > Nit: plural? Done. https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:557: apm_->gain_control()->set_mode(GainControl::kFixedDigital)); On 2015/11/04 07:08:25, kwiberg-webrtc wrote: > You enable gain control twice, each time setting a different mode afterwards. > Does this merit a comment? Good catch! I'll remove setting the digital mode, as the most important mode to test is the adaptiveanalog. Done. https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:567: if (test_config_.aec_type == AecType::BasicWebRtcAecSettingsWithAecMobile) { On 2015/11/04 07:08:25, kwiberg-webrtc wrote: > if (a) { > ... > } else { > if (b) { > ... > } else { > ... > } > } > > is better written as > > if (a) { > ... > } else if (b) { > ... > } else { > ... > } Done. https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:767: if ((capture_count_local % 2) == 0) { On 2015/11/04 07:08:25, kwiberg-webrtc wrote: > Nit: use fewer parentheses. Done.
lgtm Fix nits and suggestions as you see fit. https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:198: std::vector<TestConfig> tmp_test_configs; On 2015/11/04 09:07:15, peah-webrtc wrote: > On 2015/11/04 07:08:25, kwiberg-webrtc wrote: > > I don't quite get how you use these. You repeatedly clear and fill up one, > then > > swap. And at the end, you return just one of them. Doesn't that throw away > > almost all the work you're doing in this function? > > > > Wouldn't a single vector that you push_back onto do the job? > > The idea is to use this pattern to achieve a test set of all combinations of the > test parameters. The key is that there is a copy done at each puch_back(). So > after the first for-loop there are 4 test_configs created where only the > render_api_function variable is specified. > > Later, in > +for (TestConfig test_config : tmp_test_configs) { > + for (int capture = static_cast<int>( > + CaptureApiFunctionImplementation::ProcessStreamImplementation1); > + capture <= > + static_cast<int>( > + CaptureApiFunctionImplementation::ProcessStreamImplementation3); > + capture++) { > + test_config.capture_api_function = > + static_cast<CaptureApiFunctionImplementation>(capture); > + test_configs.push_back(test_config); > + } > + } > > each of these 4 test_configs gets their capture_api_function variable re-set to > the 3 different values to test for this, and each time a new value is set for > this, a copy is made into the test_configs vector. > > In order to avoid building up the sets with partially empty test_configs, the > swap, and clear pattern is needed. > > Not sure if that made it more clear :-). OK, I get it now. Suggestion: Break this into a bunch of separate functions. Each function would take a vector of k configs, and return a vector of k*m configs. Long functions and mutable state makes code hard to read. In order to not leak all the functions into the surrounding namespace, you could make them local lambdas: auto InitialConfigs = []{ std::vector<TestConfig> out; for (int i = 0; i < 4; ++i) out.push_back(TestConfig c(i)); return out; }; auto AddFoo = [](const std::vector<TestConfig>& in) { std::vector<TestConfig> out; for (auto c : in) { for (int i = 0; i < 17; ++i) { c.foo = i; out.push_back(c); } } return out; }; auto AddBar = ...; return AddBaz(AddBar(AddFoo(InitialConfigs())));
https://codereview.webrtc.org/1394803002/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:640: assert(false); FAIL(), RTC_NOTREACHED() or ASSERT_TRUE(false)
https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:198: std::vector<TestConfig> tmp_test_configs; On 2015/11/04 09:49:16, kwiberg-webrtc wrote: > On 2015/11/04 09:07:15, peah-webrtc wrote: > > On 2015/11/04 07:08:25, kwiberg-webrtc wrote: > > > I don't quite get how you use these. You repeatedly clear and fill up one, > > then > > > swap. And at the end, you return just one of them. Doesn't that throw away > > > almost all the work you're doing in this function? > > > > > > Wouldn't a single vector that you push_back onto do the job? > > > > The idea is to use this pattern to achieve a test set of all combinations of > the > > test parameters. The key is that there is a copy done at each puch_back(). So > > after the first for-loop there are 4 test_configs created where only the > > render_api_function variable is specified. > > > > Later, in > > +for (TestConfig test_config : tmp_test_configs) { > > + for (int capture = static_cast<int>( > > + > CaptureApiFunctionImplementation::ProcessStreamImplementation1); > > + capture <= > > + static_cast<int>( > > + > CaptureApiFunctionImplementation::ProcessStreamImplementation3); > > + capture++) { > > + test_config.capture_api_function = > > + static_cast<CaptureApiFunctionImplementation>(capture); > > + test_configs.push_back(test_config); > > + } > > + } > > > > each of these 4 test_configs gets their capture_api_function variable re-set > to > > the 3 different values to test for this, and each time a new value is set for > > this, a copy is made into the test_configs vector. > > > > In order to avoid building up the sets with partially empty test_configs, the > > swap, and clear pattern is needed. > > > > Not sure if that made it more clear :-). > > OK, I get it now. > > Suggestion: Break this into a bunch of separate functions. Each function would > take a vector of k configs, and return a vector of k*m configs. Long functions > and mutable state makes code hard to read. > > In order to not leak all the functions into the surrounding namespace, you could > make them local lambdas: > > auto InitialConfigs = []{ > std::vector<TestConfig> out; > for (int i = 0; i < 4; ++i) > out.push_back(TestConfig c(i)); > return out; > }; > > auto AddFoo = [](const std::vector<TestConfig>& in) { > std::vector<TestConfig> out; > for (auto c : in) { > for (int i = 0; i < 17; ++i) { > c.foo = i; > out.push_back(c); > } > } > return out; > }; > > auto AddBar = ...; > > return AddBaz(AddBar(AddFoo(InitialConfigs()))); Great suggestion! Done. https://codereview.webrtc.org/1394803002/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:640: assert(false); On 2015/11/04 12:36:40, the sun wrote: > FAIL(), RTC_NOTREACHED() or ASSERT_TRUE(false) Done.
lgtm https://codereview.webrtc.org/1394803002/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:640: assert(false); On 2015/11/05 10:10:09, peah-webrtc wrote: > On 2015/11/04 12:36:40, the sun wrote: > > FAIL(), RTC_NOTREACHED() or ASSERT_TRUE(false) > > Done. Nit: I still count three assert()s in the latest patch set.
https://codereview.webrtc.org/1394803002/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:640: assert(false); On 2015/11/05 10:15:23, kwiberg-webrtc wrote: > On 2015/11/05 10:10:09, peah-webrtc wrote: > > On 2015/11/04 12:36:40, the sun wrote: > > > FAIL(), RTC_NOTREACHED() or ASSERT_TRUE(false) > > > > Done. > > Nit: I still count three assert()s in the latest patch set. Done. https://codereview.webrtc.org/1394803002/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:640: assert(false); On 2015/11/05 10:15:23, kwiberg-webrtc wrote: > On 2015/11/05 10:10:09, peah-webrtc wrote: > > On 2015/11/04 12:36:40, the sun wrote: > > > FAIL(), RTC_NOTREACHED() or ASSERT_TRUE(false) > > > > Done. > > Nit: I still count three assert()s in the latest patch set. Great! Thanks! Done.
You have my rubber-stamp LGTM pending the other reviewers.
https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:16: #include "testing/gmock/include/gmock/gmock.h" not mocking anything. really needed? https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:31: class AudioProcessingImplLockTest; forward declare isn't necessary https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:47: // Store random 16 bit quantized float number between the specified nit: remove "between the specified limits" https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:65: // amplitude. nit: "float number" is a lie https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:121: AudioProcessing::ChannelLayout output_channel_layout; output_channel_layout and several other members of this struct are not default initialized. that's dangerous. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:140: for (int aec = static_cast<int>( It looks to me as if you are generating 2 identical configs. Why? This type of loop (with casting) is pretty dangerous (reordering the enum will wreak havoc). Instead you should: AecType types[] = { AecType::BasicWebRtcAecSettingsWithDelayAgnosticAec, AecType::BasicWebRtcAecSettingsWithAecMobile }; for (auto aec : types) { ... } https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:174: return ((((test_config.render_api_function == The logic here is super hard to parse and verify. Please make it clearer. Instead of making it a single statement, you could: bool valid_combination_1 = ... bool valid_combination_2 = ... bool invalid_combination = ... return valid_combination_1 && valid_combination_2 && !invalid_combination_1; etc. you get the idea. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:192: auto AddReverseProcessing = [](TestConfig test_config) { nit: I cannot find anything explicit in the style guide about named lambdas, however the only examples uses the local variable naming convention: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Formatting_La... https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:194: for (int render = dangerous loop. see above. and below... https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:287: return AddSampleRates(AddSettingsScheme(AddAecSettings( This looks pretty neat, but is it really simpler than just making a table of the valid combinations and walking through that? https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:291: RenderApiImpl render_api_function; default initialize https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:366: dd https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:378: test::Random* rand_gen_; I know you set these in the c-tor, but giving them default values is a good habit... and below. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:393: dd https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:412: dd https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:434: // Class for implementing the tests of the locks in the audio processing module. You can drop the above comment. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:440: void SetUp() override; nit: if you don't need to call SetUp() and TearDown() yourself, move the overrides into the private: section. It is common to add a comment above overrides so it is clear from which base class/interface they come, e.g.: // webrtc::SendStream implementation. void Start() override; ... https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:449: static bool RenderProcessorCallback(void* context) { If you just called this RenderThread(...) or RenderThreadFunc() etc, you could get rid of the comment. And below. didn't I comment on this before? did you get push back from other reviewers? https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:536: // apm_.reset(AudioProcessingImpl::Create()); remove this line? https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:612: const float CaptureProcessor::kCaptureInputFloatLevel = 0.03125f; put in class declaration https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:659: if (capture_count_local == 0) It may be worth to comment on the need to set up the initial state, when count == 0 https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:738: dd https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:839: // End the test is complete. malformed sentence https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:868: CaptureProcessor::CaptureProcessor( It is customary to define class methods in the same order they appear in the class definition. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:884: const float RenderProcessor::kRenderInputFloatLevel = 0.5f; initialize in class declaration https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:923: memset(frame_data_.frame.data_, 0, frame_data_.input_samples_per_channel * Why do you need to memset when you populate the frame on the next line? https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:999: if (render_count_local == 0) Consider comment about need to init when count is zero https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:1078: dd
LGTM with small nit. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:50: static_cast<float>(rand_gen->Rand(-32767 - 1, 32767)) / I think this cast is not necessary when dividing with a float.
https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:16: #include "testing/gmock/include/gmock/gmock.h" On 2015/11/05 12:27:49, the sun wrote: > not mocking anything. really needed? You are correct. It is not needed. Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:31: class AudioProcessingImplLockTest; On 2015/11/05 12:27:50, the sun wrote: > forward declare isn't necessary As far as I can see it is necessary, as this class is used by the StatsProcessor, CaptureProcessor and RenderProcessor classes, which in turn are used within the AudioProcessingImplLockTest class. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:47: // Store random 16 bit quantized float number between the specified On 2015/11/05 12:27:49, the sun wrote: > nit: remove "between the specified limits" Changed it to be more correct. Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:50: static_cast<float>(rand_gen->Rand(-32767 - 1, 32767)) / On 2015/11/05 12:28:54, ivoc wrote: > I think this cast is not necessary when dividing with a float. Totally correct, removed it. Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:65: // amplitude. On 2015/11/05 12:27:49, the sun wrote: > nit: "float number" is a lie Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:121: AudioProcessing::ChannelLayout output_channel_layout; On 2015/11/05 12:27:49, the sun wrote: > output_channel_layout and several other members of this struct are not default > initialized. that's dangerous. Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:140: for (int aec = static_cast<int>( On 2015/11/05 12:27:50, the sun wrote: > It looks to me as if you are generating 2 identical configs. Why? > > This type of loop (with casting) is pretty dangerous (reordering the enum will > wreak havoc). Instead you should: > > AecType types[] = { AecType::BasicWebRtcAecSettingsWithDelayAgnosticAec, > AecType::BasicWebRtcAecSettingsWithAecMobile }; > for (auto aec : types) { > ... > } Nice foind! It should be two different configs so a bug it is. Thanks! Nice one! Adopted this here, and also in the similar code below. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:174: return ((((test_config.render_api_function == On 2015/11/05 12:27:49, the sun wrote: > The logic here is super hard to parse and verify. Please make it clearer. > Instead of making it a single statement, you could: > > bool valid_combination_1 = ... > bool valid_combination_2 = ... > bool invalid_combination = ... > return valid_combination_1 && valid_combination_2 && !invalid_combination_1; > > etc. you get the idea. Good point! Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:192: auto AddReverseProcessing = [](TestConfig test_config) { On 2015/11/05 12:27:49, the sun wrote: > nit: I cannot find anything explicit in the style guide about named lambdas, > however the only examples uses the local variable naming convention: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Formatting_La... Good point! Will change. Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:194: for (int render = On 2015/11/05 12:27:49, the sun wrote: > dangerous loop. see above. > and below... Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:287: return AddSampleRates(AddSettingsScheme(AddAecSettings( On 2015/11/05 12:27:50, the sun wrote: > This looks pretty neat, but is it really simpler than just making a table of the > valid combinations and walking through that? Not sure how you mean, how would that table be built? Via the gtest framework? https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:291: RenderApiImpl render_api_function; On 2015/11/05 12:27:49, the sun wrote: > default initialize Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:366: On 2015/11/05 12:27:49, the sun wrote: > dd Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:378: test::Random* rand_gen_; On 2015/11/05 12:27:49, the sun wrote: > I know you set these in the c-tor, but giving them default values is a good > habit... > and below. Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:393: On 2015/11/05 12:27:50, the sun wrote: > dd Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:412: On 2015/11/05 12:27:49, the sun wrote: > dd Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:434: // Class for implementing the tests of the locks in the audio processing module. On 2015/11/05 12:27:49, the sun wrote: > You can drop the above comment. Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:440: void SetUp() override; On 2015/11/05 12:27:49, the sun wrote: > nit: if you don't need to call SetUp() and TearDown() yourself, move the > overrides into the private: section. It is common to add a comment above > overrides so it is clear from which base class/interface they come, e.g.: > > // webrtc::SendStream implementation. > void Start() override; > ... Thanks! Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:449: static bool RenderProcessorCallback(void* context) { On 2015/11/05 12:27:49, the sun wrote: > If you just called this RenderThread(...) or RenderThreadFunc() etc, you could > get rid of the comment. > And below. > > didn't I comment on this before? did you get push back from other reviewers? You are correct. I must have changed it again since then though. Sorry. Will change to *threadfunc. Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:536: // apm_.reset(AudioProcessingImpl::Create()); On 2015/11/05 12:27:50, the sun wrote: > remove this line? Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:612: const float CaptureProcessor::kCaptureInputFloatLevel = 0.03125f; On 2015/11/05 12:27:49, the sun wrote: > put in class declaration As far as I understood it, according to the C++ standard it is not allowed to do that for float constants, and the way to work aroung it is to put it outside the class definition. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:659: if (capture_count_local == 0) On 2015/11/05 12:27:50, the sun wrote: > It may be worth to comment on the need to set up the initial state, when count > == 0 I agree that it can be done elsewhere, but I think it is makes sense to set it here as that means that it is clear from this function what setting is used. Or is that what you meant? https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:738: On 2015/11/05 12:27:49, the sun wrote: > dd Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:839: // End the test is complete. On 2015/11/05 12:27:49, the sun wrote: > malformed sentence Thanks! Typo ('f' became 's') Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:868: CaptureProcessor::CaptureProcessor( On 2015/11/05 12:27:49, the sun wrote: > It is customary to define class methods in the same order they appear in the > class definition. Good point! Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:884: const float RenderProcessor::kRenderInputFloatLevel = 0.5f; On 2015/11/05 12:27:49, the sun wrote: > initialize in class declaration As above, I believe it is not possible for the float. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:923: memset(frame_data_.frame.data_, 0, frame_data_.input_samples_per_channel * On 2015/11/05 12:27:49, the sun wrote: > Why do you need to memset when you populate the frame on the next line? No need at all. Removed. Done. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:999: if (render_count_local == 0) On 2015/11/05 12:27:49, the sun wrote: > Consider comment about need to init when count is zero Please see the comment to that one. I like it to be there since then all the information about the sample rate to use is located in one place. https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:1078: On 2015/11/05 12:27:49, the sun wrote: > dd Done.
https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:31: class AudioProcessingImplLockTest; On 2015/11/05 14:48:51, peah-webrtc wrote: > On 2015/11/05 12:27:50, the sun wrote: > > forward declare isn't necessary > > As far as I can see it is necessary, as this class is used by the > StatsProcessor, CaptureProcessor and RenderProcessor classes, which in turn are > used within the AudioProcessingImplLockTest class. Ah, my bad! https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:659: if (capture_count_local == 0) On 2015/11/05 14:48:51, peah-webrtc wrote: > On 2015/11/05 12:27:50, the sun wrote: > > It may be worth to comment on the need to set up the initial state, when count > > == 0 > > I agree that it can be done elsewhere, but I think it is makes sense to set it > here as that means that it is clear from this function what setting is used. Or > is that what you meant? I just thought an extra comment would be nice but never mind.
https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:612: const float CaptureProcessor::kCaptureInputFloatLevel = 0.03125f; On 2015/11/05 14:48:51, peah-webrtc wrote: > On 2015/11/05 12:27:49, the sun wrote: > > put in class declaration > > As far as I understood it, according to the C++ standard it is not allowed to do > that for float constants, and the way to work aroung it is to put it outside the > class definition. vrey well; just out of curiosity, can you point me at the relevant document? https://codereview.webrtc.org/1394803002/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:868: CaptureProcessor::CaptureProcessor( On 2015/11/05 14:48:51, peah-webrtc wrote: > On 2015/11/05 12:27:49, the sun wrote: > > It is customary to define class methods in the same order they appear in the > > class definition. > > Good point! > Done. Not done. If I read the class declaration correctly, the c-tor comes before Process(), which comes before the other functions. https://codereview.webrtc.org/1394803002/diff/340001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:47: frame[ch][k] = (amplitude * rand_gen->Rand(-32767 - 1, 32767) / 32768.0f); nit: no need for the outer parenthesis https://codereview.webrtc.org/1394803002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:265: test_config.initial_sample_rate_hz = 8000; you could express this with tables as well: const int sample_rates[] = { 8000, 16000, 32000, 48000 }; const int sample_rates_mobile[] = { 8000, 16000 }; auto rates = test_config.aec_type == AecType::BasicWebRtcAecSettingsWithAecMobile ? sample_rates_mobile : sample_rates; for (int rate : rates) { test_config.initial_sample_rate_hz = rate; out.push_back(test_config); } https://codereview.webrtc.org/1394803002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:280: test_config.initial_sample_rate_hz = 8000; why do you add two configs each with initial rate 8k and 16k? is that correct? https://codereview.webrtc.org/1394803002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:452: // ::testing::TestWithParam<> implementation super nit: only one comment is customary, above the first method of the interface being implemented (they are assumed to appear in one chunk). https://codereview.webrtc.org/1394803002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:1093: bool RenderProcessor::Process() { Please, make order of definition same as order of declaration. This should go after the c-tor. https://codereview.webrtc.org/1394803002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:1129: void AudioProcessingImplLockTest::CheckTestCompleteness() { Help, I'm lonely, I want to be with my other friends from AudioProcessingImplLockTest!
https://codereview.webrtc.org/1394803002/diff/340001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:47: frame[ch][k] = (amplitude * rand_gen->Rand(-32767 - 1, 32767) / 32768.0f); On 2015/11/05 15:31:14, the sun wrote: > nit: no need for the outer parenthesis Done. https://codereview.webrtc.org/1394803002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:265: test_config.initial_sample_rate_hz = 8000; On 2015/11/05 15:31:14, the sun wrote: > you could express this with tables as well: > > const int sample_rates[] = { 8000, 16000, 32000, 48000 }; > const int sample_rates_mobile[] = { 8000, 16000 }; > auto rates = test_config.aec_type == > AecType::BasicWebRtcAecSettingsWithAecMobile ? sample_rates_mobile : > sample_rates; > for (int rate : rates) { > test_config.initial_sample_rate_hz = rate; > out.push_back(test_config); > } Beautiful!!! Implemented! Done. https://codereview.webrtc.org/1394803002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:280: test_config.initial_sample_rate_hz = 8000; On 2015/11/05 15:31:14, the sun wrote: > why do you add two configs each with initial rate 8k and 16k? is that correct? My error! Removed it. Done. https://codereview.webrtc.org/1394803002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:452: // ::testing::TestWithParam<> implementation On 2015/11/05 15:31:14, the sun wrote: > super nit: only one comment is customary, above the first method of the > interface being implemented (they are assumed to appear in one chunk). Done. https://codereview.webrtc.org/1394803002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:1093: bool RenderProcessor::Process() { On 2015/11/05 15:31:14, the sun wrote: > Please, make order of definition same as order of declaration. This should go > after the c-tor. Done. https://codereview.webrtc.org/1394803002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:1129: void AudioProcessingImplLockTest::CheckTestCompleteness() { On 2015/11/05 15:31:15, the sun wrote: > Help, I'm lonely, I want to be with my other friends from > AudioProcessingImplLockTest! :-) Done.
https://codereview.webrtc.org/1394803002/diff/360001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/360001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:250: if (ValidTestConfig(test_config)) { So this shouldn't be needed now that you *only* generate the valid configs? https://codereview.webrtc.org/1394803002/diff/360001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:266: struct RatesInfo { Why is this necessary? You can range-iterate over a C-style array. See e.g.: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... Also, if I'm somehow wrong about that, your original code is clearly more readable than this new version, in which case you should push back.
https://codereview.webrtc.org/1394803002/diff/360001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1394803002/diff/360001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:250: if (ValidTestConfig(test_config)) { On 2015/11/06 09:02:05, the sun wrote: > So this shouldn't be needed now that you *only* generate the valid configs? My bad! Done. https://codereview.webrtc.org/1394803002/diff/360001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:266: struct RatesInfo { On 2015/11/06 09:02:05, the sun wrote: > Why is this necessary? You can range-iterate over a C-style array. See e.g.: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > Also, if I'm somehow wrong about that, your original code is clearly more > readable than this new version, in which case you should push back. On your suggestion, the code is now rewritten using array_view. So now I think it should be better again. Please behold the new code and be delighted at the beauty of using array_view!
lgtm, ship it!
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org, ivoc@webrtc.org, henrik.lundin@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1394803002/#ps420001 (title: "Correction of the previous patch set")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1394803002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1394803002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_tsan2 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/7827)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, ivoc@webrtc.org, henrik.lundin@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1394803002/#ps460001 (title: "Fixed include path from merge that broke build.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1394803002/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1394803002/460001
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...)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, ivoc@webrtc.org, henrik.lundin@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1394803002/#ps480001 (title: "Disabled the brief lock test as it does not yet pass through memory sanitizer (until the new lock s…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1394803002/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1394803002/480001
Message was sent while issue was closed.
Committed patchset #25 (id:480001)
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/1f1912d1f0432c5ddc208b9f7f2b1c1bf210b79a Cr-Commit-Position: refs/heads/master@{#10560} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
