From dba1f945cfb65e7d47dd72d0cbe1cd3049048594 Mon Sep 17 00:00:00 2001 From: Jason Long Date: Thu, 6 Aug 2020 15:16:04 -0400 Subject: [PATCH] Added Error Checking in Ingress/Egress and Extra Unit Tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added error checking in AudioIngress and AudioEgress to detect situations where codecs have not been set; added additional unit tests for VoipCore Bug: webrtc:11251 Change-Id: Ibd57e518892c76e2865b844ba866e380a565dd6b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/180229 Commit-Queue: Tim Na Reviewed-by: Per Åhgren Reviewed-by: Tim Na Cr-Commit-Position: refs/heads/master@{#31874} --- audio/voip/audio_channel.cc | 38 ++++++++++++++++----------- audio/voip/audio_channel.h | 6 +++-- audio/voip/audio_egress.cc | 7 ++++- audio/voip/audio_egress.h | 5 ++-- audio/voip/audio_ingress.cc | 12 +++++++++ audio/voip/audio_ingress.h | 2 +- audio/voip/test/voip_core_unittest.cc | 28 ++++++++++++++++++++ audio/voip/voip_core.cc | 8 ++---- 8 files changed, 78 insertions(+), 28 deletions(-) diff --git a/audio/voip/audio_channel.cc b/audio/voip/audio_channel.cc index d9c89fcdc4..43d4d0f15d 100644 --- a/audio/voip/audio_channel.cc +++ b/audio/voip/audio_channel.cc @@ -82,44 +82,50 @@ AudioChannel::~AudioChannel() { process_thread_->DeRegisterModule(rtp_rtcp_.get()); } -void AudioChannel::StartSend() { - egress_->StartSend(); +bool AudioChannel::StartSend() { + // If encoder has not been set, return false. + if (!egress_->StartSend()) { + return false; + } // Start sending with RTP stack if it has not been sending yet. - if (!rtp_rtcp_->Sending() && rtp_rtcp_->SetSendingStatus(true) != 0) { - RTC_DLOG(LS_ERROR) << "StartSend() RTP/RTCP failed to start sending"; + if (!rtp_rtcp_->Sending()) { + rtp_rtcp_->SetSendingStatus(true); } + return true; } void AudioChannel::StopSend() { egress_->StopSend(); - // If the channel is not playing and RTP stack is active then deactivate RTP - // stack. SetSendingStatus(false) triggers the transmission of RTCP BYE + // Deactivate RTP stack when both sending and receiving are stopped. + // SetSendingStatus(false) triggers the transmission of RTCP BYE // message to remote endpoint. - if (!IsPlaying() && rtp_rtcp_->Sending() && - rtp_rtcp_->SetSendingStatus(false) != 0) { - RTC_DLOG(LS_ERROR) << "StopSend() RTP/RTCP failed to stop sending"; + if (!ingress_->IsPlaying() && rtp_rtcp_->Sending()) { + rtp_rtcp_->SetSendingStatus(false); } } -void AudioChannel::StartPlay() { - ingress_->StartPlay(); +bool AudioChannel::StartPlay() { + // If decoders have not been set, return false. + if (!ingress_->StartPlay()) { + return false; + } // If RTP stack is not sending then start sending as in recv-only mode, RTCP // receiver report is expected. - if (!rtp_rtcp_->Sending() && rtp_rtcp_->SetSendingStatus(true) != 0) { - RTC_DLOG(LS_ERROR) << "StartPlay() RTP/RTCP failed to start sending"; + if (!rtp_rtcp_->Sending()) { + rtp_rtcp_->SetSendingStatus(true); } + return true; } void AudioChannel::StopPlay() { ingress_->StopPlay(); // Deactivate RTP stack only when both sending and receiving are stopped. - if (!IsSendingMedia() && rtp_rtcp_->Sending() && - rtp_rtcp_->SetSendingStatus(false) != 0) { - RTC_DLOG(LS_ERROR) << "StopPlay() RTP/RTCP failed to stop sending"; + if (!rtp_rtcp_->SendingMedia() && rtp_rtcp_->Sending()) { + rtp_rtcp_->SetSendingStatus(false); } } diff --git a/audio/voip/audio_channel.h b/audio/voip/audio_channel.h index 659e990c30..12138ee67c 100644 --- a/audio/voip/audio_channel.h +++ b/audio/voip/audio_channel.h @@ -45,9 +45,11 @@ class AudioChannel : public rtc::RefCountInterface { ChannelId GetId() const { return id_; } // APIs to start/stop audio channel on each direction. - void StartSend(); + // StartSend/StartPlay returns false if encoder/decoders + // have not been set, respectively. + bool StartSend(); void StopSend(); - void StartPlay(); + bool StartPlay(); void StopPlay(); // APIs relayed to AudioEgress. diff --git a/audio/voip/audio_egress.cc b/audio/voip/audio_egress.cc index 305f712624..90e069e1cc 100644 --- a/audio/voip/audio_egress.cc +++ b/audio/voip/audio_egress.cc @@ -56,8 +56,13 @@ void AudioEgress::SetEncoder(int payload_type, audio_coding_->SetEncoder(std::move(encoder)); } -void AudioEgress::StartSend() { +bool AudioEgress::StartSend() { + if (!GetEncoderFormat()) { + RTC_DLOG(LS_WARNING) << "Send codec has not been set yet"; + return false; + } rtp_rtcp_->SetSendingMediaStatus(true); + return true; } void AudioEgress::StopSend() { diff --git a/audio/voip/audio_egress.h b/audio/voip/audio_egress.h index 8ec048f915..6b2d374717 100644 --- a/audio/voip/audio_egress.h +++ b/audio/voip/audio_egress.h @@ -59,8 +59,9 @@ class AudioEgress : public AudioSender, public AudioPacketizationCallback { // Start or stop sending operation of AudioEgress. This will start/stop // the RTP stack also causes encoder queue thread to start/stop - // processing input audio samples. - void StartSend(); + // processing input audio samples. StartSend will return false if + // a send codec has not been set. + bool StartSend(); void StopSend(); // Query the state of the RTP stack. This returns true if StartSend() diff --git a/audio/voip/audio_ingress.cc b/audio/voip/audio_ingress.cc index 560055d4f4..0bddb42807 100644 --- a/audio/voip/audio_ingress.cc +++ b/audio/voip/audio_ingress.cc @@ -103,6 +103,18 @@ AudioMixer::Source::AudioFrameInfo AudioIngress::GetAudioFrameWithInfo( : AudioMixer::Source::AudioFrameInfo::kNormal; } +bool AudioIngress::StartPlay() { + { + MutexLock lock(&lock_); + if (receive_codec_info_.empty()) { + RTC_DLOG(LS_WARNING) << "Receive codecs have not been set yet"; + return false; + } + } + playing_ = true; + return true; +} + void AudioIngress::SetReceiveCodecs( const std::map& codecs) { { diff --git a/audio/voip/audio_ingress.h b/audio/voip/audio_ingress.h index 5a8df21f7a..d09de606de 100644 --- a/audio/voip/audio_ingress.h +++ b/audio/voip/audio_ingress.h @@ -51,7 +51,7 @@ class AudioIngress : public AudioMixer::Source { ~AudioIngress() override; // Start or stop receiving operation of AudioIngress. - void StartPlay() { playing_ = true; } + bool StartPlay(); void StopPlay() { playing_ = false; output_audio_level_.ResetLevelFullRange(); diff --git a/audio/voip/test/voip_core_unittest.cc b/audio/voip/test/voip_core_unittest.cc index c1969d6ed0..b97b637dda 100644 --- a/audio/voip/test/voip_core_unittest.cc +++ b/audio/voip/test/voip_core_unittest.cc @@ -96,5 +96,33 @@ TEST_F(VoipCoreTest, ExpectFailToUseReleasedChannelId) { EXPECT_FALSE(voip_core_->StartPlayout(*channel)); } +TEST_F(VoipCoreTest, StartSendAndPlayoutWithoutSettingCodec) { + auto channel = voip_core_->CreateChannel(&transport_, 0xdeadc0de); + EXPECT_TRUE(channel); + + // Call StartSend and StartPlayout without setting send/receive + // codec. Code should see that codecs aren't set and return false. + EXPECT_FALSE(voip_core_->StartSend(*channel)); + EXPECT_FALSE(voip_core_->StartPlayout(*channel)); + + voip_core_->ReleaseChannel(*channel); +} + +TEST_F(VoipCoreTest, StopSendAndPlayoutWithoutStarting) { + auto channel = voip_core_->CreateChannel(&transport_, 0xdeadc0de); + EXPECT_TRUE(channel); + + voip_core_->SetSendCodec(*channel, kPcmuPayload, kPcmuFormat); + voip_core_->SetReceiveCodecs(*channel, {{kPcmuPayload, kPcmuFormat}}); + + // Call StopSend and StopPlayout without starting them in + // the first place. Should see that it is already in the + // stopped state and return true. + EXPECT_TRUE(voip_core_->StopSend(*channel)); + EXPECT_TRUE(voip_core_->StopPlayout(*channel)); + + voip_core_->ReleaseChannel(*channel); +} + } // namespace } // namespace webrtc diff --git a/audio/voip/voip_core.cc b/audio/voip/voip_core.cc index 7292644648..639022363e 100644 --- a/audio/voip/voip_core.cc +++ b/audio/voip/voip_core.cc @@ -238,12 +238,10 @@ bool VoipCore::UpdateAudioTransportWithSenders() { bool VoipCore::StartSend(ChannelId channel) { auto audio_channel = GetChannel(channel); - if (!audio_channel) { + if (!audio_channel || !audio_channel->StartSend()) { return false; } - audio_channel->StartSend(); - return UpdateAudioTransportWithSenders(); } @@ -260,12 +258,10 @@ bool VoipCore::StopSend(ChannelId channel) { bool VoipCore::StartPlayout(ChannelId channel) { auto audio_channel = GetChannel(channel); - if (!audio_channel) { + if (!audio_channel || !audio_channel->StartPlay()) { return false; } - audio_channel->StartPlay(); - if (!audio_device_module_->Playing()) { if (audio_device_module_->InitPlayout() != 0) { RTC_LOG(LS_ERROR) << "InitPlayout failed";