Remove network thread blocking call from CreateChannel Refactor the CreateChannel method in RtpTransceiver to remove the inline BlockingCall to the network thread and utilize ScopedOperationsBatcher to handle network thread operations. The batcher allows the network thread tasks—specifically setting the RTP transport and retrieving the transport name—to be queued. A finalizer task is then used to safely update the transport name on the signaling thread once the network operations are complete. Updates include: * Modified RtpTransceiver::CreateChannel to accept a batcher reference. * Updated SdpOfferAnswerHandler to initialize and execute network task batches during SDP negotiations. * Adjusted unit tests to accommodate the batcher-based flow. RenegotiateManyVideoTransceiversAndWatchAudioDelay comparison runs of blocking calls (peak values) with and without this change: Operation this CL origin/main delta ApplyLocalDescription 25 30 -5 ApplyRemoteDescription 22 32 -10 DoSetLocalDescription 26 31 -5 DoSetRemoteDescription 22 32 -10 Close 22 23 -1 Bug: webrtc:42222804 Change-Id: Ic585aa9d01b838a0366735013d58b541a41efd0f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/463000 Reviewed-by: Harald Alvestrand <hta@webrtc.org> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47415}
diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index 38adf50..60ee335 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc
@@ -442,7 +442,7 @@ RTC_DCHECK(!owned_receive_channel_); } -RTCError RtpTransceiver::CreateChannel( +void RtpTransceiver::CreateChannel( absl::string_view mid, Call* call_ptr, const MediaConfig& media_config, @@ -452,7 +452,8 @@ const VideoOptions& video_options, VideoBitrateAllocatorFactory* video_bitrate_allocator_factory, absl::AnyInvocable<RtpTransportInternal*(absl::string_view) &&> - transport_lookup) { + transport_lookup, + ScopedOperationsBatcher& network_batcher) { RTC_DCHECK_RUN_ON(thread_); RTC_DCHECK(!channel_); RTC_DCHECK(!mid_ || mid_.value() == mid); @@ -539,26 +540,27 @@ channel_ = std::move(new_channel); transport_name_ = std::nullopt; - std::optional<std::string> transport_name; - RTCError err = context()->network_thread()->BlockingCall( - [&, flag = signaling_thread_safety_, channel = channel_.get()]() { + network_batcher.AddWithFinalizer( + [this, channel = channel_.get(), + transport_lookup = std::move(transport_lookup)]() mutable + -> RTCErrorOr<ScopedOperationsBatcher::FinalizerTask> { + RTC_DCHECK_RUN_ON(context()->network_thread()); RtpTransportInternal* transport = std::move(transport_lookup)(channel->mid()); if (!channel->SetRtpTransport(transport)) { return RTCError::InvalidParameter() << "Invalid transport for mid=" << channel->mid(); } + std::optional<std::string> transport_name; if (transport) { transport_name = transport->transport_name(); } - return RTCError::OK(); + return ScopedOperationsBatcher::FinalizerTask( + [this, transport_name = std::move(transport_name)]() mutable { + RTC_DCHECK_RUN_ON(thread_); + transport_name_ = std::move(transport_name); + }); }); - - if (err.ok()) { - transport_name_ = std::move(transport_name); - } - - return err; } RTCError RtpTransceiver::SetChannelForTest(
diff --git a/pc/rtp_transceiver.h b/pc/rtp_transceiver.h index abe0893..29bf0bf 100644 --- a/pc/rtp_transceiver.h +++ b/pc/rtp_transceiver.h
@@ -150,7 +150,7 @@ RtpTransceiver& operator=(RtpTransceiver&&) = delete; // Creates the Voice/VideoChannel and sets it. - RTCError CreateChannel( + void CreateChannel( absl::string_view mid, Call* call_ptr, const MediaConfig& media_config, @@ -160,7 +160,8 @@ const VideoOptions& video_options, VideoBitrateAllocatorFactory* video_bitrate_allocator_factory, absl::AnyInvocable<RtpTransportInternal*(absl::string_view) &&> - transport_lookup); + transport_lookup, + ScopedOperationsBatcher& network_batcher); // Sets the Voice/VideoChannel. The caller must pass in the correct channel // implementation based on the type of the transceiver. The call must
diff --git a/pc/rtp_transceiver_unittest.cc b/pc/rtp_transceiver_unittest.cc index 58850fb..4c2c480 100644 --- a/pc/rtp_transceiver_unittest.cc +++ b/pc/rtp_transceiver_unittest.cc
@@ -50,6 +50,7 @@ #include "pc/rtp_sender_proxy.h" #include "pc/rtp_transport.h" #include "pc/rtp_transport_internal.h" +#include "pc/scoped_operations_batcher.h" #include "pc/session_description.h" #include "pc/test/enable_fake_media.h" #include "pc/test/fake_codec_lookup_helper.h" @@ -1031,11 +1032,13 @@ /*on_negotiation_needed=*/[] {}); EXPECT_FALSE(transceiver->HasChannel()); - auto error = transceiver->CreateChannel( + ScopedOperationsBatcher network_batcher(context()->network_thread()); + transceiver->CreateChannel( "0", call_.get(), MediaConfig(), /*srtp_required=*/false, CryptoOptions(), audio_options, VideoOptions(), nullptr, - [](absl::string_view) -> RtpTransportInternal* { return nullptr; }); - EXPECT_TRUE(error.ok()); + [](absl::string_view) -> RtpTransportInternal* { return nullptr; }, + network_batcher); + EXPECT_TRUE(network_batcher.Run().ok()); ASSERT_TRUE(transceiver->HasChannel()); auto* voice_channel = transceiver->voice_media_send_channel();
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 1b1c29c..dd66408 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc
@@ -4102,6 +4102,7 @@ } ScopedOperationsBatcher worker_tasks(context_->worker_thread()); + ScopedOperationsBatcher network_tasks(context_->network_thread()); const ContentInfos& new_contents = new_session.description()->contents(); for (size_t i = 0; i < new_contents.size(); ++i) { const ContentInfo& new_content = new_contents[i]; @@ -4136,8 +4137,8 @@ return transceiver_or_error.MoveError(); } auto transceiver = transceiver_or_error.MoveValue(); - RTCError error = - UpdateTransceiverChannel(transceiver, new_content, bundle_group); + UpdateTransceiverChannel(transceiver, new_content, bundle_group, + network_tasks); // Handle locally rejected content. This code path is only needed for apps // that SDP munge. Remote rejected content is handled in // ApplyRemoteDescriptionUpdateTransceiverState(). @@ -4169,9 +4170,6 @@ RTC_DCHECK(transceiver->internal()->stopped()); } } - if (!error.ok()) { - return error; - } } else if (media_type == MediaType::DATA) { const auto data_mid = pc_->sctp_mid(); if (data_mid && new_content.mid() != data_mid.value()) { @@ -4193,7 +4191,11 @@ } } - return RTCError::OK(); + RTCError error = worker_tasks.Run(); + if (!error.ok()) { + return error; + } + return network_tasks.Run(); } RTCErrorOr<scoped_refptr<RtpTransceiverProxyWithInternal<RtpTransceiver>>> @@ -4331,10 +4333,11 @@ return std::move(transceiver); } -RTCError SdpOfferAnswerHandler::UpdateTransceiverChannel( +void SdpOfferAnswerHandler::UpdateTransceiverChannel( scoped_refptr<RtpTransceiverProxyWithInternal<RtpTransceiver>> transceiver, const ContentInfo& content, - const ContentGroup* bundle_group) { + const ContentGroup* bundle_group, + ScopedOperationsBatcher& network_tasks) { TRACE_EVENT0("webrtc", "SdpOfferAnswerHandler::UpdateTransceiverChannel"); RTC_DCHECK(IsUnifiedPlan()); RTC_DCHECK(transceiver); @@ -4344,20 +4347,17 @@ } } else { if (!transceiver->internal()->HasChannel()) { - auto error = transceiver->internal()->CreateChannel( + transceiver->internal()->CreateChannel( content.mid(), pc_->call_ptr(), pc_->configuration()->media_config, pc_->SrtpRequired(), pc_->GetCryptoOptions(), audio_options(), video_options(), video_bitrate_allocator_factory_.get(), [&](absl::string_view mid) { RTC_DCHECK_RUN_ON(network_thread()); return transport_controller_n()->GetRtpTransport(mid); - }); - if (!error.ok()) { - return error; - } + }, + network_tasks); } } - return RTCError::OK(); } RTCError SdpOfferAnswerHandler::UpdateDataChannelTransport( @@ -5683,39 +5683,35 @@ // Creating the media channels. Transports should already have been created // at this point. RTC_DCHECK_RUN_ON(signaling_thread()); + + ScopedOperationsBatcher network_tasks(network_thread()); + const ContentInfo* voice = GetFirstAudioContent(&desc); if (voice && !voice->rejected && !rtp_manager()->GetAudioTransceiver()->internal()->HasChannel()) { - auto error = - rtp_manager()->GetAudioTransceiver()->internal()->CreateChannel( - voice->mid(), pc_->call_ptr(), pc_->configuration()->media_config, - pc_->SrtpRequired(), pc_->GetCryptoOptions(), audio_options(), - video_options(), video_bitrate_allocator_factory_.get(), - [&](absl::string_view mid) { - RTC_DCHECK_RUN_ON(network_thread()); - return transport_controller_n()->GetRtpTransport(mid); - }); - if (!error.ok()) { - return error; - } + rtp_manager()->GetAudioTransceiver()->internal()->CreateChannel( + voice->mid(), pc_->call_ptr(), pc_->configuration()->media_config, + pc_->SrtpRequired(), pc_->GetCryptoOptions(), audio_options(), + video_options(), video_bitrate_allocator_factory_.get(), + [&](absl::string_view mid) { + RTC_DCHECK_RUN_ON(network_thread()); + return transport_controller_n()->GetRtpTransport(mid); + }, + network_tasks); } const ContentInfo* video = GetFirstVideoContent(&desc); if (video && !video->rejected && !rtp_manager()->GetVideoTransceiver()->internal()->HasChannel()) { - auto error = - rtp_manager()->GetVideoTransceiver()->internal()->CreateChannel( - video->mid(), pc_->call_ptr(), pc_->configuration()->media_config, - pc_->SrtpRequired(), pc_->GetCryptoOptions(), - - audio_options(), video_options(), - video_bitrate_allocator_factory_.get(), [&](absl::string_view mid) { - RTC_DCHECK_RUN_ON(network_thread()); - return transport_controller_n()->GetRtpTransport(mid); - }); - if (!error.ok()) { - return error; - } + rtp_manager()->GetVideoTransceiver()->internal()->CreateChannel( + video->mid(), pc_->call_ptr(), pc_->configuration()->media_config, + pc_->SrtpRequired(), pc_->GetCryptoOptions(), audio_options(), + video_options(), video_bitrate_allocator_factory_.get(), + [&](absl::string_view mid) { + RTC_DCHECK_RUN_ON(network_thread()); + return transport_controller_n()->GetRtpTransport(mid); + }, + network_tasks); } const ContentInfo* data = GetFirstDataContent(&desc); @@ -5725,7 +5721,7 @@ << "Failed to create data channel."); } - return RTCError::OK(); + return network_tasks.Run(); } void SdpOfferAnswerHandler::GetMediaChannelTeardownTasks(
diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index 919459d..0b3997b 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h
@@ -384,11 +384,12 @@ // Either creates or destroys the transceiver's BaseChannel according to the // given media section. - RTCError UpdateTransceiverChannel( + void UpdateTransceiverChannel( scoped_refptr<RtpTransceiverProxyWithInternal<RtpTransceiver>> transceiver, const ContentInfo& content, - const ContentGroup* bundle_group) RTC_RUN_ON(signaling_thread()); + const ContentGroup* bundle_group, + ScopedOperationsBatcher& network_tasks) RTC_RUN_ON(signaling_thread()); // Either creates or destroys the local data channel according to the given // media section.