From 87bed4793ff8f463202f442381339626d0b27f0d Mon Sep 17 00:00:00 2001 From: Johannes Kron Date: Wed, 28 Aug 2019 12:40:45 +0000 Subject: [PATCH] Reland "Preserve min and max playout delay from RTP header extension" This reverts commit f31cc08ba01ed403e89255b5f3f38d5dbdde855e. Reason for revert: Reland with fixes Original change's description: > Revert "Preserve min and max playout delay from RTP header extension" > > This reverts commit 85ba9972c42544c4771e394c9aa1d20bf5d09a91. > > Reason for revert: Audio might be more out of sync than needed due to jitter in received video stream. > > Original change's description: > > Preserve min and max playout delay from RTP header extension > > > > Audio and video synchronization can sometimes override the minimum > > and maximum playout delay that is set through the RTP header > > extension. This CL makes sure that the playout delay always is > > within the limits set by the RTP header extension. > > > > Bug: webrtc:10886 > > Change-Id: Ie2dd4b901c4ed178759b555a8be04bd8b8f63bda > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/150645 > > Commit-Queue: Johannes Kron > > Reviewed-by: Stefan Holmer > > Cr-Commit-Position: refs/heads/master@{#28980} > > TBR=stefan@webrtc.org,kron@webrtc.org > > Change-Id: I417e440d8a7e04ab3e19faa4454b704d2b971cd7 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: webrtc:10886 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/150652 > Reviewed-by: Johannes Kron > Commit-Queue: Johannes Kron > Cr-Commit-Position: refs/heads/master@{#28984} TBR=stefan@webrtc.org,kron@webrtc.org Change-Id: I5a3908a8c45f7faedab6f009b22df81d674e13a0 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:10886 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/150653 Reviewed-by: Johannes Kron Commit-Queue: Johannes Kron Cr-Commit-Position: refs/heads/master@{#28985} --- video/video_receive_stream.cc | 22 ++++----- video/video_receive_stream_unittest.cc | 67 ++++++++++++++++++++------ 2 files changed, 64 insertions(+), 25 deletions(-) diff --git a/video/video_receive_stream.cc b/video/video_receive_stream.cc index a52dac8ea0..d1b90a07b5 100644 --- a/video/video_receive_stream.cc +++ b/video/video_receive_stream.cc @@ -578,14 +578,12 @@ void VideoReceiveStream::OnCompleteFrame( last_complete_frame_time_ms_ = time_now_ms; const PlayoutDelay& playout_delay = frame->EncodedImage().playout_delay_; - if (playout_delay.min_ms >= 0) { + // Both |min_ms| and |max_ms| must be valid if PlayoutDelay is set. + RTC_DCHECK((playout_delay.min_ms >= 0 && playout_delay.max_ms >= 0) || + (playout_delay.min_ms < 0 && playout_delay.max_ms < 0)); + if (playout_delay.min_ms >= 0 && playout_delay.max_ms >= 0) { rtc::CritScope cs(&playout_delay_lock_); frame_minimum_playout_delay_ms_ = playout_delay.min_ms; - UpdatePlayoutDelays(); - } - - if (playout_delay.max_ms >= 0) { - rtc::CritScope cs(&playout_delay_lock_); frame_maximum_playout_delay_ms_ = playout_delay.max_ms; UpdatePlayoutDelays(); } @@ -763,17 +761,19 @@ void VideoReceiveStream::HandleFrameBufferTimeout() { } void VideoReceiveStream::UpdatePlayoutDelays() const { - const int minimum_delay_ms = + int minimum_delay_ms = std::max({frame_minimum_playout_delay_ms_, base_minimum_playout_delay_ms_, syncable_minimum_playout_delay_ms_}); - if (minimum_delay_ms >= 0) { - timing_->set_min_playout_delay(minimum_delay_ms); - } - const int maximum_delay_ms = frame_maximum_playout_delay_ms_; if (maximum_delay_ms >= 0) { + // Make sure that minimum_delay_ms <= maximum_delay_ms. + minimum_delay_ms = std::min(minimum_delay_ms, maximum_delay_ms); timing_->set_max_playout_delay(maximum_delay_ms); } + + if (minimum_delay_ms >= 0) { + timing_->set_min_playout_delay(minimum_delay_ms); + } } std::vector VideoReceiveStream::GetSources() const { diff --git a/video/video_receive_stream_unittest.cc b/video/video_receive_stream_unittest.cc index 6d88f67e92..9edaa2b704 100644 --- a/video/video_receive_stream_unittest.cc +++ b/video/video_receive_stream_unittest.cc @@ -168,7 +168,7 @@ TEST_F(VideoReceiveStreamTest, CreateFrameFromH264FmtpSpropAndIdr) { } TEST_F(VideoReceiveStreamTest, PlayoutDelay) { - const PlayoutDelay kPlayoutDelayMs = {123, 321}; + const PlayoutDelay kPlayoutDelayMs = {123, 621}; std::unique_ptr test_frame(new FrameObjectFake()); test_frame->id.picture_id = 0; test_frame->SetPlayoutDelay(kPlayoutDelayMs); @@ -196,9 +196,10 @@ TEST_F(VideoReceiveStreamTest, PlayoutDelay) { EXPECT_EQ(123, timing_->min_playout_delay()); } -TEST_F(VideoReceiveStreamTest, PlayoutDelayPreservesDefaultMaxValue) { +TEST_F(VideoReceiveStreamTest, PlayoutDelayPreservesDefaultValues) { + const int default_min_playout_latency = timing_->min_playout_delay(); const int default_max_playout_latency = timing_->max_playout_delay(); - const PlayoutDelay kPlayoutDelayMs = {123, -1}; + const PlayoutDelay kPlayoutDelayMs = {-1, -1}; std::unique_ptr test_frame(new FrameObjectFake()); test_frame->id.picture_id = 0; @@ -206,26 +207,64 @@ TEST_F(VideoReceiveStreamTest, PlayoutDelayPreservesDefaultMaxValue) { video_receive_stream_->OnCompleteFrame(std::move(test_frame)); - // Ensure that -1 preserves default maximum value from |timing_|. - EXPECT_EQ(kPlayoutDelayMs.min_ms, timing_->min_playout_delay()); - EXPECT_NE(kPlayoutDelayMs.max_ms, timing_->max_playout_delay()); + // Ensure that -1 preserves default minimum and maximum value from |timing_|. + EXPECT_EQ(default_min_playout_latency, timing_->min_playout_delay()); EXPECT_EQ(default_max_playout_latency, timing_->max_playout_delay()); } -TEST_F(VideoReceiveStreamTest, PlayoutDelayPreservesDefaultMinValue) { - const int default_min_playout_latency = timing_->min_playout_delay(); - const PlayoutDelay kPlayoutDelayMs = {-1, 321}; +TEST_F(VideoReceiveStreamTest, ZeroMinMaxPlayoutDelayOverridesSyncAndBase) { + const PlayoutDelay kPlayoutDelayMs = {0, 0}; std::unique_ptr test_frame(new FrameObjectFake()); test_frame->id.picture_id = 0; test_frame->SetPlayoutDelay(kPlayoutDelayMs); - video_receive_stream_->OnCompleteFrame(std::move(test_frame)); + video_receive_stream_->SetMinimumPlayoutDelay(400); + EXPECT_EQ(400, timing_->min_playout_delay()); - // Ensure that -1 preserves default minimum value from |timing_|. - EXPECT_NE(kPlayoutDelayMs.min_ms, timing_->min_playout_delay()); - EXPECT_EQ(kPlayoutDelayMs.max_ms, timing_->max_playout_delay()); - EXPECT_EQ(default_min_playout_latency, timing_->min_playout_delay()); + video_receive_stream_->OnCompleteFrame(std::move(test_frame)); + EXPECT_EQ(0, timing_->min_playout_delay()); + EXPECT_EQ(0, timing_->max_playout_delay()); + + video_receive_stream_->SetBaseMinimumPlayoutDelayMs(1234); + EXPECT_EQ(0, timing_->min_playout_delay()); + EXPECT_EQ(0, timing_->max_playout_delay()); +} + +TEST_F(VideoReceiveStreamTest, PlayoutDelayFromFrameIsCached) { + // Expect the playout delay from one frame to be used until there's a new + // frame with a valid value. + + const PlayoutDelay kPlayoutDelay1Ms = {100, 1000}; + const PlayoutDelay kPlayoutDelay2Ms = {120, 900}; + + // Frame 1 with playout delay set. + std::unique_ptr frame1(new FrameObjectFake()); + frame1->id.picture_id = 0; + frame1->SetPlayoutDelay(kPlayoutDelay1Ms); + + video_receive_stream_->OnCompleteFrame(std::move(frame1)); + EXPECT_EQ(kPlayoutDelay1Ms.min_ms, timing_->min_playout_delay()); + EXPECT_EQ(kPlayoutDelay1Ms.max_ms, timing_->max_playout_delay()); + + // Frame 2 without playout delay set. + std::unique_ptr frame2_without_playout_delay( + new FrameObjectFake()); + frame2_without_playout_delay->id.picture_id = 1; + video_receive_stream_->OnCompleteFrame( + std::move(frame2_without_playout_delay)); + video_receive_stream_->SetBaseMinimumPlayoutDelayMs(40); + video_receive_stream_->SetMinimumPlayoutDelay(50); + EXPECT_EQ(kPlayoutDelay1Ms.min_ms, timing_->min_playout_delay()); + EXPECT_EQ(kPlayoutDelay1Ms.max_ms, timing_->max_playout_delay()); + + // Frame 3 with tighter playout delay bounds. + std::unique_ptr frame3(new FrameObjectFake()); + frame3->id.picture_id = 2; + frame3->SetPlayoutDelay(kPlayoutDelay2Ms); + video_receive_stream_->OnCompleteFrame(std::move(frame3)); + EXPECT_EQ(kPlayoutDelay2Ms.min_ms, timing_->min_playout_delay()); + EXPECT_EQ(kPlayoutDelay2Ms.max_ms, timing_->max_playout_delay()); } class VideoReceiveStreamTestWithFakeDecoder : public ::testing::Test {