diff --git a/video/frame_cadence_adapter.cc b/video/frame_cadence_adapter.cc index b11c7ba913..8a7852b369 100644 --- a/video/frame_cadence_adapter.cc +++ b/video/frame_cadence_adapter.cc @@ -279,6 +279,11 @@ class VSyncEncodeAdapterMode : public AdapterMode { worker_sequence_checker_.Detach(); } + void PrepareShutdown() { + MutexLock lock(&queue_lock_); + queue_ = nullptr; + } + // Adapter overrides. void OnFrame(Timestamp post_time, bool queue_overload, @@ -309,7 +314,12 @@ class VSyncEncodeAdapterMode : public AdapterMode { }; Clock* const clock_; - TaskQueueBase* queue_; + // Protects `queue_`. + // TODO: crbug.com/358040973 - We should eventually figure out a way to avoid + // lock protection. + Mutex queue_lock_; + TaskQueueBase* queue_ RTC_GUARDED_BY(queue_lock_) + RTC_PT_GUARDED_BY(queue_lock_); RTC_NO_UNIQUE_ADDRESS SequenceChecker queue_sequence_checker_; rtc::scoped_refptr queue_safety_flag_; // Input frame rate statistics for use when not in zero-hertz mode. @@ -826,12 +836,23 @@ void VSyncEncodeAdapterMode::EncodeAllEnqueuedFrames() { (post_time - input.time_when_posted_us).ms()); const VideoFrame frame = std::move(input.video_frame); - queue_->PostTask(SafeTask(queue_safety_flag_, [this, post_time, frame] { - RTC_DCHECK_RUN_ON(queue_); + MutexLock lock(&queue_lock_); + if (queue_) { + queue_->PostTask(SafeTask(queue_safety_flag_, [this, post_time, frame] { + { + MutexLock lock(&queue_lock_); + if (!queue_) { + return; + } + RTC_DCHECK_RUN_ON(queue_); + } - // TODO(b/304158952): Support more refined queue overload control. - callback_->OnFrame(post_time, /*queue_overload=*/false, frame); - })); + // TODO(b/304158952): Support more refined queue overload control. + // Not running under mutex is safe since `callback_` existence is + // guaranteed to exist as long as running encode queue tasks exist. + callback_->OnFrame(post_time, /*queue_overload=*/false, frame); + })); + } } input_queue_.clear(); @@ -858,6 +879,7 @@ FrameCadenceAdapterImpl::~FrameCadenceAdapterImpl() { // VSync adapter needs to be destroyed on worker queue when metronome is // valid. if (metronome_) { + vsync_encode_adapter_->PrepareShutdown(); absl::Cleanup cleanup = [adapter = std::move(vsync_encode_adapter_)] {}; worker_queue_->PostTask([cleanup = std::move(cleanup)] {}); } diff --git a/video/frame_cadence_adapter_unittest.cc b/video/frame_cadence_adapter_unittest.cc index d46434bed3..237b14299e 100644 --- a/video/frame_cadence_adapter_unittest.cc +++ b/video/frame_cadence_adapter_unittest.cc @@ -27,6 +27,7 @@ #include "rtc_base/event.h" #include "rtc_base/logging.h" #include "rtc_base/rate_statistics.h" +#include "rtc_base/task_queue_for_test.h" #include "rtc_base/time_utils.h" #include "system_wrappers/include/metrics.h" #include "system_wrappers/include/ntp_time.h" @@ -579,7 +580,7 @@ TEST(FrameCadenceAdapterTest, EncodeFramesAreAlignedWithMetronomeTick) { "queue", TaskQueueFactory::Priority::NORMAL); auto worker_queue = time_controller.GetTaskQueueFactory()->CreateTaskQueue( "work_queue", TaskQueueFactory::Priority::NORMAL); - static test::FakeMetronome metronome(kTickPeriod); + test::FakeMetronome metronome(kTickPeriod); test::ScopedKeyValueConfig no_field_trials; auto adapter = FrameCadenceAdapterInterface::Create( time_controller.GetClock(), queue.get(), &metronome, worker_queue.get(), @@ -646,6 +647,34 @@ TEST(FrameCadenceAdapterTest, EncodeFramesAreAlignedWithMetronomeTick) { finalized.Wait(rtc::Event::kForever); } +TEST(FrameCadenceAdapterTest, ShutdownUnderMetronome) { + // Regression test for crbug.com/356423094. + // The current thread takes the role of worker queue. + GlobalSimulatedTimeController time_controller(Timestamp::Zero()); + static constexpr TimeDelta kTickPeriod = TimeDelta::Millis(100); + auto queue = time_controller.GetTaskQueueFactory()->CreateTaskQueue( + "queue", TaskQueueFactory::Priority::NORMAL); + test::FakeMetronome metronome(kTickPeriod); + test::ScopedKeyValueConfig no_field_trials; + auto adapter = FrameCadenceAdapterInterface::Create( + time_controller.GetClock(), queue.get(), &metronome, + TaskQueueBase::Current(), no_field_trials); + MockCallback callback; + EXPECT_CALL(callback, OnFrame).Times(0); + adapter->Initialize(&callback); + + // Pass a frame, this is expected to trigger an encode call in the future. + adapter->OnFrame(CreateFrame()); + + // Then post destruction of the adapter and destroy the encode queue from the + // worker (i.e. current). + SendTask(queue.get(), [&] { adapter = nullptr; }); + queue = nullptr; + + // Now that we advance time, there should be no encoding happening. + time_controller.AdvanceTime(TimeDelta::Millis(100)); +} + class FrameCadenceAdapterSimulcastLayersParamTest : public ::testing::TestWithParam { public: