From 9325d343e57b2a2cf86b73080c71644e61b3c46d Mon Sep 17 00:00:00 2001 From: Tim Na Date: Thu, 10 Dec 2020 14:01:24 -0800 Subject: [PATCH] Enforcing return type handling on VoIP API. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - This CL also affects some return type handling in Android Voip demo app due to changes in return type handling. Bug: webrtc:12193 Change-Id: Id76faf7c871476ed1f2d08fb587211ae234ae8d3 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/196625 Reviewed-by: Harald Alvestrand Reviewed-by: Per Åhgren Reviewed-by: Mirko Bonadei Commit-Queue: Tim Na Cr-Commit-Position: refs/heads/master@{#32821} --- api/voip/BUILD.gn | 1 + api/voip/DEPS | 4 ++ api/voip/voip_base.h | 3 +- audio/voip/test/voip_core_unittest.cc | 21 ++++++---- .../androidvoip/jni/android_voip_client.cc | 38 +++++++++---------- .../androidvoip/jni/android_voip_client.h | 2 +- 6 files changed, 39 insertions(+), 30 deletions(-) diff --git a/api/voip/BUILD.gn b/api/voip/BUILD.gn index c099bfbfaf..4db59fd98c 100644 --- a/api/voip/BUILD.gn +++ b/api/voip/BUILD.gn @@ -21,6 +21,7 @@ rtc_source_set("voip_api") { ] deps = [ "..:array_view", + "../../rtc_base/system:unused", "../audio_codecs:audio_codecs_api", "../neteq:neteq_api", ] diff --git a/api/voip/DEPS b/api/voip/DEPS index 3845dffab0..837b9a673e 100644 --- a/api/voip/DEPS +++ b/api/voip/DEPS @@ -3,6 +3,10 @@ specific_include_rules = { "+third_party/absl/types/optional.h", ], + "voip_base.h": [ + "+rtc_base/system/unused.h", + ], + "voip_engine_factory.h": [ "+modules/audio_device/include/audio_device.h", "+modules/audio_processing/include/audio_processing.h", diff --git a/api/voip/voip_base.h b/api/voip/voip_base.h index c5f54aa9e9..6a411f8d88 100644 --- a/api/voip/voip_base.h +++ b/api/voip/voip_base.h @@ -12,6 +12,7 @@ #define API_VOIP_VOIP_BASE_H_ #include "absl/types/optional.h" +#include "rtc_base/system/unused.h" namespace webrtc { @@ -35,7 +36,7 @@ class Transport; enum class ChannelId : int {}; -enum class VoipResult { +enum class RTC_WARN_UNUSED_RESULT VoipResult { // kOk indicates the function was successfully invoked with no error. kOk, // kInvalidArgument indicates the caller specified an invalid argument, such diff --git a/audio/voip/test/voip_core_unittest.cc b/audio/voip/test/voip_core_unittest.cc index f7a82f9018..d290bd6ec3 100644 --- a/audio/voip/test/voip_core_unittest.cc +++ b/audio/voip/test/voip_core_unittest.cc @@ -70,14 +70,18 @@ TEST_F(VoipCoreTest, BasicVoipCoreOperation) { auto channel = voip_core_->CreateChannel(&transport_, 0xdeadc0de); - voip_core_->SetSendCodec(channel, kPcmuPayload, kPcmuFormat); - voip_core_->SetReceiveCodecs(channel, {{kPcmuPayload, kPcmuFormat}}); + EXPECT_EQ(voip_core_->SetSendCodec(channel, kPcmuPayload, kPcmuFormat), + VoipResult::kOk); + EXPECT_EQ( + voip_core_->SetReceiveCodecs(channel, {{kPcmuPayload, kPcmuFormat}}), + VoipResult::kOk); EXPECT_EQ(voip_core_->StartSend(channel), VoipResult::kOk); EXPECT_EQ(voip_core_->StartPlayout(channel), VoipResult::kOk); - voip_core_->RegisterTelephoneEventType(channel, kPcmuPayload, - kPcmuSampleRateHz); + EXPECT_EQ(voip_core_->RegisterTelephoneEventType(channel, kPcmuPayload, + kPcmuSampleRateHz), + VoipResult::kOk); EXPECT_EQ( voip_core_->SendDtmfEvent(channel, kDtmfEventCode, kDtmfEventDurationMs), @@ -125,7 +129,8 @@ TEST_F(VoipCoreTest, SendDtmfEventWithoutRegistering) { auto channel = voip_core_->CreateChannel(&transport_, 0xdeadc0de); - voip_core_->SetSendCodec(channel, kPcmuPayload, kPcmuFormat); + EXPECT_EQ(voip_core_->SetSendCodec(channel, kPcmuPayload, kPcmuFormat), + VoipResult::kOk); EXPECT_EQ(voip_core_->StartSend(channel), VoipResult::kOk); // Send Dtmf event without registering beforehand, thus payload @@ -145,8 +150,10 @@ TEST_F(VoipCoreTest, SendDtmfEventWithoutRegistering) { TEST_F(VoipCoreTest, SendDtmfEventWithoutStartSend) { auto channel = voip_core_->CreateChannel(&transport_, 0xdeadc0de); - voip_core_->RegisterTelephoneEventType(channel, kPcmuPayload, - kPcmuSampleRateHz); + EXPECT_EQ(voip_core_->RegisterTelephoneEventType(channel, kPcmuPayload, + kPcmuSampleRateHz), + VoipResult::kOk); + // Send Dtmf event without calling StartSend beforehand, thus // Dtmf events cannot be sent and kFailedPrecondition is expected. EXPECT_EQ( diff --git a/examples/androidvoip/jni/android_voip_client.cc b/examples/androidvoip/jni/android_voip_client.cc index d0763cdcc9..95d3ed407f 100644 --- a/examples/androidvoip/jni/android_voip_client.cc +++ b/examples/androidvoip/jni/android_voip_client.cc @@ -120,7 +120,7 @@ int GetPayloadType(const std::string& codec_name) { namespace webrtc_examples { -bool AndroidVoipClient::Init( +void AndroidVoipClient::Init( JNIEnv* env, const webrtc::JavaParamRef& application_context) { webrtc::VoipEngineConfig config; @@ -132,20 +132,16 @@ bool AndroidVoipClient::Init( config.audio_processing = webrtc::AudioProcessingBuilder().Create(); voip_thread_->Start(); + // Due to consistent thread requirement on // modules/audio_device/android/audio_device_template.h, // code is invoked in the context of voip_thread_. - return voip_thread_->Invoke(RTC_FROM_HERE, [this, &config] { + voip_thread_->Invoke(RTC_FROM_HERE, [this, &config] { RTC_DCHECK_RUN_ON(voip_thread_.get()); supported_codecs_ = config.encoder_factory->GetSupportedEncoders(); env_ = webrtc::AttachCurrentThreadIfNeeded(); voip_engine_ = webrtc::CreateVoipEngine(std::move(config)); - if (!voip_engine_) { - RTC_LOG(LS_ERROR) << "VoipEngine creation failed"; - return false; - } - return true; }); } @@ -175,9 +171,7 @@ AndroidVoipClient* AndroidVoipClient::Create( // Using `new` to access a non-public constructor. auto voip_client = absl::WrapUnique(new AndroidVoipClient(env, j_voip_client)); - if (!voip_client->Init(env, application_context)) { - return nullptr; - } + voip_client->Init(env, application_context); return voip_client.release(); } @@ -220,8 +214,9 @@ void AndroidVoipClient::SetEncoder(const std::string& encoder) { } for (const webrtc::AudioCodecSpec& codec : supported_codecs_) { if (codec.format.name == encoder) { - voip_engine_->Codec().SetSendCodec( + webrtc::VoipResult result = voip_engine_->Codec().SetSendCodec( *channel_, GetPayloadType(codec.format.name), codec.format); + RTC_CHECK(result == webrtc::VoipResult::kOk); return; } } @@ -251,7 +246,9 @@ void AndroidVoipClient::SetDecoders(const std::vector& decoders) { } } - voip_engine_->Codec().SetReceiveCodecs(*channel_, decoder_specs); + webrtc::VoipResult result = + voip_engine_->Codec().SetReceiveCodecs(*channel_, decoder_specs); + RTC_CHECK(result == webrtc::VoipResult::kOk); } void AndroidVoipClient::SetDecoders( @@ -305,13 +302,8 @@ void AndroidVoipClient::SetRemoteAddress( void AndroidVoipClient::StartSession(JNIEnv* env) { RUN_ON_VOIP_THREAD(StartSession, env); + // CreateChannel guarantees to return valid channel id. channel_ = voip_engine_->Base().CreateChannel(this, absl::nullopt); - if (!channel_) { - RTC_LOG(LS_ERROR) << "Channel creation failed"; - Java_VoipClient_onStartSessionCompleted(env_, j_voip_client_, - /*isSuccessful=*/false); - return; - } rtp_socket_.reset(rtc::AsyncUDPSocket::Create(voip_thread_->socketserver(), rtp_local_address_)); @@ -357,7 +349,9 @@ void AndroidVoipClient::StopSession(JNIEnv* env) { rtp_socket_->Close(); rtcp_socket_->Close(); - voip_engine_->Base().ReleaseChannel(*channel_); + webrtc::VoipResult result = voip_engine_->Base().ReleaseChannel(*channel_); + RTC_CHECK(result == webrtc::VoipResult::kOk); + channel_ = absl::nullopt; Java_VoipClient_onStopSessionCompleted(env_, j_voip_client_, /*isSuccessful=*/true); @@ -470,9 +464,10 @@ void AndroidVoipClient::ReadRTPPacket(const std::vector& packet_copy) { RTC_LOG(LS_ERROR) << "Channel has not been created"; return; } - voip_engine_->Network().ReceivedRTPPacket( + webrtc::VoipResult result = voip_engine_->Network().ReceivedRTPPacket( *channel_, rtc::ArrayView(packet_copy.data(), packet_copy.size())); + RTC_CHECK(result == webrtc::VoipResult::kOk); } void AndroidVoipClient::OnSignalReadRTPPacket(rtc::AsyncPacketSocket* socket, @@ -495,9 +490,10 @@ void AndroidVoipClient::ReadRTCPPacket( RTC_LOG(LS_ERROR) << "Channel has not been created"; return; } - voip_engine_->Network().ReceivedRTCPPacket( + webrtc::VoipResult result = voip_engine_->Network().ReceivedRTCPPacket( *channel_, rtc::ArrayView(packet_copy.data(), packet_copy.size())); + RTC_CHECK(result == webrtc::VoipResult::kOk); } void AndroidVoipClient::OnSignalReadRTCPPacket(rtc::AsyncPacketSocket* socket, diff --git a/examples/androidvoip/jni/android_voip_client.h b/examples/androidvoip/jni/android_voip_client.h index 4dd0b0a0fb..bfca7e8b79 100644 --- a/examples/androidvoip/jni/android_voip_client.h +++ b/examples/androidvoip/jni/android_voip_client.h @@ -141,7 +141,7 @@ class AndroidVoipClient : public webrtc::Transport, : voip_thread_(rtc::Thread::CreateWithSocketServer()), j_voip_client_(env, j_voip_client) {} - bool Init(JNIEnv* env, + void Init(JNIEnv* env, const webrtc::JavaParamRef& application_context); // Overloaded methods having native C++ variables as arguments.