From 4d6dd17153bf2d67c162e46412ae4ca73ca04b3b Mon Sep 17 00:00:00 2001
From: Tommi <tommi@webrtc.org>
Date: Mon, 23 May 2022 09:34:41 +0200
Subject: [PATCH] Change VideoStreamEncoder::Stop() to handle stopped TQ

In case the encoder TQ has been stopped and doesn't accept more tasks,
we could end up in a hung state during Stop(). This is a hypothetical
situation, but can be simulated in a test and avoided.

Bug: webrtc:14063
Change-Id: I20f48b11b6266f6875ed5e69de3529212505e439
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/258125
Reviewed-by: Evan Shrubsole <eshr@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36964}
---
 video/video_stream_encoder.cc          | 51 +++++++++++++-------------
 video/video_stream_encoder_unittest.cc |  5 +++
 2 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc
index 1aa5f5e20e..31a9e25d6e 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -731,31 +731,32 @@ void VideoStreamEncoder::Stop() {
 
   rtc::Event shutdown_event;
 
-  encoder_queue_.PostTask([this, &shutdown_event] {
-    RTC_DCHECK_RUN_ON(&encoder_queue_);
-    if (resource_adaptation_processor_) {
-      stream_resource_manager_.StopManagedResources();
-      for (auto* constraint : adaptation_constraints_) {
-        video_stream_adapter_->RemoveAdaptationConstraint(constraint);
-      }
-      for (auto& resource : additional_resources_) {
-        stream_resource_manager_.RemoveResource(resource);
-      }
-      additional_resources_.clear();
-      video_stream_adapter_->RemoveRestrictionsListener(this);
-      video_stream_adapter_->RemoveRestrictionsListener(
-          &stream_resource_manager_);
-      resource_adaptation_processor_->RemoveResourceLimitationsListener(
-          &stream_resource_manager_);
-      stream_resource_manager_.SetAdaptationProcessor(nullptr, nullptr);
-      resource_adaptation_processor_.reset();
-    }
-    rate_allocator_ = nullptr;
-    ReleaseEncoder();
-    encoder_ = nullptr;
-    frame_cadence_adapter_ = nullptr;
-    shutdown_event.Set();
-  });
+  encoder_queue_.PostTask(webrtc::ToQueuedTask(
+      [this] {
+        RTC_DCHECK_RUN_ON(&encoder_queue_);
+        if (resource_adaptation_processor_) {
+          stream_resource_manager_.StopManagedResources();
+          for (auto* constraint : adaptation_constraints_) {
+            video_stream_adapter_->RemoveAdaptationConstraint(constraint);
+          }
+          for (auto& resource : additional_resources_) {
+            stream_resource_manager_.RemoveResource(resource);
+          }
+          additional_resources_.clear();
+          video_stream_adapter_->RemoveRestrictionsListener(this);
+          video_stream_adapter_->RemoveRestrictionsListener(
+              &stream_resource_manager_);
+          resource_adaptation_processor_->RemoveResourceLimitationsListener(
+              &stream_resource_manager_);
+          stream_resource_manager_.SetAdaptationProcessor(nullptr, nullptr);
+          resource_adaptation_processor_.reset();
+        }
+        rate_allocator_ = nullptr;
+        ReleaseEncoder();
+        encoder_ = nullptr;
+        frame_cadence_adapter_ = nullptr;
+      },
+      [&shutdown_event]() { shutdown_event.Set(); }));
   shutdown_event.Wait(rtc::Event::kForever);
 }
 
diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc
index 7ecea2bb2e..9c54c88d5a 100644
--- a/video/video_stream_encoder_unittest.cc
+++ b/video/video_stream_encoder_unittest.cc
@@ -9108,6 +9108,11 @@ TEST(VideoStreamEncoderSimpleTest, CreateDestroy) {
       VideoStreamEncoder::BitrateAllocationCallbackType::
           kVideoBitrateAllocation,
       field_trials);
+
+  // Stop the encoder explicitly. This additional step tests if we could
+  // hang when calling stop and the TQ has been stopped and/or isn't accepting
+  // any more tasks.
+  encoder->Stop();
 }
 
 TEST(VideoStreamEncoderFrameCadenceTest, ActivatesFrameCadenceOnContentType) {