From f31cc08ba01ed403e89255b5f3f38d5dbdde855e Mon Sep 17 00:00:00 2001 From: Johannes Kron Date: Wed, 28 Aug 2019 12:38:28 +0000 Subject: [PATCH] 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} --- video/video_receive_stream.cc | 24 +++++----- video/video_receive_stream_unittest.cc | 65 ++++++-------------------- 2 files changed, 25 insertions(+), 64 deletions(-) diff --git a/video/video_receive_stream.cc b/video/video_receive_stream.cc index d1b90a07b5..a52dac8ea0 100644 --- a/video/video_receive_stream.cc +++ b/video/video_receive_stream.cc @@ -578,12 +578,14 @@ void VideoReceiveStream::OnCompleteFrame( last_complete_frame_time_ms_ = time_now_ms; const PlayoutDelay& playout_delay = frame->EncodedImage().playout_delay_; - // 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) { + if (playout_delay.min_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(); } @@ -761,19 +763,17 @@ void VideoReceiveStream::HandleFrameBufferTimeout() { } void VideoReceiveStream::UpdatePlayoutDelays() const { - int minimum_delay_ms = + const int minimum_delay_ms = std::max({frame_minimum_playout_delay_ms_, base_minimum_playout_delay_ms_, syncable_minimum_playout_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); } + + const int maximum_delay_ms = frame_maximum_playout_delay_ms_; + if (maximum_delay_ms >= 0) { + timing_->set_max_playout_delay(maximum_delay_ms); + } } std::vector VideoReceiveStream::GetSources() const { diff --git a/video/video_receive_stream_unittest.cc b/video/video_receive_stream_unittest.cc index 9edaa2b704..6d88f67e92 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, 621}; + const PlayoutDelay kPlayoutDelayMs = {123, 321}; std::unique_ptr test_frame(new FrameObjectFake()); test_frame->id.picture_id = 0; test_frame->SetPlayoutDelay(kPlayoutDelayMs); @@ -196,10 +196,9 @@ TEST_F(VideoReceiveStreamTest, PlayoutDelay) { EXPECT_EQ(123, timing_->min_playout_delay()); } -TEST_F(VideoReceiveStreamTest, PlayoutDelayPreservesDefaultValues) { - const int default_min_playout_latency = timing_->min_playout_delay(); +TEST_F(VideoReceiveStreamTest, PlayoutDelayPreservesDefaultMaxValue) { const int default_max_playout_latency = timing_->max_playout_delay(); - const PlayoutDelay kPlayoutDelayMs = {-1, -1}; + const PlayoutDelay kPlayoutDelayMs = {123, -1}; std::unique_ptr test_frame(new FrameObjectFake()); test_frame->id.picture_id = 0; @@ -207,64 +206,26 @@ TEST_F(VideoReceiveStreamTest, PlayoutDelayPreservesDefaultValues) { video_receive_stream_->OnCompleteFrame(std::move(test_frame)); - // Ensure that -1 preserves default minimum and maximum value from |timing_|. - EXPECT_EQ(default_min_playout_latency, timing_->min_playout_delay()); + // 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()); EXPECT_EQ(default_max_playout_latency, timing_->max_playout_delay()); } -TEST_F(VideoReceiveStreamTest, ZeroMinMaxPlayoutDelayOverridesSyncAndBase) { - const PlayoutDelay kPlayoutDelayMs = {0, 0}; +TEST_F(VideoReceiveStreamTest, PlayoutDelayPreservesDefaultMinValue) { + const int default_min_playout_latency = timing_->min_playout_delay(); + const PlayoutDelay kPlayoutDelayMs = {-1, 321}; std::unique_ptr test_frame(new FrameObjectFake()); test_frame->id.picture_id = 0; test_frame->SetPlayoutDelay(kPlayoutDelayMs); - video_receive_stream_->SetMinimumPlayoutDelay(400); - EXPECT_EQ(400, 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()); + // 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()); } class VideoReceiveStreamTestWithFakeDecoder : public ::testing::Test {