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())