Refactor RtpTransceiver channel initialization to use ScopedOperationsBatcher Operation origin/main ParentCL ThisCL ApplyLocalDescription 30 25 14 ApplyRemoteDescription 32 22 15 DoSetLocalDescription 31 26 15 DoSetRemoteDescription 32 22 15 Close 23 22 23 Bug: webrtc:42222804 Change-Id: Ie51f47fb4dff3d8f6c322a1d5881208e025e28f1 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/462821 Reviewed-by: Harald Alvestrand <hta@webrtc.org> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47449}
diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index 60ee335..b0e736e 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc
@@ -453,7 +453,8 @@ VideoBitrateAllocatorFactory* video_bitrate_allocator_factory, absl::AnyInvocable<RtpTransportInternal*(absl::string_view) &&> transport_lookup, - ScopedOperationsBatcher& network_batcher) { + ScopedOperationsBatcher& worker_tasks, + ScopedOperationsBatcher& network_tasks) { RTC_DCHECK_RUN_ON(thread_); RTC_DCHECK(!channel_); RTC_DCHECK(!mid_ || mid_.value() == mid); @@ -489,62 +490,74 @@ OnPacketReceived(packet.Ssrc(), flag); }; - std::unique_ptr<ChannelInterface> new_channel; // TODO(bugs.webrtc.org/11992): CreateVideoChannel internally switches to // the worker thread. We shouldn't be using the `call_ptr_` hack here but // simply be on the worker thread and use `call_` (update upstream code). - context()->worker_thread()->BlockingCall([&] { - RTC_DCHECK_RUN_ON(context()->worker_thread()); + worker_tasks.AddWithFinalizer( + [this, mid_str = std::string(mid), call_ptr, media_config, srtp_required, + crypto_options, audio_options, video_options, + video_bitrate_allocator_factory, + callbacks = std::move(callbacks)]() mutable + -> RTCErrorOr<ScopedOperationsBatcher::FinalizerTask> { + RTC_DCHECK_RUN_ON(context()->worker_thread()); - std::unique_ptr<MediaSendChannelInterface> media_send_channel; - std::unique_ptr<MediaReceiveChannelInterface> media_receive_channel; + std::unique_ptr<MediaSendChannelInterface> media_send_channel; + std::unique_ptr<MediaReceiveChannelInterface> media_receive_channel; - if (owned_send_channel_) { - RTC_DCHECK(owned_receive_channel_); - media_send_channel = std::move(owned_send_channel_); - media_receive_channel = std::move(owned_receive_channel_); - // Apply options to the voice channels for audio and send channel for - // video. Note that the video options are primarily for sending. - if (media_type() == MediaType::AUDIO) { - media_send_channel->AsVoiceSendChannel()->SetOptions(audio_options); - media_receive_channel->AsVoiceReceiveChannel()->SetOptions( - audio_options); - } else if (media_type() == MediaType::VIDEO) { - media_send_channel->AsVideoSendChannel()->SetOptions(video_options); - } - } else { - auto channels = CreateMediaContentChannels( - media_type(), env_, media_engine(), call_ptr, media_config, - audio_options, video_options, crypto_options, - video_bitrate_allocator_factory, GetEncoderSwitchRequestCallback()); - media_send_channel = std::move(channels.first); - media_receive_channel = std::move(channels.second); - SetMediaChannels(media_send_channel.get(), media_receive_channel.get()); - } + if (owned_send_channel_) { + RTC_DCHECK(owned_receive_channel_); + media_send_channel = std::move(owned_send_channel_); + media_receive_channel = std::move(owned_receive_channel_); + // Apply options to the voice channels for audio and send channel for + // video. Note that the video options are primarily for sending. + if (media_type() == MediaType::AUDIO) { + media_send_channel->AsVoiceSendChannel()->SetOptions(audio_options); + media_receive_channel->AsVoiceReceiveChannel()->SetOptions( + audio_options); + } else if (media_type() == MediaType::VIDEO) { + media_send_channel->AsVideoSendChannel()->SetOptions(video_options); + } + } else { + auto channels = CreateMediaContentChannels( + media_type(), env_, media_engine(), call_ptr, media_config, + audio_options, video_options, crypto_options, + video_bitrate_allocator_factory, + GetEncoderSwitchRequestCallback()); + media_send_channel = std::move(channels.first); + media_receive_channel = std::move(channels.second); + SetMediaChannels(media_send_channel.get(), + media_receive_channel.get()); + } - if (media_type() == MediaType::AUDIO) { - new_channel = - CreateMediaChannel<VoiceChannel, VoiceMediaSendChannelInterface, - VoiceMediaReceiveChannelInterface>( - context(), media_send_channel, media_receive_channel, mid, - srtp_required, crypto_options, std::move(callbacks)); - } else { - new_channel = - CreateMediaChannel<VideoChannel, VideoMediaSendChannelInterface, - VideoMediaReceiveChannelInterface>( - context(), media_send_channel, media_receive_channel, mid, - srtp_required, crypto_options, std::move(callbacks)); - } - }); + std::unique_ptr<ChannelInterface> new_channel; + if (media_type() == MediaType::AUDIO) { + new_channel = + CreateMediaChannel<VoiceChannel, VoiceMediaSendChannelInterface, + VoiceMediaReceiveChannelInterface>( + context(), media_send_channel, media_receive_channel, mid_str, + srtp_required, crypto_options, std::move(callbacks)); + } else { + new_channel = + CreateMediaChannel<VideoChannel, VideoMediaSendChannelInterface, + VideoMediaReceiveChannelInterface>( + context(), media_send_channel, media_receive_channel, mid_str, + srtp_required, crypto_options, std::move(callbacks)); + } - channel_ = std::move(new_channel); - transport_name_ = std::nullopt; + return ScopedOperationsBatcher::FinalizerTask( + [this, new_channel = std::move(new_channel)]() mutable { + RTC_DCHECK_RUN_ON(thread_); + channel_ = std::move(new_channel); + transport_name_ = std::nullopt; + }); + }); - network_batcher.AddWithFinalizer( - [this, channel = channel_.get(), - transport_lookup = std::move(transport_lookup)]() mutable + network_tasks.AddWithFinalizer( + [this, transport_lookup = std::move(transport_lookup)]() mutable -> RTCErrorOr<ScopedOperationsBatcher::FinalizerTask> { RTC_DCHECK_RUN_ON(context()->network_thread()); + auto* channel = channel_.get(); + RTC_DCHECK(channel); RtpTransportInternal* transport = std::move(transport_lookup)(channel->mid()); if (!channel->SetRtpTransport(transport)) {
diff --git a/pc/rtp_transceiver.h b/pc/rtp_transceiver.h index 29bf0bf..e455e3c 100644 --- a/pc/rtp_transceiver.h +++ b/pc/rtp_transceiver.h
@@ -150,6 +150,8 @@ RtpTransceiver& operator=(RtpTransceiver&&) = delete; // Creates the Voice/VideoChannel and sets it. + // Note: Tasks added to `worker_tasks` must be executed before tasks added to + // `network_batcher`. void CreateChannel( absl::string_view mid, Call* call_ptr, @@ -161,7 +163,8 @@ VideoBitrateAllocatorFactory* video_bitrate_allocator_factory, absl::AnyInvocable<RtpTransportInternal*(absl::string_view) &&> transport_lookup, - ScopedOperationsBatcher& network_batcher); + ScopedOperationsBatcher& worker_tasks, + ScopedOperationsBatcher& network_tasks); // 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 4c2c480..855bc2f 100644 --- a/pc/rtp_transceiver_unittest.cc +++ b/pc/rtp_transceiver_unittest.cc
@@ -1032,13 +1032,15 @@ /*on_negotiation_needed=*/[] {}); EXPECT_FALSE(transceiver->HasChannel()); - ScopedOperationsBatcher network_batcher(context()->network_thread()); + ScopedOperationsBatcher worker_tasks(context()->worker_thread()); + ScopedOperationsBatcher network_tasks(context()->network_thread()); transceiver->CreateChannel( "0", call_.get(), MediaConfig(), /*srtp_required=*/false, CryptoOptions(), audio_options, VideoOptions(), nullptr, [](absl::string_view) -> RtpTransportInternal* { return nullptr; }, - network_batcher); - EXPECT_TRUE(network_batcher.Run().ok()); + worker_tasks, network_tasks); + EXPECT_TRUE(worker_tasks.Run().ok()); + EXPECT_TRUE(network_tasks.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 8ea2195..b36b5b8 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc
@@ -4145,7 +4145,7 @@ } auto transceiver = transceiver_or_error.MoveValue(); UpdateTransceiverChannel(transceiver, new_content, bundle_group, - network_tasks); + worker_tasks, network_tasks); // Handle locally rejected content. This code path is only needed for apps // that SDP munge. Remote rejected content is handled in // ApplyRemoteDescriptionUpdateTransceiverState(). @@ -4344,6 +4344,7 @@ scoped_refptr<RtpTransceiverProxyWithInternal<RtpTransceiver>> transceiver, const ContentInfo& content, const ContentGroup* bundle_group, + ScopedOperationsBatcher& worker_tasks, ScopedOperationsBatcher& network_tasks) { TRACE_EVENT0("webrtc", "SdpOfferAnswerHandler::UpdateTransceiverChannel"); RTC_DCHECK(IsUnifiedPlan()); @@ -4362,7 +4363,7 @@ RTC_DCHECK_RUN_ON(network_thread()); return transport_controller_n()->GetRtpTransport(mid); }, - network_tasks); + worker_tasks, network_tasks); } } } @@ -5687,6 +5688,7 @@ // at this point. RTC_DCHECK_RUN_ON(signaling_thread()); + ScopedOperationsBatcher worker_tasks(context_->worker_thread()); ScopedOperationsBatcher network_tasks(network_thread()); const ContentInfo* voice = GetFirstAudioContent(&desc); @@ -5700,7 +5702,7 @@ RTC_DCHECK_RUN_ON(network_thread()); return transport_controller_n()->GetRtpTransport(mid); }, - network_tasks); + worker_tasks, network_tasks); } const ContentInfo* video = GetFirstVideoContent(&desc); @@ -5714,7 +5716,7 @@ RTC_DCHECK_RUN_ON(network_thread()); return transport_controller_n()->GetRtpTransport(mid); }, - network_tasks); + worker_tasks, network_tasks); } const ContentInfo* data = GetFirstDataContent(&desc); @@ -5724,6 +5726,10 @@ << "Failed to create data channel."); } + RTCError error = worker_tasks.Run(); + if (!error.ok()) { + return error; + } return network_tasks.Run(); }
diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index 683acd0..e2d2b6f 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h
@@ -391,6 +391,7 @@ transceiver, const ContentInfo& content, const ContentGroup* bundle_group, + ScopedOperationsBatcher& worker_tasks, ScopedOperationsBatcher& network_tasks) RTC_RUN_ON(signaling_thread()); // Either creates or destroys the local data channel according to the given