generic-library/webrtc
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

VoE: cleanup VoENetwork implementation

Browse Source 
Changes:
1. Documented return values of VoENetwork methods.
2. In VoENetworkImpl: replaced calls to SetLastError() with LOG_F(). SetLastError() is not used anymore because no one is calling LastError() to check for last error. Also, its usage is being removed in Video Engine and we want to be consistent.
3. In VoENetworkImpl: removed WEBRTC_TRACE() usage.
4. In VoENetworkImpl: replaced some defensive code with assert(). We are now assuming that the caller has called VoEBase::Init() where needed. We are also assuming that it is invalid to pass nullptr where data is expected.
5. Updated unit tests accordingly.

R=henrika@webrtc.org, kwiberg@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/53369004

Cr-Commit-Position: refs/heads/master@{#9145}
This commit is contained in:
Jelena Marusic
2015-05-06 15:04:22 +02:00
parent 1ff218fac3
commit f353dd59b5
6 changed files with 36 additions and 109 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
webrtc/voice_engine/include/voe_network.h
View File

@@ -55,16 +55,17 @@ class WEBRTC_DLLEXPORT VoENetwork {
virtual int Release() = 0; virtual int Release() = 0;
// Installs and enables a user-defined external transport protocol for a // Installs and enables a user-defined external transport protocol for a
// specified |channel|. // specified |channel|. Returns -1 in case of an error, 0 otherwise.
virtual int RegisterExternalTransport(int channel, Transport& transport) = 0; virtual int RegisterExternalTransport(int channel, Transport& transport) = 0;
// Removes and disables a user-defined external transport protocol for a // Removes and disables a user-defined external transport protocol for a
// specified |channel|. // specified |channel|. Returns -1 in case of an error, 0 otherwise.
virtual int DeRegisterExternalTransport(int channel) = 0; virtual int DeRegisterExternalTransport(int channel) = 0;
// The packets received from the network should be passed to this // The packets received from the network should be passed to this
// function when external transport is enabled. Note that the data // function when external transport is enabled. Note that the data
// including the RTP-header must also be given to the VoiceEngine. // including the RTP-header must also be given to the VoiceEngine.
// Returns -1 in case of an error, 0 otherwise.
virtual int ReceivedRTPPacket(int channel, virtual int ReceivedRTPPacket(int channel,
const void* data, const void* data,
size_t length) = 0; size_t length) = 0;
@@ -78,6 +79,7 @@ class WEBRTC_DLLEXPORT VoENetwork {
// The packets received from the network should be passed to this // The packets received from the network should be passed to this
// function when external transport is enabled. Note that the data // function when external transport is enabled. Note that the data
// including the RTCP-header must also be given to the VoiceEngine. // including the RTCP-header must also be given to the VoiceEngine.
// Returns -1 in case of an error, 0 otherwise.
virtual int ReceivedRTCPPacket(int channel, virtual int ReceivedRTCPPacket(int channel,
const void* data, const void* data,
size_t length) = 0; size_t length) = 0;

17
webrtc/voice_engine/voe_base_impl.cc
View File

@@ -325,29 +325,30 @@ int VoEBaseImpl::Init(AudioDeviceModule* external_adm,
shared_->SetLastError(VE_APM_ERROR); shared_->SetLastError(VE_APM_ERROR);
// Configure AudioProcessing components. // Configure AudioProcessing components.
if (audioproc->high_pass_filter()->Enable(true) != 0) { if (audioproc->high_pass_filter()->Enable(true) != 0) {
LOG_FERR1(LS_ERROR, high_pass_filter()->Enable, true); LOG_F(LS_ERROR) << "Failed to enable high pass filter.";
return -1; return -1;
} }
if (audioproc->echo_cancellation()->enable_drift_compensation(false) != 0) { if (audioproc->echo_cancellation()->enable_drift_compensation(false) != 0) {
LOG_FERR1(LS_ERROR, enable_drift_compensation, false); LOG_F(LS_ERROR) << "Failed to disable drift compensation.";
return -1; return -1;
} }
if (audioproc->noise_suppression()->set_level(kDefaultNsMode) != 0) { if (audioproc->noise_suppression()->set_level(kDefaultNsMode) != 0) {
LOG_FERR1(LS_ERROR, noise_suppression()->set_level, kDefaultNsMode); LOG_F(LS_ERROR) << "Failed to set noise suppression level: "
<< kDefaultNsMode;
return -1; return -1;
} }
GainControl* agc = audioproc->gain_control(); GainControl* agc = audioproc->gain_control();
if (agc->set_analog_level_limits(kMinVolumeLevel, kMaxVolumeLevel) != 0) { if (agc->set_analog_level_limits(kMinVolumeLevel, kMaxVolumeLevel) != 0) {
LOG_FERR2(LS_ERROR, agc->set_analog_level_limits, kMinVolumeLevel, LOG_F(LS_ERROR) << "Failed to set analog level limits with minimum: "
kMaxVolumeLevel); << kMinVolumeLevel << " and maximum: " << kMaxVolumeLevel;
return -1; return -1;
} }
if (agc->set_mode(kDefaultAgcMode) != 0) { if (agc->set_mode(kDefaultAgcMode) != 0) {
LOG_FERR1(LS_ERROR, agc->set_mode, kDefaultAgcMode); LOG_F(LS_ERROR) << "Failed to set mode: " << kDefaultAgcMode;
return -1; return -1;
} }
if (agc->Enable(kDefaultAgcState) != 0) { if (agc->Enable(kDefaultAgcState) != 0) {
LOG_FERR1(LS_ERROR, agc->Enable, kDefaultAgcState); LOG_F(LS_ERROR) << "Failed to set agc state: " << kDefaultAgcState;
return -1; return -1;
} }
shared_->SetLastError(0); // Clear error state. shared_->SetLastError(0); // Clear error state.
@@ -356,7 +357,7 @@ int VoEBaseImpl::Init(AudioDeviceModule* external_adm,
bool agc_enabled = bool agc_enabled =
agc->mode() == GainControl::kAdaptiveAnalog && agc->is_enabled(); agc->mode() == GainControl::kAdaptiveAnalog && agc->is_enabled();
if (shared_->audio_device()->SetAGC(agc_enabled) != 0) { if (shared_->audio_device()->SetAGC(agc_enabled) != 0) {
LOG_FERR1(LS_ERROR, audio_device()->SetAGC, agc_enabled); LOG_F(LS_ERROR) << "Failed to set agc to enabled: " << agc_enabled;
shared_->SetLastError(VE_AUDIO_DEVICE_MODULE_ERROR); shared_->SetLastError(VE_AUDIO_DEVICE_MODULE_ERROR);
// TODO(ajm): No error return here due to // TODO(ajm): No error return here due to
// https://code.google.com/p/webrtc/issues/detail?id=1464 // https://code.google.com/p/webrtc/issues/detail?id=1464

92
webrtc/voice_engine/voe_network_impl.cc
View File

@@ -10,6 +10,7 @@
#include "webrtc/voice_engine/voe_network_impl.h" #include "webrtc/voice_engine/voe_network_impl.h"
#include "webrtc/base/checks.h"
#include "webrtc/base/format_macros.h" #include "webrtc/base/format_macros.h"
#include "webrtc/system_wrappers/interface/critical_section_wrapper.h" #include "webrtc/system_wrappers/interface/critical_section_wrapper.h"
#include "webrtc/system_wrappers/interface/logging.h" #include "webrtc/system_wrappers/interface/logging.h"
@@ -21,8 +22,8 @@
namespace webrtc { namespace webrtc {
VoENetwork* VoENetwork::GetInterface(VoiceEngine* voiceEngine) { VoENetwork* VoENetwork::GetInterface(VoiceEngine* voiceEngine) {
if (NULL == voiceEngine) { if (!voiceEngine) {
return NULL; return nullptr;
} }
VoiceEngineImpl* s = static_cast<VoiceEngineImpl*>(voiceEngine); VoiceEngineImpl* s = static_cast<VoiceEngineImpl*>(voiceEngine);
s->AddRef(); s->AddRef();
@@ -30,47 +31,28 @@ VoENetwork* VoENetwork::GetInterface(VoiceEngine* voiceEngine) {
} }
VoENetworkImpl::VoENetworkImpl(voe::SharedData* shared) : _shared(shared) { VoENetworkImpl::VoENetworkImpl(voe::SharedData* shared) : _shared(shared) {
WEBRTC_TRACE(kTraceMemory, kTraceVoice, VoEId(_shared->instance_id(), -1),
"VoENetworkImpl() - ctor");
} }
VoENetworkImpl::~VoENetworkImpl() { VoENetworkImpl::~VoENetworkImpl() = default;
WEBRTC_TRACE(kTraceMemory, kTraceVoice, VoEId(_shared->instance_id(), -1),
"~VoENetworkImpl() - dtor");
}
int VoENetworkImpl::RegisterExternalTransport(int channel, int VoENetworkImpl::RegisterExternalTransport(int channel,
Transport& transport) { Transport& transport) {
WEBRTC_TRACE(kTraceApiCall, kTraceVoice, VoEId(_shared->instance_id(), -1), DCHECK(_shared->statistics().Initialized());
"SetExternalTransport(channel=%d, transport=0x%x)", channel,
&transport);
if (!_shared->statistics().Initialized()) {
_shared->SetLastError(VE_NOT_INITED, kTraceError);
return -1;
}
voe::ChannelOwner ch = _shared->channel_manager().GetChannel(channel); voe::ChannelOwner ch = _shared->channel_manager().GetChannel(channel);
voe::Channel* channelPtr = ch.channel(); voe::Channel* channelPtr = ch.channel();
if (channelPtr == NULL) { if (!channelPtr) {
_shared->SetLastError(VE_CHANNEL_NOT_VALID, kTraceError, LOG_F(LS_ERROR) << "Failed to locate channel: " << channel;
"SetExternalTransport() failed to locate channel");
return -1; return -1;
} }
return channelPtr->RegisterExternalTransport(transport); return channelPtr->RegisterExternalTransport(transport);
} }
int VoENetworkImpl::DeRegisterExternalTransport(int channel) { int VoENetworkImpl::DeRegisterExternalTransport(int channel) {
WEBRTC_TRACE(kTraceApiCall, kTraceVoice, VoEId(_shared->instance_id(), -1), CHECK(_shared->statistics().Initialized());
"DeRegisterExternalTransport(channel=%d)", channel);
if (!_shared->statistics().Initialized()) {
WEBRTC_TRACE(kTraceError, kTraceVoice, VoEId(_shared->instance_id(), -1),
"DeRegisterExternalTransport() - invalid state");
}
voe::ChannelOwner ch = _shared->channel_manager().GetChannel(channel); voe::ChannelOwner ch = _shared->channel_manager().GetChannel(channel);
voe::Channel* channelPtr = ch.channel(); voe::Channel* channelPtr = ch.channel();
if (channelPtr == NULL) { if (!channelPtr) {
_shared->SetLastError( LOG_F(LS_ERROR) << "Failed to locate channel: " << channel;
VE_CHANNEL_NOT_VALID, kTraceError,
"DeRegisterExternalTransport() failed to locate channel");
return -1; return -1;
} }
return channelPtr->DeRegisterExternalTransport(); return channelPtr->DeRegisterExternalTransport();
@@ -86,36 +68,21 @@ int VoENetworkImpl::ReceivedRTPPacket(int channel,
const void* data, const void* data,
size_t length, size_t length,
const PacketTime& packet_time) { const PacketTime& packet_time) {
WEBRTC_TRACE(kTraceStream, kTraceVoice, VoEId(_shared->instance_id(), -1), CHECK(_shared->statistics().Initialized());
"ReceivedRTPPacket(channel=%d, length=%" PRIuS ")", channel, CHECK(data);
length);
if (!_shared->statistics().Initialized()) {
_shared->SetLastError(VE_NOT_INITED, kTraceError);
return -1;
}
// L16 at 32 kHz, stereo, 10 ms frames (+12 byte RTP header) -> 1292 bytes // L16 at 32 kHz, stereo, 10 ms frames (+12 byte RTP header) -> 1292 bytes
if ((length < 12) || (length > 1292)) { if ((length < 12) || (length > 1292)) {
_shared->SetLastError(VE_INVALID_PACKET); LOG_F(LS_ERROR) << "Invalid packet length: " << length;
LOG(LS_ERROR) << "Invalid packet length: " << length;
return -1;
}
if (NULL == data) {
_shared->SetLastError(VE_INVALID_ARGUMENT, kTraceError,
"ReceivedRTPPacket() invalid data vector");
return -1; return -1;
} }
voe::ChannelOwner ch = _shared->channel_manager().GetChannel(channel); voe::ChannelOwner ch = _shared->channel_manager().GetChannel(channel);
voe::Channel* channelPtr = ch.channel(); voe::Channel* channelPtr = ch.channel();
if (channelPtr == NULL) { if (!channelPtr) {
_shared->SetLastError(VE_CHANNEL_NOT_VALID, kTraceError, LOG_F(LS_ERROR) << "Failed to locate channel: " << channel;
"ReceivedRTPPacket() failed to locate channel");
return -1; return -1;
} }
if (!channelPtr->ExternalTransport()) { if (!channelPtr->ExternalTransport()) {
_shared->SetLastError( LOG_F(LS_ERROR) << "No external transport for channel: " << channel;
VE_INVALID_OPERATION, kTraceError,
"ReceivedRTPPacket() external transport is not enabled");
return -1; return -1;
} }
return channelPtr->ReceivedRTPPacket((const int8_t*)data, length, return channelPtr->ReceivedRTPPacket((const int8_t*)data, length,
@@ -125,36 +92,23 @@ int VoENetworkImpl::ReceivedRTPPacket(int channel,
int VoENetworkImpl::ReceivedRTCPPacket(int channel, int VoENetworkImpl::ReceivedRTCPPacket(int channel,
const void* data, const void* data,
size_t length) { size_t length) {
WEBRTC_TRACE(kTraceStream, kTraceVoice, VoEId(_shared->instance_id(), -1), CHECK(_shared->statistics().Initialized());
"ReceivedRTCPPacket(channel=%d, length=%" PRIuS ")", channel, CHECK(data);
length);
if (!_shared->statistics().Initialized()) {
_shared->SetLastError(VE_NOT_INITED, kTraceError);
return -1;
}
if (length < 4) { if (length < 4) {
_shared->SetLastError(VE_INVALID_PACKET, kTraceError, LOG_F(LS_ERROR) << "Invalid packet length: " << length;
"ReceivedRTCPPacket() invalid packet length");
return -1;
}
if (NULL == data) {
_shared->SetLastError(VE_INVALID_ARGUMENT, kTraceError,
"ReceivedRTCPPacket() invalid data vector");
return -1; return -1;
} }
voe::ChannelOwner ch = _shared->channel_manager().GetChannel(channel); voe::ChannelOwner ch = _shared->channel_manager().GetChannel(channel);
voe::Channel* channelPtr = ch.channel(); voe::Channel* channelPtr = ch.channel();
if (channelPtr == NULL) { if (!channelPtr) {
_shared->SetLastError(VE_CHANNEL_NOT_VALID, kTraceError, LOG_F(LS_ERROR) << "Failed to locate channel: " << channel;
"ReceivedRTCPPacket() failed to locate channel");
return -1; return -1;
} }
if (!channelPtr->ExternalTransport()) { if (!channelPtr->ExternalTransport()) {
_shared->SetLastError( LOG_F(LS_ERROR) << "No external transport for channel: " << channel;
VE_INVALID_OPERATION, kTraceError,
"ReceivedRTCPPacket() external transport is not enabled");
return -1; return -1;
} }
return channelPtr->ReceivedRTCPPacket((const int8_t*)data, length); return channelPtr->ReceivedRTCPPacket((const int8_t*)data, length);
} }
} // namespace webrtc } // namespace webrtc

1
webrtc/voice_engine/voe_network_impl.h
View File

@@ -20,7 +20,6 @@ namespace webrtc {
class VoENetworkImpl : public VoENetwork { class VoENetworkImpl : public VoENetwork {
public: public:
int RegisterExternalTransport(int channel, Transport& transport) override; int RegisterExternalTransport(int channel, Transport& transport) override;
int DeRegisterExternalTransport(int channel) override; int DeRegisterExternalTransport(int channel) override;
int ReceivedRTPPacket(int channel, const void* data, size_t length) override; int ReceivedRTPPacket(int channel, const void* data, size_t length) override;

28
webrtc/voice_engine/voe_network_unittest.cc
View File

@@ -45,15 +45,6 @@ TEST_F(VoENetworkTest, RegisterAndDeRegisterExternalTransport) {
EXPECT_EQ(0, network_->DeRegisterExternalTransport(channelID)); EXPECT_EQ(0, network_->DeRegisterExternalTransport(channelID));
} }
TEST_F(VoENetworkTest, RegisterExternalTransportBeforeInitShouldFail) {
EXPECT_NE(
0, network_->RegisterExternalTransport(kNonExistingChannel, transport_));
}
TEST_F(VoENetworkTest, DeRegisterExternalTransportBeforeInitShouldFail) {
EXPECT_NE(0, network_->DeRegisterExternalTransport(kNonExistingChannel));
}
TEST_F(VoENetworkTest, TEST_F(VoENetworkTest,
RegisterExternalTransportOnNonExistingChannelShouldFail) { RegisterExternalTransportOnNonExistingChannelShouldFail) {
EXPECT_EQ(0, base_->Init(&adm_, nullptr)); EXPECT_EQ(0, base_->Init(&adm_, nullptr));
@@ -80,10 +71,6 @@ TEST_F(VoENetworkTest, ReceivedRTPPacketWithJunkDataShouldFail) {
sizeof(kPacketJunk))); sizeof(kPacketJunk)));
} }
TEST_F(VoENetworkTest, ReceivedRTPPacketBeforeInitShouldFail) {
EXPECT_EQ(-1, network_->ReceivedRTPPacket(0, kPacket, sizeof(kPacket)));
}
TEST_F(VoENetworkTest, ReceivedRTPPacketOnNonExistingChannelShouldFail) { TEST_F(VoENetworkTest, ReceivedRTPPacketOnNonExistingChannelShouldFail) {
EXPECT_EQ(0, base_->Init(&adm_, nullptr)); EXPECT_EQ(0, base_->Init(&adm_, nullptr));
EXPECT_EQ(-1, network_->ReceivedRTPPacket(kNonExistingChannel, kPacket, EXPECT_EQ(-1, network_->ReceivedRTPPacket(kNonExistingChannel, kPacket,
@@ -110,11 +97,6 @@ TEST_F(VoENetworkTest, ReceivedTooLargeRTPPacketShouldFail) {
channelID, kPacket, kMaxValidSizeOfRtpPacketInBytes + 1)); channelID, kPacket, kMaxValidSizeOfRtpPacketInBytes + 1));
} }
TEST_F(VoENetworkTest, ReceivedRTPPacketWithNullDataShouldFail) {
int channelID = CreateChannelAndRegisterExternalTransport();
EXPECT_EQ(-1, network_->ReceivedRTPPacket(channelID, nullptr, 0));
}
TEST_F(VoENetworkTest, ReceivedRTCPPacketWithJunkDataShouldFail) { TEST_F(VoENetworkTest, ReceivedRTCPPacketWithJunkDataShouldFail) {
int channelID = CreateChannelAndRegisterExternalTransport(); int channelID = CreateChannelAndRegisterExternalTransport();
EXPECT_EQ(0, network_->ReceivedRTCPPacket(channelID, kPacketJunk, EXPECT_EQ(0, network_->ReceivedRTCPPacket(channelID, kPacketJunk,
@@ -122,11 +104,6 @@ TEST_F(VoENetworkTest, ReceivedRTCPPacketWithJunkDataShouldFail) {
EXPECT_EQ(VE_SOCKET_TRANSPORT_MODULE_ERROR, base_->LastError()); EXPECT_EQ(VE_SOCKET_TRANSPORT_MODULE_ERROR, base_->LastError());
} }
TEST_F(VoENetworkTest, ReceivedRTCPPacketBeforeInitShouldFail) {
EXPECT_EQ(-1, network_->ReceivedRTCPPacket(kNonExistingChannel, kPacket,
sizeof(kPacket)));
}
TEST_F(VoENetworkTest, ReceivedRTCPPacketOnNonExistingChannelShouldFail) { TEST_F(VoENetworkTest, ReceivedRTCPPacketOnNonExistingChannelShouldFail) {
EXPECT_EQ(0, base_->Init(&adm_, nullptr)); EXPECT_EQ(0, base_->Init(&adm_, nullptr));
EXPECT_EQ(-1, network_->ReceivedRTCPPacket(kNonExistingChannel, kPacket, EXPECT_EQ(-1, network_->ReceivedRTCPPacket(kNonExistingChannel, kPacket,
@@ -147,9 +124,4 @@ TEST_F(VoENetworkTest, ReceivedTooSmallRTCPPacket4ShouldFail) {
channelID, kPacket, kMinValidSizeOfRtcpPacketInBytes - 1)); channelID, kPacket, kMinValidSizeOfRtcpPacketInBytes - 1));
} }
TEST_F(VoENetworkTest, ReceivedRTCPPacketWithNullDataShouldFail) {
int channelID = CreateChannelAndRegisterExternalTransport();
EXPECT_EQ(-1, network_->ReceivedRTCPPacket(channelID, nullptr, 0));
}
} // namespace webrtc } // namespace webrtc

1
webrtc/voice_engine/voice_engine_defines.h
View File

@@ -19,7 +19,6 @@
#include "webrtc/common_types.h" #include "webrtc/common_types.h"
#include "webrtc/engine_configurations.h" #include "webrtc/engine_configurations.h"
#include "webrtc/modules/audio_processing/include/audio_processing.h" #include "webrtc/modules/audio_processing/include/audio_processing.h"
#include "webrtc/system_wrappers/interface/logging.h"
// ---------------------------------------------------------------------------- // ----------------------------------------------------------------------------
// Enumerators // Enumerators