From f3930e941c15da48c037c62cdb1eebbcbf89c9c7 Mon Sep 17 00:00:00 2001 From: "andrew@webrtc.org" Date: Wed, 18 Sep 2013 22:37:32 +0000 Subject: [PATCH] Small refactoring of AudioProcessing use in channel.cc. - Apply consistent naming. - Use a scoped_ptr for rx_audioproc_. - Remove now unnecessary AudioProcessing::Destroy(). R=bjornv@webrtc.org, xians@webrtc.org Review URL: https://webrtc-codereview.appspot.com/2184007 git-svn-id: http://webrtc.googlecode.com/svn/trunk@4784 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../audio_processing/audio_processing_impl.cc | 4 - .../include/audio_processing.h | 8 +- .../test/audio_processing_unittest.cc | 29 ++-- .../audio_processing/test/process_test.cc | 8 +- webrtc/voice_engine/channel.cc | 163 +++++++----------- webrtc/voice_engine/channel.h | 4 +- 6 files changed, 81 insertions(+), 135 deletions(-) diff --git a/webrtc/modules/audio_processing/audio_processing_impl.cc b/webrtc/modules/audio_processing/audio_processing_impl.cc index 143da44cdf..edf20bc2f2 100644 --- a/webrtc/modules/audio_processing/audio_processing_impl.cc +++ b/webrtc/modules/audio_processing/audio_processing_impl.cc @@ -47,10 +47,6 @@ AudioProcessing* AudioProcessing::Create(int id) { return apm; } -void AudioProcessing::Destroy(AudioProcessing* apm) { - delete static_cast(apm); -} - int32_t AudioProcessing::TimeUntilNextProcess() { return -1; } int32_t AudioProcessing::Process() { return -1; } diff --git a/webrtc/modules/audio_processing/include/audio_processing.h b/webrtc/modules/audio_processing/include/audio_processing.h index 2edea8ad70..b01cbb32fb 100644 --- a/webrtc/modules/audio_processing/include/audio_processing.h +++ b/webrtc/modules/audio_processing/include/audio_processing.h @@ -105,8 +105,7 @@ class VoiceDetection; // apm->Initialize(); // // // Close the application... -// AudioProcessing::Destroy(apm); -// apm = NULL; +// delete apm; // class AudioProcessing : public Module { public: @@ -118,11 +117,6 @@ class AudioProcessing : public Module { static AudioProcessing* Create(int id); virtual ~AudioProcessing() {} - // TODO(andrew): remove this method. We now allow users to delete instances - // directly, useful for scoped_ptr. - // Destroys a |apm| instance. - static void Destroy(AudioProcessing* apm); - // Initializes internal states, while retaining all user settings. This // should be called before beginning to process a new audio stream. However, // it is not necessary to call before processing the first stream after diff --git a/webrtc/modules/audio_processing/test/audio_processing_unittest.cc b/webrtc/modules/audio_processing/test/audio_processing_unittest.cc index f93b7b8bfc..d8870a5275 100644 --- a/webrtc/modules/audio_processing/test/audio_processing_unittest.cc +++ b/webrtc/modules/audio_processing/test/audio_processing_unittest.cc @@ -42,6 +42,7 @@ using webrtc::NoiseSuppression; using webrtc::EchoCancellation; using webrtc::EventWrapper; using webrtc::scoped_array; +using webrtc::scoped_ptr; using webrtc::Trace; using webrtc::LevelEstimator; using webrtc::EchoCancellation; @@ -244,9 +245,9 @@ class ApmTest : public ::testing::Test { const std::string output_path_; const std::string ref_path_; const std::string ref_filename_; - webrtc::AudioProcessing* apm_; - webrtc::AudioFrame* frame_; - webrtc::AudioFrame* revframe_; + scoped_ptr apm_; + AudioFrame* frame_; + AudioFrame* revframe_; FILE* far_file_; FILE* near_file_; FILE* out_file_; @@ -261,7 +262,7 @@ ApmTest::ApmTest() #elif defined(WEBRTC_AUDIOPROC_FLOAT_PROFILE) ref_filename_(ref_path_ + "output_data_float.pb"), #endif - apm_(NULL), + apm_(AudioProcessing::Create(0)), frame_(NULL), revframe_(NULL), far_file_(NULL), @@ -269,8 +270,7 @@ ApmTest::ApmTest() out_file_(NULL) {} void ApmTest::SetUp() { - apm_ = AudioProcessing::Create(0); - ASSERT_TRUE(apm_ != NULL); + ASSERT_TRUE(apm_.get() != NULL); frame_ = new AudioFrame(); revframe_ = new AudioFrame(); @@ -303,11 +303,6 @@ void ApmTest::TearDown() { ASSERT_EQ(0, fclose(out_file_)); } out_file_ = NULL; - - if (apm_ != NULL) { - AudioProcessing::Destroy(apm_); - } - apm_ = NULL; } std::string ApmTest::ResourceFilePath(std::string name, int sample_rate_hz) { @@ -466,8 +461,8 @@ void ApmTest::ChangeTriggersInit(F f, AudioProcessing* ap, int initial_value, EXPECT_FALSE(FrameDataAreEqual(*frame_, frame_copy)); // Test that a change in value triggers an init. - f(apm_, changed_value); - f(apm_, initial_value); + f(apm_.get(), changed_value); + f(apm_.get(), initial_value); ProcessWithDefaultStreamParameters(&frame_copy); EXPECT_TRUE(FrameDataAreEqual(*frame_, frame_copy)); @@ -492,7 +487,7 @@ void ApmTest::ChangeTriggersInit(F f, AudioProcessing* ap, int initial_value, apm_->Initialize(); ProcessWithDefaultStreamParameters(&frame_copy); // Test that the same value does not trigger an init. - f(apm_, initial_value); + f(apm_.get(), initial_value); ProcessWithDefaultStreamParameters(&frame_copy); EXPECT_TRUE(FrameDataAreEqual(*frame_, frame_copy)); } @@ -737,15 +732,15 @@ void SetNumOutputChannels(AudioProcessing* ap, int value) { } TEST_F(ApmTest, SampleRateChangeTriggersInit) { - ChangeTriggersInit(SetSampleRate, apm_, 16000, 8000); + ChangeTriggersInit(SetSampleRate, apm_.get(), 16000, 8000); } TEST_F(ApmTest, ReverseChannelChangeTriggersInit) { - ChangeTriggersInit(SetNumReverseChannels, apm_, 2, 1); + ChangeTriggersInit(SetNumReverseChannels, apm_.get(), 2, 1); } TEST_F(ApmTest, ChannelChangeTriggersInit) { - ChangeTriggersInit(SetNumOutputChannels, apm_, 2, 1); + ChangeTriggersInit(SetNumOutputChannels, apm_.get(), 2, 1); } TEST_F(ApmTest, EchoCancellation) { diff --git a/webrtc/modules/audio_processing/test/process_test.cc b/webrtc/modules/audio_processing/test/process_test.cc index 8ab2be7e6f..cab93fea76 100644 --- a/webrtc/modules/audio_processing/test/process_test.cc +++ b/webrtc/modules/audio_processing/test/process_test.cc @@ -38,6 +38,7 @@ using webrtc::EchoCancellation; using webrtc::GainControl; using webrtc::NoiseSuppression; using webrtc::scoped_array; +using webrtc::scoped_ptr; using webrtc::TickInterval; using webrtc::TickTime; using webrtc::VoiceDetection; @@ -167,8 +168,8 @@ void void_main(int argc, char* argv[]) { printf("Try `process_test --help' for more information.\n\n"); } - AudioProcessing* apm = AudioProcessing::Create(0); - ASSERT_TRUE(apm != NULL); + scoped_ptr apm(AudioProcessing::Create(0)); + ASSERT_TRUE(apm.get() != NULL); const char* pb_filename = NULL; const char* far_filename = NULL; @@ -1057,9 +1058,6 @@ void void_main(int argc, char* argv[]) { printf("Warning: no capture frames\n"); } } - - AudioProcessing::Destroy(apm); - apm = NULL; } } // namespace diff --git a/webrtc/voice_engine/channel.cc b/webrtc/voice_engine/channel.cc index b1a2aa6c2d..eda0ddcf49 100644 --- a/webrtc/voice_engine/channel.cc +++ b/webrtc/voice_engine/channel.cc @@ -53,11 +53,10 @@ Channel::SendData(FrameType frameType, if (_includeAudioLevelIndication) { - assert(_rtpAudioProc.get() != NULL); // Store current audio level in the RTP/RTCP module. // The level will be used in combination with voice-activity state // (frameType) to add an RTP header extension - _rtpRtcpModule->SetAudioLevel(_rtpAudioProc->level_estimator()->RMS()); + _rtpRtcpModule->SetAudioLevel(rtp_audioproc_->level_estimator()->RMS()); } // Push data from ACM to RTP/RTCP-module to deliver audio frame for @@ -960,8 +959,8 @@ Channel::Channel(int32_t channelId, _callbackCritSectPtr(NULL), _transportPtr(NULL), _encryptionPtr(NULL), - _rtpAudioProc(NULL), - _rxAudioProcessingModulePtr(NULL), + rtp_audioproc_(NULL), + rx_audioproc_(AudioProcessing::Create(VoEModuleId(instanceId, channelId))), _rxVadObserverPtr(NULL), _oldVadDecision(-1), _sendFrameType(0), @@ -1025,10 +1024,6 @@ Channel::Channel(int32_t channelId, configuration.receive_statistics = rtp_receive_statistics_.get(); _rtpRtcpModule.reset(RtpRtcp::CreateRtpRtcp(configuration)); - - // Create far end AudioProcessing Module - _rxAudioProcessingModulePtr = AudioProcessing::Create( - VoEModuleId(instanceId, channelId)); } Channel::~Channel() @@ -1100,11 +1095,6 @@ Channel::~Channel() // Destroy modules AudioCodingModule::Destroy(&_audioCodingModule); - if (_rxAudioProcessingModulePtr != NULL) - { - AudioProcessing::Destroy(_rxAudioProcessingModulePtr); // far end APM - _rxAudioProcessingModulePtr = NULL; - } // End of modules shutdown @@ -1277,19 +1267,8 @@ Channel::Init() #endif } - // Initialize the far end AP module - // Using 8 kHz as initial Fs, the same as in transmission. Might be - // changed at the first receiving audio. - if (_rxAudioProcessingModulePtr == NULL) - { - _engineStatisticsPtr->SetLastError( - VE_NO_MEMORY, kTraceCritical, - "Channel::Init() failed to create the far-end AudioProcessing" - " module"); - return -1; - } - if (_rxAudioProcessingModulePtr->set_sample_rate_hz(8000)) + if (rx_audioproc_->set_sample_rate_hz(8000)) { _engineStatisticsPtr->SetLastError( VE_APM_ERROR, kTraceWarning, @@ -1297,14 +1276,14 @@ Channel::Init() " far-end AP module"); } - if (_rxAudioProcessingModulePtr->set_num_channels(1, 1) != 0) + if (rx_audioproc_->set_num_channels(1, 1) != 0) { _engineStatisticsPtr->SetLastError( VE_SOUNDCARD_ERROR, kTraceWarning, "Init() failed to set channels for the primary audio stream"); } - if (_rxAudioProcessingModulePtr->high_pass_filter()->Enable( + if (rx_audioproc_->high_pass_filter()->Enable( WEBRTC_VOICE_ENGINE_RX_HP_DEFAULT_STATE) != 0) { _engineStatisticsPtr->SetLastError( @@ -1313,7 +1292,7 @@ Channel::Init() " far-end AP module"); } - if (_rxAudioProcessingModulePtr->noise_suppression()->set_level( + if (rx_audioproc_->noise_suppression()->set_level( (NoiseSuppression::Level)WEBRTC_VOICE_ENGINE_RX_NS_DEFAULT_MODE) != 0) { _engineStatisticsPtr->SetLastError( @@ -1321,7 +1300,7 @@ Channel::Init() "Init() failed to set noise reduction level for far-end" " AP module"); } - if (_rxAudioProcessingModulePtr->noise_suppression()->Enable( + if (rx_audioproc_->noise_suppression()->Enable( WEBRTC_VOICE_ENGINE_RX_NS_DEFAULT_STATE) != 0) { _engineStatisticsPtr->SetLastError( @@ -1330,14 +1309,14 @@ Channel::Init() " AP module"); } - if (_rxAudioProcessingModulePtr->gain_control()->set_mode( + if (rx_audioproc_->gain_control()->set_mode( (GainControl::Mode)WEBRTC_VOICE_ENGINE_RX_AGC_DEFAULT_MODE) != 0) { _engineStatisticsPtr->SetLastError( VE_APM_ERROR, kTraceWarning, "Init() failed to set AGC mode for far-end AP module"); } - if (_rxAudioProcessingModulePtr->gain_control()->Enable( + if (rx_audioproc_->gain_control()->Enable( WEBRTC_VOICE_ENGINE_RX_AGC_DEFAULT_STATE) != 0) { _engineStatisticsPtr->SetLastError( @@ -3317,7 +3296,7 @@ Channel::SetRxAgcStatus(bool enable, AgcModes mode) agcMode = GainControl::kAdaptiveDigital; break; case kAgcUnchanged: - agcMode = _rxAudioProcessingModulePtr->gain_control()->mode(); + agcMode = rx_audioproc_->gain_control()->mode(); break; case kAgcFixedDigital: agcMode = GainControl::kFixedDigital; @@ -3332,14 +3311,14 @@ Channel::SetRxAgcStatus(bool enable, AgcModes mode) return -1; } - if (_rxAudioProcessingModulePtr->gain_control()->set_mode(agcMode) != 0) + if (rx_audioproc_->gain_control()->set_mode(agcMode) != 0) { _engineStatisticsPtr->SetLastError( VE_APM_ERROR, kTraceError, "SetRxAgcStatus() failed to set Agc mode"); return -1; } - if (_rxAudioProcessingModulePtr->gain_control()->Enable(enable) != 0) + if (rx_audioproc_->gain_control()->Enable(enable) != 0) { _engineStatisticsPtr->SetLastError( VE_APM_ERROR, kTraceError, @@ -3359,9 +3338,9 @@ Channel::GetRxAgcStatus(bool& enabled, AgcModes& mode) WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId,_channelId), "Channel::GetRxAgcStatus(enable=?, mode=?)"); - bool enable = _rxAudioProcessingModulePtr->gain_control()->is_enabled(); + bool enable = rx_audioproc_->gain_control()->is_enabled(); GainControl::Mode agcMode = - _rxAudioProcessingModulePtr->gain_control()->mode(); + rx_audioproc_->gain_control()->mode(); enabled = enable; @@ -3389,7 +3368,7 @@ Channel::SetRxAgcConfig(AgcConfig config) WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId,_channelId), "Channel::SetRxAgcConfig()"); - if (_rxAudioProcessingModulePtr->gain_control()->set_target_level_dbfs( + if (rx_audioproc_->gain_control()->set_target_level_dbfs( config.targetLeveldBOv) != 0) { _engineStatisticsPtr->SetLastError( @@ -3398,7 +3377,7 @@ Channel::SetRxAgcConfig(AgcConfig config) "(or envelope) of the Agc"); return -1; } - if (_rxAudioProcessingModulePtr->gain_control()->set_compression_gain_db( + if (rx_audioproc_->gain_control()->set_compression_gain_db( config.digitalCompressionGaindB) != 0) { _engineStatisticsPtr->SetLastError( @@ -3407,7 +3386,7 @@ Channel::SetRxAgcConfig(AgcConfig config) " digital compression stage may apply"); return -1; } - if (_rxAudioProcessingModulePtr->gain_control()->enable_limiter( + if (rx_audioproc_->gain_control()->enable_limiter( config.limiterEnable) != 0) { _engineStatisticsPtr->SetLastError( @@ -3426,11 +3405,11 @@ Channel::GetRxAgcConfig(AgcConfig& config) "Channel::GetRxAgcConfig(config=%?)"); config.targetLeveldBOv = - _rxAudioProcessingModulePtr->gain_control()->target_level_dbfs(); + rx_audioproc_->gain_control()->target_level_dbfs(); config.digitalCompressionGaindB = - _rxAudioProcessingModulePtr->gain_control()->compression_gain_db(); + rx_audioproc_->gain_control()->compression_gain_db(); config.limiterEnable = - _rxAudioProcessingModulePtr->gain_control()->is_limiter_enabled(); + rx_audioproc_->gain_control()->is_limiter_enabled(); WEBRTC_TRACE(kTraceStateInfo, kTraceVoice, VoEId(_instanceId,_channelId), "GetRxAgcConfig() => " @@ -3464,7 +3443,7 @@ Channel::SetRxNsStatus(bool enable, NsModes mode) WEBRTC_VOICE_ENGINE_RX_NS_DEFAULT_MODE; break; case kNsUnchanged: - nsLevel = _rxAudioProcessingModulePtr->noise_suppression()->level(); + nsLevel = rx_audioproc_->noise_suppression()->level(); break; case kNsConference: nsLevel = NoiseSuppression::kHigh; @@ -3483,7 +3462,7 @@ Channel::SetRxNsStatus(bool enable, NsModes mode) break; } - if (_rxAudioProcessingModulePtr->noise_suppression()->set_level(nsLevel) + if (rx_audioproc_->noise_suppression()->set_level(nsLevel) != 0) { _engineStatisticsPtr->SetLastError( @@ -3491,7 +3470,7 @@ Channel::SetRxNsStatus(bool enable, NsModes mode) "SetRxAgcStatus() failed to set Ns level"); return -1; } - if (_rxAudioProcessingModulePtr->noise_suppression()->Enable(enable) != 0) + if (rx_audioproc_->noise_suppression()->Enable(enable) != 0) { _engineStatisticsPtr->SetLastError( VE_APM_ERROR, kTraceError, @@ -3512,9 +3491,9 @@ Channel::GetRxNsStatus(bool& enabled, NsModes& mode) "Channel::GetRxNsStatus(enable=?, mode=?)"); bool enable = - _rxAudioProcessingModulePtr->noise_suppression()->is_enabled(); + rx_audioproc_->noise_suppression()->is_enabled(); NoiseSuppression::Level ncLevel = - _rxAudioProcessingModulePtr->noise_suppression()->level(); + rx_audioproc_->noise_suppression()->level(); enabled = enable; @@ -3702,34 +3681,28 @@ Channel::GetRemoteCSRCs(unsigned int arrCSRC[15]) int Channel::SetRTPAudioLevelIndicationStatus(bool enable, unsigned char ID) { - if (_rtpAudioProc.get() == NULL) - { - _rtpAudioProc.reset(AudioProcessing::Create(VoEModuleId(_instanceId, - _channelId))); - if (_rtpAudioProc.get() == NULL) - { - _engineStatisticsPtr->SetLastError(VE_NO_MEMORY, kTraceCritical, - "Failed to create AudioProcessing"); - return -1; - } - } + if (rtp_audioproc_.get() == NULL) { + rtp_audioproc_.reset(AudioProcessing::Create(VoEModuleId(_instanceId, + _channelId))); + } - if (_rtpAudioProc->level_estimator()->Enable(enable) != - AudioProcessing::kNoError) - { - _engineStatisticsPtr->SetLastError(VE_APM_ERROR, kTraceWarning, - "Failed to enable AudioProcessing::level_estimator()"); - } + if (rtp_audioproc_->level_estimator()->Enable(enable) != + AudioProcessing::kNoError) { + _engineStatisticsPtr->SetLastError(VE_APM_ERROR, kTraceError, + "Failed to enable AudioProcessing::level_estimator()"); + return -1; + } - _includeAudioLevelIndication = enable; - if (enable) { - rtp_header_parser_->RegisterRtpHeaderExtension(kRtpExtensionAudioLevel, - ID); - } else { - rtp_header_parser_->DeregisterRtpHeaderExtension(kRtpExtensionAudioLevel); - } - return _rtpRtcpModule->SetRTPAudioLevelIndicationStatus(enable, ID); + _includeAudioLevelIndication = enable; + if (enable) { + rtp_header_parser_->RegisterRtpHeaderExtension(kRtpExtensionAudioLevel, + ID); + } else { + rtp_header_parser_->DeregisterRtpHeaderExtension(kRtpExtensionAudioLevel); + } + return _rtpRtcpModule->SetRTPAudioLevelIndicationStatus(enable, ID); } + int Channel::GetRTPAudioLevelIndicationStatus(bool& enabled, unsigned char& ID) { @@ -4506,36 +4479,27 @@ Channel::PrepareEncodeAndSend(int mixingFrequency) if (_includeAudioLevelIndication) { - assert(_rtpAudioProc.get() != NULL); - - // Check if settings need to be updated. - if (_rtpAudioProc->sample_rate_hz() != _audioFrame.sample_rate_hz_) + if (rtp_audioproc_->set_sample_rate_hz(_audioFrame.sample_rate_hz_) != + AudioProcessing::kNoError) { - if (_rtpAudioProc->set_sample_rate_hz(_audioFrame.sample_rate_hz_) != - AudioProcessing::kNoError) - { - WEBRTC_TRACE(kTraceWarning, kTraceVoice, - VoEId(_instanceId, _channelId), - "Error setting AudioProcessing sample rate"); - return -1; - } + WEBRTC_TRACE(kTraceWarning, kTraceVoice, + VoEId(_instanceId, _channelId), + "Error setting AudioProcessing sample rate"); + return -1; } - if (_rtpAudioProc->num_input_channels() != _audioFrame.num_channels_) + if (rtp_audioproc_->set_num_channels(_audioFrame.num_channels_, + _audioFrame.num_channels_) != + AudioProcessing::kNoError) { - if (_rtpAudioProc->set_num_channels(_audioFrame.num_channels_, - _audioFrame.num_channels_) - != AudioProcessing::kNoError) - { - WEBRTC_TRACE(kTraceWarning, kTraceVoice, - VoEId(_instanceId, _channelId), - "Error setting AudioProcessing channels"); - return -1; - } + WEBRTC_TRACE(kTraceWarning, kTraceVoice, + VoEId(_instanceId, _channelId), + "Error setting AudioProcessing channels"); + return -1; } // Performs level analysis only; does not affect the signal. - _rtpAudioProc->ProcessStream(&_audioFrame); + rtp_audioproc_->ProcessStream(&_audioFrame); } return 0; @@ -5271,16 +5235,15 @@ Channel::RegisterReceiveCodecsToRTPModule() } int Channel::ApmProcessRx(AudioFrame& frame) { - AudioProcessing* audioproc = _rxAudioProcessingModulePtr; // Register the (possibly new) frame parameters. - if (audioproc->set_sample_rate_hz(frame.sample_rate_hz_) != 0) { + if (rx_audioproc_->set_sample_rate_hz(frame.sample_rate_hz_) != 0) { LOG_FERR1(LS_WARNING, set_sample_rate_hz, frame.sample_rate_hz_); } - if (audioproc->set_num_channels(frame.num_channels_, - frame.num_channels_) != 0) { + if (rx_audioproc_->set_num_channels(frame.num_channels_, + frame.num_channels_) != 0) { LOG_FERR1(LS_WARNING, set_num_channels, frame.num_channels_); } - if (audioproc->ProcessStream(&frame) != 0) { + if (rx_audioproc_->ProcessStream(&frame) != 0) { LOG_FERR0(LS_WARNING, ProcessStream); } return 0; diff --git a/webrtc/voice_engine/channel.h b/webrtc/voice_engine/channel.h index 4d9146eb1b..48e57469a8 100644 --- a/webrtc/voice_engine/channel.h +++ b/webrtc/voice_engine/channel.h @@ -520,8 +520,8 @@ private: CriticalSectionWrapper* _callbackCritSectPtr; // owned by base Transport* _transportPtr; // WebRtc socket or external transport Encryption* _encryptionPtr; // WebRtc SRTP or external encryption - scoped_ptr _rtpAudioProc; - AudioProcessing* _rxAudioProcessingModulePtr; // far end AudioProcessing + scoped_ptr rtp_audioproc_; + scoped_ptr rx_audioproc_; // far end AudioProcessing VoERxVadCallback* _rxVadObserverPtr; int32_t _oldVadDecision; int32_t _sendFrameType; // Send data is voice, 1-voice, 0-otherwise