From 3fa9a66f22a8877e922f6b1e40b839db0cd5708d Mon Sep 17 00:00:00 2001 From: Evan Shrubsole Date: Mon, 13 Jun 2022 14:19:10 +0000 Subject: [PATCH] Cap max decode delay for FrameBuffer3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a large queue of frames builds up due to a lost frame, the decode delay can sometimes become quite large. In this case the stream may signal as timed out when in fact it is not. Instead, the delay should be capped at the timeout limit. Bug: webrtc:14168 Change-Id: I5b4e8851b2c6d7d27a698627dc1633931d7fc00e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/265404 Reviewed-by: Erik Språng Commit-Queue: Evan Shrubsole Cr-Commit-Position: refs/heads/main@{#37199} --- video/frame_buffer_proxy.cc | 3 +- video/frame_decode_timing.cc | 5 ++- video/frame_decode_timing.h | 1 + video/frame_decode_timing_unittest.cc | 51 +++++++++++++++++++++------ video/video_receive_stream2.cc | 2 -- video/video_receive_stream2.h | 3 ++ 6 files changed, 50 insertions(+), 15 deletions(-) diff --git a/video/frame_buffer_proxy.cc b/video/frame_buffer_proxy.cc index 85ad274f92..c701122a76 100644 --- a/video/frame_buffer_proxy.cc +++ b/video/frame_buffer_proxy.cc @@ -468,7 +468,8 @@ class FrameBuffer3Proxy : public FrameBufferProxy { while (decodable_tu_info) { schedule = decode_timing_.OnFrameBufferUpdated( decodable_tu_info->next_rtp_timestamp, - decodable_tu_info->last_rtp_timestamp, IsTooManyFramesQueued()); + decodable_tu_info->last_rtp_timestamp, MaxWait(), + IsTooManyFramesQueued()); if (schedule) { // Don't schedule if already waiting for the same frame. if (frame_decode_scheduler_->ScheduledRtpTimestamp() != diff --git a/video/frame_decode_timing.cc b/video/frame_decode_timing.cc index c683834502..15fcefb357 100644 --- a/video/frame_decode_timing.cc +++ b/video/frame_decode_timing.cc @@ -28,7 +28,9 @@ FrameDecodeTiming::FrameDecodeTiming(Clock* clock, absl::optional FrameDecodeTiming::OnFrameBufferUpdated(uint32_t next_temporal_unit_rtp, uint32_t last_temporal_unit_rtp, + TimeDelta max_wait_for_frame, bool too_many_frames_queued) { + RTC_DCHECK_GT(max_wait_for_frame, TimeDelta::Zero()); const Timestamp now = clock_->CurrentTime(); Timestamp render_time = timing_->RenderTime(next_temporal_unit_rtp, now); TimeDelta max_wait = @@ -48,7 +50,8 @@ FrameDecodeTiming::OnFrameBufferUpdated(uint32_t next_temporal_unit_rtp, << " render time " << render_time.ms() << " with a max wait of " << max_wait.ms() << "ms"; - Timestamp latest_decode_time = now + std::max(max_wait, TimeDelta::Zero()); + max_wait.Clamp(TimeDelta::Zero(), max_wait_for_frame); + Timestamp latest_decode_time = now + max_wait; return FrameSchedule{.latest_decode_time = latest_decode_time, .render_time = render_time}; } diff --git a/video/frame_decode_timing.h b/video/frame_decode_timing.h index 991cfecdcd..59247dca3c 100644 --- a/video/frame_decode_timing.h +++ b/video/frame_decode_timing.h @@ -41,6 +41,7 @@ class FrameDecodeTiming { absl::optional OnFrameBufferUpdated( uint32_t next_temporal_unit_rtp, uint32_t last_temporal_unit_rtp, + TimeDelta max_wait_for_frame, bool too_many_frames_queued); private: diff --git a/video/frame_decode_timing_unittest.cc b/video/frame_decode_timing_unittest.cc index 51d115abba..83ea91692c 100644 --- a/video/frame_decode_timing_unittest.cc +++ b/video/frame_decode_timing_unittest.cc @@ -19,6 +19,7 @@ #include "test/gmock.h" #include "test/gtest.h" #include "test/scoped_key_value_config.h" +#include "video/video_receive_stream2.h" namespace webrtc { @@ -80,13 +81,13 @@ TEST_F(FrameDecodeTimingTest, ReturnsWaitTimesWhenValid) { const Timestamp render_time = clock_.CurrentTime() + TimeDelta::Millis(60); timing_.SetTimes(90000, render_time, decode_delay); - EXPECT_THAT( - frame_decode_scheduler_.OnFrameBufferUpdated(90000, 180000, false), - Optional( - AllOf(Field(&FrameDecodeTiming::FrameSchedule::latest_decode_time, - Eq(clock_.CurrentTime() + decode_delay)), - Field(&FrameDecodeTiming::FrameSchedule::render_time, - Eq(render_time))))); + EXPECT_THAT(frame_decode_scheduler_.OnFrameBufferUpdated( + 90000, 180000, kMaxWaitForFrame, false), + Optional(AllOf( + Field(&FrameDecodeTiming::FrameSchedule::latest_decode_time, + Eq(clock_.CurrentTime() + decode_delay)), + Field(&FrameDecodeTiming::FrameSchedule::render_time, + Eq(render_time))))); } TEST_F(FrameDecodeTimingTest, FastForwardsFrameTooFarInThePast) { @@ -95,9 +96,9 @@ TEST_F(FrameDecodeTimingTest, FastForwardsFrameTooFarInThePast) { const Timestamp render_time = clock_.CurrentTime(); timing_.SetTimes(90000, render_time, decode_delay); - EXPECT_THAT( - frame_decode_scheduler_.OnFrameBufferUpdated(90000, 180000, false), - Eq(absl::nullopt)); + EXPECT_THAT(frame_decode_scheduler_.OnFrameBufferUpdated( + 90000, 180000, kMaxWaitForFrame, false), + Eq(absl::nullopt)); } TEST_F(FrameDecodeTimingTest, NoFastForwardIfOnlyFrameToDecode) { @@ -107,7 +108,8 @@ TEST_F(FrameDecodeTimingTest, NoFastForwardIfOnlyFrameToDecode) { timing_.SetTimes(90000, render_time, decode_delay); // Negative `decode_delay` means that `latest_decode_time` is now. - EXPECT_THAT(frame_decode_scheduler_.OnFrameBufferUpdated(90000, 90000, false), + EXPECT_THAT(frame_decode_scheduler_.OnFrameBufferUpdated( + 90000, 90000, kMaxWaitForFrame, false), Optional(AllOf( Field(&FrameDecodeTiming::FrameSchedule::latest_decode_time, Eq(clock_.CurrentTime())), @@ -115,4 +117,31 @@ TEST_F(FrameDecodeTimingTest, NoFastForwardIfOnlyFrameToDecode) { Eq(render_time))))); } +TEST_F(FrameDecodeTimingTest, MaxWaitCapped) { + TimeDelta frame_delay = TimeDelta::Millis(30); + const TimeDelta decode_delay = TimeDelta::Seconds(3); + const Timestamp render_time = clock_.CurrentTime() + TimeDelta::Seconds(3); + timing_.SetTimes(90000, render_time, decode_delay); + timing_.SetTimes(180000, render_time + frame_delay, + decode_delay + frame_delay); + + EXPECT_THAT(frame_decode_scheduler_.OnFrameBufferUpdated( + 90000, 270000, kMaxWaitForFrame, false), + Optional(AllOf( + Field(&FrameDecodeTiming::FrameSchedule::latest_decode_time, + Eq(clock_.CurrentTime() + kMaxWaitForFrame)), + Field(&FrameDecodeTiming::FrameSchedule::render_time, + Eq(render_time))))); + + // Test cap keyframe. + clock_.AdvanceTime(frame_delay); + EXPECT_THAT(frame_decode_scheduler_.OnFrameBufferUpdated( + 180000, 270000, kMaxWaitForKeyFrame, false), + Optional(AllOf( + Field(&FrameDecodeTiming::FrameSchedule::latest_decode_time, + Eq(clock_.CurrentTime() + kMaxWaitForKeyFrame)), + Field(&FrameDecodeTiming::FrameSchedule::render_time, + Eq(render_time + frame_delay))))); +} + } // namespace webrtc diff --git a/video/video_receive_stream2.cc b/video/video_receive_stream2.cc index 2cf4ba5ba8..ee708697dd 100644 --- a/video/video_receive_stream2.cc +++ b/video/video_receive_stream2.cc @@ -74,10 +74,8 @@ namespace internal { namespace { // The default delay before re-requesting a key frame to be sent. -constexpr TimeDelta kMaxWaitForKeyFrame = TimeDelta::Millis(200); constexpr TimeDelta kMinBaseMinimumDelay = TimeDelta::Zero(); constexpr TimeDelta kMaxBaseMinimumDelay = TimeDelta::Seconds(10); -constexpr TimeDelta kMaxWaitForFrame = TimeDelta::Seconds(3); // Create a decoder for the preferred codec before the stream starts and any // other decoder lazily on demand. diff --git a/video/video_receive_stream2.h b/video/video_receive_stream2.h index 9add6a1d42..71dbf41b2f 100644 --- a/video/video_receive_stream2.h +++ b/video/video_receive_stream2.h @@ -47,6 +47,9 @@ class RtpStreamReceiverControllerInterface; class RtxReceiveStream; class VCMTiming; +constexpr TimeDelta kMaxWaitForKeyFrame = TimeDelta::Millis(200); +constexpr TimeDelta kMaxWaitForFrame = TimeDelta::Seconds(3); + namespace internal { class CallStats;