From 4f8a58c3d2e1fa5fcf7dc7f07bd89ad428fb09f0 Mon Sep 17 00:00:00 2001 From: Tomas Gunnarsson Date: Wed, 19 Jan 2022 11:36:23 +0100 Subject: [PATCH] Remove 2 Invokes to the network thread when creating a channel. ...and one when destroying a channel object. This CL removes Init_n() and Deinit_n() from the BaseChannel class. Channel classes now use SetRtpTransport to do initialization and uninitialization on the network thread. Notably if an implementation has called SetRtpTransport() with a valid transport pointer, it is required that SetRtpTransport be called again with a nullptr before the channel object can be deleted. In situations where multiple channels are created, this can mean a substantial reduction in thread hops. We still hop to the worker in order to construct the objects - this can probably be avoided and SetChannel() is still a synchronous operation for the transceivers. Furthermore, teardown of channel objects also still happens synchronously and across network/worker/signaling threads. Bug: webrtc:11992 Change-Id: I68ca7596e181fc82996e3e290733d97381aa5e78 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/246740 Reviewed-by: Harald Alvestrand Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#35738} --- pc/channel.cc | 28 ++++------------- pc/channel.h | 4 --- pc/channel_manager.cc | 34 ++++----------------- pc/channel_manager.h | 2 -- pc/channel_manager_unittest.cc | 18 ++++++----- pc/channel_unittest.cc | 8 ++--- pc/rtp_sender_receiver_unittest.cc | 19 ++++++++---- pc/rtp_transceiver.cc | 7 ++++- pc/rtp_transceiver.h | 32 +++++++++++++++++-- pc/rtp_transceiver_unittest.cc | 34 +++++++++++++++++---- pc/sdp_offer_answer.cc | 39 ++++++++++++------------ pc/test/fake_peer_connection_for_stats.h | 6 ++-- 12 files changed, 126 insertions(+), 105 deletions(-) diff --git a/pc/channel.cc b/pc/channel.cc index fa1cddf1d9..dac0cbcf68 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -184,28 +184,7 @@ void BaseChannel::DisconnectFromRtpTransport_n() { rtp_transport_->SignalWritableState.disconnect(this); rtp_transport_->SignalSentPacket.disconnect(this); rtp_transport_ = nullptr; -} - -void BaseChannel::Init_n(webrtc::RtpTransportInternal* rtp_transport) { - // Set the transport before we call SetInterface() since setting the interface - // pointer will call us back to set transport options. - SetRtpTransport(rtp_transport); - - // Both RTP and RTCP channels should be set, we can call SetInterface on - // the media channel and it can set network options. - RTC_DCHECK(!media_channel_->HasNetworkInterface()); - media_channel_->SetInterface(this); -} - -void BaseChannel::Deinit_n() { - // Packets arrive on the network thread, processing packets calls virtual - // functions, so need to stop this process in Deinit that is called in - // derived classes destructor. - media_channel_->SetInterface(/*iface=*/nullptr); - if (rtp_transport_) { - DisconnectFromRtpTransport_n(); - } - RTC_DCHECK(!network_initialized()); + media_channel_->SetInterface(nullptr); } bool BaseChannel::SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport) { @@ -229,6 +208,10 @@ bool BaseChannel::SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport) { if (!ConnectToRtpTransport_n()) { return false; } + + RTC_DCHECK(!media_channel_->HasNetworkInterface()); + media_channel_->SetInterface(this); + media_channel_->OnReadyToSend(rtp_transport_->IsReadyToSend()); UpdateWritableState_n(); @@ -242,6 +225,7 @@ bool BaseChannel::SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport) { } } } + return true; } diff --git a/pc/channel.h b/pc/channel.h index 7d7aeb6aea..fd5495c152 100644 --- a/pc/channel.h +++ b/pc/channel.h @@ -111,10 +111,6 @@ class BaseChannel : public ChannelInterface, rtc::UniqueRandomIdGenerator* ssrc_generator); virtual ~BaseChannel(); - void Init_n(webrtc::RtpTransportInternal* rtp_transport) - RTC_RUN_ON(network_thread()); - void Deinit_n() RTC_RUN_ON(network_thread()); - rtc::Thread* worker_thread() const { return worker_thread_; } rtc::Thread* network_thread() const { return network_thread_; } const std::string& content_name() const override { diff --git a/pc/channel_manager.cc b/pc/channel_manager.cc index 7f1cd982a4..304283eca4 100644 --- a/pc/channel_manager.cc +++ b/pc/channel_manager.cc @@ -143,7 +143,6 @@ ChannelManager::GetSupportedVideoRtpHeaderExtensions() const { VoiceChannel* ChannelManager::CreateVoiceChannel( webrtc::Call* call, const MediaConfig& media_config, - webrtc::RtpTransportInternal* rtp_transport, rtc::Thread* signaling_thread, const std::string& content_name, bool srtp_required, @@ -157,9 +156,9 @@ VoiceChannel* ChannelManager::CreateVoiceChannel( // thread. if (!worker_thread_->IsCurrent()) { return worker_thread_->Invoke(RTC_FROM_HERE, [&] { - return CreateVoiceChannel(call, media_config, rtp_transport, - signaling_thread, content_name, srtp_required, - crypto_options, ssrc_generator, options); + return CreateVoiceChannel(call, media_config, signaling_thread, + content_name, srtp_required, crypto_options, + ssrc_generator, options); }); } @@ -176,11 +175,6 @@ VoiceChannel* ChannelManager::CreateVoiceChannel( absl::WrapUnique(media_channel), content_name, srtp_required, crypto_options, ssrc_generator); - network_thread_->Invoke(RTC_FROM_HERE, [&] { - RTC_DCHECK_RUN_ON(voice_channel->network_thread()); - voice_channel->Init_n(rtp_transport); - }); - VoiceChannel* voice_channel_ptr = voice_channel.get(); voice_channels_.push_back(std::move(voice_channel)); return voice_channel_ptr; @@ -190,11 +184,6 @@ void ChannelManager::DestroyVoiceChannel(VoiceChannel* voice_channel) { TRACE_EVENT0("webrtc", "ChannelManager::DestroyVoiceChannel"); RTC_DCHECK(voice_channel); - network_thread_->Invoke(RTC_FROM_HERE, [&] { - RTC_DCHECK_RUN_ON(voice_channel->network_thread()); - voice_channel->Deinit_n(); - }); - if (!worker_thread_->IsCurrent()) { worker_thread_->Invoke(RTC_FROM_HERE, [&] { DestroyVoiceChannel(voice_channel); }); @@ -212,7 +201,6 @@ void ChannelManager::DestroyVoiceChannel(VoiceChannel* voice_channel) { VideoChannel* ChannelManager::CreateVideoChannel( webrtc::Call* call, const MediaConfig& media_config, - webrtc::RtpTransportInternal* rtp_transport, rtc::Thread* signaling_thread, const std::string& content_name, bool srtp_required, @@ -227,9 +215,9 @@ VideoChannel* ChannelManager::CreateVideoChannel( // thread. if (!worker_thread_->IsCurrent()) { return worker_thread_->Invoke(RTC_FROM_HERE, [&] { - return CreateVideoChannel(call, media_config, rtp_transport, - signaling_thread, content_name, srtp_required, - crypto_options, ssrc_generator, options, + return CreateVideoChannel(call, media_config, signaling_thread, + content_name, srtp_required, crypto_options, + ssrc_generator, options, video_bitrate_allocator_factory); }); } @@ -248,11 +236,6 @@ VideoChannel* ChannelManager::CreateVideoChannel( absl::WrapUnique(media_channel), content_name, srtp_required, crypto_options, ssrc_generator); - network_thread_->Invoke(RTC_FROM_HERE, [&] { - RTC_DCHECK_RUN_ON(video_channel->network_thread()); - video_channel->Init_n(rtp_transport); - }); - VideoChannel* video_channel_ptr = video_channel.get(); video_channels_.push_back(std::move(video_channel)); return video_channel_ptr; @@ -262,11 +245,6 @@ void ChannelManager::DestroyVideoChannel(VideoChannel* video_channel) { TRACE_EVENT0("webrtc", "ChannelManager::DestroyVideoChannel"); RTC_DCHECK(video_channel); - network_thread_->Invoke(RTC_FROM_HERE, [&] { - RTC_DCHECK_RUN_ON(video_channel->network_thread()); - video_channel->Deinit_n(); - }); - if (!worker_thread_->IsCurrent()) { worker_thread_->Invoke(RTC_FROM_HERE, [&] { DestroyVideoChannel(video_channel); }); diff --git a/pc/channel_manager.h b/pc/channel_manager.h index 43fa27935f..0952099e82 100644 --- a/pc/channel_manager.h +++ b/pc/channel_manager.h @@ -81,7 +81,6 @@ class ChannelManager final { // Creates a voice channel, to be associated with the specified session. VoiceChannel* CreateVoiceChannel(webrtc::Call* call, const MediaConfig& media_config, - webrtc::RtpTransportInternal* rtp_transport, rtc::Thread* signaling_thread, const std::string& content_name, bool srtp_required, @@ -97,7 +96,6 @@ class ChannelManager final { VideoChannel* CreateVideoChannel( webrtc::Call* call, const MediaConfig& media_config, - webrtc::RtpTransportInternal* rtp_transport, rtc::Thread* signaling_thread, const std::string& content_name, bool srtp_required, diff --git a/pc/channel_manager_unittest.cc b/pc/channel_manager_unittest.cc index 88de1f6a48..80f30c1c71 100644 --- a/pc/channel_manager_unittest.cc +++ b/pc/channel_manager_unittest.cc @@ -69,16 +69,18 @@ class ChannelManagerTest : public ::testing::Test { void TestCreateDestroyChannels(webrtc::RtpTransportInternal* rtp_transport) { RTC_DCHECK_RUN_ON(worker_); cricket::VoiceChannel* voice_channel = cm_->CreateVoiceChannel( - &fake_call_, cricket::MediaConfig(), rtp_transport, - rtc::Thread::Current(), cricket::CN_AUDIO, kDefaultSrtpRequired, - webrtc::CryptoOptions(), &ssrc_generator_, AudioOptions()); - EXPECT_TRUE(voice_channel != nullptr); + &fake_call_, cricket::MediaConfig(), rtc::Thread::Current(), + cricket::CN_AUDIO, kDefaultSrtpRequired, webrtc::CryptoOptions(), + &ssrc_generator_, AudioOptions()); + ASSERT_TRUE(voice_channel != nullptr); + cricket::VideoChannel* video_channel = cm_->CreateVideoChannel( - &fake_call_, cricket::MediaConfig(), rtp_transport, - rtc::Thread::Current(), cricket::CN_VIDEO, kDefaultSrtpRequired, - webrtc::CryptoOptions(), &ssrc_generator_, VideoOptions(), + &fake_call_, cricket::MediaConfig(), rtc::Thread::Current(), + cricket::CN_VIDEO, kDefaultSrtpRequired, webrtc::CryptoOptions(), + &ssrc_generator_, VideoOptions(), video_bitrate_allocator_factory_.get()); - EXPECT_TRUE(video_channel != nullptr); + ASSERT_TRUE(video_channel != nullptr); + cm_->DestroyVideoChannel(video_channel); cm_->DestroyVoiceChannel(voice_channel); } diff --git a/pc/channel_unittest.cc b/pc/channel_unittest.cc index 42497b86df..0a12a7fed0 100644 --- a/pc/channel_unittest.cc +++ b/pc/channel_unittest.cc @@ -280,11 +280,11 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { network_thread_->Invoke(RTC_FROM_HERE, [this]() { if (channel1_) { RTC_DCHECK_RUN_ON(channel1_->network_thread()); - channel1_->Deinit_n(); + channel1_->SetRtpTransport(nullptr); } if (channel2_) { RTC_DCHECK_RUN_ON(channel2_->network_thread()); - channel2_->Deinit_n(); + channel2_->SetRtpTransport(nullptr); } }); } @@ -1450,7 +1450,7 @@ std::unique_ptr ChannelTest::CreateChannel( &ssrc_generator_); network_thread->Invoke(RTC_FROM_HERE, [&]() { RTC_DCHECK_RUN_ON(channel->network_thread()); - channel->Init_n(rtp_transport); + channel->SetRtpTransport(rtp_transport); }); return channel; } @@ -1536,7 +1536,7 @@ std::unique_ptr ChannelTest::CreateChannel( &ssrc_generator_); network_thread->Invoke(RTC_FROM_HERE, [&]() { RTC_DCHECK_RUN_ON(channel->network_thread()); - channel->Init_n(rtp_transport); + channel->SetRtpTransport(rtp_transport); }); return channel; } diff --git a/pc/rtp_sender_receiver_unittest.cc b/pc/rtp_sender_receiver_unittest.cc index 8043dd0d5c..6127622482 100644 --- a/pc/rtp_sender_receiver_unittest.cc +++ b/pc/rtp_sender_receiver_unittest.cc @@ -122,14 +122,18 @@ class RtpSenderReceiverTest rtp_transport_ = CreateDtlsSrtpTransport(); voice_channel_ = channel_manager_->CreateVoiceChannel( - &fake_call_, cricket::MediaConfig(), rtp_transport_.get(), - rtc::Thread::Current(), cricket::CN_AUDIO, srtp_required, - webrtc::CryptoOptions(), &ssrc_generator_, cricket::AudioOptions()); + &fake_call_, cricket::MediaConfig(), rtc::Thread::Current(), + cricket::CN_AUDIO, srtp_required, webrtc::CryptoOptions(), + &ssrc_generator_, cricket::AudioOptions()); video_channel_ = channel_manager_->CreateVideoChannel( - &fake_call_, cricket::MediaConfig(), rtp_transport_.get(), - rtc::Thread::Current(), cricket::CN_VIDEO, srtp_required, - webrtc::CryptoOptions(), &ssrc_generator_, cricket::VideoOptions(), + &fake_call_, cricket::MediaConfig(), rtc::Thread::Current(), + cricket::CN_VIDEO, srtp_required, webrtc::CryptoOptions(), + &ssrc_generator_, cricket::VideoOptions(), video_bitrate_allocator_factory_.get()); + + voice_channel_->SetRtpTransport(rtp_transport_.get()); + video_channel_->SetRtpTransport(rtp_transport_.get()); + voice_channel_->Enable(true); video_channel_->Enable(true); voice_media_channel_ = media_engine_->GetVoiceChannel(0); @@ -170,6 +174,9 @@ class RtpSenderReceiverTest video_track_ = nullptr; audio_track_ = nullptr; + voice_channel_->SetRtpTransport(nullptr); + video_channel_->SetRtpTransport(nullptr); + channel_manager_->DestroyVoiceChannel(voice_channel_); channel_manager_->DestroyVideoChannel(video_channel_); diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index 25ae8043ec..e152d76bf5 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc @@ -156,7 +156,9 @@ RtpTransceiver::~RtpTransceiver() { } } -void RtpTransceiver::SetChannel(cricket::ChannelInterface* channel) { +void RtpTransceiver::SetChannel( + cricket::ChannelInterface* channel, + std::function transport_lookup) { RTC_DCHECK_RUN_ON(thread_); // Cannot set a non-null channel on a stopped transceiver. if (stopped_ && channel) { @@ -164,6 +166,7 @@ void RtpTransceiver::SetChannel(cricket::ChannelInterface* channel) { } RTC_DCHECK(channel || channel_); + RTC_DCHECK(!channel || transport_lookup) << "lookup function not supplied"; RTC_LOG_THREAD_BLOCK_COUNT(); @@ -189,11 +192,13 @@ void RtpTransceiver::SetChannel(cricket::ChannelInterface* channel) { channel_manager_->network_thread()->Invoke(RTC_FROM_HERE, [&]() { if (channel_) { channel_->SetFirstPacketReceivedCallback(nullptr); + channel_->SetRtpTransport(nullptr); } channel_ = channel; if (channel_) { + channel_->SetRtpTransport(transport_lookup(channel_->content_name())); channel_->SetFirstPacketReceivedCallback( [thread = thread_, flag = signaling_thread_safety_, this]() mutable { thread->PostTask(ToQueuedTask( diff --git a/pc/rtp_transceiver.h b/pc/rtp_transceiver.h index c995329273..b8dbb677dd 100644 --- a/pc/rtp_transceiver.h +++ b/pc/rtp_transceiver.h @@ -100,8 +100,36 @@ class RtpTransceiver final cricket::ChannelInterface* channel() const { return channel_; } // Sets the Voice/VideoChannel. The caller must pass in the correct channel - // implementation based on the type of the transceiver. - void SetChannel(cricket::ChannelInterface* channel); + // implementation based on the type of the transceiver. The call must + // furthermore be made on the signaling thread. + // + // `channel`: The channel instance to be associated with the transceiver. + // When a valid pointer is passed for `channel`, the state of the object + // is expected to be newly constructed and not initalized for network + // activity (see next parameter for more). + // + // NOTE: For all practical purposes, the ownership of the channel + // object should be considered to lie with the transceiver until + // `SetChannel()` is called again with nullptr set as the new channel. + // Moving forward, this parameter will change to either be a + // std::unique_ptr<> or the full construction of the channel object will + // be moved to happen within the context of the transceiver class. + // + // `transport_lookup`: When `channel` points to a valid channel object, this + // callback function will be used to look up the `RtpTransport` object + // to associate with the channel via `BaseChannel::SetRtpTransport`. + // The lookup function will be called on the network thread, synchronously + // during the call to `SetChannel`. This means that the caller of + // `SetChannel()` may provide a callback function that references state + // that exists within the calling scope of SetChannel (e.g. a variable + // on the stack). + // The reason for this design is to limit the number of times we jump + // synchronously to the network thread from the signaling thread. + // The callback allows us to combine the transport lookup with network + // state initialization of the channel object. + void SetChannel(cricket::ChannelInterface* channel, + std::function + transport_lookup); // Adds an RtpSender of the appropriate type to be owned by this transceiver. // Must not be null. diff --git a/pc/rtp_transceiver_unittest.cc b/pc/rtp_transceiver_unittest.cc index 35d9265e03..a1a2a197fb 100644 --- a/pc/rtp_transceiver_unittest.cc +++ b/pc/rtp_transceiver_unittest.cc @@ -36,13 +36,19 @@ namespace webrtc { TEST(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) { auto cm = cricket::ChannelManager::Create( nullptr, true, rtc::Thread::Current(), rtc::Thread::Current()); + const std::string content_name("my_mid"); RtpTransceiver transceiver(cricket::MediaType::MEDIA_TYPE_AUDIO, cm.get()); cricket::MockChannelInterface channel1; EXPECT_CALL(channel1, media_type()) .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO)); + EXPECT_CALL(channel1, content_name()).WillRepeatedly(ReturnRef(content_name)); EXPECT_CALL(channel1, SetFirstPacketReceivedCallback(_)); + EXPECT_CALL(channel1, SetRtpTransport(_)).WillRepeatedly(Return(true)); - transceiver.SetChannel(&channel1); + transceiver.SetChannel(&channel1, [&](const std::string& mid) { + EXPECT_EQ(mid, content_name); + return nullptr; + }); EXPECT_EQ(&channel1, transceiver.channel()); // Stop the transceiver. @@ -54,7 +60,7 @@ TEST(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) { .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO)); // Channel can no longer be set, so this call should be a no-op. - transceiver.SetChannel(&channel2); + transceiver.SetChannel(&channel2, [](const std::string&) { return nullptr; }); EXPECT_EQ(&channel1, transceiver.channel()); } @@ -62,14 +68,20 @@ TEST(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) { TEST(RtpTransceiverTest, CanUnsetChannelOnStoppedTransceiver) { auto cm = cricket::ChannelManager::Create( nullptr, true, rtc::Thread::Current(), rtc::Thread::Current()); + const std::string content_name("my_mid"); RtpTransceiver transceiver(cricket::MediaType::MEDIA_TYPE_VIDEO, cm.get()); cricket::MockChannelInterface channel; EXPECT_CALL(channel, media_type()) .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_VIDEO)); + EXPECT_CALL(channel, content_name()).WillRepeatedly(ReturnRef(content_name)); EXPECT_CALL(channel, SetFirstPacketReceivedCallback(_)) .WillRepeatedly(testing::Return()); + EXPECT_CALL(channel, SetRtpTransport(_)).WillRepeatedly(Return(true)); - transceiver.SetChannel(&channel); + transceiver.SetChannel(&channel, [&](const std::string& mid) { + EXPECT_EQ(mid, content_name); + return nullptr; + }); EXPECT_EQ(&channel, transceiver.channel()); // Stop the transceiver. @@ -77,7 +89,7 @@ TEST(RtpTransceiverTest, CanUnsetChannelOnStoppedTransceiver) { EXPECT_EQ(&channel, transceiver.channel()); // Set the channel to `nullptr`. - transceiver.SetChannel(nullptr); + transceiver.SetChannel(nullptr, nullptr); EXPECT_EQ(nullptr, transceiver.channel()); } @@ -279,6 +291,7 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, TEST_F(RtpTransceiverTestForHeaderExtensions, NoNegotiatedHdrExtsWithChannelWithoutNegotiation) { + const std::string content_name("my_mid"); EXPECT_CALL(*receiver_.get(), SetMediaChannel(_)); EXPECT_CALL(*receiver_.get(), StopAndEndTrack()); EXPECT_CALL(*sender_.get(), SetMediaChannel(_)); @@ -289,11 +302,16 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, EXPECT_CALL(mock_channel, media_type()) .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO)); EXPECT_CALL(mock_channel, media_channel()).WillRepeatedly(Return(nullptr)); - transceiver_.SetChannel(&mock_channel); + EXPECT_CALL(mock_channel, content_name()) + .WillRepeatedly(ReturnRef(content_name)); + EXPECT_CALL(mock_channel, SetRtpTransport(_)).WillRepeatedly(Return(true)); + transceiver_.SetChannel(&mock_channel, + [](const std::string&) { return nullptr; }); EXPECT_THAT(transceiver_.HeaderExtensionsNegotiated(), ElementsAre()); } TEST_F(RtpTransceiverTestForHeaderExtensions, ReturnsNegotiatedHdrExts) { + const std::string content_name("my_mid"); EXPECT_CALL(*receiver_.get(), SetMediaChannel(_)); EXPECT_CALL(*receiver_.get(), StopAndEndTrack()); EXPECT_CALL(*sender_.get(), SetMediaChannel(_)); @@ -305,6 +323,9 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, ReturnsNegotiatedHdrExts) { EXPECT_CALL(mock_channel, media_type()) .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO)); EXPECT_CALL(mock_channel, media_channel()).WillRepeatedly(Return(nullptr)); + EXPECT_CALL(mock_channel, content_name()) + .WillRepeatedly(ReturnRef(content_name)); + EXPECT_CALL(mock_channel, SetRtpTransport(_)).WillRepeatedly(Return(true)); cricket::RtpHeaderExtensions extensions = {webrtc::RtpExtension("uri1", 1), webrtc::RtpExtension("uri2", 2)}; @@ -312,7 +333,8 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, ReturnsNegotiatedHdrExts) { description.set_rtp_header_extensions(extensions); transceiver_.OnNegotiationUpdate(SdpType::kAnswer, &description); - transceiver_.SetChannel(&mock_channel); + transceiver_.SetChannel(&mock_channel, + [](const std::string&) { return nullptr; }); EXPECT_THAT(transceiver_.HeaderExtensionsNegotiated(), ElementsAre(RtpHeaderExtensionCapability( "uri1", 1, RtpTransceiverDirection::kSendRecv), diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 4ab336c19f..ea1c4c4d23 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -3522,7 +3522,7 @@ RTCError SdpOfferAnswerHandler::UpdateTransceiverChannel( cricket::ChannelInterface* channel = transceiver->internal()->channel(); if (content.rejected) { if (channel) { - transceiver->internal()->SetChannel(nullptr); + transceiver->internal()->SetChannel(nullptr, nullptr); DestroyChannelInterface(channel); } } else { @@ -3537,7 +3537,9 @@ RTCError SdpOfferAnswerHandler::UpdateTransceiverChannel( return RTCError(RTCErrorType::INTERNAL_ERROR, "Failed to create channel for mid=" + content.name); } - transceiver->internal()->SetChannel(channel); + transceiver->internal()->SetChannel(channel, [&](const std::string& mid) { + return transport_controller()->GetRtpTransport(mid); + }); } } return RTCError::OK(); @@ -4714,7 +4716,10 @@ RTCError SdpOfferAnswerHandler::CreateChannels(const SessionDescription& desc) { return RTCError(RTCErrorType::INTERNAL_ERROR, "Failed to create voice channel."); } - rtp_manager()->GetAudioTransceiver()->internal()->SetChannel(voice_channel); + rtp_manager()->GetAudioTransceiver()->internal()->SetChannel( + voice_channel, [&](const std::string& mid) { + return transport_controller()->GetRtpTransport(mid); + }); } const cricket::ContentInfo* video = cricket::GetFirstVideoContent(&desc); @@ -4725,7 +4730,10 @@ RTCError SdpOfferAnswerHandler::CreateChannels(const SessionDescription& desc) { return RTCError(RTCErrorType::INTERNAL_ERROR, "Failed to create video channel."); } - rtp_manager()->GetVideoTransceiver()->internal()->SetChannel(video_channel); + rtp_manager()->GetVideoTransceiver()->internal()->SetChannel( + video_channel, [&](const std::string& mid) { + return transport_controller()->GetRtpTransport(mid); + }); } const cricket::ContentInfo* data = cricket::GetFirstDataContent(&desc); @@ -4748,18 +4756,13 @@ cricket::VoiceChannel* SdpOfferAnswerHandler::CreateVoiceChannel( if (!channel_manager()->media_engine()) return nullptr; - // TODO(tommi): Avoid hop to network thread. - RtpTransportInternal* rtp_transport = pc_->GetRtpTransport(mid); - // TODO(bugs.webrtc.org/11992): CreateVoiceChannel 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). - // TODO(tommi): This hops to the worker and from the worker to the network - // thread (blocking both signal and worker). return channel_manager()->CreateVoiceChannel( - pc_->call_ptr(), pc_->configuration()->media_config, rtp_transport, - signaling_thread(), mid, pc_->SrtpRequired(), pc_->GetCryptoOptions(), - &ssrc_generator_, audio_options()); + pc_->call_ptr(), pc_->configuration()->media_config, signaling_thread(), + mid, pc_->SrtpRequired(), pc_->GetCryptoOptions(), &ssrc_generator_, + audio_options()); } // TODO(steveanton): Perhaps this should be managed by the RtpTransceiver. @@ -4770,17 +4773,13 @@ cricket::VideoChannel* SdpOfferAnswerHandler::CreateVideoChannel( if (!channel_manager()->media_engine()) return nullptr; - // NOTE: This involves a non-ideal hop (Invoke) over to the network thread. - RtpTransportInternal* rtp_transport = pc_->GetRtpTransport(mid); - // 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). return channel_manager()->CreateVideoChannel( - pc_->call_ptr(), pc_->configuration()->media_config, rtp_transport, - signaling_thread(), mid, pc_->SrtpRequired(), pc_->GetCryptoOptions(), - &ssrc_generator_, video_options(), - video_bitrate_allocator_factory_.get()); + pc_->call_ptr(), pc_->configuration()->media_config, signaling_thread(), + mid, pc_->SrtpRequired(), pc_->GetCryptoOptions(), &ssrc_generator_, + video_options(), video_bitrate_allocator_factory_.get()); } bool SdpOfferAnswerHandler::CreateDataChannel(const std::string& mid) { @@ -4825,7 +4824,7 @@ void SdpOfferAnswerHandler::DestroyTransceiverChannel( // so if ownership of the channel object lies with the transceiver, we could // un-set the channel pointer and uninitialize/destruct the channel object // at the same time, rather than in separate steps. - transceiver->internal()->SetChannel(nullptr); + transceiver->internal()->SetChannel(nullptr, nullptr); // TODO(tommi): All channel objects end up getting deleted on the // worker thread (ideally should be on the network thread but the // MediaChannel objects are tied to the worker. Can the teardown be done diff --git a/pc/test/fake_peer_connection_for_stats.h b/pc/test/fake_peer_connection_for_stats.h index 5a9bc5d7c0..877949d2e0 100644 --- a/pc/test/fake_peer_connection_for_stats.h +++ b/pc/test/fake_peer_connection_for_stats.h @@ -208,7 +208,8 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase { webrtc::CryptoOptions(), &ssrc_generator_, transport_name); GetOrCreateFirstTransceiverOfType(cricket::MEDIA_TYPE_AUDIO) ->internal() - ->SetChannel(voice_channel_.get()); + ->SetChannel(voice_channel_.get(), + [](const std::string&) { return nullptr; }); return voice_media_channel_ptr; } @@ -225,7 +226,8 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase { webrtc::CryptoOptions(), &ssrc_generator_, transport_name); GetOrCreateFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO) ->internal() - ->SetChannel(video_channel_.get()); + ->SetChannel(video_channel_.get(), + [](const std::string&) { return nullptr; }); return video_media_channel_ptr; }