Added Error Checking in Ingress/Egress and Extra Unit Tests

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 <natim@webrtc.org>
Reviewed-by: Per Åhgren <peah@webrtc.org>
Reviewed-by: Tim Na <natim@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31874}

audio/voip/audio_channel.cc

@@ -82,44 +82,50 @@ AudioChannel::~AudioChannel() {
   process_thread_->DeRegisterModule(rtp_rtcp_.get());
 }
 
-void AudioChannel::StartSend() {
+bool AudioChannel::StartSend() {
-  egress_->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) {
+  if (!rtp_rtcp_->Sending()) {
-    RTC_DLOG(LS_ERROR) << "StartSend() RTP/RTCP failed to start 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
+  // Deactivate RTP stack when both sending and receiving are stopped.
-  // stack. SetSendingStatus(false) triggers the transmission of RTCP BYE
+  // SetSendingStatus(false) triggers the transmission of RTCP BYE
   // message to remote endpoint.
-  if (!IsPlaying() && rtp_rtcp_->Sending() &&
+  if (!ingress_->IsPlaying() && rtp_rtcp_->Sending()) {
-      rtp_rtcp_->SetSendingStatus(false) != 0) {
+    rtp_rtcp_->SetSendingStatus(false);
-    RTC_DLOG(LS_ERROR) << "StopSend() RTP/RTCP failed to stop sending";
   }
 }
 
-void AudioChannel::StartPlay() {
+bool AudioChannel::StartPlay() {
-  ingress_->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) {
+  if (!rtp_rtcp_->Sending()) {
-    RTC_DLOG(LS_ERROR) << "StartPlay() RTP/RTCP failed to start 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() &&
+  if (!rtp_rtcp_->SendingMedia() && rtp_rtcp_->Sending()) {
-      rtp_rtcp_->SetSendingStatus(false) != 0) {
+    rtp_rtcp_->SetSendingStatus(false);
-    RTC_DLOG(LS_ERROR) << "StopPlay() RTP/RTCP failed to stop sending";
   }
 }
 

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.

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() {

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.
+  // processing input audio samples. StartSend will return false if
-  void StartSend();
+  // a send codec has not been set.
+  bool StartSend();
   void StopSend();
 
   // Query the state of the RTP stack. This returns true if StartSend()

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<int, SdpAudioFormat>& codecs) {
   {

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();

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

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";