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}
diff --git a/media/BUILD.gn b/media/BUILD.gn
index 9430a96..9c57a1d 100644
--- a/media/BUILD.gn
+++ b/media/BUILD.gn
@@ -640,6 +640,7 @@
"../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",
diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc
index 1cbd2dc..4805e97 100644
--- a/media/engine/webrtc_video_engine_unittest.cc
+++ b/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 @@
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 @@
// 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 @@
// 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 @@
// 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, 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;
diff --git a/video/video_send_stream.cc b/video/video_send_stream.cc
index 6795b23..591a8d5 100644
--- a/video/video_send_stream.cc
+++ b/video/video_send_stream.cc
@@ -205,12 +205,10 @@
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 @@
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 @@
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 @@
// 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();
diff --git a/video/video_send_stream.h b/video/video_send_stream.h
index 5d4cf80..7e89c46 100644
--- a/video/video_send_stream.h
+++ b/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 @@
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_;
diff --git a/video/video_send_stream_impl.cc b/video/video_send_stream_impl.cc
index 687121a..3fc6b67 100644
--- a/video/video_send_stream_impl.cc
+++ b/video/video_send_stream_impl.cc
@@ -364,11 +364,6 @@
}
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())
