From 648af7423f9802e2cc7925c4bd99aed00c42f2a7 Mon Sep 17 00:00:00 2001 From: "andrew@webrtc.org" Date: Wed, 8 Feb 2012 01:57:29 +0000 Subject: [PATCH] Clean up MapSetting(). - Add assert(false) for "impossible" cases. - Remove tests for invalid enum values. - Modify MapError() to look the same way. BUG= TEST=audioproc_unittest Review URL: https://webrtc-codereview.appspot.com/386001 git-svn-id: http://webrtc.googlecode.com/svn/trunk@1631 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../echo_cancellation_impl.cc | 8 +-- .../echo_control_mobile_impl.cc | 5 +- .../audio_processing/gain_control_impl.cc | 19 +----- .../include/audio_processing.h | 14 ++--- .../noise_suppression_impl.cc | 3 +- .../audio_processing/test/unit_test.cc | 58 +++---------------- .../audio_processing/voice_detection_impl.cc | 8 +-- 7 files changed, 21 insertions(+), 94 deletions(-) diff --git a/src/modules/audio_processing/echo_cancellation_impl.cc b/src/modules/audio_processing/echo_cancellation_impl.cc index 77e076aa95..d4c5523da4 100644 --- a/src/modules/audio_processing/echo_cancellation_impl.cc +++ b/src/modules/audio_processing/echo_cancellation_impl.cc @@ -33,22 +33,18 @@ WebRtc_Word16 MapSetting(EchoCancellation::SuppressionLevel level) { case EchoCancellation::kHighSuppression: return kAecNlpAggressive; } - // TODO(mflodman) Needed for gcc to compile and assert can't be added due to - // ApmTest triggers this. + assert(false); return -1; } -int MapError(int err) { +AudioProcessing::Error MapError(int err) { switch (err) { case AEC_UNSUPPORTED_FUNCTION_ERROR: return AudioProcessing::kUnsupportedFunctionError; - break; case AEC_BAD_PARAMETER_ERROR: return AudioProcessing::kBadParameterError; - break; case AEC_BAD_PARAMETER_WARNING: return AudioProcessing::kBadStreamParameterWarning; - break; default: // AEC_UNSPECIFIED_ERROR // AEC_UNINITIALIZED_ERROR diff --git a/src/modules/audio_processing/echo_control_mobile_impl.cc b/src/modules/audio_processing/echo_control_mobile_impl.cc index e337d424b6..94277890b2 100644 --- a/src/modules/audio_processing/echo_control_mobile_impl.cc +++ b/src/modules/audio_processing/echo_control_mobile_impl.cc @@ -37,12 +37,11 @@ WebRtc_Word16 MapSetting(EchoControlMobile::RoutingMode mode) { case EchoControlMobile::kLoudSpeakerphone: return 4; } - // TODO(mflodman) Needed for gcc to compile and assert can't be added due to - // ApmTest triggers this. + assert(false); return -1; } -int MapError(int err) { +AudioProcessing::Error MapError(int err) { switch (err) { case AECM_UNSUPPORTED_FUNCTION_ERROR: return AudioProcessing::kUnsupportedFunctionError; diff --git a/src/modules/audio_processing/gain_control_impl.cc b/src/modules/audio_processing/gain_control_impl.cc index eb7ca611fd..a518ab5e3c 100644 --- a/src/modules/audio_processing/gain_control_impl.cc +++ b/src/modules/audio_processing/gain_control_impl.cc @@ -22,34 +22,17 @@ namespace webrtc { typedef void Handle; -/*template -class GainControlHandle : public ComponentHandle { - public: - GainControlHandle(); - virtual ~GainControlHandle(); - - virtual int Create(); - virtual T* ptr() const; - - private: - T* handle; -};*/ - namespace { WebRtc_Word16 MapSetting(GainControl::Mode mode) { switch (mode) { case GainControl::kAdaptiveAnalog: return kAgcModeAdaptiveAnalog; - break; case GainControl::kAdaptiveDigital: return kAgcModeAdaptiveDigital; - break; case GainControl::kFixedDigital: return kAgcModeFixedDigital; - break; } - // TODO(mflodman) Needed for gcc to compile and assert can't be added due to - // ApmTest triggers this. + assert(false); return -1; } } // namespace diff --git a/src/modules/audio_processing/include/audio_processing.h b/src/modules/audio_processing/include/audio_processing.h index ee4d06f71b..aefb824d3e 100644 --- a/src/modules/audio_processing/include/audio_processing.h +++ b/src/modules/audio_processing/include/audio_processing.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2011 The WebRTC project authors. All Rights Reserved. + * Copyright (c) 2012 The WebRTC project authors. All Rights Reserved. * * Use of this source code is governed by a BSD-style license * that can be found in the LICENSE file in the root of the source @@ -216,8 +216,8 @@ class AudioProcessing : public Module { int minimum; // Long-term minimum. }; - // Fatal errors. - enum Errors { + enum Error { + // Fatal errors. kNoError = 0, kUnspecifiedError = -1, kCreationFailedError = -2, @@ -230,14 +230,12 @@ class AudioProcessing : public Module { kBadNumberChannelsError = -9, kFileError = -10, kStreamParameterNotSetError = -11, - kNotEnabledError = -12 - }; + kNotEnabledError = -12, - // Warnings are non-fatal. - enum Warnings { + // Warnings are non-fatal. // This results when a set_stream_ parameter is out of range. Processing // will continue, but the parameter may have been truncated. - kBadStreamParameterWarning = -13, + kBadStreamParameterWarning = -13 }; // Inherited from Module. diff --git a/src/modules/audio_processing/noise_suppression_impl.cc b/src/modules/audio_processing/noise_suppression_impl.cc index 5c16e0e0a2..c44d3fedd3 100644 --- a/src/modules/audio_processing/noise_suppression_impl.cc +++ b/src/modules/audio_processing/noise_suppression_impl.cc @@ -42,8 +42,7 @@ int MapSetting(NoiseSuppression::Level level) { case NoiseSuppression::kVeryHigh: return 3; } - // TODO(mflodman) Needed for gcc to compile and assert can't be added due to - // ApmTest triggers this. + assert(false); return -1; } } // namespace diff --git a/src/modules/audio_processing/test/unit_test.cc b/src/modules/audio_processing/test/unit_test.cc index d3ead7a5c6..e43d497a6c 100644 --- a/src/modules/audio_processing/test/unit_test.cc +++ b/src/modules/audio_processing/test/unit_test.cc @@ -573,14 +573,6 @@ TEST_F(ApmTest, EchoCancellation) { apm_->echo_cancellation()->device_sample_rate_hz()); } - EXPECT_EQ(apm_->kBadParameterError, - apm_->echo_cancellation()->set_suppression_level( - static_cast(-1))); - - EXPECT_EQ(apm_->kBadParameterError, - apm_->echo_cancellation()->set_suppression_level( - static_cast(4))); - EchoCancellation::SuppressionLevel level[] = { EchoCancellation::kLowSuppression, EchoCancellation::kModerateSuppression, @@ -631,13 +623,6 @@ TEST_F(ApmTest, EchoControlMobile) { EXPECT_EQ(apm_->kNoError, apm_->echo_control_mobile()->Enable(true)); EXPECT_TRUE(apm_->echo_control_mobile()->is_enabled()); - EXPECT_EQ(apm_->kBadParameterError, - apm_->echo_control_mobile()->set_routing_mode( - static_cast(-1))); - EXPECT_EQ(apm_->kBadParameterError, - apm_->echo_control_mobile()->set_routing_mode( - static_cast(5))); - // Toggle routing modes EchoControlMobile::RoutingMode mode[] = { EchoControlMobile::kQuietEarpieceOrHeadset, @@ -694,12 +679,6 @@ TEST_F(ApmTest, EchoControlMobile) { TEST_F(ApmTest, GainControl) { // Testing gain modes - EXPECT_EQ(apm_->kBadParameterError, - apm_->gain_control()->set_mode(static_cast(-1))); - - EXPECT_EQ(apm_->kBadParameterError, - apm_->gain_control()->set_mode(static_cast(3))); - EXPECT_EQ(apm_->kNoError, apm_->gain_control()->set_mode( apm_->gain_control()->mode())); @@ -795,18 +774,7 @@ TEST_F(ApmTest, GainControl) { } TEST_F(ApmTest, NoiseSuppression) { - // Tesing invalid suppression levels - - // TODO(mflodman) Check at these failures. -// EXPECT_EQ(apm_->kBadParameterError, -// apm_->noise_suppression()->set_level( -// static_cast(-1))); - -// EXPECT_EQ(apm_->kBadParameterError, -// apm_->noise_suppression()->set_level( -// static_cast(5))); - - // Tesing valid suppression levels + // Test valid suppression levels. NoiseSuppression::Level level[] = { NoiseSuppression::kLow, NoiseSuppression::kModerate, @@ -819,7 +787,7 @@ TEST_F(ApmTest, NoiseSuppression) { EXPECT_EQ(level[i], apm_->noise_suppression()->level()); } - // Turing NS on/off + // Turn NS on/off EXPECT_EQ(apm_->kNoError, apm_->noise_suppression()->Enable(true)); EXPECT_TRUE(apm_->noise_suppression()->is_enabled()); EXPECT_EQ(apm_->kNoError, apm_->noise_suppression()->Enable(false)); @@ -827,7 +795,7 @@ TEST_F(ApmTest, NoiseSuppression) { } TEST_F(ApmTest, HighPassFilter) { - // Turing HP filter on/off + // Turn HP filter on/off EXPECT_EQ(apm_->kNoError, apm_->high_pass_filter()->Enable(true)); EXPECT_TRUE(apm_->high_pass_filter()->is_enabled()); EXPECT_EQ(apm_->kNoError, apm_->high_pass_filter()->Enable(false)); @@ -835,7 +803,7 @@ TEST_F(ApmTest, HighPassFilter) { } TEST_F(ApmTest, LevelEstimator) { - // Turning level estimator on/off + // Turn level estimator on/off EXPECT_EQ(apm_->kNoError, apm_->level_estimator()->Enable(false)); EXPECT_FALSE(apm_->level_estimator()->is_enabled()); @@ -918,17 +886,7 @@ TEST_F(ApmTest, VoiceDetection) { apm_->voice_detection()->set_stream_has_voice(false)); EXPECT_FALSE(apm_->voice_detection()->stream_has_voice()); - // Tesing invalid likelihoods - // TODO(mflodman) Check at these failures. -// EXPECT_EQ(apm_->kBadParameterError, -// apm_->voice_detection()->set_likelihood( -// static_cast(-1))); - -// EXPECT_EQ(apm_->kBadParameterError, -// apm_->voice_detection()->set_likelihood( -// static_cast(5))); - - // Tesing valid likelihoods + // Test valid likelihoods VoiceDetection::Likelihood likelihood[] = { VoiceDetection::kVeryLowLikelihood, VoiceDetection::kLowLikelihood, @@ -942,11 +900,11 @@ TEST_F(ApmTest, VoiceDetection) { } /* TODO(bjornv): Enable once VAD supports other frame lengths than 10 ms - // Tesing invalid frame sizes + // Test invalid frame sizes EXPECT_EQ(apm_->kBadParameterError, apm_->voice_detection()->set_frame_size_ms(12)); - // Tesing valid frame sizes + // Test valid frame sizes for (int i = 10; i <= 30; i += 10) { EXPECT_EQ(apm_->kNoError, apm_->voice_detection()->set_frame_size_ms(i)); @@ -954,7 +912,7 @@ TEST_F(ApmTest, VoiceDetection) { } */ - // Turing VAD on/off + // Turn VAD on/off EXPECT_EQ(apm_->kNoError, apm_->voice_detection()->Enable(true)); EXPECT_TRUE(apm_->voice_detection()->is_enabled()); EXPECT_EQ(apm_->kNoError, apm_->voice_detection()->Enable(false)); diff --git a/src/modules/audio_processing/voice_detection_impl.cc b/src/modules/audio_processing/voice_detection_impl.cc index 98242b90ca..50b99a0d46 100644 --- a/src/modules/audio_processing/voice_detection_impl.cc +++ b/src/modules/audio_processing/voice_detection_impl.cc @@ -27,24 +27,18 @@ int MapSetting(VoiceDetection::Likelihood likelihood) { switch (likelihood) { case VoiceDetection::kVeryLowLikelihood: return 3; - break; case VoiceDetection::kLowLikelihood: return 2; - break; case VoiceDetection::kModerateLikelihood: return 1; - break; case VoiceDetection::kHighLikelihood: return 0; - break; } - // TODO(mflodman) Needed for gcc to compile and assert can't be added due to - // ApmTest triggers this. + assert(false); return -1; } } // namespace - VoiceDetectionImpl::VoiceDetectionImpl(const AudioProcessingImpl* apm) : ProcessingComponent(apm), apm_(apm),