From d41c2a6b8aeb8a2bfec29689c68cba79638d902a Mon Sep 17 00:00:00 2001 From: Tomas Gunnarsson Date: Mon, 21 Sep 2020 15:56:42 +0200 Subject: [PATCH] Remove AsyncInvoker from WebRtcVideoChannel. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RequestEncoderFallback, RequestEncoderSwitch and SetVideoCodecSwitchingEnabledRequest are now all called on the worker thread. Before, the work already happened on that thread but WebRtcVideoChannel adapted internally when needed. With this CL, there are thread checks to make sure that these calls are always made the same way, we don't need the async invoker and there are fewer calls out from the encoder thread in VideoStreamEncoder (reducing the chance of unintentional blocking). Bug: webrtc:11908 Change-Id: If8738bc2a708a0fefc6fe850b32655f049f30bdc Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/184603 Commit-Queue: Tommi Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/master@{#32151} --- media/engine/webrtc_video_engine.cc | 149 +++++++++++-------------- media/engine/webrtc_video_engine.h | 5 - pc/peer_connection.cc | 18 ++- video/video_stream_encoder.cc | 34 ++++-- video/video_stream_encoder.h | 7 ++ video/video_stream_encoder_unittest.cc | 4 + 6 files changed, 119 insertions(+), 98 deletions(-) diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 2b4e3a4e39..4b7d9e5953 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -39,6 +39,7 @@ #include "rtc_base/logging.h" #include "rtc_base/numerics/safe_conversions.h" #include "rtc_base/strings/string_builder.h" +#include "rtc_base/thread.h" #include "rtc_base/time_utils.h" #include "rtc_base/trace_event.h" #include "system_wrappers/include/field_trial.h" @@ -836,97 +837,85 @@ bool WebRtcVideoChannel::SetSendParameters(const VideoSendParameters& params) { } void WebRtcVideoChannel::RequestEncoderFallback() { - invoker_.AsyncInvoke( - RTC_FROM_HERE, worker_thread_, [this] { - RTC_DCHECK_RUN_ON(&thread_checker_); - if (negotiated_codecs_.size() <= 1) { - RTC_LOG(LS_WARNING) - << "Encoder failed but no fallback codec is available"; - return; - } + RTC_DCHECK_RUN_ON(&thread_checker_); + if (negotiated_codecs_.size() <= 1) { + RTC_LOG(LS_WARNING) << "Encoder failed but no fallback codec is available"; + return; + } - ChangedSendParameters params; - params.negotiated_codecs = negotiated_codecs_; - params.negotiated_codecs->erase(params.negotiated_codecs->begin()); - params.send_codec = params.negotiated_codecs->front(); - ApplyChangedParams(params); - }); + ChangedSendParameters params; + params.negotiated_codecs = negotiated_codecs_; + params.negotiated_codecs->erase(params.negotiated_codecs->begin()); + params.send_codec = params.negotiated_codecs->front(); + ApplyChangedParams(params); } void WebRtcVideoChannel::RequestEncoderSwitch( const EncoderSwitchRequestCallback::Config& conf) { - invoker_.AsyncInvoke(RTC_FROM_HERE, worker_thread_, [this, conf] { - RTC_DCHECK_RUN_ON(&thread_checker_); + RTC_DCHECK_RUN_ON(&thread_checker_); - if (!allow_codec_switching_) { - RTC_LOG(LS_INFO) << "Encoder switch requested but codec switching has" - " not been enabled yet."; - requested_encoder_switch_ = conf; - return; - } + if (!allow_codec_switching_) { + RTC_LOG(LS_INFO) << "Encoder switch requested but codec switching has" + " not been enabled yet."; + requested_encoder_switch_ = conf; + return; + } - for (const VideoCodecSettings& codec_setting : negotiated_codecs_) { - if (codec_setting.codec.name == conf.codec_name) { - if (conf.param) { - auto it = codec_setting.codec.params.find(*conf.param); + for (const VideoCodecSettings& codec_setting : negotiated_codecs_) { + if (codec_setting.codec.name == conf.codec_name) { + if (conf.param) { + auto it = codec_setting.codec.params.find(*conf.param); + if (it == codec_setting.codec.params.end()) + continue; - if (it == codec_setting.codec.params.end()) { - continue; - } + if (conf.value && it->second != *conf.value) + continue; + } - if (conf.value && it->second != *conf.value) { - continue; - } - } - - if (send_codec_ == codec_setting) { - // Already using this codec, no switch required. - return; - } - - ChangedSendParameters params; - params.send_codec = codec_setting; - ApplyChangedParams(params); + if (send_codec_ == codec_setting) { + // Already using this codec, no switch required. return; } - } - RTC_LOG(LS_WARNING) << "Requested encoder with codec_name:" - << conf.codec_name - << ", param:" << conf.param.value_or("none") - << " and value:" << conf.value.value_or("none") - << "not found. No switch performed."; - }); + ChangedSendParameters params; + params.send_codec = codec_setting; + ApplyChangedParams(params); + return; + } + } + + RTC_LOG(LS_WARNING) << "Requested encoder with codec_name:" << conf.codec_name + << ", param:" << conf.param.value_or("none") + << " and value:" << conf.value.value_or("none") + << "not found. No switch performed."; } void WebRtcVideoChannel::RequestEncoderSwitch( const webrtc::SdpVideoFormat& format) { - invoker_.AsyncInvoke(RTC_FROM_HERE, worker_thread_, [this, format] { - RTC_DCHECK_RUN_ON(&thread_checker_); + RTC_DCHECK_RUN_ON(&thread_checker_); - for (const VideoCodecSettings& codec_setting : negotiated_codecs_) { - if (IsSameCodec(format.name, format.parameters, codec_setting.codec.name, - codec_setting.codec.params)) { - VideoCodecSettings new_codec_setting = codec_setting; - for (const auto& kv : format.parameters) { - new_codec_setting.codec.params[kv.first] = kv.second; - } + for (const VideoCodecSettings& codec_setting : negotiated_codecs_) { + if (IsSameCodec(format.name, format.parameters, codec_setting.codec.name, + codec_setting.codec.params)) { + VideoCodecSettings new_codec_setting = codec_setting; + for (const auto& kv : format.parameters) { + new_codec_setting.codec.params[kv.first] = kv.second; + } - if (send_codec_ == new_codec_setting) { - // Already using this codec, no switch required. - return; - } - - ChangedSendParameters params; - params.send_codec = new_codec_setting; - ApplyChangedParams(params); + if (send_codec_ == new_codec_setting) { + // Already using this codec, no switch required. return; } - } - RTC_LOG(LS_WARNING) << "Encoder switch failed: SdpVideoFormat " - << format.ToString() << " not negotiated."; - }); + ChangedSendParameters params; + params.send_codec = new_codec_setting; + ApplyChangedParams(params); + return; + } + } + + RTC_LOG(LS_WARNING) << "Encoder switch failed: SdpVideoFormat " + << format.ToString() << " not negotiated."; } bool WebRtcVideoChannel::ApplyChangedParams( @@ -1830,18 +1819,16 @@ void WebRtcVideoChannel::SetFrameEncryptor( } void WebRtcVideoChannel::SetVideoCodecSwitchingEnabled(bool enabled) { - invoker_.AsyncInvoke(RTC_FROM_HERE, worker_thread_, [this, enabled] { - RTC_DCHECK_RUN_ON(&thread_checker_); - allow_codec_switching_ = enabled; - if (allow_codec_switching_) { - RTC_LOG(LS_INFO) << "Encoder switching enabled."; - if (requested_encoder_switch_) { - RTC_LOG(LS_INFO) << "Executing cached video encoder switch request."; - RequestEncoderSwitch(*requested_encoder_switch_); - requested_encoder_switch_.reset(); - } + RTC_DCHECK_RUN_ON(&thread_checker_); + allow_codec_switching_ = enabled; + if (allow_codec_switching_) { + RTC_LOG(LS_INFO) << "Encoder switching enabled."; + if (requested_encoder_switch_) { + RTC_LOG(LS_INFO) << "Executing cached video encoder switch request."; + RequestEncoderSwitch(*requested_encoder_switch_); + requested_encoder_switch_.reset(); } - }); + } } bool WebRtcVideoChannel::SetBaseMinimumPlayoutDelayMs(uint32_t ssrc, diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h index b9f27b4eec..f00d0c88e4 100644 --- a/media/engine/webrtc_video_engine.h +++ b/media/engine/webrtc_video_engine.h @@ -31,7 +31,6 @@ #include "media/base/media_engine.h" #include "media/engine/constants.h" #include "media/engine/unhandled_packets_buffer.h" -#include "rtc_base/async_invoker.h" #include "rtc_base/network_route.h" #include "rtc_base/synchronization/mutex.h" #include "rtc_base/thread_annotations.h" @@ -634,10 +633,6 @@ class WebRtcVideoChannel : public VideoMediaChannel, bool allow_codec_switching_ = false; absl::optional requested_encoder_switch_; - - // In order for the |invoker_| to protect other members from being destructed - // as they are used in asynchronous tasks it has to be destructed first. - rtc::AsyncInvoker invoker_; }; class EncoderStreamFactory diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 36dc5b27a4..303f5f97f0 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -4082,16 +4082,24 @@ RTCError PeerConnection::SetConfiguration( } if (modified_config.allow_codec_switching.has_value()) { + std::vector channels; for (const auto& transceiver : transceivers_) { - if (transceiver->media_type() != cricket::MEDIA_TYPE_VIDEO || - !transceiver->internal()->channel()) { + if (transceiver->media_type() != cricket::MEDIA_TYPE_VIDEO) continue; - } + auto* video_channel = static_cast( transceiver->internal()->channel()); - video_channel->media_channel()->SetVideoCodecSwitchingEnabled( - *modified_config.allow_codec_switching); + if (video_channel) + channels.push_back(video_channel->media_channel()); } + + worker_thread()->Invoke( + RTC_FROM_HERE, + [channels = std::move(channels), + allow_codec_switching = *modified_config.allow_codec_switching]() { + for (auto* ch : channels) + ch->SetVideoCodecSwitchingEnabled(allow_codec_switching); + }); } configuration_ = modified_config; diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index f8ca4dce3c..65bd27bf25 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -543,7 +543,7 @@ void VideoStreamEncoder::ReconfigureEncoder() { conf.codec_name = encoder_switch_experiment_.to_codec; conf.param = encoder_switch_experiment_.to_param; conf.value = encoder_switch_experiment_.to_value; - settings_.encoder_switch_request_callback->RequestEncoderSwitch(conf); + QueueRequestEncoderSwitch(conf); encoder_switch_requested_ = true; } @@ -1425,12 +1425,14 @@ void VideoStreamEncoder::EncodeVideoFrame(const VideoFrame& video_frame, if (settings_.encoder_switch_request_callback) { if (encoder_selector_) { if (auto encoder = encoder_selector_->OnEncoderBroken()) { - settings_.encoder_switch_request_callback->RequestEncoderSwitch( - *encoder); + QueueRequestEncoderSwitch(*encoder); } } else { encoder_failed_ = true; - settings_.encoder_switch_request_callback->RequestEncoderFallback(); + main_queue_->PostTask(ToQueuedTask(task_safety_, [this]() { + RTC_DCHECK_RUN_ON(main_queue_); + settings_.encoder_switch_request_callback->RequestEncoderFallback(); + })); } } else { RTC_LOG(LS_ERROR) @@ -1690,8 +1692,7 @@ void VideoStreamEncoder::OnBitrateUpdated(DataRate target_bitrate, if (encoder_selector_) { if (auto encoder = encoder_selector_->OnAvailableBitrate(link_allocation)) { - settings_.encoder_switch_request_callback->RequestEncoderSwitch( - *encoder); + QueueRequestEncoderSwitch(*encoder); } } else if (encoder_switch_experiment_.IsBitrateBelowThreshold( target_bitrate) && @@ -1700,7 +1701,7 @@ void VideoStreamEncoder::OnBitrateUpdated(DataRate target_bitrate, conf.codec_name = encoder_switch_experiment_.to_codec; conf.param = encoder_switch_experiment_.to_param; conf.value = encoder_switch_experiment_.to_value; - settings_.encoder_switch_request_callback->RequestEncoderSwitch(conf); + QueueRequestEncoderSwitch(conf); encoder_switch_requested_ = true; } @@ -2060,6 +2061,25 @@ void VideoStreamEncoder::CheckForAnimatedContent( })); } } + +// 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_); + settings_.encoder_switch_request_callback->RequestEncoderSwitch(conf); + })); +} + +// 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_); + settings_.encoder_switch_request_callback->RequestEncoderSwitch(format); + })); +} + void VideoStreamEncoder::InjectAdaptationResource( rtc::scoped_refptr resource, VideoAdaptationReason reason) { diff --git a/video/video_stream_encoder.h b/video/video_stream_encoder.h index 5ab0840059..e80d1203c7 100644 --- a/video/video_stream_encoder.h +++ b/video/video_stream_encoder.h @@ -214,6 +214,13 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface, int64_t time_when_posted_in_ms) RTC_RUN_ON(&encoder_queue_); + // TODO(bugs.webrtc.org/11341) : Remove this version of RequestEncoderSwitch. + void QueueRequestEncoderSwitch( + const EncoderSwitchRequestCallback::Config& conf) + RTC_RUN_ON(&encoder_queue_); + void QueueRequestEncoderSwitch(const webrtc::SdpVideoFormat& format) + RTC_RUN_ON(&encoder_queue_); + TaskQueueBase* const main_queue_; const uint32_t number_of_cores_; diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index 00c22ffdff..504b7b7b9c 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -5789,6 +5789,7 @@ TEST_F(VideoStreamEncoderTest, BitrateEncoderSwitch) { /*fraction_lost=*/0, /*rtt_ms=*/0, /*cwnd_reduce_ratio=*/0); + AdvanceTime(TimeDelta::Millis(0)); video_stream_encoder_->Stop(); } @@ -5924,6 +5925,7 @@ TEST_F(VideoStreamEncoderTest, EncoderSelectorBitrateSwitch) { /*fraction_lost=*/0, /*rtt_ms=*/0, /*cwnd_reduce_ratio=*/0); + AdvanceTime(TimeDelta::Millis(0)); video_stream_encoder_->Stop(); } @@ -5972,6 +5974,8 @@ TEST_F(VideoStreamEncoderTest, EncoderSelectorBrokenEncoderSwitch) { video_source_.IncomingCapturedFrame(CreateFrame(1, kDontCare, kDontCare)); encode_attempted.Wait(3000); + AdvanceTime(TimeDelta::Millis(0)); + video_stream_encoder_->Stop(); // The encoders produces by the VideoEncoderProxyFactory have a pointer back