Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1804373002:
Added a bitexactness test for the voice activity detector in the audio processing module. (Closed)
|
Created:
4 years, 9 months ago by peah-webrtc Modified:
4 years, 9 months ago Reviewers:
hlundin-webrtc CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@LevelEstimatorBitExactness_CL Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded a bitexactness test for the voice activity detector in the audio processing module.
BUG=webrtc:5340
Committed: https://crrev.com/bdbceeffe8a7360dd90958ccc1c30b478a08c350
Cr-Commit-Position: refs/heads/master@{#12066}
Patch Set 1 #
Total comments: 30
Patch Set 2 : Changes in response to reviewer comments and to harmonize with the other bitexactness test code #Patch Set 3 : Minor parameter changes #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 13 (5 generated)
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/bitexactness_tools.cc (right): https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/bitexactness_tools.cc:20: ::testing::AssertionResult AssertBoolsNotEqual(const char* m_expr, No, use EXPECT_EQ instead. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/voice_detection_unittest.cc (right): https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. 2016 https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:27: ::testing::AssertionResult AssertLikelihoodsNotEqual( This is a lot of code to compare two enum (integer) values. Use EXPECT_EQ instead. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:76: if (sample_rate_hz > AudioProcessing::kSampleRate16kHz) { What is the rationale for merging the bands again? Is this buffer used any more after the voice detection is done? https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:94: float signal_gain = 0.0f; Consider: float signal_gain = 0.1f; // TestSignalLevels::kLow if (signal_level == TestSignalLevels::kMedium) signal_gain = 0.5f; else if (signal_level == TestSignalLevels::kHigh) signal_gain = 1.0f; More compact, and if you make TestSignalLevels an enum class, you won't have to worry about other values. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:122: std::string GetTestVectorFileName(int sample_rate_hz) { You can get rid of this function if you follow my suggestion below. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:156: test::AudioLoop audio_loop; I would actually recommend you use a ResampleInputAudioFile instead. It is a specialization of InputAudioFile. The differences are that it reads directly from file instead of from memory, but it supports resampling so you can use a single input file for all rates. It also supports looping, just like AudioLoop. Bonus: it has a DuplicateInterleaved utility. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:164: std::string filename; Not used. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:169: RTC_DCHECK(success); This is test code; you might as well CHECK things like this, since continuing with the test without an input file makes no sense, release or debug alike. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:190: EXPECT_PRED_FORMAT2(test::AssertIntegersNotEqual, frame_size_ms, I think all of these should be changed to regular EXPECT_EQ. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:203: const int kFrameSizeMsReference = 10; kFrameSizeMsReference is always 10; define it once before the TESTs. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:204: const bool kStreamHAsVoiceReference = true; HAs -> Has https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:204: const bool kStreamHAsVoiceReference = true; Define once before the TESTs. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:205: const VoiceDetection::Likelihood kLlikelihoodReference = kLli -> kLi https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:205: const VoiceDetection::Likelihood kLlikelihoodReference = ... and define it before the TESTs.
https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/bitexactness_tools.cc (right): https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/bitexactness_tools.cc:20: ::testing::AssertionResult AssertBoolsNotEqual(const char* m_expr, On 2016/03/16 15:50:20, hlundin-webrtc wrote: > No, use EXPECT_EQ instead. Done. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/voice_detection_unittest.cc (right): https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. On 2016/03/16 15:50:20, hlundin-webrtc wrote: > 2016 Done. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:27: ::testing::AssertionResult AssertLikelihoodsNotEqual( On 2016/03/16 15:50:20, hlundin-webrtc wrote: > This is a lot of code to compare two enum (integer) values. Use EXPECT_EQ > instead. Done. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:76: if (sample_rate_hz > AudioProcessing::kSampleRate16kHz) { On 2016/03/16 15:50:20, hlundin-webrtc wrote: > What is the rationale for merging the bands again? Is this buffer used any more > after the voice detection is done? No point at all in this case, I will remove that. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:94: float signal_gain = 0.0f; On 2016/03/16 15:50:20, hlundin-webrtc wrote: > Consider: > > float signal_gain = 0.1f; // TestSignalLevels::kLow > if (signal_level == TestSignalLevels::kMedium) > signal_gain = 0.5f; > else if (signal_level == TestSignalLevels::kHigh) > signal_gain = 1.0f; > > More compact, and if you make TestSignalLevels an enum class, you won't have to > worry about other values. Thanks. This code is now removed. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:122: std::string GetTestVectorFileName(int sample_rate_hz) { On 2016/03/16 15:50:20, hlundin-webrtc wrote: > You can get rid of this function if you follow my suggestion below. Done. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:156: test::AudioLoop audio_loop; On 2016/03/16 15:50:20, hlundin-webrtc wrote: > I would actually recommend you use a ResampleInputAudioFile instead. It is a > specialization of InputAudioFile. The differences are that it reads directly > from file instead of from memory, but it supports resampling so you can use a > single input file for all rates. It also supports looping, just like AudioLoop. > > Bonus: it has a DuplicateInterleaved utility. Thanks for the suggestion!!! I found some Resource files for 8-48 kHz for far-end and nearend so I will for now use those instead, using InputAudioFile. But if they prove not to be sufficiently good, I'll instead go for ResampleInputAudioFile. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:164: std::string filename; On 2016/03/16 15:50:20, hlundin-webrtc wrote: > Not used. Done. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:169: RTC_DCHECK(success); On 2016/03/16 15:50:20, hlundin-webrtc wrote: > This is test code; you might as well CHECK things like this, since continuing > with the test without an input file makes no sense, release or debug alike. Good point!!! Removed this one but will change other DCHECK to CHECK within the tests. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:190: EXPECT_PRED_FORMAT2(test::AssertIntegersNotEqual, frame_size_ms, On 2016/03/16 15:50:20, hlundin-webrtc wrote: > I think all of these should be changed to regular EXPECT_EQ. Done. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:203: const int kFrameSizeMsReference = 10; On 2016/03/16 15:50:20, hlundin-webrtc wrote: > kFrameSizeMsReference is always 10; define it once before the TESTs. Done. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:204: const bool kStreamHAsVoiceReference = true; On 2016/03/16 15:50:20, hlundin-webrtc wrote: > HAs -> Has Done. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:204: const bool kStreamHAsVoiceReference = true; On 2016/03/16 15:50:20, hlundin-webrtc wrote: > Define once before the TESTs. Done. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:205: const VoiceDetection::Likelihood kLlikelihoodReference = On 2016/03/16 15:50:20, hlundin-webrtc wrote: > ... and define it before the TESTs. Done. https://codereview.webrtc.org/1804373002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/voice_detection_unittest.cc:205: const VoiceDetection::Likelihood kLlikelihoodReference = On 2016/03/16 15:50:20, hlundin-webrtc wrote: > ... and define it before the TESTs. Done.
lgtm Nice!
The CQ bit was checked by peah@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1811443002 Patch 20001). Please resolve the dependency and try again.
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1804373002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804373002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Added a bitexactness test for the voice activity detector in the audio processing module. BUG=webrtc:5340 ========== to ========== Added a bitexactness test for the voice activity detector in the audio processing module. BUG=webrtc:5340 Committed: https://crrev.com/bdbceeffe8a7360dd90958ccc1c30b478a08c350 Cr-Commit-Position: refs/heads/master@{#12066} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bdbceeffe8a7360dd90958ccc1c30b478a08c350 Cr-Commit-Position: refs/heads/master@{#12066} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
