From aee5b17f669341d9af03ab4dc15b38382f423628 Mon Sep 17 00:00:00 2001 From: Markus Handell Date: Fri, 28 Apr 2023 11:16:30 +0200 Subject: [PATCH] DecodeSynchronizer: avoid duplicate tick callback registration. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With repeated CreateSynchronizedFrameScheduler/Stop calls, the DecodeSynchronizer can register & keep multiple callbacks in the metronome. Fix this to only keep at most one callback installed. Fixed: chromium:1434747 Change-Id: I61f67a871339dbcc7560e9d545a5217f361a9b87 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/303840 Reviewed-by: Ilya Nikolaevskiy Reviewed-by: Henrik Boström Commit-Queue: Markus Handell Cr-Commit-Position: refs/heads/main@{#39964} --- video/decode_synchronizer.cc | 5 ++++ video/decode_synchronizer.h | 1 + video/decode_synchronizer_unittest.cc | 33 +++++++++++++++++++++------ 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/video/decode_synchronizer.cc b/video/decode_synchronizer.cc index 2b9a658426..32702c2e81 100644 --- a/video/decode_synchronizer.cc +++ b/video/decode_synchronizer.cc @@ -175,6 +175,10 @@ void DecodeSynchronizer::RemoveFrameScheduler( void DecodeSynchronizer::ScheduleNextTick() { RTC_DCHECK_RUN_ON(worker_queue_); + if (tick_scheduled_) { + return; + } + tick_scheduled_ = true; metronome_->RequestCallOnNextTick( SafeTask(safety_.flag(), [this] { OnTick(); })); } @@ -182,6 +186,7 @@ void DecodeSynchronizer::ScheduleNextTick() { void DecodeSynchronizer::OnTick() { TRACE_EVENT0("webrtc", __func__); RTC_DCHECK_RUN_ON(worker_queue_); + tick_scheduled_ = false; expected_next_tick_ = clock_->CurrentTime() + metronome_->TickPeriod(); for (auto* scheduler : schedulers_) { diff --git a/video/decode_synchronizer.h b/video/decode_synchronizer.h index c6f8efdb29..17181862a4 100644 --- a/video/decode_synchronizer.h +++ b/video/decode_synchronizer.h @@ -129,6 +129,7 @@ class DecodeSynchronizer { Timestamp expected_next_tick_ = Timestamp::PlusInfinity(); std::set schedulers_ RTC_GUARDED_BY(worker_queue_); + bool tick_scheduled_ = false; ScopedTaskSafetyDetached safety_; }; diff --git a/video/decode_synchronizer_unittest.cc b/video/decode_synchronizer_unittest.cc index 7a0d833812..fb48a7e3f6 100644 --- a/video/decode_synchronizer_unittest.cc +++ b/video/decode_synchronizer_unittest.cc @@ -27,6 +27,8 @@ using ::testing::_; using ::testing::Eq; using ::testing::Invoke; +using ::testing::Mock; +using ::testing::MockFunction; using ::testing::Return; namespace webrtc { @@ -60,10 +62,10 @@ class DecodeSynchronizerTest : public ::testing::Test { }; TEST_F(DecodeSynchronizerTest, AllFramesReadyBeforeNextTickDecoded) { - ::testing::MockFunction mock_callback1; + MockFunction mock_callback1; auto scheduler1 = decode_synchronizer_.CreateSynchronizedFrameScheduler(); - testing::MockFunction mock_callback2; + MockFunction mock_callback2; auto scheduler2 = decode_synchronizer_.CreateSynchronizedFrameScheduler(); { @@ -97,7 +99,7 @@ TEST_F(DecodeSynchronizerTest, AllFramesReadyBeforeNextTickDecoded) { } TEST_F(DecodeSynchronizerTest, FramesNotDecodedIfDecodeTimeIsInNextInterval) { - ::testing::MockFunction mock_callback; + MockFunction mock_callback; auto scheduler = decode_synchronizer_.CreateSynchronizedFrameScheduler(); uint32_t frame_rtp = 90000; @@ -125,7 +127,7 @@ TEST_F(DecodeSynchronizerTest, FramesNotDecodedIfDecodeTimeIsInNextInterval) { } TEST_F(DecodeSynchronizerTest, FrameDecodedOnce) { - ::testing::MockFunction mock_callback; + MockFunction mock_callback; auto scheduler = decode_synchronizer_.CreateSynchronizedFrameScheduler(); uint32_t frame_rtp = 90000; @@ -149,7 +151,7 @@ TEST_F(DecodeSynchronizerTest, FrameDecodedOnce) { } TEST_F(DecodeSynchronizerTest, FrameWithDecodeTimeInPastDecodedImmediately) { - ::testing::MockFunction mock_callback; + MockFunction mock_callback; auto scheduler = decode_synchronizer_.CreateSynchronizedFrameScheduler(); uint32_t frame_rtp = 90000; @@ -171,7 +173,7 @@ TEST_F(DecodeSynchronizerTest, FrameWithDecodeTimeInPastDecodedImmediately) { TEST_F(DecodeSynchronizerTest, FrameWithDecodeTimeFarBeforeNextTickDecodedImmediately) { - ::testing::MockFunction mock_callback; + MockFunction mock_callback; auto scheduler = decode_synchronizer_.CreateSynchronizedFrameScheduler(); // Frame which would be behind by more than kMaxAllowedFrameDelay after @@ -210,7 +212,7 @@ TEST_F(DecodeSynchronizerTest, } TEST_F(DecodeSynchronizerTest, FramesNotReleasedAfterStop) { - ::testing::MockFunction mock_callback; + MockFunction mock_callback; auto scheduler = decode_synchronizer_.CreateSynchronizedFrameScheduler(); uint32_t frame_rtp = 90000; @@ -249,4 +251,21 @@ TEST(DecodeSynchronizerStandaloneTest, (std::move)(callback)(); } +TEST(DecodeSynchronizerStandaloneTest, + RegistersCallbackOnceDuringRepeatedRegistrations) { + GlobalSimulatedTimeController time_controller(Timestamp::Millis(4711)); + Clock* clock(time_controller.GetClock()); + MockMetronome metronome; + ON_CALL(metronome, TickPeriod).WillByDefault(Return(TimeDelta::Seconds(1))); + DecodeSynchronizer decode_synchronizer_(clock, &metronome, + time_controller.GetMainThread()); + // Expect at most 1 call to register a callback. + EXPECT_CALL(metronome, RequestCallOnNextTick); + auto scheduler1 = decode_synchronizer_.CreateSynchronizedFrameScheduler(); + scheduler1->Stop(); + auto scheduler2 = decode_synchronizer_.CreateSynchronizedFrameScheduler(); + Mock::VerifyAndClearExpectations(&metronome); + scheduler2->Stop(); +} + } // namespace webrtc