Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1806853004:
Added a bitexactness test for the beamformer 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, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@AgcBitExactness_CL Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded a bitexactness test for the beamformer in the audio processing module
BUG=webrtc:5341
Committed: https://crrev.com/ceef04613b1bea79a0b650db9e5ca5d8e153adbc
Cr-Commit-Position: refs/heads/master@{#12137}
Patch Set 1 : #
Total comments: 118
Patch Set 2 : Changes in response to reviewer comments #
Total comments: 8
Patch Set 3 : Moved the bitexactness unittest code to the proper file #Patch Set 4 : Changes in response to reviewer comments #Patch Set 5 : Reenabled a test #Patch Set 6 : Rebase #Patch Set 7 : Disabled all the bitexactness tests for the beamformer due to problems with division by zero in the… #Messages
Total messages: 29 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
peah@webrtc.org changed reviewers: + aluebs@webrtc.org, hlundin@google.com
Hi, I've been setting up bitexactness tests for all the APM submodules. This test is for the beamformer. 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/beamformer. Furthermore, most likely the filename should also be changed to something in line with nonlinear_beamformer*
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org - hlundin@google.com
Correcting reviewer email address.
lgtm
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. 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 beamformer see that it suppresses noise from all directions except the desired one, 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/1806853004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer_unittest.cc (right): https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:22: const int kNumFramesToProcess = 1000; Should probably be a size_t. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:24: 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/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:31: beamformer->ProcessChunk(*capture_audio_buffer->split_data_f(), After running the beamformer, the number of channels should be set to mono: capture_audio_buffer->set_num_channels(1); https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:42: 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/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:44: beamformer.Initialize(10, sample_rate_hz > 16000 ? 16000 : sample_rate_hz); Use AudioProcessing::kChunkSizeMs instead of 10, or define your own constant. Similarly with AudioProcessing::kSampleRate16kHz. Also, I would probably make the split_rate calculation in a separate function with a clear name. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:46: int samples_per_channel = rtc::CheckedDivExact(sample_rate_hz, 100); You don't need to calculate this explicitly, you can use capture_config.num_frames() or even the capture_config.num_frames(). https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:47: const StreamConfig capture_config(sample_rate_hz, 2, false); Use array_geometry.size() instead of hardcoding 2. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:50: capture_config.num_frames(), capture_config.num_channels(), num_process_channels should probably be mono, when running the beamformer. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:54: std::vector<float> capture_input(samples_per_channel * 2); Use capture_config.num_channels() or capture_buffer.num_channels() instead of 2, and capture_config.num_frames() or capture_config.num_frames() instead of samples_pre_channel. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:55: for (size_t frame_no = 0; frame_no < kNumFramesToProcess; ++frame_no) { 0u? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:56: ReadFloatSamplesFromStereoFile(samples_per_channel, 2, &capture_file, Where does this function come from? Also use capture_config.num_channels() or capture_buffer.num_channels() instead of 2, and capture_config.num_frames() or capture_config.num_frames() instead of samples_pre_channel. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:70: 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/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:77: EXPECT_TRUE(test::BitExactFrame( 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/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:84: switch (variant) { Maybe have a variant with more than 2 points? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:90: Remove blank line? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:93: Remove blank line? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:96: Remove blank line? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:111: 2.f); The radius is completely ignored, but it would be interesting to add some non-broadside direction. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:115: // TODO(peah): Investigate why the nonlinear_beamformer.cc causes a DCHECK in I think this should be resolved before landing this test. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:117: TEST(BeamformerBitExactnessTest, This test would be a lot simpler if we use TEST_P and INSTANTIATE_TEST_CASE_P. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:122: std::vector<Point> array_geometry = CreateArrayGeometry(1); Do we need this intermediate variable? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:124: RunBitExactnessTest(8000, array_geometry, TargetDirection1, kOutputReference); Use AudioProcessing::kSampleRate8kHz or define your own constant. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:131: std::vector<Point> array_geometry = CreateArrayGeometry(1); Do we need this intermediate variable? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:133: RunBitExactnessTest(16000, array_geometry, TargetDirection1, Use AudioProcessing::kSampleRate16kHz or define your own constant. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:141: std::vector<Point> array_geometry = CreateArrayGeometry(1); Do we need this intermediate variable? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:143: RunBitExactnessTest(32000, array_geometry, TargetDirection1, Use AudioProcessing::kSampleRate32kHz or define your own constant. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:151: std::vector<Point> array_geometry = CreateArrayGeometry(1); Do we need this intermediate variable? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:153: RunBitExactnessTest(48000, array_geometry, TargetDirection1, Use AudioProcessing::kSampleRate48kHz or define your own constant. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:162: std::vector<Point> array_geometry = CreateArrayGeometry(1); Do we need this intermediate variable? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:164: RunBitExactnessTest(8000, array_geometry, TargetDirection2, kOutputReference); Use AudioProcessing::kSampleRate8kHz or define your own constant. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:171: std::vector<Point> array_geometry = CreateArrayGeometry(1); Do we need this intermediate variable? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:173: RunBitExactnessTest(16000, array_geometry, TargetDirection2, Use AudioProcessing::kSampleRate16kHz or define your own constant. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:181: std::vector<Point> array_geometry = CreateArrayGeometry(1); Do we need this intermediate variable? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:183: RunBitExactnessTest(32000, array_geometry, TargetDirection2, Use AudioProcessing::kSampleRate32kHz or define your own constant. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:191: std::vector<Point> array_geometry = CreateArrayGeometry(1); Do we need this intermediate variable? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:193: RunBitExactnessTest(48000, array_geometry, TargetDirection2, Use AudioProcessing::kSampleRate48kHz or define your own constant. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:201: std::vector<Point> array_geometry = CreateArrayGeometry(2); Do we need this intermediate variable? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:203: RunBitExactnessTest(8000, array_geometry, TargetDirection2, kOutputReference); Use AudioProcessing::kSampleRate8kHz or define your own constant. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:210: std::vector<Point> array_geometry = CreateArrayGeometry(2); Do we need this intermediate variable? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:212: RunBitExactnessTest(16000, array_geometry, TargetDirection2, Use AudioProcessing::kSampleRate16kHz or define your own constant. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:220: std::vector<Point> array_geometry = CreateArrayGeometry(2); Do we need this intermediate variable? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:222: RunBitExactnessTest(32000, array_geometry, TargetDirection2, Use AudioProcessing::kSampleRate32kHz or define your own constant. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:230: std::vector<Point> array_geometry = CreateArrayGeometry(2); Do we need this intermediate variable? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:232: RunBitExactnessTest(48000, array_geometry, TargetDirection2, Use AudioProcessing::kSampleRate48kHz or define your own constant. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:236: // TODO(peah): Investigate why the nonlinear_beamformer.cc causes a DCHECK in I think this should be resolved before landing this test. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:243: std::vector<Point> array_geometry = CreateArrayGeometry(3); Do we need this intermediate variable? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/modules.gyp File webrtc/modules/modules.gyp (right): https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/modules.gy... webrtc/modules/modules.gyp:444: 'audio_processing/beamformer_unittest.cc', Shouldn't this unittest be under the beamformer/ folder? Also, shouldn't it be named nonlinear_beamformer_unittest, since it only test the nonlinear_beamformer? On top of that, shouldn't it then be combined with the unittest?
I partly agree about the usefulness. The best thing would definitely be to have a test that tests the behavior of the beamformer. That is definitely partly doable but I think it is very hard to objectively capture all aspects of the beamformer output quality in a unit test. Typically nonbitexact changes anyway requires manual listening to fully verify. I'm definitely in favor of test that verifies the behavior of the beamformer but I think that bitexactness tests is a good complement to such. Bitexactness test are really good to catch code changes done by mistake, changes in module behavior due to code in lower-level functions and to combat bit-rot. The drawback with the bitexactness tests is, as you say, that they require manual updating for each nonbitexact change. But I think that is an acceptable price to pay for ensure intact code quality. Furthermore, once the code have stabilized, there are in my experience not that many trivial nonbitexact code changes that do not anyway require extensive testing. And if you do lots of nonbitexact changes due to extensive upgrading of the beamformer it would probably make sense to temporarily disable the tests during that process.
https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer_unittest.cc (right): https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:22: const int kNumFramesToProcess = 1000; On 2016/03/18 22:51:51, aluebs-webrtc wrote: > Should probably be a size_t. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:24: void ProcessOneFrame(int sample_rate_hz, On 2016/03/18 22:51:52, 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/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:31: beamformer->ProcessChunk(*capture_audio_buffer->split_data_f(), On 2016/03/18 22:51:50, aluebs-webrtc wrote: > After running the beamformer, the number of channels should be set to mono: > capture_audio_buffer->set_num_channels(1); Thanks! I saw that from the AudioProcessingImpl but it did not really make sense to me. Why is that done? There is no comment about that. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:42: const rtc::ArrayView<const float>& output_reference) { On 2016/03/18 22:51:52, 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/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:44: beamformer.Initialize(10, sample_rate_hz > 16000 ? 16000 : sample_rate_hz); On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Use AudioProcessing::kChunkSizeMs instead of 10, or define your own constant. > Similarly with AudioProcessing::kSampleRate16kHz. Also, I would probably make > the split_rate calculation in a separate function with a clear name. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:46: int samples_per_channel = rtc::CheckedDivExact(sample_rate_hz, 100); On 2016/03/18 22:51:52, aluebs-webrtc wrote: > You don't need to calculate this explicitly, you can use > capture_config.num_frames() or even the capture_config.num_frames(). Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:47: const StreamConfig capture_config(sample_rate_hz, 2, false); On 2016/03/18 22:51:51, aluebs-webrtc wrote: > Use array_geometry.size() instead of hardcoding 2. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:50: capture_config.num_frames(), capture_config.num_channels(), On 2016/03/18 22:51:51, aluebs-webrtc wrote: > num_process_channels should probably be mono, when running the beamformer. No, that does not work, as the CopyFrom then produces a mono audio buffer which (correctly) causes an assert in the beamformer. I don't fully understand the automated conversion in AudioBuffer but my guess is that if num_input_channels == 2 and num_process_channels == 1 AudioBuffer will automatically downmix the data before providing it to the user. I think this is the reason why you explicitly need to set capture_audio_buffer->set_num_channels(1); after running the beamformer (right?). https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:54: std::vector<float> capture_input(samples_per_channel * 2); On 2016/03/18 22:51:51, aluebs-webrtc wrote: > Use capture_config.num_channels() or capture_buffer.num_channels() instead of 2, > and capture_config.num_frames() or capture_config.num_frames() instead of > samples_pre_channel. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:55: for (size_t frame_no = 0; frame_no < kNumFramesToProcess; ++frame_no) { On 2016/03/18 22:51:51, aluebs-webrtc wrote: > 0u? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:56: ReadFloatSamplesFromStereoFile(samples_per_channel, 2, &capture_file, On 2016/03/18 22:51:51, aluebs-webrtc wrote: > Where does this function come from? Also use capture_config.num_channels() or > capture_buffer.num_channels() instead of 2, and capture_config.num_frames() or > capture_config.num_frames() instead of > samples_pre_channel. The function is declared in webrtc/modules/audio_processing/test/bitexactness_tools.h. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:70: const float kTolerance = 1.0f / 32768.0f; On 2016/03/18 22:51:51, 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. Good point! Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:77: EXPECT_TRUE(test::BitExactFrame( On 2016/03/18 22:51:50, 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/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:84: switch (variant) { On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Maybe have a variant with more than 2 points? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:90: On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Remove blank line? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:93: On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Remove blank line? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:96: On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Remove blank line? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:111: 2.f); On 2016/03/18 22:51:51, aluebs-webrtc wrote: > The radius is completely ignored, but it would be interesting to add some > non-broadside direction. Absolutely, what about the new one? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:115: // TODO(peah): Investigate why the nonlinear_beamformer.cc causes a DCHECK in On 2016/03/18 22:51:52, aluebs-webrtc wrote: > I think this should be resolved before landing this test. I don't agree and I cannot see why that would be desired. The test will anyway provide test coverage for the rest of the code and I see no value at all for delaying adding that test coverage. I was considering removing that test case and create a bug instead, but it is quite nice to have a test-case ready for bug recreation and as an upcoming regression test. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:117: TEST(BeamformerBitExactnessTest, On 2016/03/18 22:51:52, 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/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:122: std::vector<Point> array_geometry = CreateArrayGeometry(1); On 2016/03/18 22:51:51, aluebs-webrtc wrote: > Do we need this intermediate variable? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:124: RunBitExactnessTest(8000, array_geometry, TargetDirection1, kOutputReference); On 2016/03/18 22:51:51, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate8kHz or define your own constant. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:131: std::vector<Point> array_geometry = CreateArrayGeometry(1); On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Do we need this intermediate variable? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:133: RunBitExactnessTest(16000, array_geometry, TargetDirection1, On 2016/03/18 22:51:51, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate16kHz or define your own constant. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:141: std::vector<Point> array_geometry = CreateArrayGeometry(1); On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Do we need this intermediate variable? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:143: RunBitExactnessTest(32000, array_geometry, TargetDirection1, On 2016/03/18 22:51:51, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate32kHz or define your own constant. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:151: std::vector<Point> array_geometry = CreateArrayGeometry(1); On 2016/03/18 22:51:51, aluebs-webrtc wrote: > Do we need this intermediate variable? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:153: RunBitExactnessTest(48000, array_geometry, TargetDirection1, On 2016/03/18 22:51:51, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate48kHz or define your own constant. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:162: std::vector<Point> array_geometry = CreateArrayGeometry(1); On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Do we need this intermediate variable? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:164: RunBitExactnessTest(8000, array_geometry, TargetDirection2, kOutputReference); On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate8kHz or define your own constant. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:171: std::vector<Point> array_geometry = CreateArrayGeometry(1); On 2016/03/18 22:51:51, aluebs-webrtc wrote: > Do we need this intermediate variable? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:173: RunBitExactnessTest(16000, array_geometry, TargetDirection2, On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate16kHz or define your own constant. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:181: std::vector<Point> array_geometry = CreateArrayGeometry(1); On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Do we need this intermediate variable? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:183: RunBitExactnessTest(32000, array_geometry, TargetDirection2, On 2016/03/18 22:51:51, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate32kHz or define your own constant. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:191: std::vector<Point> array_geometry = CreateArrayGeometry(1); On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Do we need this intermediate variable? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:193: RunBitExactnessTest(48000, array_geometry, TargetDirection2, On 2016/03/18 22:51:50, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate48kHz or define your own constant. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:201: std::vector<Point> array_geometry = CreateArrayGeometry(2); On 2016/03/18 22:51:51, aluebs-webrtc wrote: > Do we need this intermediate variable? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:203: RunBitExactnessTest(8000, array_geometry, TargetDirection2, kOutputReference); On 2016/03/18 22:51:51, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate8kHz or define your own constant. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:210: std::vector<Point> array_geometry = CreateArrayGeometry(2); On 2016/03/18 22:51:50, aluebs-webrtc wrote: > Do we need this intermediate variable? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:212: RunBitExactnessTest(16000, array_geometry, TargetDirection2, On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate16kHz or define your own constant. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:220: std::vector<Point> array_geometry = CreateArrayGeometry(2); On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Do we need this intermediate variable? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:222: RunBitExactnessTest(32000, array_geometry, TargetDirection2, On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate32kHz or define your own constant. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:230: std::vector<Point> array_geometry = CreateArrayGeometry(2); On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Do we need this intermediate variable? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:232: RunBitExactnessTest(48000, array_geometry, TargetDirection2, On 2016/03/18 22:51:51, aluebs-webrtc wrote: > Use AudioProcessing::kSampleRate48kHz or define your own constant. Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:236: // TODO(peah): Investigate why the nonlinear_beamformer.cc causes a DCHECK in On 2016/03/18 22:51:51, aluebs-webrtc wrote: > I think this should be resolved before landing this test. As for the corresponding above comment, I don't agree and I don't see the reason for waiting with landing the test due to that. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:243: std::vector<Point> array_geometry = CreateArrayGeometry(3); On 2016/03/18 22:51:51, aluebs-webrtc wrote: > Do we need this intermediate variable? Done. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/modules.gyp File webrtc/modules/modules.gyp (right): https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/modules.gy... webrtc/modules/modules.gyp:444: 'audio_processing/beamformer_unittest.cc', On 2016/03/18 22:51:52, aluebs-webrtc wrote: > Shouldn't this unittest be under the beamformer/ folder? Also, shouldn't it be > named nonlinear_beamformer_unittest, since it only test the > nonlinear_beamformer? On top of that, shouldn't it then be combined with the > unittest? Absolutely! That is also what I wrote in the CL review invite. I will make that change.
Thank you for doing the moving in a different patch. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer_unittest.cc (right): https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:24: void ProcessOneFrame(int sample_rate_hz, On 2016/03/21 08:02:07, peah-webrtc wrote: > On 2016/03/18 22:51:52, 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/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:31: beamformer->ProcessChunk(*capture_audio_buffer->split_data_f(), On 2016/03/21 08:02:08, peah-webrtc wrote: > On 2016/03/18 22:51:50, aluebs-webrtc wrote: > > After running the beamformer, the number of channels should be set to mono: > > capture_audio_buffer->set_num_channels(1); > > Thanks! I saw that from the AudioProcessingImpl but it did not really make sense > to me. Why is that done? There is no comment about that. That is done so that the AudioBuffer is aware about the change in number of channels. In the APM this helps next components know the number of channels the have to work on. But here I think it only helps when copying-to the AudioBuffer. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:50: capture_config.num_frames(), capture_config.num_channels(), On 2016/03/21 08:02:07, peah-webrtc wrote: > On 2016/03/18 22:51:51, aluebs-webrtc wrote: > > num_process_channels should probably be mono, when running the beamformer. > > No, that does not work, as the CopyFrom then produces a mono audio buffer which > (correctly) causes an assert in the beamformer. > I don't fully understand the automated conversion in AudioBuffer but my guess is > that if num_input_channels == 2 and num_process_channels == 1 AudioBuffer will > automatically downmix the data before providing it to the user. > > I think this is the reason why you explicitly need to set > capture_audio_buffer->set_num_channels(1); after running the beamformer > (right?). Yes, you are right. My bad. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:56: ReadFloatSamplesFromStereoFile(samples_per_channel, 2, &capture_file, On 2016/03/21 08:02:06, peah-webrtc wrote: > On 2016/03/18 22:51:51, aluebs-webrtc wrote: > > Where does this function come from? Also use capture_config.num_channels() or > > capture_buffer.num_channels() instead of 2, and capture_config.num_frames() or > > capture_config.num_frames() instead of > > samples_pre_channel. > > The function is declared in > webrtc/modules/audio_processing/test/bitexactness_tools.h. > > Done. > Thanks for clarifying. I am not sure why this didn't appear on my initial grep. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:77: EXPECT_TRUE(test::BitExactFrame( On 2016/03/21 08:02:08, peah-webrtc wrote: > On 2016/03/18 22:51:50, 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 is much more intuitive. But if it will touch many files, you probably want to do it in a separate CL. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:111: 2.f); On 2016/03/21 08:02:06, peah-webrtc wrote: > On 2016/03/18 22:51:51, aluebs-webrtc wrote: > > The radius is completely ignored, but it would be interesting to add some > > non-broadside direction. > > Absolutely, what about the new one? That is elevation, right? I meant azimuth, the first parameter. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:115: // TODO(peah): Investigate why the nonlinear_beamformer.cc causes a DCHECK in On 2016/03/21 08:02:08, peah-webrtc wrote: > On 2016/03/18 22:51:52, aluebs-webrtc wrote: > > I think this should be resolved before landing this test. > > I don't agree and I cannot see why that would be desired. The test will anyway > provide test coverage for the rest of the code and I see no value at all for > delaying adding that test coverage. > > I was considering removing that test case and create a bug instead, but it is > quite nice to have a test-case ready for bug recreation and as an upcoming > regression test. What I am afraid of, is that this might be an issue with the test and not the code it is testing. But if you are positive that it isn't that way, I see how they are independent. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:117: TEST(BeamformerBitExactnessTest, On 2016/03/21 08:02:08, peah-webrtc wrote: > On 2016/03/18 22:51:52, 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? You can have a function, similar to CreateArrayGeometry, that returns the reference from the sample rate, the geometry case and the direction case. https://codereview.webrtc.org/1806853004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer_unittest.cc (right): https://codereview.webrtc.org/1806853004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:107: case 4: The 4th case is never called. https://codereview.webrtc.org/1806853004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:140: 0.001144f, -0.001026f, 0.001074f}; Why did these change?
Hi, Thanks for the comments. I have made changes accordingly but there is currently an issue with the tools I use to generate test data. I found out that they only can manage stereo data and therefore adding support for 3 microphones is a bigger task than I anticipated. Would it be fine to limit the test to using 2 microphones? Are we running the beamformer on any systems with more than 2 microphones? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer_unittest.cc (right): https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:24: void ProcessOneFrame(int sample_rate_hz, On 2016/03/22 12:15:55, aluebs-webrtc wrote: > On 2016/03/21 08:02:07, peah-webrtc wrote: > > On 2016/03/18 22:51:52, 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/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:31: beamformer->ProcessChunk(*capture_audio_buffer->split_data_f(), On 2016/03/22 12:15:55, aluebs-webrtc wrote: > On 2016/03/21 08:02:08, peah-webrtc wrote: > > On 2016/03/18 22:51:50, aluebs-webrtc wrote: > > > After running the beamformer, the number of channels should be set to mono: > > > capture_audio_buffer->set_num_channels(1); > > > > Thanks! I saw that from the AudioProcessingImpl but it did not really make > sense > > to me. Why is that done? There is no comment about that. > > That is done so that the AudioBuffer is aware about the change in number of > channels. In the APM this helps next components know the number of channels the > have to work on. But here I think it only helps when copying-to the AudioBuffer. Acknowledged. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:50: capture_config.num_frames(), capture_config.num_channels(), On 2016/03/22 12:15:55, aluebs-webrtc wrote: > On 2016/03/21 08:02:07, peah-webrtc wrote: > > On 2016/03/18 22:51:51, aluebs-webrtc wrote: > > > num_process_channels should probably be mono, when running the beamformer. > > > > No, that does not work, as the CopyFrom then produces a mono audio buffer > which > > (correctly) causes an assert in the beamformer. > > I don't fully understand the automated conversion in AudioBuffer but my guess > is > > that if num_input_channels == 2 and num_process_channels == 1 AudioBuffer will > > automatically downmix the data before providing it to the user. > > > > I think this is the reason why you explicitly need to set > > capture_audio_buffer->set_num_channels(1); after running the beamformer > > (right?). > > Yes, you are right. My bad. Acknowledged. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:56: ReadFloatSamplesFromStereoFile(samples_per_channel, 2, &capture_file, On 2016/03/22 12:15:55, aluebs-webrtc wrote: > On 2016/03/21 08:02:06, peah-webrtc wrote: > > On 2016/03/18 22:51:51, aluebs-webrtc wrote: > > > Where does this function come from? Also use capture_config.num_channels() > or > > > capture_buffer.num_channels() instead of 2, and capture_config.num_frames() > or > > > capture_config.num_frames() instead of > > > samples_pre_channel. > > > > The function is declared in > > webrtc/modules/audio_processing/test/bitexactness_tools.h. > > > > Done. > > > > Thanks for clarifying. I am not sure why this didn't appear on my initial grep. Acknowledged. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:77: EXPECT_TRUE(test::BitExactFrame( On 2016/03/22 12:15:56, aluebs-webrtc wrote: > On 2016/03/21 08:02:08, peah-webrtc wrote: > > On 2016/03/18 22:51:50, 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 is much more intuitive. But if it will touch many files, > you probably want to do it in a separate CL. Good, I think that change will be for the better. But I'll do it in another CL. Acknowledged. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:111: 2.f); On 2016/03/22 12:15:56, aluebs-webrtc wrote: > On 2016/03/21 08:02:06, peah-webrtc wrote: > > On 2016/03/18 22:51:51, aluebs-webrtc wrote: > > > The radius is completely ignored, but it would be interesting to add some > > > non-broadside direction. > > > > Absolutely, what about the new one? > > That is elevation, right? I meant azimuth, the first parameter. Ah, sorry about that! I will change that. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:115: // TODO(peah): Investigate why the nonlinear_beamformer.cc causes a DCHECK in On 2016/03/22 12:15:55, aluebs-webrtc wrote: > On 2016/03/21 08:02:08, peah-webrtc wrote: > > On 2016/03/18 22:51:52, aluebs-webrtc wrote: > > > I think this should be resolved before landing this test. > > > > I don't agree and I cannot see why that would be desired. The test will anyway > > provide test coverage for the rest of the code and I see no value at all for > > delaying adding that test coverage. > > > > I was considering removing that test case and create a bug instead, but it is > > quite nice to have a test-case ready for bug recreation and as an upcoming > > regression test. > > What I am afraid of, is that this might be an issue with the test and not the > code it is testing. But if you are positive that it isn't that way, I see how > they are independent. I don't think it is an issue with the test as the test is so simple. Regardless of that, it should not be possible to set up the beamformer so that it DCHECKS in the processing function so I think it is an issue with the beamformer. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:117: TEST(BeamformerBitExactnessTest, On 2016/03/22 12:15:55, aluebs-webrtc wrote: > On 2016/03/21 08:02:08, peah-webrtc wrote: > > On 2016/03/18 22:51:52, 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? > > You can have a function, similar to CreateArrayGeometry, that returns the > reference from the sample rate, the geometry case and the direction case. Ah, then I see. 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/1806853004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer_unittest.cc (right): https://codereview.webrtc.org/1806853004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:107: case 4: On 2016/03/22 12:15:56, aluebs-webrtc wrote: > The 4th case is never called. Good catch, missed actually using that configuration and instead just wrote that in the test description. Done. https://codereview.webrtc.org/1806853004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:140: 0.001144f, -0.001026f, 0.001074f}; On 2016/03/22 12:15:56, aluebs-webrtc wrote: > Why did these change? I'm not sure. It may be that the beamformer testvectors were not fully updated yet.
Today the beamformer is not used anywhere with more than 2 mics as far as we know, but WebRTC supports that use case. SO because of the platform nature of WebRTC I think we should add a test for more than 2 mics. This should be done in another CL, but could you add a TODO? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer_unittest.cc (right): https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:77: EXPECT_TRUE(test::BitExactFrame( On 2016/03/23 22:06:19, peah-webrtc wrote: > On 2016/03/22 12:15:56, aluebs-webrtc wrote: > > On 2016/03/21 08:02:08, peah-webrtc wrote: > > > On 2016/03/18 22:51:50, 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 is much more intuitive. But if it will touch many > files, > > you probably want to do it in a separate CL. > > Good, I think that change will be for the better. But I'll do it in another CL. > > Acknowledged. Acknowledged. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:115: // TODO(peah): Investigate why the nonlinear_beamformer.cc causes a DCHECK in On 2016/03/23 22:06:19, peah-webrtc wrote: > On 2016/03/22 12:15:55, aluebs-webrtc wrote: > > On 2016/03/21 08:02:08, peah-webrtc wrote: > > > On 2016/03/18 22:51:52, aluebs-webrtc wrote: > > > > I think this should be resolved before landing this test. > > > > > > I don't agree and I cannot see why that would be desired. The test will > anyway > > > provide test coverage for the rest of the code and I see no value at all for > > > delaying adding that test coverage. > > > > > > I was considering removing that test case and create a bug instead, but it > is > > > quite nice to have a test-case ready for bug recreation and as an upcoming > > > regression test. > > > > What I am afraid of, is that this might be an issue with the test and not the > > code it is testing. But if you are positive that it isn't that way, I see how > > they are independent. > > I don't think it is an issue with the test as the test is so simple. Regardless > of that, it should not be possible to set up the beamformer so that it DCHECKS > in the processing function so I think it is an issue with the beamformer. Well, it definitively is possible to DCHECK on the processing function. Just out of curiosity, where does it DCHECK? https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:117: TEST(BeamformerBitExactnessTest, On 2016/03/23 22:06:19, peah-webrtc wrote: > On 2016/03/22 12:15:55, aluebs-webrtc wrote: > > On 2016/03/21 08:02:08, peah-webrtc wrote: > > > On 2016/03/18 22:51:52, 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? > > > > You can have a function, similar to CreateArrayGeometry, that returns the > > reference from the sample rate, the geometry case and the direction case. > > Ah, then I see. 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. I think using TEST_P is exactly as verbose, since you have to manually add each case you want to test, but it avoids to repeat the code. And you could disable a specific test by commenting out the line where you instantiate it. Did the original reviewers evaluate using TEST_P? Because they might also prefer this, and in that case it would be good to also port all the rest of the tests to this mode. https://codereview.webrtc.org/1806853004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer_unittest.cc (right): https://codereview.webrtc.org/1806853004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:107: case 4: On 2016/03/23 22:06:20, peah-webrtc wrote: > On 2016/03/22 12:15:56, aluebs-webrtc wrote: > > The 4th case is never called. > Good catch, missed actually using that configuration and instead just wrote that > in the test description. > Done. Acknowledged. https://codereview.webrtc.org/1806853004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:140: 0.001144f, -0.001026f, 0.001074f}; On 2016/03/23 22:06:20, peah-webrtc wrote: > On 2016/03/22 12:15:56, aluebs-webrtc wrote: > > Why did these change? > > I'm not sure. It may be that the beamformer testvectors were not fully updated > yet. This exactly the reason I think these kind of tests are not useful. You changed the test in a way that should be bitexact, but the test fails, but you have no idea if there was a big or small change. So you just blindly change the values to the new ones that make the test pass, without understanding why.
https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer_unittest.cc (right): https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:115: // TODO(peah): Investigate why the nonlinear_beamformer.cc causes a DCHECK in On 2016/03/24 11:14:25, aluebs-webrtc wrote: > On 2016/03/23 22:06:19, peah-webrtc wrote: > > On 2016/03/22 12:15:55, aluebs-webrtc wrote: > > > On 2016/03/21 08:02:08, peah-webrtc wrote: > > > > On 2016/03/18 22:51:52, aluebs-webrtc wrote: > > > > > I think this should be resolved before landing this test. > > > > > > > > I don't agree and I cannot see why that would be desired. The test will > > anyway > > > > provide test coverage for the rest of the code and I see no value at all > for > > > > delaying adding that test coverage. > > > > > > > > I was considering removing that test case and create a bug instead, but it > > is > > > > quite nice to have a test-case ready for bug recreation and as an upcoming > > > > regression test. > > > > > > What I am afraid of, is that this might be an issue with the test and not > the > > > code it is testing. But if you are positive that it isn't that way, I see > how > > > they are independent. > > > > I don't think it is an issue with the test as the test is so simple. > Regardless > > of that, it should not be possible to set up the beamformer so that it DCHECKS > > in the processing function so I think it is an issue with the beamformer. > > Well, it definitively is possible to DCHECK on the processing function. Just out > of curiosity, where does it DCHECK? True. Firstly, using the new configurations, I was able to reactivate this test. The other two disabled tests DCHECKS in the InitHighFrequencyCorrectionRanges function at lines 259 and 261 that is called via the Initialize call. I first thought it was the DCHECKING in the ProcessChunk function which I don't think should happen but in the Initialize function I guess a DCHECK is much more fine. I have no idea about the consequenses of this or reasons for why it DCHECKS though. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:117: TEST(BeamformerBitExactnessTest, On 2016/03/24 11:14:25, aluebs-webrtc wrote: > On 2016/03/23 22:06:19, peah-webrtc wrote: > > On 2016/03/22 12:15:55, aluebs-webrtc wrote: > > > On 2016/03/21 08:02:08, peah-webrtc wrote: > > > > On 2016/03/18 22:51:52, 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? > > > > > > You can have a function, similar to CreateArrayGeometry, that returns the > > > reference from the sample rate, the geometry case and the direction case. > > > > Ah, then I see. 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. > > I think using TEST_P is exactly as verbose, since you have to manually add each > case you want to test, but it avoids to repeat the code. And you could disable a > specific test by commenting out the line where you instantiate it. Did the > original reviewers evaluate using TEST_P? Because they might also prefer this, > and in that case it would be good to also port all the rest of the tests to this > mode. 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. https://codereview.webrtc.org/1806853004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer_unittest.cc (right): https://codereview.webrtc.org/1806853004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:140: 0.001144f, -0.001026f, 0.001074f}; On 2016/03/24 11:14:25, aluebs-webrtc wrote: > On 2016/03/23 22:06:20, peah-webrtc wrote: > > On 2016/03/22 12:15:56, aluebs-webrtc wrote: > > > Why did these change? > > > > I'm not sure. It may be that the beamformer testvectors were not fully updated > > yet. > > This exactly the reason I think these kind of tests are not useful. You changed > the test in a way that should be bitexact, but the test fails, but you have no > idea if there was a big or small change. So you just blindly change the values > to the new ones that make the test pass, without understanding why. The bitexactness tests are there to identify when something happens, and not why/what happens. And by limiting their functionality to that, they can detect changes in a much more refined way than any other tests. Once the test code is finished, there are only changes in the submodule that should cause bitinexactness. If bitinexactness occurs without any apparent reason for it, the changes done in the submodule needs to be reviewed in a more refined manner. But if they are expected the vectors should simply be updated. In this case I know that I changed only the test code and not the code of the submodule, so I'm confident it is not a problem with changes done in the submodule.
lgtm, once you add a TODO to test on more channels. 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. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer_unittest.cc (right): https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:115: // TODO(peah): Investigate why the nonlinear_beamformer.cc causes a DCHECK in On 2016/03/24 11:51:30, peah-webrtc wrote: > On 2016/03/24 11:14:25, aluebs-webrtc wrote: > > On 2016/03/23 22:06:19, peah-webrtc wrote: > > > On 2016/03/22 12:15:55, aluebs-webrtc wrote: > > > > On 2016/03/21 08:02:08, peah-webrtc wrote: > > > > > On 2016/03/18 22:51:52, aluebs-webrtc wrote: > > > > > > I think this should be resolved before landing this test. > > > > > > > > > > I don't agree and I cannot see why that would be desired. The test will > > > anyway > > > > > provide test coverage for the rest of the code and I see no value at all > > for > > > > > delaying adding that test coverage. > > > > > > > > > > I was considering removing that test case and create a bug instead, but > it > > > is > > > > > quite nice to have a test-case ready for bug recreation and as an > upcoming > > > > > regression test. > > > > > > > > What I am afraid of, is that this might be an issue with the test and not > > the > > > > code it is testing. But if you are positive that it isn't that way, I see > > how > > > > they are independent. > > > > > > I don't think it is an issue with the test as the test is so simple. > > Regardless > > > of that, it should not be possible to set up the beamformer so that it > DCHECKS > > > in the processing function so I think it is an issue with the beamformer. > > > > Well, it definitively is possible to DCHECK on the processing function. Just > out > > of curiosity, where does it DCHECK? > True. Firstly, using the new configurations, I was able to reactivate this test. > > The other two disabled tests DCHECKS in the InitHighFrequencyCorrectionRanges > function at lines 259 and 261 that is called via the Initialize call. > > I first thought it was the DCHECKING in the ProcessChunk function which I don't > think should happen but in the Initialize function I guess a DCHECK is much more > fine. I have no idea about the consequenses of this or reasons for why it > DCHECKS though. That is weird, since those DCHECKs only depend on the sampling_rate (and some constants). It only checks that the frequency bins of the extremes of the range for high and low frequency corrections are in the right order. We should look into that, to see if it is a test-issue or and actual bug caught by this test. https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:117: TEST(BeamformerBitExactnessTest, On 2016/03/24 11:51:30, peah-webrtc wrote: > On 2016/03/24 11:14:25, aluebs-webrtc wrote: > > On 2016/03/23 22:06:19, peah-webrtc wrote: > > > On 2016/03/22 12:15:55, aluebs-webrtc wrote: > > > > On 2016/03/21 08:02:08, peah-webrtc wrote: > > > > > On 2016/03/18 22:51:52, 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? > > > > > > > > You can have a function, similar to CreateArrayGeometry, that returns the > > > > reference from the sample rate, the geometry case and the direction case. > > > > > > Ah, then I see. 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. > > > > I think using TEST_P is exactly as verbose, since you have to manually add > each > > case you want to test, but it avoids to repeat the code. And you could disable > a > > specific test by commenting out the line where you instantiate it. Did the > > original reviewers evaluate using TEST_P? Because they might also prefer this, > > and in that case it would be good to also port all the rest of the tests to > this > > mode. > > 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. https://codereview.webrtc.org/1806853004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer_unittest.cc (right): https://codereview.webrtc.org/1806853004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:140: 0.001144f, -0.001026f, 0.001074f}; On 2016/03/24 11:51:30, peah-webrtc wrote: > On 2016/03/24 11:14:25, aluebs-webrtc wrote: > > On 2016/03/23 22:06:20, peah-webrtc wrote: > > > On 2016/03/22 12:15:56, aluebs-webrtc wrote: > > > > Why did these change? > > > > > > I'm not sure. It may be that the beamformer testvectors were not fully > updated > > > yet. > > > > This exactly the reason I think these kind of tests are not useful. You > changed > > the test in a way that should be bitexact, but the test fails, but you have no > > idea if there was a big or small change. So you just blindly change the values > > to the new ones that make the test pass, without understanding why. > > The bitexactness tests are there to identify when something happens, and not > why/what happens. And by limiting their functionality to that, they can detect > changes in a much more refined way than any other tests. > > Once the test code is finished, there are only changes in the submodule that > should cause bitinexactness. If bitinexactness occurs without any apparent > reason for it, the changes done in the submodule needs to be reviewed in a more > refined manner. But if they are expected the vectors should simply be updated. > > In this case I know that I changed only the test code and not the code of the > submodule, so I'm confident it is not a problem with changes done in the > submodule. I am still not convinced, but let's discuss offline.
https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer_unittest.cc (right): https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer_unittest.cc:117: TEST(BeamformerBitExactnessTest, On 2016/03/28 23:01:17, aluebs-webrtc wrote: > On 2016/03/24 11:51:30, peah-webrtc wrote: > > On 2016/03/24 11:14:25, aluebs-webrtc wrote: > > > On 2016/03/23 22:06:19, peah-webrtc wrote: > > > > On 2016/03/22 12:15:55, aluebs-webrtc wrote: > > > > > On 2016/03/21 08:02:08, peah-webrtc wrote: > > > > > > On 2016/03/18 22:51:52, 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? > > > > > > > > > > You can have a function, similar to CreateArrayGeometry, that returns > the > > > > > reference from the sample rate, the geometry case and the direction > case. > > > > > > > > Ah, then I see. 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. > > > > > > I think using TEST_P is exactly as verbose, since you have to manually add > > each > > > case you want to test, but it avoids to repeat the code. And you could > disable > > a > > > specific test by commenting out the line where you instantiate it. Did the > > > original reviewers evaluate using TEST_P? Because they might also prefer > this, > > > and in that case it would be good to also port all the rest of the tests to > > this > > > mode. > > > > 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. The comment is there with the timestamp 2015-12-10 08:53:49 UTC.
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/1806853004/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1806853004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1806853004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/825)
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/1806853004/#ps160001 (title: "Disabled all the bitexactness tests for the beamformer due to problems with division by zero in the…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1806853004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1806853004/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Added a bitexactness test for the beamformer in the audio processing module BUG=webrtc:5341 ========== to ========== Added a bitexactness test for the beamformer in the audio processing module BUG=webrtc:5341 Committed: https://crrev.com/ceef04613b1bea79a0b650db9e5ca5d8e153adbc Cr-Commit-Position: refs/heads/master@{#12137} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ceef04613b1bea79a0b650db9e5ca5d8e153adbc Cr-Commit-Position: refs/heads/master@{#12137}
Message was sent while issue was closed.
On 2016/03/29 05:19:19, peah-webrtc wrote: > https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/beamformer_unittest.cc (right): > > https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/beamformer_unittest.cc:117: > TEST(BeamformerBitExactnessTest, > On 2016/03/28 23:01:17, aluebs-webrtc wrote: > > On 2016/03/24 11:51:30, peah-webrtc wrote: > > > On 2016/03/24 11:14:25, aluebs-webrtc wrote: > > > > On 2016/03/23 22:06:19, peah-webrtc wrote: > > > > > On 2016/03/22 12:15:55, aluebs-webrtc wrote: > > > > > > On 2016/03/21 08:02:08, peah-webrtc wrote: > > > > > > > On 2016/03/18 22:51:52, 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? > > > > > > > > > > > > You can have a function, similar to CreateArrayGeometry, that returns > > the > > > > > > reference from the sample rate, the geometry case and the direction > > case. > > > > > > > > > > Ah, then I see. 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. > > > > > > > > I think using TEST_P is exactly as verbose, since you have to manually add > > > each > > > > case you want to test, but it avoids to repeat the code. And you could > > disable > > > a > > > > specific test by commenting out the line where you instantiate it. Did the > > > > original reviewers evaluate using TEST_P? Because they might also prefer > > this, > > > > and in that case it would be good to also port all the rest of the tests > to > > > this > > > > mode. > > > > > > 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. > > The comment is there with the timestamp 2015-12-10 08:53:49 UTC. I didn't see that comment, because I was checking the comments on the patches themselves. Thank you for pointing that out. But the TODO was never added.
Message was sent while issue was closed.
On 2016/03/29 15:04:25, aluebs-webrtc wrote: > On 2016/03/29 05:19:19, peah-webrtc wrote: > > > https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... > > File webrtc/modules/audio_processing/beamformer_unittest.cc (right): > > > > > https://codereview.webrtc.org/1806853004/diff/40001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/beamformer_unittest.cc:117: > > TEST(BeamformerBitExactnessTest, > > On 2016/03/28 23:01:17, aluebs-webrtc wrote: > > > On 2016/03/24 11:51:30, peah-webrtc wrote: > > > > On 2016/03/24 11:14:25, aluebs-webrtc wrote: > > > > > On 2016/03/23 22:06:19, peah-webrtc wrote: > > > > > > On 2016/03/22 12:15:55, aluebs-webrtc wrote: > > > > > > > On 2016/03/21 08:02:08, peah-webrtc wrote: > > > > > > > > On 2016/03/18 22:51:52, 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? > > > > > > > > > > > > > > You can have a function, similar to CreateArrayGeometry, that > returns > > > the > > > > > > > reference from the sample rate, the geometry case and the direction > > > case. > > > > > > > > > > > > Ah, then I see. 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. > > > > > > > > > > I think using TEST_P is exactly as verbose, since you have to manually > add > > > > each > > > > > case you want to test, but it avoids to repeat the code. And you could > > > disable > > > > a > > > > > specific test by commenting out the line where you instantiate it. Did > the > > > > > original reviewers evaluate using TEST_P? Because they might also prefer > > > this, > > > > > and in that case it would be good to also port all the rest of the tests > > to > > > > this > > > > > mode. > > > > > > > > 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. > > > > The comment is there with the timestamp 2015-12-10 08:53:49 UTC. > > I didn't see that comment, because I was checking the comments on the patches > themselves. Thank you for pointing that out. > But the TODO was never added. Ouch! Sorry about. I'm creating a new CL for that now. |
