From 986e7451061384222b6fc6ca3110e56cda08597a Mon Sep 17 00:00:00 2001 From: Shyam Sadhwani Date: Mon, 14 Sep 2020 10:12:54 -0700 Subject: [PATCH] Fix for unbounded increase in audio delay when no audio packets are flowing in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WebRTC’s Audio Video sync can go in unbounded loop and keep on increasing audio delay if audio packets stop coming in. The issue happens, if StreamSynchronization::ComputeDelays has: 1. relative_delay_ms = some positive value which causes avg_diff_ms_ > 30ms 2. current_audio_delay_ms < current_video_delay_ms 3. audio_delay_.extra_ms > 0 and video_delay_.extra_ms = 0 To compensate for relative delay, audio_delay_.extra_ms gets incremented every time StreamSynchronization::ComputeDelays is called by RtpStreamsSynchronizer::Process(), which happens every 1sec RtpStreamsSynchronizer::Process() will try to set the new delay to audio stream by calling syncable_audio_->SetMinimumPlayoutDelay(target_audio_delay_ms); This ends up calling DelayManager::SetMinimumDelay and update minimum_delay_ms_ But this update has no impact on the value returned by NetEqImpl::FilteredCurrentDelayMs (as there are no audio packets flowing in, hence neteq is not running) which is called next time RtpStreamsSynchronizer::Process(), runs and tried to compute the new audio delay (audio_info→current_delay_ms) This causes audio delay to be increased in every iteration and it grows unbounded. I guess it will stop growing above 10sec as that is hardcoded max delay in NetEQ. To avoid this added a check to not adjust delays when no new audio stream has come in. Bug: webrtc:11894 Change-Id: If648f9227e43c351f887d054876cb119cc1a917e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/183340 Reviewed-by: Åsa Persson Reviewed-by: Ivo Creusen Commit-Queue: Shyam Sadhwani Cr-Commit-Position: refs/heads/master@{#32106} --- video/rtp_streams_synchronizer.cc | 7 +++ video/rtp_streams_synchronizer2.cc | 7 +++ video/stream_synchronization_unittest.cc | 57 ++++++++++++++++++++++++ 3 files changed, 71 insertions(+) diff --git a/video/rtp_streams_synchronizer.cc b/video/rtp_streams_synchronizer.cc index 28e9a0ba9d..29ace90431 100644 --- a/video/rtp_streams_synchronizer.cc +++ b/video/rtp_streams_synchronizer.cc @@ -89,11 +89,18 @@ void RtpStreamsSynchronizer::Process() { log_stats = true; } + int64_t last_audio_receive_time_ms = + audio_measurement_.latest_receive_time_ms; absl::optional audio_info = syncable_audio_->GetInfo(); if (!audio_info || !UpdateMeasurements(&audio_measurement_, *audio_info)) { return; } + if (last_audio_receive_time_ms == audio_measurement_.latest_receive_time_ms) { + // No new audio packet has been received since last update. + return; + } + int64_t last_video_receive_ms = video_measurement_.latest_receive_time_ms; absl::optional video_info = syncable_video_->GetInfo(); if (!video_info || !UpdateMeasurements(&video_measurement_, *video_info)) { diff --git a/video/rtp_streams_synchronizer2.cc b/video/rtp_streams_synchronizer2.cc index cc084bc0aa..4096fceb99 100644 --- a/video/rtp_streams_synchronizer2.cc +++ b/video/rtp_streams_synchronizer2.cc @@ -92,11 +92,18 @@ void RtpStreamsSynchronizer::UpdateDelay() { log_stats = true; } + int64_t last_audio_receive_time_ms = + audio_measurement_.latest_receive_time_ms; absl::optional audio_info = syncable_audio_->GetInfo(); if (!audio_info || !UpdateMeasurements(&audio_measurement_, *audio_info)) { return; } + if (last_audio_receive_time_ms == audio_measurement_.latest_receive_time_ms) { + // No new audio packet has been received since last update. + return; + } + int64_t last_video_receive_ms = video_measurement_.latest_receive_time_ms; absl::optional video_info = syncable_video_->GetInfo(); if (!video_info || !UpdateMeasurements(&video_measurement_, *video_info)) { diff --git a/video/stream_synchronization_unittest.cc b/video/stream_synchronization_unittest.cc index 04a43c21f9..3d6fdd82a7 100644 --- a/video/stream_synchronization_unittest.cc +++ b/video/stream_synchronization_unittest.cc @@ -383,6 +383,63 @@ TEST_F(StreamSynchronizationTest, AudioDelayed) { total_audio_delay_ms); } +TEST_F(StreamSynchronizationTest, NoAudioIncomingUnboundedIncrease) { + // Test how audio delay can grow unbounded when audio stops coming in. + // This is handled in caller of RtpStreamsSynchronizer, for example in + // RtpStreamsSynchronizer by not updating delays when audio samples stop + // coming in. + const int kVideoDelayMs = 300; + const int kAudioDelayMs = 100; + int current_audio_delay_ms = kAudioDelayMs; + int total_audio_delay_ms = 0; + int total_video_delay_ms = 0; + + EXPECT_TRUE(DelayedStreams(/*audio_delay_ms=*/0, kVideoDelayMs, + current_audio_delay_ms, &total_audio_delay_ms, + &total_video_delay_ms)); + EXPECT_EQ(0, total_video_delay_ms); + // The delay is not allowed to change more than this. + EXPECT_EQ((kVideoDelayMs - kAudioDelayMs) / kSmoothingFilter, + total_audio_delay_ms); + int last_total_audio_delay_ms = total_audio_delay_ms; + + // Set new current audio delay: simulate audio samples are flowing in. + current_audio_delay_ms = total_audio_delay_ms; + + clock_sender_.AdvanceTimeMilliseconds(1000); + clock_receiver_.AdvanceTimeMilliseconds(1000); + EXPECT_TRUE(DelayedStreams(/*audio_delay_ms=*/0, kVideoDelayMs, + current_audio_delay_ms, &total_audio_delay_ms, + &total_video_delay_ms)); + EXPECT_EQ(0, total_video_delay_ms); + EXPECT_EQ(last_total_audio_delay_ms + + MaxAudioDelayChangeMs(current_audio_delay_ms, kVideoDelayMs), + total_audio_delay_ms); + last_total_audio_delay_ms = total_audio_delay_ms; + + // Simulate no incoming audio by not update audio delay. + const int kSimulationSecs = 300; // 5min + const int kMaxDeltaDelayMs = 10000; // max delay for audio in webrtc + for (auto time_secs = 0; time_secs < kSimulationSecs; time_secs++) { + clock_sender_.AdvanceTimeMilliseconds(1000); + clock_receiver_.AdvanceTimeMilliseconds(1000); + EXPECT_TRUE(DelayedStreams(/*audio_delay_ms=*/0, kVideoDelayMs, + current_audio_delay_ms, &total_audio_delay_ms, + &total_video_delay_ms)); + EXPECT_EQ(0, total_video_delay_ms); + + // Audio delay does not go above kMaxDeltaDelayMs. + EXPECT_EQ(std::min(kMaxDeltaDelayMs, + last_total_audio_delay_ms + + MaxAudioDelayChangeMs(current_audio_delay_ms, + kVideoDelayMs)), + total_audio_delay_ms); + last_total_audio_delay_ms = total_audio_delay_ms; + } + // By now the audio delay has grown unbounded to kMaxDeltaDelayMs. + EXPECT_EQ(kMaxDeltaDelayMs, last_total_audio_delay_ms); +} + TEST_F(StreamSynchronizationTest, BothDelayedVideoLater) { BothDelayedVideoLaterTest(0); }