Make VideoSendStream::UpdateActiveSimulcastLayers not block.

UpdateActiveSimulcastLayers has been blocking
WebRtcVideoChannel::SetSend which may be called quite frequently during
negotiations. This CL changes UpdateActiveSimulcastLayers to not
synchronize with the transport's task queue to wait for the changes to
get applied.

This synchronization is quite costly, but so too are other remaining
things in VideoSendStream, so we should aim to get rid of the
`thread_sync_event_` in VideoSendStream.

Bug: webrtc:12840, webrtc:12854
Change-Id: Idb48d29b6b8382881c7c1e6f1d0f5e708dbca30f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/221203
Commit-Queue: Tommi <tommi@webrtc.org>
Reviewed-by: Markus Handell <handellm@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34228}

media/BUILD.gn

@@ -640,6 +640,7 @@ if (rtc_include_tests) {
         "../rtc_base/experiments:min_video_bitrate_experiment",
         "../rtc_base/synchronization:mutex",
         "../rtc_base/third_party/sigslot",
+        "../system_wrappers:field_trial",
         "../test:audio_codec_mocks",
         "../test:fake_video_codecs",
         "../test:field_trial",

media/engine/webrtc_video_engine_unittest.cc

@@ -53,11 +53,13 @@
 #include "media/engine/webrtc_voice_engine.h"
 #include "modules/rtp_rtcp/source/rtp_packet.h"
 #include "rtc_base/arraysize.h"
+#include "rtc_base/event.h"
 #include "rtc_base/experiments/min_video_bitrate_experiment.h"
 #include "rtc_base/fake_clock.h"
 #include "rtc_base/gunit.h"
 #include "rtc_base/numerics/safe_conversions.h"
 #include "rtc_base/time_utils.h"
+#include "system_wrappers/include/field_trial.h"
 #include "test/fake_decoder.h"
 #include "test/field_trial.h"
 #include "test/frame_forwarder.h"
@@ -1738,6 +1740,7 @@ class WebRtcVideoChannelBaseTest : public ::testing::Test {
 
   webrtc::RtcEventLogNull event_log_;
   webrtc::FieldTrialBasedConfig field_trials_;
+  std::unique_ptr<webrtc::test::ScopedFieldTrials> override_field_trials_;
   std::unique_ptr<webrtc::TaskQueueFactory> task_queue_factory_;
   std::unique_ptr<webrtc::Call> call_;
   std::unique_ptr<webrtc::VideoBitrateAllocatorFactory>
@@ -1792,8 +1795,10 @@ TEST_F(WebRtcVideoChannelBaseTest, OverridesRecvBufferSize) {
   // Set field trial to override the default recv buffer size, and then re-run
   // setup where the interface is created and configured.
   const int kCustomRecvBufferSize = 123456;
-  webrtc::test::ScopedFieldTrials field_trial(
+  RTC_DCHECK(!override_field_trials_);
+  override_field_trials_ = std::make_unique<webrtc::test::ScopedFieldTrials>(
       "WebRTC-IncreasedReceivebuffers/123456/");
+
   ResetTest();
 
   EXPECT_TRUE(SetOneCodec(DefaultCodec()));
@@ -1808,7 +1813,8 @@ TEST_F(WebRtcVideoChannelBaseTest, OverridesRecvBufferSizeWithSuffix) {
   // Set field trial to override the default recv buffer size, and then re-run
   // setup where the interface is created and configured.
   const int kCustomRecvBufferSize = 123456;
-  webrtc::test::ScopedFieldTrials field_trial(
+  RTC_DCHECK(!override_field_trials_);
+  override_field_trials_ = std::make_unique<webrtc::test::ScopedFieldTrials>(
       "WebRTC-IncreasedReceivebuffers/123456_Dogfood/");
   ResetTest();
 
@@ -1825,18 +1831,46 @@ TEST_F(WebRtcVideoChannelBaseTest, InvalidRecvBufferSize) {
   // then re-run setup where the interface is created and configured. The
   // default value should still be used.
 
+  const char* prev_field_trials = webrtc::field_trial::GetFieldTrialString();
+
+  std::string field_trial_string;
   for (std::string group : {" ", "NotANumber", "-1", "0"}) {
-    std::string field_trial_string = "WebRTC-IncreasedReceivebuffers/";
-    field_trial_string += group;
-    field_trial_string += "/";
-    webrtc::test::ScopedFieldTrials field_trial(field_trial_string);
-    ResetTest();
+    std::string trial_string = "WebRTC-IncreasedReceivebuffers/";
+    trial_string += group;
+    trial_string += "/";
+
+    // Dear reader. Sorry for this... it's a bit of a mess.
+    // TODO(bugs.webrtc.org/12854): This test needs to be rewritten to not use
+    // ResetTest and changing global field trials in a loop.
+    TearDown();
+    // This is a hack to appease tsan. Because of the way the test is written
+    // active state within Call, including running task queues may race with
+    // the test changing the global field trial variable.
+    // This particular hack, pauses the transport controller TQ while we
+    // change the field trial.
+    rtc::TaskQueue* tq = call_->GetTransportControllerSend()->GetWorkerQueue();
+    rtc::Event waiting, resume;
+    tq->PostTask([&waiting, &resume]() {
+      waiting.Set();
+      resume.Wait(rtc::Event::kForever);
+    });
+
+    waiting.Wait(rtc::Event::kForever);
+    field_trial_string = std::move(trial_string);
+    webrtc::field_trial::InitFieldTrialsFromString(field_trial_string.c_str());
+
+    SetUp();
+    resume.Set();
+
+    // OK, now the test can carry on.
 
     EXPECT_TRUE(SetOneCodec(DefaultCodec()));
     EXPECT_TRUE(SetSend(true));
     EXPECT_EQ(64 * 1024, network_interface_.sendbuf_size());
     EXPECT_EQ(256 * 1024, network_interface_.recvbuf_size());
   }
+
+  webrtc::field_trial::InitFieldTrialsFromString(prev_field_trials);
 }
 
 // Test that stats work properly for a 1-1 call.
@@ -2940,7 +2974,7 @@ TEST_F(WebRtcVideoChannelTest, RecvAbsoluteSendTimeHeaderExtensions) {
 }
 
 TEST_F(WebRtcVideoChannelTest, FiltersExtensionsPicksTransportSeqNum) {
-  webrtc::test::ScopedFieldTrials override_field_trials_(
+  webrtc::test::ScopedFieldTrials override_field_trials(
       "WebRTC-FilterAbsSendTimeExtension/Enabled/");
   // Enable three redundant extensions.
   std::vector<std::string> extensions;

video/video_send_stream.cc

@@ -205,12 +205,10 @@ void VideoSendStream::UpdateActiveSimulcastLayers(
   RTC_LOG(LS_INFO) << "UpdateActiveSimulcastLayers: "
                    << active_layers_string.str();
 
-  rtp_transport_queue_->PostTask([this, active_layers] {
-    send_stream_.UpdateActiveSimulcastLayers(active_layers);
-    thread_sync_event_.Set();
-  });
-
-  thread_sync_event_.Wait(rtc::Event::kForever);
+  rtp_transport_queue_->PostTask(
+      ToQueuedTask(transport_queue_safety_, [this, active_layers] {
+        send_stream_.UpdateActiveSimulcastLayers(active_layers);
+      }));
 }
 
 void VideoSendStream::Start() {
@@ -221,14 +219,16 @@ void VideoSendStream::Start() {
 
   running_ = true;
 
-  rtp_transport_queue_->PostTask([this] {
+  rtp_transport_queue_->PostTask(ToQueuedTask([this] {
+    transport_queue_safety_->SetAlive();
     send_stream_.Start();
     thread_sync_event_.Set();
-  });
+  }));
 
   // It is expected that after VideoSendStream::Start has been called, incoming
   // frames are not dropped in VideoStreamEncoder. To ensure this, Start has to
   // be synchronized.
+  // TODO(tommi): ^^^ Validate if this still holds.
   thread_sync_event_.Wait(rtc::Event::kForever);
 }
 
@@ -238,7 +238,10 @@ void VideoSendStream::Stop() {
     return;
   RTC_DLOG(LS_INFO) << "VideoSendStream::Stop";
   running_ = false;
-  send_stream_.Stop();  // Stop() will proceed asynchronously.
+  rtp_transport_queue_->PostTask(ToQueuedTask(transport_queue_safety_, [this] {
+    transport_queue_safety_->SetNotAlive();
+    send_stream_.Stop();
+  }));
 }
 
 void VideoSendStream::AddAdaptationResource(
@@ -290,6 +293,7 @@ void VideoSendStream::StopPermanentlyAndGetRtpStates(
   // or not. This will unregister callbacks before destruction.
   // See `VideoSendStreamImpl::StopVideoSendStream` for more.
   rtp_transport_queue_->PostTask([this, rtp_state_map, payload_state_map]() {
+    transport_queue_safety_->SetNotAlive();
     send_stream_.Stop();
     *rtp_state_map = send_stream_.GetRtpStates();
     *payload_state_map = send_stream_.GetRtpPayloadStates();

video/video_send_stream.h

@@ -24,6 +24,7 @@
 #include "rtc_base/event.h"
 #include "rtc_base/system/no_unique_address.h"
 #include "rtc_base/task_queue.h"
+#include "rtc_base/task_utils/pending_task_safety_flag.h"
 #include "video/encoder_rtcp_feedback.h"
 #include "video/send_delay_stats.h"
 #include "video/send_statistics_proxy.h"
@@ -103,6 +104,8 @@ class VideoSendStream : public webrtc::VideoSendStream {
   rtc::TaskQueue* const rtp_transport_queue_;
   RtpTransportControllerSendInterface* const transport_;
   rtc::Event thread_sync_event_;
+  rtc::scoped_refptr<PendingTaskSafetyFlag> transport_queue_safety_ =
+      PendingTaskSafetyFlag::CreateDetached();
 
   SendStatisticsProxy stats_proxy_;
   const VideoSendStream::Config config_;

video/video_send_stream_impl.cc

@@ -364,11 +364,6 @@ void VideoSendStreamImpl::StartupVideoSendStream() {
 }
 
 void VideoSendStreamImpl::Stop() {
-  if (!rtp_transport_queue_->IsCurrent()) {
-    rtp_transport_queue_->PostTask(
-        ToQueuedTask(transport_queue_safety_, [this] { Stop(); }));
-    return;
-  }
   RTC_DCHECK_RUN_ON(rtp_transport_queue_);
   RTC_LOG(LS_INFO) << "VideoSendStreamImpl::Stop";
   if (!rtp_video_sender_->IsActive())