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 <kron@webrtc.org>
> Reviewed-by: Stefan Holmer <stefan@webrtc.org>
> 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 <kron@webrtc.org>
Commit-Queue: Johannes Kron <kron@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28984}
diff --git a/video/video_receive_stream.cc b/video/video_receive_stream.cc
index d1b90a0..a52dac8 100644
--- a/video/video_receive_stream.cc
+++ b/video/video_receive_stream.cc
@@ -578,12 +578,14 @@
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::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<webrtc::RtpSource> VideoReceiveStream::GetSources() const {
diff --git a/video/video_receive_stream_unittest.cc b/video/video_receive_stream_unittest.cc
index 9edaa2b..6d88f67 100644
--- a/video/video_receive_stream_unittest.cc
+++ b/video/video_receive_stream_unittest.cc
@@ -168,7 +168,7 @@
}
TEST_F(VideoReceiveStreamTest, PlayoutDelay) {
- const PlayoutDelay kPlayoutDelayMs = {123, 621};
+ const PlayoutDelay kPlayoutDelayMs = {123, 321};
std::unique_ptr<FrameObjectFake> test_frame(new FrameObjectFake());
test_frame->id.picture_id = 0;
test_frame->SetPlayoutDelay(kPlayoutDelayMs);
@@ -196,10 +196,9 @@
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<FrameObjectFake> test_frame(new FrameObjectFake());
test_frame->id.picture_id = 0;
@@ -207,64 +206,26 @@
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<FrameObjectFake> 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<FrameObjectFake> 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<FrameObjectFake> 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<FrameObjectFake> 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 {