From 8fe5579136c82b558022fe7d1393fa248b151eae Mon Sep 17 00:00:00 2001 From: Evan Shrubsole Date: Thu, 27 Oct 2022 13:05:44 +0200 Subject: [PATCH] Ensure video frame buffer is still decodable before decoding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This ensures that if for some reason, the frame buffer becomes undecodable while waiting to decode a frame, the decoding is halted. This also guards against receiving an empty temporal unit from the frame buffer, even though this should never happen when the frame buffer has a decodable temporal unit. Bug: chromium:1378253, chromium:1361623 Change-Id: I8c4c897bf474d5cbda5f0f357781bf1dc0701fe4 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/280701 Commit-Queue: Evan Shrubsole Reviewed-by: Erik Språng Cr-Commit-Position: refs/heads/main@{#38494} --- video/task_queue_frame_decode_scheduler.cc | 4 +- video/video_stream_buffer_controller.cc | 29 +++++++++--- ...video_stream_buffer_controller_unittest.cc | 44 +++++++++++++++++++ 3 files changed, 70 insertions(+), 7 deletions(-) diff --git a/video/task_queue_frame_decode_scheduler.cc b/video/task_queue_frame_decode_scheduler.cc index c53a53073e..cd109c2932 100644 --- a/video/task_queue_frame_decode_scheduler.cc +++ b/video/task_queue_frame_decode_scheduler.cc @@ -49,8 +49,8 @@ void TaskQueueFrameDecodeScheduler::ScheduleFrame( SafeTask(task_safety_.flag(), [this, rtp, schedule, cb = std::move(cb)]() mutable { RTC_DCHECK_RUN_ON(bookkeeping_queue_); - // If the next frame rtp has changed since this task was - // this scheduled release should be skipped. + // If the next frame rtp has changed since this task was + // this scheduled release should be skipped. if (scheduled_rtp_ != rtp) return; scheduled_rtp_ = absl::nullopt; diff --git a/video/video_stream_buffer_controller.cc b/video/video_stream_buffer_controller.cc index 0e0f2ccc9e..f7d3acdaf6 100644 --- a/video/video_stream_buffer_controller.cc +++ b/video/video_stream_buffer_controller.cc @@ -191,7 +191,8 @@ void VideoStreamBufferController::OnFrameReady( absl::InlinedVector, 4> frames, Timestamp render_time) { RTC_DCHECK_RUN_ON(&worker_sequence_checker_); - RTC_DCHECK(!frames.empty()); + RTC_CHECK(!frames.empty()) + << "Callers must ensure there is at least one frame to decode."; timeout_tracker_.OnEncodedFrameReleased(); @@ -275,11 +276,29 @@ void VideoStreamBufferController::OnTimeout(TimeDelta delay) { void VideoStreamBufferController::FrameReadyForDecode(uint32_t rtp_timestamp, Timestamp render_time) { RTC_DCHECK_RUN_ON(&worker_sequence_checker_); - auto frames = buffer_->ExtractNextDecodableTemporalUnit(); - RTC_DCHECK(frames[0]->Timestamp() == rtp_timestamp) + // Check that the frame to decode is still valid before passing the frame for + // decoding. + auto decodable_tu_info = buffer_->DecodableTemporalUnitsInfo(); + if (!decodable_tu_info) { + RTC_LOG(LS_ERROR) + << "The frame buffer became undecodable during the wait " + "to decode frame with rtp-timestamp " + << rtp_timestamp + << ". Cancelling the decode of this frame, decoding " + "will resume when the frame buffers become decodable again."; + return; + } + RTC_DCHECK_EQ(rtp_timestamp, decodable_tu_info->next_rtp_timestamp) << "Frame buffer's next decodable frame was not the one sent for " - "extraction rtp=" - << rtp_timestamp << " extracted rtp=" << frames[0]->Timestamp(); + "extraction."; + auto frames = buffer_->ExtractNextDecodableTemporalUnit(); + if (frames.empty()) { + RTC_LOG(LS_ERROR) + << "The frame buffer should never return an empty temporal until list " + "when there is a decodable temporal unit."; + RTC_DCHECK_NOTREACHED(); + return; + } OnFrameReady(std::move(frames), render_time); } diff --git a/video/video_stream_buffer_controller_unittest.cc b/video/video_stream_buffer_controller_unittest.cc index ea9eaede2e..3e6c352fb1 100644 --- a/video/video_stream_buffer_controller_unittest.cc +++ b/video/video_stream_buffer_controller_unittest.cc @@ -754,6 +754,50 @@ TEST_P(VideoStreamBufferControllerTest, NextFrameWithOldTimestamp) { EXPECT_THAT(WaitForFrameOrTimeout(kFps30Delay), Frame(test::WithId(2))); } +TEST_P(VideoStreamBufferControllerTest, + FrameNotSetForDecodedIfFrameBufferBecomesNonDecodable) { + // This can happen if the frame buffer receives non-standard input. This test + // will simply clear the frame buffer to replicate this. + StartNextDecodeForceKeyframe(); + // Initial keyframe. + buffer_->InsertFrame(WithReceiveTimeFromRtpTimestamp( + test::FakeFrameBuilder().Id(0).Time(0).SpatialLayer(1).AsLast().Build())); + EXPECT_THAT(WaitForFrameOrTimeout(TimeDelta::Zero()), Frame(test::WithId(0))); + + // Insert a frame that will become non-decodable. + buffer_->InsertFrame(WithReceiveTimeFromRtpTimestamp(test::FakeFrameBuilder() + .Id(11) + .Time(kFps30Rtp) + .Refs({0}) + .SpatialLayer(1) + .AsLast() + .Build())); + StartNextDecode(); + // Second layer inserted after last layer for the same frame out-of-order. + // This second frame requires some older frame to be decoded and so now the + // super-frame is no longer decodable despite already being scheduled. + buffer_->InsertFrame(WithReceiveTimeFromRtpTimestamp(test::FakeFrameBuilder() + .Id(10) + .Time(kFps30Rtp) + .SpatialLayer(0) + .Refs({2}) + .Build())); + EXPECT_THAT(WaitForFrameOrTimeout(kMaxWaitForFrame), TimedOut()); + + // Ensure that this frame can be decoded later. + StartNextDecode(); + buffer_->InsertFrame(WithReceiveTimeFromRtpTimestamp(test::FakeFrameBuilder() + .Id(2) + .Time(kFps30Rtp / 2) + .SpatialLayer(0) + .Refs({0}) + .AsLast() + .Build())); + EXPECT_THAT(WaitForFrameOrTimeout(kFps30Delay), Frame(test::WithId(2))); + StartNextDecode(); + EXPECT_THAT(WaitForFrameOrTimeout(kFps30Delay), Frame(test::WithId(10))); +} + INSTANTIATE_TEST_SUITE_P(VideoStreamBufferController, VideoStreamBufferControllerTest, ::testing::Combine(::testing::Bool(),