From 2b10c479cef4d2390251343be1c9d8a4a270634f Mon Sep 17 00:00:00 2001 From: Markus Handell Date: Thu, 28 Oct 2021 15:29:42 +0200 Subject: [PATCH] VideoStreamEncoder: clean up threading constraints. The sequences of threads entering the VideoStreamEncoder has been unclear. Fix this by renaming the uninformational |main_queue_| to |worker_queue_|, and introduce a new |network_queue_| which is set on construction. Bug: chromium:1255737 Change-Id: Ic4d3a5b8188b8cc98e60b72aee2c09c9afbc7356 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/236523 Reviewed-by: Henrik Andreassson Reviewed-by: Ilya Nikolaevskiy Commit-Queue: Markus Handell Cr-Commit-Position: refs/heads/main@{#35283} --- call/call.cc | 2 +- video/video_send_stream.cc | 2 + video/video_send_stream.h | 1 + video/video_stream_encoder.cc | 68 ++++++++++++++------------ video/video_stream_encoder.h | 10 ++-- video/video_stream_encoder_unittest.cc | 1 + 6 files changed, 48 insertions(+), 36 deletions(-) diff --git a/call/call.cc b/call/call.cc index 39fb3cfac9..0d58ed8ed9 100644 --- a/call/call.cc +++ b/call/call.cc @@ -1029,7 +1029,7 @@ webrtc::VideoSendStream* Call::CreateVideoSendStream( std::vector ssrcs = config.rtp.ssrcs; VideoSendStream* send_stream = new VideoSendStream( - clock_, num_cpu_cores_, task_queue_factory_, + clock_, num_cpu_cores_, task_queue_factory_, network_thread_, call_stats_->AsRtcpRttStats(), transport_send_.get(), bitrate_allocator_.get(), video_send_delay_stats_.get(), event_log_, std::move(config), std::move(encoder_config), suspended_video_send_ssrcs_, diff --git a/video/video_send_stream.cc b/video/video_send_stream.cc index 8c0f8f6f72..10c209f143 100644 --- a/video/video_send_stream.cc +++ b/video/video_send_stream.cc @@ -113,6 +113,7 @@ VideoSendStream::VideoSendStream( Clock* clock, int num_cpu_cores, TaskQueueFactory* task_queue_factory, + TaskQueueBase* network_queue, RtcpRttStats* call_stats, RtpTransportControllerSendInterface* transport, BitrateAllocatorInterface* bitrate_allocator, @@ -135,6 +136,7 @@ VideoSendStream::VideoSendStream( config_.encoder_settings, std::make_unique(&stats_proxy_), task_queue_factory, + network_queue, GetBitrateAllocationCallbackType(config_))), encoder_feedback_( clock, diff --git a/video/video_send_stream.h b/video/video_send_stream.h index 0d132dd666..58a0f989b2 100644 --- a/video/video_send_stream.h +++ b/video/video_send_stream.h @@ -58,6 +58,7 @@ class VideoSendStream : public webrtc::VideoSendStream { Clock* clock, int num_cpu_cores, TaskQueueFactory* task_queue_factory, + TaskQueueBase* network_queue, RtcpRttStats* call_stats, RtpTransportControllerSendInterface* transport, BitrateAllocatorInterface* bitrate_allocator, diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index 1e68c3d46c..1916a59bd5 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -593,8 +593,10 @@ VideoStreamEncoder::VideoStreamEncoder( const VideoStreamEncoderSettings& settings, std::unique_ptr overuse_detector, TaskQueueFactory* task_queue_factory, + TaskQueueBase* network_queue, BitrateAllocationCallbackType allocation_cb_type) - : main_queue_(TaskQueueBase::Current()), + : worker_queue_(TaskQueueBase::Current()), + network_queue_(network_queue), number_of_cores_(number_of_cores), sink_(nullptr), settings_(settings), @@ -665,7 +667,7 @@ VideoStreamEncoder::VideoStreamEncoder( "EncoderQueue", TaskQueueFactory::Priority::NORMAL)) { TRACE_EVENT0("webrtc", "VideoStreamEncoder::VideoStreamEncoder"); - RTC_DCHECK(main_queue_); + RTC_DCHECK(worker_queue_); RTC_DCHECK(encoder_stats_observer); RTC_DCHECK_GE(number_of_cores, 1); @@ -694,13 +696,13 @@ VideoStreamEncoder::VideoStreamEncoder( } VideoStreamEncoder::~VideoStreamEncoder() { - RTC_DCHECK_RUN_ON(main_queue_); + RTC_DCHECK_RUN_ON(worker_queue_); RTC_DCHECK(!video_source_sink_controller_.HasSource()) << "Must call ::Stop() before destruction."; } void VideoStreamEncoder::Stop() { - RTC_DCHECK_RUN_ON(main_queue_); + RTC_DCHECK_RUN_ON(worker_queue_); video_source_sink_controller_.SetSource(nullptr); rtc::Event shutdown_event; @@ -746,7 +748,7 @@ void VideoStreamEncoder::SetFecControllerOverride( void VideoStreamEncoder::AddAdaptationResource( rtc::scoped_refptr resource) { - RTC_DCHECK_RUN_ON(main_queue_); + RTC_DCHECK_RUN_ON(worker_queue_); TRACE_EVENT0("webrtc", "VideoStreamEncoder::AddAdaptationResource"); // Map any externally added resources as kCpu for the sake of stats reporting. // TODO(hbos): Make the manager map any unknown resources to kCpu and get rid @@ -767,14 +769,14 @@ void VideoStreamEncoder::AddAdaptationResource( std::vector> VideoStreamEncoder::GetAdaptationResources() { - RTC_DCHECK_RUN_ON(main_queue_); + RTC_DCHECK_RUN_ON(worker_queue_); return resource_adaptation_processor_->GetResources(); } void VideoStreamEncoder::SetSource( rtc::VideoSourceInterface* source, const DegradationPreference& degradation_preference) { - RTC_DCHECK_RUN_ON(main_queue_); + RTC_DCHECK_RUN_ON(worker_queue_); video_source_sink_controller_.SetSource(source); input_state_provider_.OnHasInputChanged(source); @@ -794,7 +796,7 @@ void VideoStreamEncoder::SetSource( } void VideoStreamEncoder::SetSink(EncoderSink* sink, bool rotation_applied) { - RTC_DCHECK_RUN_ON(main_queue_); + RTC_DCHECK_RUN_ON(worker_queue_); video_source_sink_controller_.SetRotationApplied(rotation_applied); video_source_sink_controller_.PushSourceSinkSettings(); @@ -818,6 +820,7 @@ void VideoStreamEncoder::SetStartBitrate(int start_bitrate_bps) { void VideoStreamEncoder::ConfigureEncoder(VideoEncoderConfig config, size_t max_data_payload_length) { + RTC_DCHECK_RUN_ON(worker_queue_); encoder_queue_.PostTask( [this, config = std::move(config), max_data_payload_length]() mutable { RTC_DCHECK_RUN_ON(&encoder_queue_); @@ -1099,10 +1102,10 @@ void VideoStreamEncoder::ReconfigureEncoder() { encoder_resolutions.emplace_back(simulcastStream.width, simulcastStream.height); } - main_queue_->PostTask(ToQueuedTask( + worker_queue_->PostTask(ToQueuedTask( task_safety_, [this, max_framerate, alignment, encoder_resolutions = std::move(encoder_resolutions)]() { - RTC_DCHECK_RUN_ON(main_queue_); + RTC_DCHECK_RUN_ON(worker_queue_); if (max_framerate != video_source_sink_controller_.frame_rate_upper_limit() || alignment != video_source_sink_controller_.resolution_alignment() || @@ -1269,6 +1272,8 @@ void VideoStreamEncoder::OnEncoderSettingsChanged() { } void VideoStreamEncoder::OnFrame(const VideoFrame& video_frame) { + // Threading context here under Chromium is the network thread. Test + // environments may currently call in from other alien contexts. RTC_DCHECK_RUNS_SERIALIZED(&incoming_frame_race_checker_); VideoFrame incoming_frame = video_frame; @@ -1379,12 +1384,12 @@ void VideoStreamEncoder::OnDiscardedFrame() { void VideoStreamEncoder::OnConstraintsChanged( const webrtc::VideoTrackSourceConstraints& constraints) { - // This method is called on the network thread. + RTC_DCHECK_RUN_ON(network_queue_); RTC_LOG(LS_INFO) << __func__ << " min_fps " << constraints.min_fps.value_or(-1) << " max_fps " << constraints.max_fps.value_or(-1); - main_queue_->PostTask(ToQueuedTask(task_safety_, [this, constraints] { - RTC_DCHECK_RUN_ON(main_queue_); + worker_queue_->PostTask(ToQueuedTask(task_safety_, [this, constraints] { + RTC_DCHECK_RUN_ON(worker_queue_); source_constraints_ = constraints; })); } @@ -1809,8 +1814,8 @@ void VideoStreamEncoder::EncodeVideoFrame(const VideoFrame& video_frame, } } else { encoder_failed_ = true; - main_queue_->PostTask(ToQueuedTask(task_safety_, [this]() { - RTC_DCHECK_RUN_ON(main_queue_); + worker_queue_->PostTask(ToQueuedTask(task_safety_, [this]() { + RTC_DCHECK_RUN_ON(worker_queue_); settings_.encoder_switch_request_callback->RequestEncoderFallback(); })); } @@ -2171,9 +2176,9 @@ void VideoStreamEncoder::OnVideoSourceRestrictionsUpdated( RTC_LOG(INFO) << "Updating sink restrictions from " << (reason ? reason->Name() : std::string("")) << " to " << restrictions.ToString(); - main_queue_->PostTask(ToQueuedTask( + worker_queue_->PostTask(ToQueuedTask( task_safety_, [this, restrictions = std::move(restrictions)]() { - RTC_DCHECK_RUN_ON(main_queue_); + RTC_DCHECK_RUN_ON(worker_queue_); video_source_sink_controller_.SetRestrictions(std::move(restrictions)); video_source_sink_controller_.PushSourceSinkSettings(); })); @@ -2329,22 +2334,23 @@ void VideoStreamEncoder::CheckForAnimatedContent( RTC_LOG(LS_INFO) << "Removing resolution cap due to no consistent " "animation detection."; } - main_queue_->PostTask(ToQueuedTask(task_safety_, [this, - should_cap_resolution]() { - RTC_DCHECK_RUN_ON(main_queue_); - video_source_sink_controller_.SetPixelsPerFrameUpperLimit( - should_cap_resolution ? absl::optional(kMaxAnimationPixels) - : absl::nullopt); - video_source_sink_controller_.PushSourceSinkSettings(); - })); + worker_queue_->PostTask( + ToQueuedTask(task_safety_, [this, should_cap_resolution]() { + RTC_DCHECK_RUN_ON(worker_queue_); + video_source_sink_controller_.SetPixelsPerFrameUpperLimit( + should_cap_resolution + ? absl::optional(kMaxAnimationPixels) + : absl::nullopt); + video_source_sink_controller_.PushSourceSinkSettings(); + })); } } // RTC_RUN_ON(&encoder_queue_) void VideoStreamEncoder::QueueRequestEncoderSwitch( const EncoderSwitchRequestCallback::Config& conf) { - main_queue_->PostTask(ToQueuedTask(task_safety_, [this, conf]() { - RTC_DCHECK_RUN_ON(main_queue_); + worker_queue_->PostTask(ToQueuedTask(task_safety_, [this, conf]() { + RTC_DCHECK_RUN_ON(worker_queue_); settings_.encoder_switch_request_callback->RequestEncoderSwitch(conf); })); } @@ -2352,8 +2358,8 @@ void VideoStreamEncoder::QueueRequestEncoderSwitch( // RTC_RUN_ON(&encoder_queue_) void VideoStreamEncoder::QueueRequestEncoderSwitch( const webrtc::SdpVideoFormat& format) { - main_queue_->PostTask(ToQueuedTask(task_safety_, [this, format]() { - RTC_DCHECK_RUN_ON(main_queue_); + worker_queue_->PostTask(ToQueuedTask(task_safety_, [this, format]() { + RTC_DCHECK_RUN_ON(worker_queue_); settings_.encoder_switch_request_callback->RequestEncoderSwitch(format); })); } @@ -2367,8 +2373,8 @@ void VideoStreamEncoder::MaybeReportFrameRateConstraintUmas() { encoder_config_.content_type == VideoEncoderConfig::ContentType::kScreen; if (!is_screenshare) return; - main_queue_->PostTask(ToQueuedTask(task_safety_, [this] { - RTC_DCHECK_RUN_ON(main_queue_); + worker_queue_->PostTask(ToQueuedTask(task_safety_, [this] { + RTC_DCHECK_RUN_ON(worker_queue_); RTC_HISTOGRAM_BOOLEAN("WebRTC.Screenshare.FrameRateConstraints.Exists", source_constraints_.has_value()); if (source_constraints_.has_value()) { diff --git a/video/video_stream_encoder.h b/video/video_stream_encoder.h index 8ba0342e2c..0738e1d2f5 100644 --- a/video/video_stream_encoder.h +++ b/video/video_stream_encoder.h @@ -75,6 +75,7 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface, const VideoStreamEncoderSettings& settings, std::unique_ptr overuse_detector, TaskQueueFactory* task_queue_factory, + TaskQueueBase* network_queue, BitrateAllocationCallbackType allocation_cb_type); ~VideoStreamEncoder() override; @@ -231,7 +232,8 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface, // Reports UMAs on frame rate constraints usage on the first call. void MaybeReportFrameRateConstraintUmas() RTC_RUN_ON(&encoder_queue_); - TaskQueueBase* const main_queue_; + TaskQueueBase* const worker_queue_; + TaskQueueBase* const network_queue_; const uint32_t number_of_cores_; @@ -246,7 +248,7 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface, // The source's constraints. absl::optional source_constraints_ - RTC_GUARDED_BY(main_queue_); + RTC_GUARDED_BY(worker_queue_); bool has_reported_screenshare_frame_rate_umas_ RTC_GUARDED_BY(&encoder_queue_) = false; @@ -410,7 +412,7 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface, // to provide us with different resolution or frame rate. // This class is thread-safe. VideoSourceSinkController video_source_sink_controller_ - RTC_GUARDED_BY(main_queue_); + RTC_GUARDED_BY(worker_queue_); // Default bitrate limits in EncoderInfoSettings allowed. const bool default_limits_allowed_; @@ -424,7 +426,7 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface, // first to make sure no tasks run that use other members. rtc::TaskQueue encoder_queue_; - // Used to cancel any potentially pending tasks to the main thread. + // Used to cancel any potentially pending tasks to the worker thread. ScopedTaskSafety task_safety_; RTC_DISALLOW_COPY_AND_ASSIGN(VideoStreamEncoder); diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index 9782e8358e..ca1a527b3d 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -356,6 +356,7 @@ class VideoStreamEncoderUnderTest : public VideoStreamEncoder { overuse_detector_proxy_ = new CpuOveruseDetectorProxy(stats_proxy)), task_queue_factory, + TaskQueueBase::Current(), allocation_callback_type), time_controller_(time_controller), fake_cpu_resource_(FakeResource::Create("FakeResource[CPU]")),