From 4552e8f2d4d46adf94e214963cf9e67dec1ead2e Mon Sep 17 00:00:00 2001 From: Tim Na Date: Wed, 4 Nov 2020 10:42:57 -0800 Subject: [PATCH] Enable continuous audio polling from ADM after StopPlay in VoIP API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Current VoIP Engine logic stops ADM from polling registered audio channel when caller invokes StopPlay which can leads to incoming RTP to be flushed and undesirable statistics report. Instead, VoipBase::StopPlay should silence the decoded audio sample from NetEq as muted to avoid mixing while allowing it go through prior process for correct ingress statistic values. The ADM stop playing logic will be triggered when all audio channels are released by VoipBase::ReleaseChannel API. Bug: webrtc:12121 Change-Id: I410eea4ea13f93acb465ab162a3c14c9819e2b92 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/191140 Commit-Queue: Tim Na Reviewed-by: Per Åhgren Cr-Commit-Position: refs/heads/master@{#32553} --- audio/voip/audio_ingress.cc | 10 +++--- audio/voip/test/audio_ingress_unittest.cc | 43 +++++++++++++++++++++++ audio/voip/voip_core.cc | 43 ++++++++++------------- 3 files changed, 68 insertions(+), 28 deletions(-) diff --git a/audio/voip/audio_ingress.cc b/audio/voip/audio_ingress.cc index 0bddb42807..07def99559 100644 --- a/audio/voip/audio_ingress.cc +++ b/audio/voip/audio_ingress.cc @@ -73,6 +73,12 @@ AudioMixer::Source::AudioFrameInfo AudioIngress::GetAudioFrameWithInfo( constexpr double kAudioSampleDurationSeconds = 0.01; output_audio_level_.ComputeLevel(*audio_frame, kAudioSampleDurationSeconds); + // If caller invoked StopPlay(), then mute the frame. + if (!playing_) { + AudioFrameOperations::Mute(audio_frame); + muted = true; + } + // Set first rtp timestamp with first audio frame with valid timestamp. if (first_rtp_timestamp_ < 0 && audio_frame->timestamp_ != 0) { first_rtp_timestamp_ = audio_frame->timestamp_; @@ -127,10 +133,6 @@ void AudioIngress::SetReceiveCodecs( } void AudioIngress::ReceivedRTPPacket(rtc::ArrayView rtp_packet) { - if (!IsPlaying()) { - return; - } - RtpPacketReceived rtp_packet_received; rtp_packet_received.Parse(rtp_packet.data(), rtp_packet.size()); diff --git a/audio/voip/test/audio_ingress_unittest.cc b/audio/voip/test/audio_ingress_unittest.cc index 3a2a66a325..01b4d67dad 100644 --- a/audio/voip/test/audio_ingress_unittest.cc +++ b/audio/voip/test/audio_ingress_unittest.cc @@ -181,5 +181,48 @@ TEST_F(AudioIngressTest, PreferredSampleRate) { EXPECT_EQ(ingress_->PreferredSampleRate(), kPcmuFormat.clockrate_hz); } +// This test highlights the case where caller invokes StopPlay() which then +// AudioIngress should play silence frame afterwards. +TEST_F(AudioIngressTest, GetMutedAudioFrameAfterRtpReceivedAndStopPlay) { + // StopPlay before we start sending RTP packet with sine wave. + ingress_->StopPlay(); + + // Send 6 RTP packets to generate more than 100 ms audio sample to get + // valid speech level. + constexpr int kNumRtp = 6; + int rtp_count = 0; + rtc::Event event; + auto handle_rtp = [&](const uint8_t* packet, size_t length, Unused) { + ingress_->ReceivedRTPPacket(rtc::ArrayView(packet, length)); + if (++rtp_count == kNumRtp) { + event.Set(); + } + return true; + }; + EXPECT_CALL(transport_, SendRtp).WillRepeatedly(Invoke(handle_rtp)); + for (int i = 0; i < kNumRtp * 2; i++) { + egress_->SendAudioData(GetAudioFrame(i)); + fake_clock_.AdvanceTimeMilliseconds(10); + } + event.Wait(/*give_up_after_ms=*/1000); + + for (int i = 0; i < kNumRtp * 2; ++i) { + AudioFrame audio_frame; + EXPECT_EQ( + ingress_->GetAudioFrameWithInfo(kPcmuFormat.clockrate_hz, &audio_frame), + AudioMixer::Source::AudioFrameInfo::kMuted); + const int16_t* audio_data = audio_frame.data(); + size_t length = + audio_frame.samples_per_channel_ * audio_frame.num_channels_; + for (size_t j = 0; j < length; ++j) { + EXPECT_EQ(audio_data[j], 0); + } + } + + // Now we should still see valid speech output level as StopPlay won't affect + // the measurement. + EXPECT_EQ(ingress_->GetSpeechOutputLevelFullRange(), kAudioLevel); +} + } // namespace } // namespace webrtc diff --git a/audio/voip/voip_core.cc b/audio/voip/voip_core.cc index a93df73a22..2a29033a2b 100644 --- a/audio/voip/voip_core.cc +++ b/audio/voip/voip_core.cc @@ -161,8 +161,7 @@ void VoipCore::ReleaseChannel(ChannelId channel) { // Destroy channel outside of the lock. rtc::scoped_refptr audio_channel; - // Check if process thread is no longer needed. - bool stop_process_thread = false; + bool no_channels_after_release = false; { MutexLock lock(&lock_); @@ -173,18 +172,24 @@ void VoipCore::ReleaseChannel(ChannelId channel) { channels_.erase(iter); } - // Check if this is the last channel we have. - stop_process_thread = channels_.empty(); + no_channels_after_release = channels_.empty(); } if (!audio_channel) { RTC_LOG(LS_WARNING) << "Channel " << channel << " not found"; } - if (stop_process_thread) { + if (no_channels_after_release) { // Release audio channel first to have it DeRegisterModule first. audio_channel = nullptr; process_thread_->Stop(); + + // Make sure to stop playout on ADM if it is playing. + if (audio_device_module_->Playing()) { + if (audio_device_module_->StopPlayout() != 0) { + RTC_LOG(LS_WARNING) << "StopPlayout failed"; + } + } } } @@ -280,7 +285,15 @@ bool VoipCore::StopSend(ChannelId channel) { bool VoipCore::StartPlayout(ChannelId channel) { auto audio_channel = GetChannel(channel); - if (!audio_channel || !audio_channel->StartPlay()) { + if (!audio_channel) { + return false; + } + + if (audio_channel->IsPlaying()) { + return true; + } + + if (!audio_channel->StartPlay()) { return false; } @@ -305,24 +318,6 @@ bool VoipCore::StopPlayout(ChannelId channel) { audio_channel->StopPlay(); - bool stop_device = true; - { - MutexLock lock(&lock_); - for (auto kv : channels_) { - rtc::scoped_refptr& channel = kv.second; - if (channel->IsPlaying()) { - stop_device = false; - break; - } - } - } - - if (stop_device && audio_device_module_->Playing()) { - if (audio_device_module_->StopPlayout() != 0) { - RTC_LOG(LS_ERROR) << "StopPlayout failed"; - return false; - } - } return true; }