From a334dc68f378b9378649d85ebe83c21e87c0aa3c Mon Sep 17 00:00:00 2001 From: Tommi Date: Fri, 4 Jun 2021 12:50:01 +0200 Subject: [PATCH] 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 Reviewed-by: Markus Handell Cr-Commit-Position: refs/heads/master@{#34228} --- media/BUILD.gn | 1 + media/engine/webrtc_video_engine_unittest.cc | 50 ++++++++++++++++---- video/video_send_stream.cc | 22 +++++---- video/video_send_stream.h | 3 ++ video/video_send_stream_impl.cc | 5 -- 5 files changed, 59 insertions(+), 22 deletions(-) diff --git a/media/BUILD.gn b/media/BUILD.gn index 9430a96039..9c57a1d16b 100644 --- a/media/BUILD.gn +++ b/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", diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index 1cbd2dce26..4805e9792e 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 @@ class WebRtcVideoChannelBaseTest : public ::testing::Test { webrtc::RtcEventLogNull event_log_; webrtc::FieldTrialBasedConfig field_trials_; + std::unique_ptr override_field_trials_; std::unique_ptr task_queue_factory_; std::unique_ptr call_; std::unique_ptr @@ -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-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-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 extensions; diff --git a/video/video_send_stream.cc b/video/video_send_stream.cc index 6795b23dd3..591a8d58d8 100644 --- a/video/video_send_stream.cc +++ b/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(); diff --git a/video/video_send_stream.h b/video/video_send_stream.h index 5d4cf80f75..7e89c46abd 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 @@ class VideoSendStream : public webrtc::VideoSendStream { rtc::TaskQueue* const rtp_transport_queue_; RtpTransportControllerSendInterface* const transport_; rtc::Event thread_sync_event_; + rtc::scoped_refptr 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 687121ae2b..3fc6b676dc 100644 --- a/video/video_send_stream_impl.cc +++ b/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())