Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1814723003:
Added a bitexactness test for the intelligibility enhancer in the audio processing module (Closed)
|
Created:
4 years, 9 months ago by peah-webrtc Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, kwiberg-webrtc, minyue-webrtc, the sun, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@BeamformerBitExactness_CL Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded a bitexactness test for the intelligibility enhancer in the audio processing module
BUG=webrtc:5242
Committed: https://crrev.com/4b0c74172ed0a1bb40dc25535da53dee4c4767f2
Cr-Commit-Position: refs/heads/master@{#12129}
Patch Set 1 : #
Total comments: 68
Patch Set 2 : Changes in response to reviewer comments #
Total comments: 2
Patch Set 3 : Moved the bitexactness test code for the intelligibility enhancer #Patch Set 4 : Corrected to use the number of frames for the capture buffer instead of for the render buffer #Patch Set 5 : Disabled all the bitexactness tests until the code has reached a less intense development phase #Patch Set 6 : Rebase #Messages
Total messages: 24 (5 generated)
Patchset #1 (id:1) has been deleted
peah@webrtc.org changed reviewers: + aluebs@webrtc.org, henrik.lundin@webrtc.org
Hi, I've been setting up bitexactness tests for all the APM submodules. This test is for the intelligibility enhancer. A general question is where it should be put. As it is now, the filter is located beneath audio_processing but I think it would probably make more sense to have it beneath audio_processing/intelligibility.
The unit test should sit in the same folder as the class it tests. Moving the implementation of the class is a separate question, for which I have no strong opinion. As long as any subsequent move of the implementation also moves the unit tests, I'm fine. LGTM https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:57: int samples_per_channel = rtc::CheckedDivExact(sample_rate_hz, 100); const
I am not sure how useful these kind of tests are, since they don't capture at all what the component does and only test similarity to a hard-coded value that doesn't mean anything. Any trivial changes in the component's code would require these hard-coded values to be changes to any new arbitrary ones and there is no way to know if those new values make sense. This is specially annoying since this component is still under development and will definitively require some tweaking. Ideally we would like to have a test that checks the over-all behavior of a component, let's say in the case of the intelligibility enhancer check that some intelligibility metric improves, but I understand that is difficult. I still think we could do a better job in at least having a metric that would tell us how big of a difference in the output certain change introduces, so instead of having some arbitrary values, we could calculate the SNR to the reference or something similar. So if we introduce a small change to the component if the SNR is slightly off the change probably makes sense, but if it is completely off it is suspicious. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:23: const int kNumFramesToProcess = 1000; Should probably be a size_t. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:26: void ProcessOneFrame(int sample_rate_hz, You don't need to pass in the sample_rate_hz separately, you can get it from the AudioBuffer. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:41: sample_rate_hz > 16000 ? 16000 : sample_rate_hz, Use AudioProcessing::kSampleRate16kHz instead of 16000, or define your own constant. Also, I would probably make the split_rate calculation in a separate function with a clear name, specially since it is used more than once. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:44: intelligibility_enhancer->SetCaptureNoiseEstimate( Just to mimic the APM, you should probably put this together with the noise_suppressor calls in the "capture side". https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:56: const rtc::ArrayView<const float>& output_reference) { In the ArrayView documentation it says: "ArrayView is tiny (just a pointer and a count) and trivially copyable, so it's probably cheaper to pass it by value than by const reference". https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:57: int samples_per_channel = rtc::CheckedDivExact(sample_rate_hz, 100); You don't need to calculate this explicitly, you can use render_config.num_frames() or even the render_buffer.num_frames(). https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:65: std::vector<float> render_input(samples_per_channel * num_channels); Use render_config.num_frames() or even the render_buffer.num_frames() instead of samples_per_channel. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:70: capture_config.num_frames(), 1, capture_config.num_frames()); Why do you need to hard-code to mono? Also, it would be nice to have a constant for it. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:73: std::vector<float> capture_input(samples_per_channel * num_channels); Use render_config.num_frames() or even the render_buffer.num_frames() instead of samples_per_channel. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:75: rtc::CriticalSection crit_render; Not used. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:78: noise_suppressor.Initialize(1, sample_rate_hz); Why do you need to hard-code to mono? Also, it would be nice to have a constant for it. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:82: sample_rate_hz > 16000 ? 16000 : sample_rate_hz, Use AudioProcessing::kSampleRate16kHz instead of 16000, or define your own constant. Also, I would probably make the split_rate calculation in a separate function with a clear name, specially since it is used more than once. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:85: for (int frame_no = 0; frame_no < kNumFramesToProcess; ++frame_no) { This should probably be size_t. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:86: ReadFloatSamplesFromStereoFile(samples_per_channel, num_channels, Where does this function come from? Also, use render_config.num_frames() or even the render_buffer.num_frames() instead of samples_per_channel. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:88: ReadFloatSamplesFromStereoFile(samples_per_channel, num_channels, Where does this function come from? Also, use capture_config.num_frames() or even the capture_buffer.num_frames() instead of samples_per_channel. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:104: const float kTolerance = 1.0f / 32768.0f; The 0s after the dot are not necessary. There is probably something in numerical_limits you can use instead of this. Else you can at least use some shifting to make clear it is the 1 << 15. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:111: EXPECT_TRUE(test::BitExactFrame(render_config.num_frames(), If it receives a tolerance it is not a bit-exact test. I think this should be renamed to something that reflects the real test that is done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:118: TEST(IntelligibilityEnhancerBitExactnessTest, Mono8kHz) { This test would be a lot simpler if we use TEST_P and INSTANTIATE_TEST_CASE_P. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:121: RunBitexactnessTest(8000, 1, kOutputReference); Use AudioProcessing::kSampleRate8kHz and define your own constant for mono. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:127: RunBitexactnessTest(16000, 1, kOutputReference); Use AudioProcessing::kSampleRate16kHz and define your own constant for mono. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:133: RunBitexactnessTest(32000, 1, kOutputReference); Use AudioProcessing::kSampleRate32kHz and define your own constant for mono. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:139: RunBitexactnessTest(48000, 1, kOutputReference); Use AudioProcessing::kSampleRate48kHz and define your own constant for mono. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:146: RunBitexactnessTest(8000, 2, kOutputReference); Use AudioProcessing::kSampleRate8kHz and define your own constant for stereo. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:153: RunBitexactnessTest(16000, 2, kOutputReference); Use AudioProcessing::kSampleRate16kHz and define your own constant for stereo. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:160: RunBitexactnessTest(32000, 2, kOutputReference); Use AudioProcessing::kSampleRate32kHz and define your own constant for stereo. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:167: RunBitexactnessTest(48000, 2, kOutputReference); Use AudioProcessing::kSampleRate32kHz and define your own constant for stereo. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/modules.gyp File webrtc/modules/modules.gyp (right): https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/modules.gy... webrtc/modules/modules.gyp:449: 'audio_processing/intelligibility_enhancer_unittest.cc', I agree with henrik, that it probably makes more sense to put this under the intelligibility/ folder. On top of that, shouldn't it then be combined with the existing unittest?
https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:23: const int kNumFramesToProcess = 1000; On 2016/03/18 23:26:40, aluebs-webrtc wrote: > Should probably be a size_t. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:26: void ProcessOneFrame(int sample_rate_hz, On 2016/03/18 23:26:41, aluebs-webrtc wrote: > You don't need to pass in the sample_rate_hz separately, you can get it from the > AudioBuffer. I cannot see that the AudioBuffer returns the sample rate in any way. Could you please help me find the function where that is done? https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:41: sample_rate_hz > 16000 ? 16000 : sample_rate_hz, On 2016/03/18 23:26:41, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate16kHz instead of 16000, or define your own > constant. Also, I would probably make the split_rate calculation in a separate > function with a clear name, specially since it is used more than once. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:44: intelligibility_enhancer->SetCaptureNoiseEstimate( On 2016/03/18 23:26:40, aluebs-webrtc wrote: > Just to mimic the APM, you should probably put this together with the > noise_suppressor calls in the "capture side". Good point! Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:56: const rtc::ArrayView<const float>& output_reference) { On 2016/03/18 23:26:40, aluebs-webrtc wrote: > In the ArrayView documentation it says: "ArrayView is tiny (just a pointer and a > count) and trivially copyable, so it's probably cheaper to pass it by value than > by const reference". Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:57: int samples_per_channel = rtc::CheckedDivExact(sample_rate_hz, 100); On 2016/03/18 23:26:41, aluebs-webrtc wrote: > You don't need to calculate this explicitly, you can use > render_config.num_frames() or even the render_buffer.num_frames(). Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:57: int samples_per_channel = rtc::CheckedDivExact(sample_rate_hz, 100); On 2016/03/18 08:47:43, hlundin-webrtc wrote: > const Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:65: std::vector<float> render_input(samples_per_channel * num_channels); On 2016/03/18 23:26:40, aluebs-webrtc wrote: > Use render_config.num_frames() or even the render_buffer.num_frames() instead of > samples_per_channel. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:70: capture_config.num_frames(), 1, capture_config.num_frames()); On 2016/03/18 23:26:41, aluebs-webrtc wrote: > Why do you need to hard-code to mono? Also, it would be nice to have a constant > for it. That is a mistake done caused by mistakenly only creating 1 noise suppressor. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:73: std::vector<float> capture_input(samples_per_channel * num_channels); On 2016/03/18 23:26:40, aluebs-webrtc wrote: > Use render_config.num_frames() or even the render_buffer.num_frames() instead of > samples_per_channel. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:75: rtc::CriticalSection crit_render; On 2016/03/18 23:26:41, aluebs-webrtc wrote: > Not used. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:78: noise_suppressor.Initialize(1, sample_rate_hz); On 2016/03/18 23:26:41, aluebs-webrtc wrote: > Why do you need to hard-code to mono? Also, it would be nice to have a constant > for it. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:82: sample_rate_hz > 16000 ? 16000 : sample_rate_hz, On 2016/03/18 23:26:40, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate16kHz instead of 16000, or define your own > constant. Also, I would probably make the split_rate calculation in a separate > function with a clear name, specially since it is used more than once. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:85: for (int frame_no = 0; frame_no < kNumFramesToProcess; ++frame_no) { On 2016/03/18 23:26:41, aluebs-webrtc wrote: > This should probably be size_t. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:86: ReadFloatSamplesFromStereoFile(samples_per_channel, num_channels, On 2016/03/18 23:26:40, aluebs-webrtc wrote: > Where does this function come from? Also, use render_config.num_frames() or even > the render_buffer.num_frames() instead of samples_per_channel. The function is declared in webrtc/modules/audio_processing/test/bitexactness_tools.h. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:88: ReadFloatSamplesFromStereoFile(samples_per_channel, num_channels, On 2016/03/18 23:26:40, aluebs-webrtc wrote: > Where does this function come from? Also, use capture_config.num_frames() or > even the capture_buffer.num_frames() instead of samples_per_channel. The function is declared in webrtc/modules/audio_processing/test/bitexactness_tools.h. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:104: const float kTolerance = 1.0f / 32768.0f; On 2016/03/18 23:26:40, aluebs-webrtc wrote: > The 0s after the dot are not necessary. There is probably something in > numerical_limits you can use instead of this. Else you can at least use some > shifting to make clear it is the 1 << 15. Great point! Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:111: EXPECT_TRUE(test::BitExactFrame(render_config.num_frames(), On 2016/03/18 23:26:40, aluebs-webrtc wrote: > If it receives a tolerance it is not a bit-exact test. I think this should be > renamed to something that reflects the real test that is done. Good point! What about VectorDifferenceBounded and that kTolerance is renamed to kVectorItemBound? I'll wait with that change until we have agreed on a name as that will affect other files as well. It should for sure be part of this CL but I don't want to create a patch for it until we have a good name. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:118: TEST(IntelligibilityEnhancerBitExactnessTest, Mono8kHz) { On 2016/03/18 23:26:41, aluebs-webrtc wrote: > This test would be a lot simpler if we use TEST_P and INSTANTIATE_TEST_CASE_P. Very likely, but I could not come up with a good way to in an explicit manner combine the testvectors with the input parameters in INSTANTIATE_TEST_CASE_P. Do you have any suggestion on how to achieve that? https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:121: RunBitexactnessTest(8000, 1, kOutputReference); On 2016/03/18 23:26:41, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate8kHz and define your own constant for mono. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:127: RunBitexactnessTest(16000, 1, kOutputReference); On 2016/03/18 23:26:41, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate16kHz and define your own constant for mono. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:133: RunBitexactnessTest(32000, 1, kOutputReference); On 2016/03/18 23:26:41, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate32kHz and define your own constant for mono. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:139: RunBitexactnessTest(48000, 1, kOutputReference); On 2016/03/18 23:26:41, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate48kHz and define your own constant for mono. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:146: RunBitexactnessTest(8000, 2, kOutputReference); On 2016/03/18 23:26:40, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate8kHz and define your own constant for stereo. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:153: RunBitexactnessTest(16000, 2, kOutputReference); On 2016/03/18 23:26:41, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate16kHz and define your own constant for stereo. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:160: RunBitexactnessTest(32000, 2, kOutputReference); On 2016/03/18 23:26:41, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate32kHz and define your own constant for stereo. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:167: RunBitexactnessTest(48000, 2, kOutputReference); On 2016/03/18 23:26:41, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate32kHz and define your own constant for stereo. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/modules.gyp File webrtc/modules/modules.gyp (right): https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/modules.gy... webrtc/modules/modules.gyp:449: 'audio_processing/intelligibility_enhancer_unittest.cc', On 2016/03/18 23:26:41, aluebs-webrtc wrote: > I agree with henrik, that it probably makes more sense to put this under the > intelligibility/ folder. On top of that, shouldn't it then be combined with the > existing unittest? Definitely, I'll do that. Done.
Thank you for separating the moving into its own patch. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:26: void ProcessOneFrame(int sample_rate_hz, On 2016/03/21 11:54:49, peah-webrtc wrote: > On 2016/03/18 23:26:41, aluebs-webrtc wrote: > > You don't need to pass in the sample_rate_hz separately, you can get it from > the > > AudioBuffer. > > I cannot see that the AudioBuffer returns the sample rate in any way. Could you > please help me find the function where that is done? You are right, my bad. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:86: ReadFloatSamplesFromStereoFile(samples_per_channel, num_channels, On 2016/03/21 11:54:50, peah-webrtc wrote: > On 2016/03/18 23:26:40, aluebs-webrtc wrote: > > Where does this function come from? Also, use render_config.num_frames() or > even > > the render_buffer.num_frames() instead of samples_per_channel. > > The function is declared in > webrtc/modules/audio_processing/test/bitexactness_tools.h. Thanks for clarifying. I don't know why my grepping didn't find it originally. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:88: ReadFloatSamplesFromStereoFile(samples_per_channel, num_channels, On 2016/03/21 11:54:50, peah-webrtc wrote: > On 2016/03/18 23:26:40, aluebs-webrtc wrote: > > Where does this function come from? Also, use capture_config.num_frames() or > > even the capture_buffer.num_frames() instead of samples_per_channel. > > The function is declared in > webrtc/modules/audio_processing/test/bitexactness_tools.h. > > Done. Thanks for clarifying. I don't know why my grepping didn't find it originally. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:111: EXPECT_TRUE(test::BitExactFrame(render_config.num_frames(), On 2016/03/21 11:54:49, peah-webrtc wrote: > On 2016/03/18 23:26:40, aluebs-webrtc wrote: > > If it receives a tolerance it is not a bit-exact test. I think this should be > > renamed to something that reflects the real test that is done. > > Good point! > What about VectorDifferenceBounded and that kTolerance is renamed to > kVectorItemBound? > I'll wait with that change until we have agreed on a name as that will affect > other files as well. It should for sure be part of this CL but I don't want to > create a patch for it until we have a good name. I think your suggestion sounds much more intuitive of what it actually does. But if it touches many other files, it might be a better idea to do it in another CL. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:118: TEST(IntelligibilityEnhancerBitExactnessTest, Mono8kHz) { On 2016/03/21 11:54:49, peah-webrtc wrote: > On 2016/03/18 23:26:41, aluebs-webrtc wrote: > > This test would be a lot simpler if we use TEST_P and INSTANTIATE_TEST_CASE_P. > > Very likely, but I could not come up with a good way to in an explicit manner > combine the testvectors with the input parameters in INSTANTIATE_TEST_CASE_P. Do > you have any suggestion on how to achieve that? How about having a method that returns a reference vector from a sample rate and a number of channels? In that way you can only instantiate an the tests using these 2 parameters and then get the reference from them. https://codereview.webrtc.org/1814723003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1814723003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:96: ReadFloatSamplesFromStereoFile(render_buffer.num_frames(), capture_buffer
https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:26: void ProcessOneFrame(int sample_rate_hz, On 2016/03/22 11:53:29, aluebs-webrtc wrote: > On 2016/03/21 11:54:49, peah-webrtc wrote: > > On 2016/03/18 23:26:41, aluebs-webrtc wrote: > > > You don't need to pass in the sample_rate_hz separately, you can get it from > > the > > > AudioBuffer. > > > > I cannot see that the AudioBuffer returns the sample rate in any way. Could > you > > please help me find the function where that is done? > > You are right, my bad. Acknowledged. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:88: ReadFloatSamplesFromStereoFile(samples_per_channel, num_channels, On 2016/03/22 11:53:29, aluebs-webrtc wrote: > On 2016/03/21 11:54:50, peah-webrtc wrote: > > On 2016/03/18 23:26:40, aluebs-webrtc wrote: > > > Where does this function come from? Also, use capture_config.num_frames() or > > > even the capture_buffer.num_frames() instead of samples_per_channel. > > > > The function is declared in > > webrtc/modules/audio_processing/test/bitexactness_tools.h. > > > > Done. > > Thanks for clarifying. I don't know why my grepping didn't find it originally. Acknowledged. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:111: EXPECT_TRUE(test::BitExactFrame(render_config.num_frames(), On 2016/03/22 11:53:29, aluebs-webrtc wrote: > On 2016/03/21 11:54:49, peah-webrtc wrote: > > On 2016/03/18 23:26:40, aluebs-webrtc wrote: > > > If it receives a tolerance it is not a bit-exact test. I think this should > be > > > renamed to something that reflects the real test that is done. > > > > Good point! > > What about VectorDifferenceBounded and that kTolerance is renamed to > > kVectorItemBound? > > I'll wait with that change until we have agreed on a name as that will affect > > other files as well. It should for sure be part of this CL but I don't want to > > create a patch for it until we have a good name. > > I think your suggestion sounds much more intuitive of what it actually does. But > if it touches many other files, it might be a better idea to do it in another > CL. Great! Then I'll do that in another CL. Done. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:118: TEST(IntelligibilityEnhancerBitExactnessTest, Mono8kHz) { On 2016/03/22 11:53:29, aluebs-webrtc wrote: > On 2016/03/21 11:54:49, peah-webrtc wrote: > > On 2016/03/18 23:26:41, aluebs-webrtc wrote: > > > This test would be a lot simpler if we use TEST_P and > INSTANTIATE_TEST_CASE_P. > > > > Very likely, but I could not come up with a good way to in an explicit manner > > combine the testvectors with the input parameters in INSTANTIATE_TEST_CASE_P. > Do > > you have any suggestion on how to achieve that? > > How about having a method that returns a reference vector from a sample rate and > a number of channels? In that way you can only instantiate an the tests using > these 2 parameters and then get the reference from them. As I wrote in the CL for the bitexactness test for the beamformer I think that is a good suggestion and most likely it would produce more compact code. One drawback is, however, that it is not possible to disable tests. The way the tests are set up in this test is how the reviewers of the first of the submodule bitexactness tests preferred it to be. And even though it is a bit verbose, I like that it is very explicit.
https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:86: ReadFloatSamplesFromStereoFile(samples_per_channel, num_channels, On 2016/03/22 11:53:28, aluebs-webrtc wrote: > On 2016/03/21 11:54:50, peah-webrtc wrote: > > On 2016/03/18 23:26:40, aluebs-webrtc wrote: > > > Where does this function come from? Also, use render_config.num_frames() or > > even > > > the render_buffer.num_frames() instead of samples_per_channel. > > > > The function is declared in > > webrtc/modules/audio_processing/test/bitexactness_tools.h. > > Thanks for clarifying. I don't know why my grepping didn't find it originally. Done. https://codereview.webrtc.org/1814723003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1814723003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:96: ReadFloatSamplesFromStereoFile(render_buffer.num_frames(), On 2016/03/22 11:53:29, aluebs-webrtc wrote: > capture_buffer Good catch!!! Done.
https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:111: EXPECT_TRUE(test::BitExactFrame(render_config.num_frames(), On 2016/03/23 22:09:34, peah-webrtc wrote: > On 2016/03/22 11:53:29, aluebs-webrtc wrote: > > On 2016/03/21 11:54:49, peah-webrtc wrote: > > > On 2016/03/18 23:26:40, aluebs-webrtc wrote: > > > > If it receives a tolerance it is not a bit-exact test. I think this should > > be > > > > renamed to something that reflects the real test that is done. > > > > > > Good point! > > > What about VectorDifferenceBounded and that kTolerance is renamed to > > > kVectorItemBound? > > > I'll wait with that change until we have agreed on a name as that will > affect > > > other files as well. It should for sure be part of this CL but I don't want > to > > > create a patch for it until we have a good name. > > > > I think your suggestion sounds much more intuitive of what it actually does. > But > > if it touches many other files, it might be a better idea to do it in another > > CL. > Great! Then I'll do that in another CL. > Done. Acknowledged. https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:118: TEST(IntelligibilityEnhancerBitExactnessTest, Mono8kHz) { On 2016/03/23 22:09:34, peah-webrtc wrote: > On 2016/03/22 11:53:29, aluebs-webrtc wrote: > > On 2016/03/21 11:54:49, peah-webrtc wrote: > > > On 2016/03/18 23:26:41, aluebs-webrtc wrote: > > > > This test would be a lot simpler if we use TEST_P and > > INSTANTIATE_TEST_CASE_P. > > > > > > Very likely, but I could not come up with a good way to in an explicit > manner > > > combine the testvectors with the input parameters in > INSTANTIATE_TEST_CASE_P. > > Do > > > you have any suggestion on how to achieve that? > > > > How about having a method that returns a reference vector from a sample rate > and > > a number of channels? In that way you can only instantiate an the tests using > > these 2 parameters and then get the reference from them. > > As I wrote in the CL for the bitexactness test for the beamformer I think that > is a good suggestion and most likely it would produce more > compact code. One drawback is, however, that it is not possible to disable > tests. The way the tests are set up in this test is how the reviewers of the > first of the submodule bitexactness tests preferred it to be. And even though it > is a bit verbose, I like that it is very explicit. As I wrote in the other CL, TEST_P would be exactly as verbose, since you need to instantiate each test case manually, but it avoids repeated code. And to disable tests, you can always comment out that specific instance. Did the original reviewers evaluate TEST_P, because they might prefer this approach and then it would be great to port all tests to this one.
On 2016/03/24 11:18:40, aluebs-webrtc wrote: > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc > (right): > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:111: > EXPECT_TRUE(test::BitExactFrame(render_config.num_frames(), > On 2016/03/23 22:09:34, peah-webrtc wrote: > > On 2016/03/22 11:53:29, aluebs-webrtc wrote: > > > On 2016/03/21 11:54:49, peah-webrtc wrote: > > > > On 2016/03/18 23:26:40, aluebs-webrtc wrote: > > > > > If it receives a tolerance it is not a bit-exact test. I think this > should > > > be > > > > > renamed to something that reflects the real test that is done. > > > > > > > > Good point! > > > > What about VectorDifferenceBounded and that kTolerance is renamed to > > > > kVectorItemBound? > > > > I'll wait with that change until we have agreed on a name as that will > > affect > > > > other files as well. It should for sure be part of this CL but I don't > want > > to > > > > create a patch for it until we have a good name. > > > > > > I think your suggestion sounds much more intuitive of what it actually does. > > But > > > if it touches many other files, it might be a better idea to do it in > another > > > CL. > > Great! Then I'll do that in another CL. > > Done. > > Acknowledged. > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:118: > TEST(IntelligibilityEnhancerBitExactnessTest, Mono8kHz) { > On 2016/03/23 22:09:34, peah-webrtc wrote: > > On 2016/03/22 11:53:29, aluebs-webrtc wrote: > > > On 2016/03/21 11:54:49, peah-webrtc wrote: > > > > On 2016/03/18 23:26:41, aluebs-webrtc wrote: > > > > > This test would be a lot simpler if we use TEST_P and > > > INSTANTIATE_TEST_CASE_P. > > > > > > > > Very likely, but I could not come up with a good way to in an explicit > > manner > > > > combine the testvectors with the input parameters in > > INSTANTIATE_TEST_CASE_P. > > > Do > > > > you have any suggestion on how to achieve that? > > > > > > How about having a method that returns a reference vector from a sample rate > > and > > > a number of channels? In that way you can only instantiate an the tests > using > > > these 2 parameters and then get the reference from them. > > > > As I wrote in the CL for the bitexactness test for the beamformer I think that > > is a good suggestion and most likely it would produce more > > compact code. One drawback is, however, that it is not possible to disable > > tests. The way the tests are set up in this test is how the reviewers of the > > first of the submodule bitexactness tests preferred it to be. And even though > it > > is a bit verbose, I like that it is very explicit. > > As I wrote in the other CL, TEST_P would be exactly as verbose, since you need > to instantiate each test case manually, but it avoids repeated code. And to > disable tests, you can always comment out that specific instance. Did the > original reviewers evaluate TEST_P, because they might prefer this approach and > then it would be great to port all tests to this one. Yes, the initial version of the bitexactness test for the highpass filter (https://codereview.webrtc.org/1510493004) was using TEST_P and INSTANTIATE_TEST_CASE_P. The argument against that was that normal tests was easier to understand and maintain which I agree with. Furthermore, I think it is more explicit to disable test in the GTEST manner rather than commenting them out. I think that it makes sense to have the bitexactness tests for all the APM submodules done in the same way.
On 2016/03/24 11:30:49, peah-webrtc wrote: > On 2016/03/24 11:18:40, aluebs-webrtc wrote: > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > File webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc > > (right): > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:111: > > EXPECT_TRUE(test::BitExactFrame(render_config.num_frames(), > > On 2016/03/23 22:09:34, peah-webrtc wrote: > > > On 2016/03/22 11:53:29, aluebs-webrtc wrote: > > > > On 2016/03/21 11:54:49, peah-webrtc wrote: > > > > > On 2016/03/18 23:26:40, aluebs-webrtc wrote: > > > > > > If it receives a tolerance it is not a bit-exact test. I think this > > should > > > > be > > > > > > renamed to something that reflects the real test that is done. > > > > > > > > > > Good point! > > > > > What about VectorDifferenceBounded and that kTolerance is renamed to > > > > > kVectorItemBound? > > > > > I'll wait with that change until we have agreed on a name as that will > > > affect > > > > > other files as well. It should for sure be part of this CL but I don't > > want > > > to > > > > > create a patch for it until we have a good name. > > > > > > > > I think your suggestion sounds much more intuitive of what it actually > does. > > > But > > > > if it touches many other files, it might be a better idea to do it in > > another > > > > CL. > > > Great! Then I'll do that in another CL. > > > Done. > > > > Acknowledged. > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:118: > > TEST(IntelligibilityEnhancerBitExactnessTest, Mono8kHz) { > > On 2016/03/23 22:09:34, peah-webrtc wrote: > > > On 2016/03/22 11:53:29, aluebs-webrtc wrote: > > > > On 2016/03/21 11:54:49, peah-webrtc wrote: > > > > > On 2016/03/18 23:26:41, aluebs-webrtc wrote: > > > > > > This test would be a lot simpler if we use TEST_P and > > > > INSTANTIATE_TEST_CASE_P. > > > > > > > > > > Very likely, but I could not come up with a good way to in an explicit > > > manner > > > > > combine the testvectors with the input parameters in > > > INSTANTIATE_TEST_CASE_P. > > > > Do > > > > > you have any suggestion on how to achieve that? > > > > > > > > How about having a method that returns a reference vector from a sample > rate > > > and > > > > a number of channels? In that way you can only instantiate an the tests > > using > > > > these 2 parameters and then get the reference from them. > > > > > > As I wrote in the CL for the bitexactness test for the beamformer I think > that > > > is a good suggestion and most likely it would produce more > > > compact code. One drawback is, however, that it is not possible to disable > > > tests. The way the tests are set up in this test is how the reviewers of the > > > first of the submodule bitexactness tests preferred it to be. And even > though > > it > > > is a bit verbose, I like that it is very explicit. > > > > As I wrote in the other CL, TEST_P would be exactly as verbose, since you need > > to instantiate each test case manually, but it avoids repeated code. And to > > disable tests, you can always comment out that specific instance. Did the > > original reviewers evaluate TEST_P, because they might prefer this approach > and > > then it would be great to port all tests to this one. > Yes, the initial version of the bitexactness test for the highpass filter > (https://codereview.webrtc.org/1510493004) was using TEST_P and > INSTANTIATE_TEST_CASE_P. > The argument against that was that normal tests was easier to understand and > maintain which I agree with. Furthermore, I think it is more explicit to disable > test in the GTEST manner rather than commenting them out. I think that it makes > sense to have the bitexactness tests for all the APM submodules done in the same > way. I don't see any comments of the reviewers before the patch you removed the TEST_P, but I trust you had an offline discussion with them. I agree that all submodule-tests should be consistent, but I personally think that if there is a nice feature to make tests parametric, why not use it. If everybody agrees on not using TEST_P, I will not oppose to that. So it lgtm, given that all these tests are disabled before landing, since the IntelligibilityEnhancer is still under development and this test is more annoying than helpful right now. Also, let's discuss offline if we can change these bitexactness tests for something more useful and intuitive. Please add me as a reviewer to the rename of the bitexactness test.
On 2016/03/28 22:45:36, aluebs-webrtc wrote: > On 2016/03/24 11:30:49, peah-webrtc wrote: > > On 2016/03/24 11:18:40, aluebs-webrtc wrote: > > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > > File webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc > > > (right): > > > > > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:111: > > > EXPECT_TRUE(test::BitExactFrame(render_config.num_frames(), > > > On 2016/03/23 22:09:34, peah-webrtc wrote: > > > > On 2016/03/22 11:53:29, aluebs-webrtc wrote: > > > > > On 2016/03/21 11:54:49, peah-webrtc wrote: > > > > > > On 2016/03/18 23:26:40, aluebs-webrtc wrote: > > > > > > > If it receives a tolerance it is not a bit-exact test. I think this > > > should > > > > > be > > > > > > > renamed to something that reflects the real test that is done. > > > > > > > > > > > > Good point! > > > > > > What about VectorDifferenceBounded and that kTolerance is renamed to > > > > > > kVectorItemBound? > > > > > > I'll wait with that change until we have agreed on a name as that will > > > > affect > > > > > > other files as well. It should for sure be part of this CL but I don't > > > want > > > > to > > > > > > create a patch for it until we have a good name. > > > > > > > > > > I think your suggestion sounds much more intuitive of what it actually > > does. > > > > But > > > > > if it touches many other files, it might be a better idea to do it in > > > another > > > > > CL. > > > > Great! Then I'll do that in another CL. > > > > Done. > > > > > > Acknowledged. > > > > > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:118: > > > TEST(IntelligibilityEnhancerBitExactnessTest, Mono8kHz) { > > > On 2016/03/23 22:09:34, peah-webrtc wrote: > > > > On 2016/03/22 11:53:29, aluebs-webrtc wrote: > > > > > On 2016/03/21 11:54:49, peah-webrtc wrote: > > > > > > On 2016/03/18 23:26:41, aluebs-webrtc wrote: > > > > > > > This test would be a lot simpler if we use TEST_P and > > > > > INSTANTIATE_TEST_CASE_P. > > > > > > > > > > > > Very likely, but I could not come up with a good way to in an explicit > > > > manner > > > > > > combine the testvectors with the input parameters in > > > > INSTANTIATE_TEST_CASE_P. > > > > > Do > > > > > > you have any suggestion on how to achieve that? > > > > > > > > > > How about having a method that returns a reference vector from a sample > > rate > > > > and > > > > > a number of channels? In that way you can only instantiate an the tests > > > using > > > > > these 2 parameters and then get the reference from them. > > > > > > > > As I wrote in the CL for the bitexactness test for the beamformer I think > > that > > > > is a good suggestion and most likely it would produce more > > > > compact code. One drawback is, however, that it is not possible to disable > > > > tests. The way the tests are set up in this test is how the reviewers of > the > > > > first of the submodule bitexactness tests preferred it to be. And even > > though > > > it > > > > is a bit verbose, I like that it is very explicit. > > > > > > As I wrote in the other CL, TEST_P would be exactly as verbose, since you > need > > > to instantiate each test case manually, but it avoids repeated code. And to > > > disable tests, you can always comment out that specific instance. Did the > > > original reviewers evaluate TEST_P, because they might prefer this approach > > and > > > then it would be great to port all tests to this one. > > Yes, the initial version of the bitexactness test for the highpass filter > > (https://codereview.webrtc.org/1510493004) was using TEST_P and > > INSTANTIATE_TEST_CASE_P. > > The argument against that was that normal tests was easier to understand and > > maintain which I agree with. Furthermore, I think it is more explicit to > disable > > test in the GTEST manner rather than commenting them out. I think that it > makes > > sense to have the bitexactness tests for all the APM submodules done in the > same > > way. > > I don't see any comments of the reviewers before the patch you removed the > TEST_P, but I trust you had an offline discussion with them. I agree that all > submodule-tests should be consistent, but I personally think that if there is a > nice feature to make tests parametric, why not use it. If everybody agrees on > not using TEST_P, I will not oppose to that. > So it lgtm, given that all these tests are disabled before landing, since the > IntelligibilityEnhancer is still under development and this test is more > annoying than helpful right now. Also, let's discuss offline if we can change > these bitexactness tests for something more useful and intuitive. > Please add me as a reviewer to the rename of the bitexactness test. Thanks. The comment is there with the timestamp 2015-12-10 08:53:49 UTC. Regarding the rename, you mean the BitExactFrame function, right? I will add you to that CL as a reviewer.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, aluebs@webrtc.org Link to the patchset: https://codereview.webrtc.org/1814723003/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814723003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814723003/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Added a bitexactness test for the intelligibility enhancer in the audio processing module BUG=webrtc:5242 ========== to ========== Added a bitexactness test for the intelligibility enhancer in the audio processing module BUG=webrtc:5242 Committed: https://crrev.com/4b0c74172ed0a1bb40dc25535da53dee4c4767f2 Cr-Commit-Position: refs/heads/master@{#12129} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4b0c74172ed0a1bb40dc25535da53dee4c4767f2 Cr-Commit-Position: refs/heads/master@{#12129}
Message was sent while issue was closed.
On 2016/03/29 05:15:58, peah-webrtc wrote: > On 2016/03/28 22:45:36, aluebs-webrtc wrote: > > On 2016/03/24 11:30:49, peah-webrtc wrote: > > > On 2016/03/24 11:18:40, aluebs-webrtc wrote: > > > > > > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > > > File webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > > > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:111: > > > > EXPECT_TRUE(test::BitExactFrame(render_config.num_frames(), > > > > On 2016/03/23 22:09:34, peah-webrtc wrote: > > > > > On 2016/03/22 11:53:29, aluebs-webrtc wrote: > > > > > > On 2016/03/21 11:54:49, peah-webrtc wrote: > > > > > > > On 2016/03/18 23:26:40, aluebs-webrtc wrote: > > > > > > > > If it receives a tolerance it is not a bit-exact test. I think > this > > > > should > > > > > > be > > > > > > > > renamed to something that reflects the real test that is done. > > > > > > > > > > > > > > Good point! > > > > > > > What about VectorDifferenceBounded and that kTolerance is renamed to > > > > > > > kVectorItemBound? > > > > > > > I'll wait with that change until we have agreed on a name as that > will > > > > > affect > > > > > > > other files as well. It should for sure be part of this CL but I > don't > > > > want > > > > > to > > > > > > > create a patch for it until we have a good name. > > > > > > > > > > > > I think your suggestion sounds much more intuitive of what it actually > > > does. > > > > > But > > > > > > if it touches many other files, it might be a better idea to do it in > > > > another > > > > > > CL. > > > > > Great! Then I'll do that in another CL. > > > > > Done. > > > > > > > > Acknowledged. > > > > > > > > > > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > > > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:118: > > > > TEST(IntelligibilityEnhancerBitExactnessTest, Mono8kHz) { > > > > On 2016/03/23 22:09:34, peah-webrtc wrote: > > > > > On 2016/03/22 11:53:29, aluebs-webrtc wrote: > > > > > > On 2016/03/21 11:54:49, peah-webrtc wrote: > > > > > > > On 2016/03/18 23:26:41, aluebs-webrtc wrote: > > > > > > > > This test would be a lot simpler if we use TEST_P and > > > > > > INSTANTIATE_TEST_CASE_P. > > > > > > > > > > > > > > Very likely, but I could not come up with a good way to in an > explicit > > > > > manner > > > > > > > combine the testvectors with the input parameters in > > > > > INSTANTIATE_TEST_CASE_P. > > > > > > Do > > > > > > > you have any suggestion on how to achieve that? > > > > > > > > > > > > How about having a method that returns a reference vector from a > sample > > > rate > > > > > and > > > > > > a number of channels? In that way you can only instantiate an the > tests > > > > using > > > > > > these 2 parameters and then get the reference from them. > > > > > > > > > > As I wrote in the CL for the bitexactness test for the beamformer I > think > > > that > > > > > is a good suggestion and most likely it would produce more > > > > > compact code. One drawback is, however, that it is not possible to > disable > > > > > tests. The way the tests are set up in this test is how the reviewers of > > the > > > > > first of the submodule bitexactness tests preferred it to be. And even > > > though > > > > it > > > > > is a bit verbose, I like that it is very explicit. > > > > > > > > As I wrote in the other CL, TEST_P would be exactly as verbose, since you > > need > > > > to instantiate each test case manually, but it avoids repeated code. And > to > > > > disable tests, you can always comment out that specific instance. Did the > > > > original reviewers evaluate TEST_P, because they might prefer this > approach > > > and > > > > then it would be great to port all tests to this one. > > > Yes, the initial version of the bitexactness test for the highpass filter > > > (https://codereview.webrtc.org/1510493004) was using TEST_P and > > > INSTANTIATE_TEST_CASE_P. > > > The argument against that was that normal tests was easier to understand and > > > maintain which I agree with. Furthermore, I think it is more explicit to > > disable > > > test in the GTEST manner rather than commenting them out. I think that it > > makes > > > sense to have the bitexactness tests for all the APM submodules done in the > > same > > > way. > > > > I don't see any comments of the reviewers before the patch you removed the > > TEST_P, but I trust you had an offline discussion with them. I agree that all > > submodule-tests should be consistent, but I personally think that if there is > a > > nice feature to make tests parametric, why not use it. If everybody agrees on > > not using TEST_P, I will not oppose to that. > > So it lgtm, given that all these tests are disabled before landing, since the > > IntelligibilityEnhancer is still under development and this test is more > > annoying than helpful right now. Also, let's discuss offline if we can change > > these bitexactness tests for something more useful and intuitive. > > Please add me as a reviewer to the rename of the bitexactness test. > > Thanks. The comment is there with the timestamp 2015-12-10 08:53:49 UTC. > > Regarding the rename, you mean the BitExactFrame function, right? I will add you > to that CL as a reviewer. There I saw the comment, I was looking at the comments on patches themselves. Thank you for pointing that out. Yes, I do meant the test::BitExactFrame function, as we agreed.
Message was sent while issue was closed.
On 2016/03/29 15:01:33, aluebs-webrtc wrote: > On 2016/03/29 05:15:58, peah-webrtc wrote: > > On 2016/03/28 22:45:36, aluebs-webrtc wrote: > > > On 2016/03/24 11:30:49, peah-webrtc wrote: > > > > On 2016/03/24 11:18:40, aluebs-webrtc wrote: > > > > > > > > > > > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > > > > File > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > > > > > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:111: > > > > > EXPECT_TRUE(test::BitExactFrame(render_config.num_frames(), > > > > > On 2016/03/23 22:09:34, peah-webrtc wrote: > > > > > > On 2016/03/22 11:53:29, aluebs-webrtc wrote: > > > > > > > On 2016/03/21 11:54:49, peah-webrtc wrote: > > > > > > > > On 2016/03/18 23:26:40, aluebs-webrtc wrote: > > > > > > > > > If it receives a tolerance it is not a bit-exact test. I think > > this > > > > > should > > > > > > > be > > > > > > > > > renamed to something that reflects the real test that is done. > > > > > > > > > > > > > > > > Good point! > > > > > > > > What about VectorDifferenceBounded and that kTolerance is renamed > to > > > > > > > > kVectorItemBound? > > > > > > > > I'll wait with that change until we have agreed on a name as that > > will > > > > > > affect > > > > > > > > other files as well. It should for sure be part of this CL but I > > don't > > > > > want > > > > > > to > > > > > > > > create a patch for it until we have a good name. > > > > > > > > > > > > > > I think your suggestion sounds much more intuitive of what it > actually > > > > does. > > > > > > But > > > > > > > if it touches many other files, it might be a better idea to do it > in > > > > > another > > > > > > > CL. > > > > > > Great! Then I'll do that in another CL. > > > > > > Done. > > > > > > > > > > Acknowledged. > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > > > > > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:118: > > > > > TEST(IntelligibilityEnhancerBitExactnessTest, Mono8kHz) { > > > > > On 2016/03/23 22:09:34, peah-webrtc wrote: > > > > > > On 2016/03/22 11:53:29, aluebs-webrtc wrote: > > > > > > > On 2016/03/21 11:54:49, peah-webrtc wrote: > > > > > > > > On 2016/03/18 23:26:41, aluebs-webrtc wrote: > > > > > > > > > This test would be a lot simpler if we use TEST_P and > > > > > > > INSTANTIATE_TEST_CASE_P. > > > > > > > > > > > > > > > > Very likely, but I could not come up with a good way to in an > > explicit > > > > > > manner > > > > > > > > combine the testvectors with the input parameters in > > > > > > INSTANTIATE_TEST_CASE_P. > > > > > > > Do > > > > > > > > you have any suggestion on how to achieve that? > > > > > > > > > > > > > > How about having a method that returns a reference vector from a > > sample > > > > rate > > > > > > and > > > > > > > a number of channels? In that way you can only instantiate an the > > tests > > > > > using > > > > > > > these 2 parameters and then get the reference from them. > > > > > > > > > > > > As I wrote in the CL for the bitexactness test for the beamformer I > > think > > > > that > > > > > > is a good suggestion and most likely it would produce more > > > > > > compact code. One drawback is, however, that it is not possible to > > disable > > > > > > tests. The way the tests are set up in this test is how the reviewers > of > > > the > > > > > > first of the submodule bitexactness tests preferred it to be. And even > > > > though > > > > > it > > > > > > is a bit verbose, I like that it is very explicit. > > > > > > > > > > As I wrote in the other CL, TEST_P would be exactly as verbose, since > you > > > need > > > > > to instantiate each test case manually, but it avoids repeated code. And > > to > > > > > disable tests, you can always comment out that specific instance. Did > the > > > > > original reviewers evaluate TEST_P, because they might prefer this > > approach > > > > and > > > > > then it would be great to port all tests to this one. > > > > Yes, the initial version of the bitexactness test for the highpass filter > > > > (https://codereview.webrtc.org/1510493004) was using TEST_P and > > > > INSTANTIATE_TEST_CASE_P. > > > > The argument against that was that normal tests was easier to understand > and > > > > maintain which I agree with. Furthermore, I think it is more explicit to > > > disable > > > > test in the GTEST manner rather than commenting them out. I think that it > > > makes > > > > sense to have the bitexactness tests for all the APM submodules done in > the > > > same > > > > way. > > > > > > I don't see any comments of the reviewers before the patch you removed the > > > TEST_P, but I trust you had an offline discussion with them. I agree that > all > > > submodule-tests should be consistent, but I personally think that if there > is > > a > > > nice feature to make tests parametric, why not use it. If everybody agrees > on > > > not using TEST_P, I will not oppose to that. > > > So it lgtm, given that all these tests are disabled before landing, since > the > > > IntelligibilityEnhancer is still under development and this test is more > > > annoying than helpful right now. Also, let's discuss offline if we can > change > > > these bitexactness tests for something more useful and intuitive. > > > Please add me as a reviewer to the rename of the bitexactness test. > > > > Thanks. The comment is there with the timestamp 2015-12-10 08:53:49 UTC. > > > > Regarding the rename, you mean the BitExactFrame function, right? I will add > you > > to that CL as a reviewer. > > There I saw the comment, I was looking at the comments on patches themselves. > Thank you for pointing that out. > Yes, I do meant the test::BitExactFrame function, as we agreed. Just sharing my 2 cents on the TEST_P discussion, after the fact. I think TEST_Ps are nice since they render less code duplication. But, the test cases get weird (automatically generated) names, which make them unintuitive to filter. I tend to not use TEST_P for the latter reason.
Message was sent while issue was closed.
On 2016/03/29 19:58:45, hlundin-webrtc wrote: > On 2016/03/29 15:01:33, aluebs-webrtc wrote: > > On 2016/03/29 05:15:58, peah-webrtc wrote: > > > On 2016/03/28 22:45:36, aluebs-webrtc wrote: > > > > On 2016/03/24 11:30:49, peah-webrtc wrote: > > > > > On 2016/03/24 11:18:40, aluebs-webrtc wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > > > > > File > > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > > > > > > > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:111: > > > > > > EXPECT_TRUE(test::BitExactFrame(render_config.num_frames(), > > > > > > On 2016/03/23 22:09:34, peah-webrtc wrote: > > > > > > > On 2016/03/22 11:53:29, aluebs-webrtc wrote: > > > > > > > > On 2016/03/21 11:54:49, peah-webrtc wrote: > > > > > > > > > On 2016/03/18 23:26:40, aluebs-webrtc wrote: > > > > > > > > > > If it receives a tolerance it is not a bit-exact test. I think > > > this > > > > > > should > > > > > > > > be > > > > > > > > > > renamed to something that reflects the real test that is done. > > > > > > > > > > > > > > > > > > Good point! > > > > > > > > > What about VectorDifferenceBounded and that kTolerance is > renamed > > to > > > > > > > > > kVectorItemBound? > > > > > > > > > I'll wait with that change until we have agreed on a name as > that > > > will > > > > > > > affect > > > > > > > > > other files as well. It should for sure be part of this CL but I > > > don't > > > > > > want > > > > > > > to > > > > > > > > > create a patch for it until we have a good name. > > > > > > > > > > > > > > > > I think your suggestion sounds much more intuitive of what it > > actually > > > > > does. > > > > > > > But > > > > > > > > if it touches many other files, it might be a better idea to do it > > in > > > > > > another > > > > > > > > CL. > > > > > > > Great! Then I'll do that in another CL. > > > > > > > Done. > > > > > > > > > > > > Acknowledged. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > > > > > > > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:118: > > > > > > TEST(IntelligibilityEnhancerBitExactnessTest, Mono8kHz) { > > > > > > On 2016/03/23 22:09:34, peah-webrtc wrote: > > > > > > > On 2016/03/22 11:53:29, aluebs-webrtc wrote: > > > > > > > > On 2016/03/21 11:54:49, peah-webrtc wrote: > > > > > > > > > On 2016/03/18 23:26:41, aluebs-webrtc wrote: > > > > > > > > > > This test would be a lot simpler if we use TEST_P and > > > > > > > > INSTANTIATE_TEST_CASE_P. > > > > > > > > > > > > > > > > > > Very likely, but I could not come up with a good way to in an > > > explicit > > > > > > > manner > > > > > > > > > combine the testvectors with the input parameters in > > > > > > > INSTANTIATE_TEST_CASE_P. > > > > > > > > Do > > > > > > > > > you have any suggestion on how to achieve that? > > > > > > > > > > > > > > > > How about having a method that returns a reference vector from a > > > sample > > > > > rate > > > > > > > and > > > > > > > > a number of channels? In that way you can only instantiate an the > > > tests > > > > > > using > > > > > > > > these 2 parameters and then get the reference from them. > > > > > > > > > > > > > > As I wrote in the CL for the bitexactness test for the beamformer I > > > think > > > > > that > > > > > > > is a good suggestion and most likely it would produce more > > > > > > > compact code. One drawback is, however, that it is not possible to > > > disable > > > > > > > tests. The way the tests are set up in this test is how the > reviewers > > of > > > > the > > > > > > > first of the submodule bitexactness tests preferred it to be. And > even > > > > > though > > > > > > it > > > > > > > is a bit verbose, I like that it is very explicit. > > > > > > > > > > > > As I wrote in the other CL, TEST_P would be exactly as verbose, since > > you > > > > need > > > > > > to instantiate each test case manually, but it avoids repeated code. > And > > > to > > > > > > disable tests, you can always comment out that specific instance. Did > > the > > > > > > original reviewers evaluate TEST_P, because they might prefer this > > > approach > > > > > and > > > > > > then it would be great to port all tests to this one. > > > > > Yes, the initial version of the bitexactness test for the highpass > filter > > > > > (https://codereview.webrtc.org/1510493004) was using TEST_P and > > > > > INSTANTIATE_TEST_CASE_P. > > > > > The argument against that was that normal tests was easier to understand > > and > > > > > maintain which I agree with. Furthermore, I think it is more explicit to > > > > disable > > > > > test in the GTEST manner rather than commenting them out. I think that > it > > > > makes > > > > > sense to have the bitexactness tests for all the APM submodules done in > > the > > > > same > > > > > way. > > > > > > > > I don't see any comments of the reviewers before the patch you removed the > > > > TEST_P, but I trust you had an offline discussion with them. I agree that > > all > > > > submodule-tests should be consistent, but I personally think that if there > > is > > > a > > > > nice feature to make tests parametric, why not use it. If everybody agrees > > on > > > > not using TEST_P, I will not oppose to that. > > > > So it lgtm, given that all these tests are disabled before landing, since > > the > > > > IntelligibilityEnhancer is still under development and this test is more > > > > annoying than helpful right now. Also, let's discuss offline if we can > > change > > > > these bitexactness tests for something more useful and intuitive. > > > > Please add me as a reviewer to the rename of the bitexactness test. > > > > > > Thanks. The comment is there with the timestamp 2015-12-10 08:53:49 UTC. > > > > > > Regarding the rename, you mean the BitExactFrame function, right? I will add > > you > > > to that CL as a reviewer. > > > > There I saw the comment, I was looking at the comments on patches themselves. > > Thank you for pointing that out. > > Yes, I do meant the test::BitExactFrame function, as we agreed. > > Just sharing my 2 cents on the TEST_P discussion, after the fact. I think > TEST_Ps are nice since they render less code duplication. But, the test cases > get weird (automatically generated) names, which make them unintuitive to > filter. I tend to not use TEST_P for the latter reason. Duplication isn't necessarily the same thing as complexity. If you want parametric tests, just make a little function and call that from test cases you define. There will be very little duplicated code (a few lines per test case) and is easy for anyone to read, with no requirement to be familiar with intricate macros and templates of the test framework. You can decide on sensible names too, and don't need to jump through any hoops to create the parameter sets.
Message was sent while issue was closed.
On 2016/03/30 08:00:34, solenberg wrote: > On 2016/03/29 19:58:45, hlundin-webrtc wrote: > > On 2016/03/29 15:01:33, aluebs-webrtc wrote: > > > On 2016/03/29 05:15:58, peah-webrtc wrote: > > > > On 2016/03/28 22:45:36, aluebs-webrtc wrote: > > > > > On 2016/03/24 11:30:49, peah-webrtc wrote: > > > > > > On 2016/03/24 11:18:40, aluebs-webrtc wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > > > > > > File > > > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > > > > > > > > > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:111: > > > > > > > EXPECT_TRUE(test::BitExactFrame(render_config.num_frames(), > > > > > > > On 2016/03/23 22:09:34, peah-webrtc wrote: > > > > > > > > On 2016/03/22 11:53:29, aluebs-webrtc wrote: > > > > > > > > > On 2016/03/21 11:54:49, peah-webrtc wrote: > > > > > > > > > > On 2016/03/18 23:26:40, aluebs-webrtc wrote: > > > > > > > > > > > If it receives a tolerance it is not a bit-exact test. I > think > > > > this > > > > > > > should > > > > > > > > > be > > > > > > > > > > > renamed to something that reflects the real test that is > done. > > > > > > > > > > > > > > > > > > > > Good point! > > > > > > > > > > What about VectorDifferenceBounded and that kTolerance is > > renamed > > > to > > > > > > > > > > kVectorItemBound? > > > > > > > > > > I'll wait with that change until we have agreed on a name as > > that > > > > will > > > > > > > > affect > > > > > > > > > > other files as well. It should for sure be part of this CL but > I > > > > don't > > > > > > > want > > > > > > > > to > > > > > > > > > > create a patch for it until we have a good name. > > > > > > > > > > > > > > > > > > I think your suggestion sounds much more intuitive of what it > > > actually > > > > > > does. > > > > > > > > But > > > > > > > > > if it touches many other files, it might be a better idea to do > it > > > in > > > > > > > another > > > > > > > > > CL. > > > > > > > > Great! Then I'll do that in another CL. > > > > > > > > Done. > > > > > > > > > > > > > > Acknowledged. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/1814723003/diff/20001/webrtc/modules/audio_proc... > > > > > > > > > > webrtc/modules/audio_processing/intelligibility_enhancer_unittest.cc:118: > > > > > > > TEST(IntelligibilityEnhancerBitExactnessTest, Mono8kHz) { > > > > > > > On 2016/03/23 22:09:34, peah-webrtc wrote: > > > > > > > > On 2016/03/22 11:53:29, aluebs-webrtc wrote: > > > > > > > > > On 2016/03/21 11:54:49, peah-webrtc wrote: > > > > > > > > > > On 2016/03/18 23:26:41, aluebs-webrtc wrote: > > > > > > > > > > > This test would be a lot simpler if we use TEST_P and > > > > > > > > > INSTANTIATE_TEST_CASE_P. > > > > > > > > > > > > > > > > > > > > Very likely, but I could not come up with a good way to in an > > > > explicit > > > > > > > > manner > > > > > > > > > > combine the testvectors with the input parameters in > > > > > > > > INSTANTIATE_TEST_CASE_P. > > > > > > > > > Do > > > > > > > > > > you have any suggestion on how to achieve that? > > > > > > > > > > > > > > > > > > How about having a method that returns a reference vector from a > > > > sample > > > > > > rate > > > > > > > > and > > > > > > > > > a number of channels? In that way you can only instantiate an > the > > > > tests > > > > > > > using > > > > > > > > > these 2 parameters and then get the reference from them. > > > > > > > > > > > > > > > > As I wrote in the CL for the bitexactness test for the beamformer > I > > > > think > > > > > > that > > > > > > > > is a good suggestion and most likely it would produce more > > > > > > > > compact code. One drawback is, however, that it is not possible to > > > > disable > > > > > > > > tests. The way the tests are set up in this test is how the > > reviewers > > > of > > > > > the > > > > > > > > first of the submodule bitexactness tests preferred it to be. And > > even > > > > > > though > > > > > > > it > > > > > > > > is a bit verbose, I like that it is very explicit. > > > > > > > > > > > > > > As I wrote in the other CL, TEST_P would be exactly as verbose, > since > > > you > > > > > need > > > > > > > to instantiate each test case manually, but it avoids repeated code. > > And > > > > to > > > > > > > disable tests, you can always comment out that specific instance. > Did > > > the > > > > > > > original reviewers evaluate TEST_P, because they might prefer this > > > > approach > > > > > > and > > > > > > > then it would be great to port all tests to this one. > > > > > > Yes, the initial version of the bitexactness test for the highpass > > filter > > > > > > (https://codereview.webrtc.org/1510493004) was using TEST_P and > > > > > > INSTANTIATE_TEST_CASE_P. > > > > > > The argument against that was that normal tests was easier to > understand > > > and > > > > > > maintain which I agree with. Furthermore, I think it is more explicit > to > > > > > disable > > > > > > test in the GTEST manner rather than commenting them out. I think that > > it > > > > > makes > > > > > > sense to have the bitexactness tests for all the APM submodules done > in > > > the > > > > > same > > > > > > way. > > > > > > > > > > I don't see any comments of the reviewers before the patch you removed > the > > > > > TEST_P, but I trust you had an offline discussion with them. I agree > that > > > all > > > > > submodule-tests should be consistent, but I personally think that if > there > > > is > > > > a > > > > > nice feature to make tests parametric, why not use it. If everybody > agrees > > > on > > > > > not using TEST_P, I will not oppose to that. > > > > > So it lgtm, given that all these tests are disabled before landing, > since > > > the > > > > > IntelligibilityEnhancer is still under development and this test is > more > > > > > annoying than helpful right now. Also, let's discuss offline if we can > > > change > > > > > these bitexactness tests for something more useful and intuitive. > > > > > Please add me as a reviewer to the rename of the bitexactness test. > > > > > > > > Thanks. The comment is there with the timestamp 2015-12-10 08:53:49 UTC. > > > > > > > > Regarding the rename, you mean the BitExactFrame function, right? I will > add > > > you > > > > to that CL as a reviewer. > > > > > > There I saw the comment, I was looking at the comments on patches > themselves. > > > Thank you for pointing that out. > > > Yes, I do meant the test::BitExactFrame function, as we agreed. > > > > Just sharing my 2 cents on the TEST_P discussion, after the fact. I think > > TEST_Ps are nice since they render less code duplication. But, the test cases > > get weird (automatically generated) names, which make them unintuitive to > > filter. I tend to not use TEST_P for the latter reason. > > Duplication isn't necessarily the same thing as complexity. > > If you want parametric tests, just make a little function and call that from > test cases you define. There will be very little duplicated code (a few lines > per test case) and is easy for anyone to read, with no requirement to be > familiar with intricate macros and templates of the test framework. You can > decide on sensible names too, and don't need to jump through any hoops to create > the parameter sets. I think creating the parameters set is fairly simple and I find the TEST_P to be more intuitive to read than manual tests cases, where it is hard to find the case you are looking for. And I understand the naming issue, but I think it is not so terrible and could be annotated with a comment. But I still think TEST_P is a great tool to use, specially to avoid repeating cases or forgetting to change the parameters from one case to the other because of copying and pasting the code (that would have happened with the 3-mic-geometry case if I wouldn't have caught that on reviewing). In general, I find that having all cases added manually is hard to review and navigate and bug-prone in general. But as I said, if everybody agrees on not using TEST_P, I will not oppose to it. |
