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 <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35738}
This commit is contained in:
Tomas Gunnarsson
2022-01-19 11:36:23 +01:00
committed by WebRTC LUCI CQ
parent 4f19950660
commit 4f8a58c3d2
12 changed files with 126 additions and 105 deletions

View File

@ -184,28 +184,7 @@ void BaseChannel::DisconnectFromRtpTransport_n() {
rtp_transport_->SignalWritableState.disconnect(this); rtp_transport_->SignalWritableState.disconnect(this);
rtp_transport_->SignalSentPacket.disconnect(this); rtp_transport_->SignalSentPacket.disconnect(this);
rtp_transport_ = nullptr; rtp_transport_ = nullptr;
} media_channel_->SetInterface(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());
} }
bool BaseChannel::SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport) { bool BaseChannel::SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport) {
@ -229,6 +208,10 @@ bool BaseChannel::SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport) {
if (!ConnectToRtpTransport_n()) { if (!ConnectToRtpTransport_n()) {
return false; return false;
} }
RTC_DCHECK(!media_channel_->HasNetworkInterface());
media_channel_->SetInterface(this);
media_channel_->OnReadyToSend(rtp_transport_->IsReadyToSend()); media_channel_->OnReadyToSend(rtp_transport_->IsReadyToSend());
UpdateWritableState_n(); UpdateWritableState_n();
@ -242,6 +225,7 @@ bool BaseChannel::SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport) {
} }
} }
} }
return true; return true;
} }

View File

@ -111,10 +111,6 @@ class BaseChannel : public ChannelInterface,
rtc::UniqueRandomIdGenerator* ssrc_generator); rtc::UniqueRandomIdGenerator* ssrc_generator);
virtual ~BaseChannel(); 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* worker_thread() const { return worker_thread_; }
rtc::Thread* network_thread() const { return network_thread_; } rtc::Thread* network_thread() const { return network_thread_; }
const std::string& content_name() const override { const std::string& content_name() const override {

View File

@ -143,7 +143,6 @@ ChannelManager::GetSupportedVideoRtpHeaderExtensions() const {
VoiceChannel* ChannelManager::CreateVoiceChannel( VoiceChannel* ChannelManager::CreateVoiceChannel(
webrtc::Call* call, webrtc::Call* call,
const MediaConfig& media_config, const MediaConfig& media_config,
webrtc::RtpTransportInternal* rtp_transport,
rtc::Thread* signaling_thread, rtc::Thread* signaling_thread,
const std::string& content_name, const std::string& content_name,
bool srtp_required, bool srtp_required,
@ -157,9 +156,9 @@ VoiceChannel* ChannelManager::CreateVoiceChannel(
// thread. // thread.
if (!worker_thread_->IsCurrent()) { if (!worker_thread_->IsCurrent()) {
return worker_thread_->Invoke<VoiceChannel*>(RTC_FROM_HERE, [&] { return worker_thread_->Invoke<VoiceChannel*>(RTC_FROM_HERE, [&] {
return CreateVoiceChannel(call, media_config, rtp_transport, return CreateVoiceChannel(call, media_config, signaling_thread,
signaling_thread, content_name, srtp_required, content_name, srtp_required, crypto_options,
crypto_options, ssrc_generator, options); ssrc_generator, options);
}); });
} }
@ -176,11 +175,6 @@ VoiceChannel* ChannelManager::CreateVoiceChannel(
absl::WrapUnique(media_channel), content_name, srtp_required, absl::WrapUnique(media_channel), content_name, srtp_required,
crypto_options, ssrc_generator); crypto_options, ssrc_generator);
network_thread_->Invoke<void>(RTC_FROM_HERE, [&] {
RTC_DCHECK_RUN_ON(voice_channel->network_thread());
voice_channel->Init_n(rtp_transport);
});
VoiceChannel* voice_channel_ptr = voice_channel.get(); VoiceChannel* voice_channel_ptr = voice_channel.get();
voice_channels_.push_back(std::move(voice_channel)); voice_channels_.push_back(std::move(voice_channel));
return voice_channel_ptr; return voice_channel_ptr;
@ -190,11 +184,6 @@ void ChannelManager::DestroyVoiceChannel(VoiceChannel* voice_channel) {
TRACE_EVENT0("webrtc", "ChannelManager::DestroyVoiceChannel"); TRACE_EVENT0("webrtc", "ChannelManager::DestroyVoiceChannel");
RTC_DCHECK(voice_channel); RTC_DCHECK(voice_channel);
network_thread_->Invoke<void>(RTC_FROM_HERE, [&] {
RTC_DCHECK_RUN_ON(voice_channel->network_thread());
voice_channel->Deinit_n();
});
if (!worker_thread_->IsCurrent()) { if (!worker_thread_->IsCurrent()) {
worker_thread_->Invoke<void>(RTC_FROM_HERE, worker_thread_->Invoke<void>(RTC_FROM_HERE,
[&] { DestroyVoiceChannel(voice_channel); }); [&] { DestroyVoiceChannel(voice_channel); });
@ -212,7 +201,6 @@ void ChannelManager::DestroyVoiceChannel(VoiceChannel* voice_channel) {
VideoChannel* ChannelManager::CreateVideoChannel( VideoChannel* ChannelManager::CreateVideoChannel(
webrtc::Call* call, webrtc::Call* call,
const MediaConfig& media_config, const MediaConfig& media_config,
webrtc::RtpTransportInternal* rtp_transport,
rtc::Thread* signaling_thread, rtc::Thread* signaling_thread,
const std::string& content_name, const std::string& content_name,
bool srtp_required, bool srtp_required,
@ -227,9 +215,9 @@ VideoChannel* ChannelManager::CreateVideoChannel(
// thread. // thread.
if (!worker_thread_->IsCurrent()) { if (!worker_thread_->IsCurrent()) {
return worker_thread_->Invoke<VideoChannel*>(RTC_FROM_HERE, [&] { return worker_thread_->Invoke<VideoChannel*>(RTC_FROM_HERE, [&] {
return CreateVideoChannel(call, media_config, rtp_transport, return CreateVideoChannel(call, media_config, signaling_thread,
signaling_thread, content_name, srtp_required, content_name, srtp_required, crypto_options,
crypto_options, ssrc_generator, options, ssrc_generator, options,
video_bitrate_allocator_factory); video_bitrate_allocator_factory);
}); });
} }
@ -248,11 +236,6 @@ VideoChannel* ChannelManager::CreateVideoChannel(
absl::WrapUnique(media_channel), content_name, srtp_required, absl::WrapUnique(media_channel), content_name, srtp_required,
crypto_options, ssrc_generator); crypto_options, ssrc_generator);
network_thread_->Invoke<void>(RTC_FROM_HERE, [&] {
RTC_DCHECK_RUN_ON(video_channel->network_thread());
video_channel->Init_n(rtp_transport);
});
VideoChannel* video_channel_ptr = video_channel.get(); VideoChannel* video_channel_ptr = video_channel.get();
video_channels_.push_back(std::move(video_channel)); video_channels_.push_back(std::move(video_channel));
return video_channel_ptr; return video_channel_ptr;
@ -262,11 +245,6 @@ void ChannelManager::DestroyVideoChannel(VideoChannel* video_channel) {
TRACE_EVENT0("webrtc", "ChannelManager::DestroyVideoChannel"); TRACE_EVENT0("webrtc", "ChannelManager::DestroyVideoChannel");
RTC_DCHECK(video_channel); RTC_DCHECK(video_channel);
network_thread_->Invoke<void>(RTC_FROM_HERE, [&] {
RTC_DCHECK_RUN_ON(video_channel->network_thread());
video_channel->Deinit_n();
});
if (!worker_thread_->IsCurrent()) { if (!worker_thread_->IsCurrent()) {
worker_thread_->Invoke<void>(RTC_FROM_HERE, worker_thread_->Invoke<void>(RTC_FROM_HERE,
[&] { DestroyVideoChannel(video_channel); }); [&] { DestroyVideoChannel(video_channel); });

View File

@ -81,7 +81,6 @@ class ChannelManager final {
// Creates a voice channel, to be associated with the specified session. // Creates a voice channel, to be associated with the specified session.
VoiceChannel* CreateVoiceChannel(webrtc::Call* call, VoiceChannel* CreateVoiceChannel(webrtc::Call* call,
const MediaConfig& media_config, const MediaConfig& media_config,
webrtc::RtpTransportInternal* rtp_transport,
rtc::Thread* signaling_thread, rtc::Thread* signaling_thread,
const std::string& content_name, const std::string& content_name,
bool srtp_required, bool srtp_required,
@ -97,7 +96,6 @@ class ChannelManager final {
VideoChannel* CreateVideoChannel( VideoChannel* CreateVideoChannel(
webrtc::Call* call, webrtc::Call* call,
const MediaConfig& media_config, const MediaConfig& media_config,
webrtc::RtpTransportInternal* rtp_transport,
rtc::Thread* signaling_thread, rtc::Thread* signaling_thread,
const std::string& content_name, const std::string& content_name,
bool srtp_required, bool srtp_required,

View File

@ -69,16 +69,18 @@ class ChannelManagerTest : public ::testing::Test {
void TestCreateDestroyChannels(webrtc::RtpTransportInternal* rtp_transport) { void TestCreateDestroyChannels(webrtc::RtpTransportInternal* rtp_transport) {
RTC_DCHECK_RUN_ON(worker_); RTC_DCHECK_RUN_ON(worker_);
cricket::VoiceChannel* voice_channel = cm_->CreateVoiceChannel( cricket::VoiceChannel* voice_channel = cm_->CreateVoiceChannel(
&fake_call_, cricket::MediaConfig(), rtp_transport, &fake_call_, cricket::MediaConfig(), rtc::Thread::Current(),
rtc::Thread::Current(), cricket::CN_AUDIO, kDefaultSrtpRequired, cricket::CN_AUDIO, kDefaultSrtpRequired, webrtc::CryptoOptions(),
webrtc::CryptoOptions(), &ssrc_generator_, AudioOptions()); &ssrc_generator_, AudioOptions());
EXPECT_TRUE(voice_channel != nullptr); ASSERT_TRUE(voice_channel != nullptr);
cricket::VideoChannel* video_channel = cm_->CreateVideoChannel( cricket::VideoChannel* video_channel = cm_->CreateVideoChannel(
&fake_call_, cricket::MediaConfig(), rtp_transport, &fake_call_, cricket::MediaConfig(), rtc::Thread::Current(),
rtc::Thread::Current(), cricket::CN_VIDEO, kDefaultSrtpRequired, cricket::CN_VIDEO, kDefaultSrtpRequired, webrtc::CryptoOptions(),
webrtc::CryptoOptions(), &ssrc_generator_, VideoOptions(), &ssrc_generator_, VideoOptions(),
video_bitrate_allocator_factory_.get()); video_bitrate_allocator_factory_.get());
EXPECT_TRUE(video_channel != nullptr); ASSERT_TRUE(video_channel != nullptr);
cm_->DestroyVideoChannel(video_channel); cm_->DestroyVideoChannel(video_channel);
cm_->DestroyVoiceChannel(voice_channel); cm_->DestroyVoiceChannel(voice_channel);
} }

View File

@ -280,11 +280,11 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> {
network_thread_->Invoke<void>(RTC_FROM_HERE, [this]() { network_thread_->Invoke<void>(RTC_FROM_HERE, [this]() {
if (channel1_) { if (channel1_) {
RTC_DCHECK_RUN_ON(channel1_->network_thread()); RTC_DCHECK_RUN_ON(channel1_->network_thread());
channel1_->Deinit_n(); channel1_->SetRtpTransport(nullptr);
} }
if (channel2_) { if (channel2_) {
RTC_DCHECK_RUN_ON(channel2_->network_thread()); RTC_DCHECK_RUN_ON(channel2_->network_thread());
channel2_->Deinit_n(); channel2_->SetRtpTransport(nullptr);
} }
}); });
} }
@ -1450,7 +1450,7 @@ std::unique_ptr<cricket::VoiceChannel> ChannelTest<VoiceTraits>::CreateChannel(
&ssrc_generator_); &ssrc_generator_);
network_thread->Invoke<void>(RTC_FROM_HERE, [&]() { network_thread->Invoke<void>(RTC_FROM_HERE, [&]() {
RTC_DCHECK_RUN_ON(channel->network_thread()); RTC_DCHECK_RUN_ON(channel->network_thread());
channel->Init_n(rtp_transport); channel->SetRtpTransport(rtp_transport);
}); });
return channel; return channel;
} }
@ -1536,7 +1536,7 @@ std::unique_ptr<cricket::VideoChannel> ChannelTest<VideoTraits>::CreateChannel(
&ssrc_generator_); &ssrc_generator_);
network_thread->Invoke<void>(RTC_FROM_HERE, [&]() { network_thread->Invoke<void>(RTC_FROM_HERE, [&]() {
RTC_DCHECK_RUN_ON(channel->network_thread()); RTC_DCHECK_RUN_ON(channel->network_thread());
channel->Init_n(rtp_transport); channel->SetRtpTransport(rtp_transport);
}); });
return channel; return channel;
} }

View File

@ -122,14 +122,18 @@ class RtpSenderReceiverTest
rtp_transport_ = CreateDtlsSrtpTransport(); rtp_transport_ = CreateDtlsSrtpTransport();
voice_channel_ = channel_manager_->CreateVoiceChannel( voice_channel_ = channel_manager_->CreateVoiceChannel(
&fake_call_, cricket::MediaConfig(), rtp_transport_.get(), &fake_call_, cricket::MediaConfig(), rtc::Thread::Current(),
rtc::Thread::Current(), cricket::CN_AUDIO, srtp_required, cricket::CN_AUDIO, srtp_required, webrtc::CryptoOptions(),
webrtc::CryptoOptions(), &ssrc_generator_, cricket::AudioOptions()); &ssrc_generator_, cricket::AudioOptions());
video_channel_ = channel_manager_->CreateVideoChannel( video_channel_ = channel_manager_->CreateVideoChannel(
&fake_call_, cricket::MediaConfig(), rtp_transport_.get(), &fake_call_, cricket::MediaConfig(), rtc::Thread::Current(),
rtc::Thread::Current(), cricket::CN_VIDEO, srtp_required, cricket::CN_VIDEO, srtp_required, webrtc::CryptoOptions(),
webrtc::CryptoOptions(), &ssrc_generator_, cricket::VideoOptions(), &ssrc_generator_, cricket::VideoOptions(),
video_bitrate_allocator_factory_.get()); video_bitrate_allocator_factory_.get());
voice_channel_->SetRtpTransport(rtp_transport_.get());
video_channel_->SetRtpTransport(rtp_transport_.get());
voice_channel_->Enable(true); voice_channel_->Enable(true);
video_channel_->Enable(true); video_channel_->Enable(true);
voice_media_channel_ = media_engine_->GetVoiceChannel(0); voice_media_channel_ = media_engine_->GetVoiceChannel(0);
@ -170,6 +174,9 @@ class RtpSenderReceiverTest
video_track_ = nullptr; video_track_ = nullptr;
audio_track_ = nullptr; audio_track_ = nullptr;
voice_channel_->SetRtpTransport(nullptr);
video_channel_->SetRtpTransport(nullptr);
channel_manager_->DestroyVoiceChannel(voice_channel_); channel_manager_->DestroyVoiceChannel(voice_channel_);
channel_manager_->DestroyVideoChannel(video_channel_); channel_manager_->DestroyVideoChannel(video_channel_);

View File

@ -156,7 +156,9 @@ RtpTransceiver::~RtpTransceiver() {
} }
} }
void RtpTransceiver::SetChannel(cricket::ChannelInterface* channel) { void RtpTransceiver::SetChannel(
cricket::ChannelInterface* channel,
std::function<RtpTransportInternal*(const std::string&)> transport_lookup) {
RTC_DCHECK_RUN_ON(thread_); RTC_DCHECK_RUN_ON(thread_);
// Cannot set a non-null channel on a stopped transceiver. // Cannot set a non-null channel on a stopped transceiver.
if (stopped_ && channel) { if (stopped_ && channel) {
@ -164,6 +166,7 @@ void RtpTransceiver::SetChannel(cricket::ChannelInterface* channel) {
} }
RTC_DCHECK(channel || channel_); RTC_DCHECK(channel || channel_);
RTC_DCHECK(!channel || transport_lookup) << "lookup function not supplied";
RTC_LOG_THREAD_BLOCK_COUNT(); RTC_LOG_THREAD_BLOCK_COUNT();
@ -189,11 +192,13 @@ void RtpTransceiver::SetChannel(cricket::ChannelInterface* channel) {
channel_manager_->network_thread()->Invoke<void>(RTC_FROM_HERE, [&]() { channel_manager_->network_thread()->Invoke<void>(RTC_FROM_HERE, [&]() {
if (channel_) { if (channel_) {
channel_->SetFirstPacketReceivedCallback(nullptr); channel_->SetFirstPacketReceivedCallback(nullptr);
channel_->SetRtpTransport(nullptr);
} }
channel_ = channel; channel_ = channel;
if (channel_) { if (channel_) {
channel_->SetRtpTransport(transport_lookup(channel_->content_name()));
channel_->SetFirstPacketReceivedCallback( channel_->SetFirstPacketReceivedCallback(
[thread = thread_, flag = signaling_thread_safety_, this]() mutable { [thread = thread_, flag = signaling_thread_safety_, this]() mutable {
thread->PostTask(ToQueuedTask( thread->PostTask(ToQueuedTask(

View File

@ -100,8 +100,36 @@ class RtpTransceiver final
cricket::ChannelInterface* channel() const { return channel_; } cricket::ChannelInterface* channel() const { return channel_; }
// Sets the Voice/VideoChannel. The caller must pass in the correct channel // Sets the Voice/VideoChannel. The caller must pass in the correct channel
// implementation based on the type of the transceiver. // implementation based on the type of the transceiver. The call must
void SetChannel(cricket::ChannelInterface* channel); // 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<RtpTransportInternal*(const std::string&)>
transport_lookup);
// Adds an RtpSender of the appropriate type to be owned by this transceiver. // Adds an RtpSender of the appropriate type to be owned by this transceiver.
// Must not be null. // Must not be null.

View File

@ -36,13 +36,19 @@ namespace webrtc {
TEST(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) { TEST(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) {
auto cm = cricket::ChannelManager::Create( auto cm = cricket::ChannelManager::Create(
nullptr, true, rtc::Thread::Current(), rtc::Thread::Current()); nullptr, true, rtc::Thread::Current(), rtc::Thread::Current());
const std::string content_name("my_mid");
RtpTransceiver transceiver(cricket::MediaType::MEDIA_TYPE_AUDIO, cm.get()); RtpTransceiver transceiver(cricket::MediaType::MEDIA_TYPE_AUDIO, cm.get());
cricket::MockChannelInterface channel1; cricket::MockChannelInterface channel1;
EXPECT_CALL(channel1, media_type()) EXPECT_CALL(channel1, media_type())
.WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO)); .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO));
EXPECT_CALL(channel1, content_name()).WillRepeatedly(ReturnRef(content_name));
EXPECT_CALL(channel1, SetFirstPacketReceivedCallback(_)); 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()); EXPECT_EQ(&channel1, transceiver.channel());
// Stop the transceiver. // Stop the transceiver.
@ -54,7 +60,7 @@ TEST(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) {
.WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO)); .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO));
// Channel can no longer be set, so this call should be a no-op. // 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()); EXPECT_EQ(&channel1, transceiver.channel());
} }
@ -62,14 +68,20 @@ TEST(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) {
TEST(RtpTransceiverTest, CanUnsetChannelOnStoppedTransceiver) { TEST(RtpTransceiverTest, CanUnsetChannelOnStoppedTransceiver) {
auto cm = cricket::ChannelManager::Create( auto cm = cricket::ChannelManager::Create(
nullptr, true, rtc::Thread::Current(), rtc::Thread::Current()); nullptr, true, rtc::Thread::Current(), rtc::Thread::Current());
const std::string content_name("my_mid");
RtpTransceiver transceiver(cricket::MediaType::MEDIA_TYPE_VIDEO, cm.get()); RtpTransceiver transceiver(cricket::MediaType::MEDIA_TYPE_VIDEO, cm.get());
cricket::MockChannelInterface channel; cricket::MockChannelInterface channel;
EXPECT_CALL(channel, media_type()) EXPECT_CALL(channel, media_type())
.WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_VIDEO)); .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_VIDEO));
EXPECT_CALL(channel, content_name()).WillRepeatedly(ReturnRef(content_name));
EXPECT_CALL(channel, SetFirstPacketReceivedCallback(_)) EXPECT_CALL(channel, SetFirstPacketReceivedCallback(_))
.WillRepeatedly(testing::Return()); .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()); EXPECT_EQ(&channel, transceiver.channel());
// Stop the transceiver. // Stop the transceiver.
@ -77,7 +89,7 @@ TEST(RtpTransceiverTest, CanUnsetChannelOnStoppedTransceiver) {
EXPECT_EQ(&channel, transceiver.channel()); EXPECT_EQ(&channel, transceiver.channel());
// Set the channel to `nullptr`. // Set the channel to `nullptr`.
transceiver.SetChannel(nullptr); transceiver.SetChannel(nullptr, nullptr);
EXPECT_EQ(nullptr, transceiver.channel()); EXPECT_EQ(nullptr, transceiver.channel());
} }
@ -279,6 +291,7 @@ TEST_F(RtpTransceiverTestForHeaderExtensions,
TEST_F(RtpTransceiverTestForHeaderExtensions, TEST_F(RtpTransceiverTestForHeaderExtensions,
NoNegotiatedHdrExtsWithChannelWithoutNegotiation) { NoNegotiatedHdrExtsWithChannelWithoutNegotiation) {
const std::string content_name("my_mid");
EXPECT_CALL(*receiver_.get(), SetMediaChannel(_)); EXPECT_CALL(*receiver_.get(), SetMediaChannel(_));
EXPECT_CALL(*receiver_.get(), StopAndEndTrack()); EXPECT_CALL(*receiver_.get(), StopAndEndTrack());
EXPECT_CALL(*sender_.get(), SetMediaChannel(_)); EXPECT_CALL(*sender_.get(), SetMediaChannel(_));
@ -289,11 +302,16 @@ TEST_F(RtpTransceiverTestForHeaderExtensions,
EXPECT_CALL(mock_channel, media_type()) EXPECT_CALL(mock_channel, media_type())
.WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO)); .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO));
EXPECT_CALL(mock_channel, media_channel()).WillRepeatedly(Return(nullptr)); 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()); EXPECT_THAT(transceiver_.HeaderExtensionsNegotiated(), ElementsAre());
} }
TEST_F(RtpTransceiverTestForHeaderExtensions, ReturnsNegotiatedHdrExts) { TEST_F(RtpTransceiverTestForHeaderExtensions, ReturnsNegotiatedHdrExts) {
const std::string content_name("my_mid");
EXPECT_CALL(*receiver_.get(), SetMediaChannel(_)); EXPECT_CALL(*receiver_.get(), SetMediaChannel(_));
EXPECT_CALL(*receiver_.get(), StopAndEndTrack()); EXPECT_CALL(*receiver_.get(), StopAndEndTrack());
EXPECT_CALL(*sender_.get(), SetMediaChannel(_)); EXPECT_CALL(*sender_.get(), SetMediaChannel(_));
@ -305,6 +323,9 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, ReturnsNegotiatedHdrExts) {
EXPECT_CALL(mock_channel, media_type()) EXPECT_CALL(mock_channel, media_type())
.WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO)); .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO));
EXPECT_CALL(mock_channel, media_channel()).WillRepeatedly(Return(nullptr)); 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), cricket::RtpHeaderExtensions extensions = {webrtc::RtpExtension("uri1", 1),
webrtc::RtpExtension("uri2", 2)}; webrtc::RtpExtension("uri2", 2)};
@ -312,7 +333,8 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, ReturnsNegotiatedHdrExts) {
description.set_rtp_header_extensions(extensions); description.set_rtp_header_extensions(extensions);
transceiver_.OnNegotiationUpdate(SdpType::kAnswer, &description); transceiver_.OnNegotiationUpdate(SdpType::kAnswer, &description);
transceiver_.SetChannel(&mock_channel); transceiver_.SetChannel(&mock_channel,
[](const std::string&) { return nullptr; });
EXPECT_THAT(transceiver_.HeaderExtensionsNegotiated(), EXPECT_THAT(transceiver_.HeaderExtensionsNegotiated(),
ElementsAre(RtpHeaderExtensionCapability( ElementsAre(RtpHeaderExtensionCapability(
"uri1", 1, RtpTransceiverDirection::kSendRecv), "uri1", 1, RtpTransceiverDirection::kSendRecv),

View File

@ -3522,7 +3522,7 @@ RTCError SdpOfferAnswerHandler::UpdateTransceiverChannel(
cricket::ChannelInterface* channel = transceiver->internal()->channel(); cricket::ChannelInterface* channel = transceiver->internal()->channel();
if (content.rejected) { if (content.rejected) {
if (channel) { if (channel) {
transceiver->internal()->SetChannel(nullptr); transceiver->internal()->SetChannel(nullptr, nullptr);
DestroyChannelInterface(channel); DestroyChannelInterface(channel);
} }
} else { } else {
@ -3537,7 +3537,9 @@ RTCError SdpOfferAnswerHandler::UpdateTransceiverChannel(
return RTCError(RTCErrorType::INTERNAL_ERROR, return RTCError(RTCErrorType::INTERNAL_ERROR,
"Failed to create channel for mid=" + content.name); "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(); return RTCError::OK();
@ -4714,7 +4716,10 @@ RTCError SdpOfferAnswerHandler::CreateChannels(const SessionDescription& desc) {
return RTCError(RTCErrorType::INTERNAL_ERROR, return RTCError(RTCErrorType::INTERNAL_ERROR,
"Failed to create voice channel."); "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); const cricket::ContentInfo* video = cricket::GetFirstVideoContent(&desc);
@ -4725,7 +4730,10 @@ RTCError SdpOfferAnswerHandler::CreateChannels(const SessionDescription& desc) {
return RTCError(RTCErrorType::INTERNAL_ERROR, return RTCError(RTCErrorType::INTERNAL_ERROR,
"Failed to create video channel."); "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); const cricket::ContentInfo* data = cricket::GetFirstDataContent(&desc);
@ -4748,18 +4756,13 @@ cricket::VoiceChannel* SdpOfferAnswerHandler::CreateVoiceChannel(
if (!channel_manager()->media_engine()) if (!channel_manager()->media_engine())
return nullptr; 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 // TODO(bugs.webrtc.org/11992): CreateVoiceChannel internally switches to the
// worker thread. We shouldn't be using the `call_ptr_` hack here but simply // 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). // 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( return channel_manager()->CreateVoiceChannel(
pc_->call_ptr(), pc_->configuration()->media_config, rtp_transport, pc_->call_ptr(), pc_->configuration()->media_config, signaling_thread(),
signaling_thread(), mid, pc_->SrtpRequired(), pc_->GetCryptoOptions(), mid, pc_->SrtpRequired(), pc_->GetCryptoOptions(), &ssrc_generator_,
&ssrc_generator_, audio_options()); audio_options());
} }
// TODO(steveanton): Perhaps this should be managed by the RtpTransceiver. // TODO(steveanton): Perhaps this should be managed by the RtpTransceiver.
@ -4770,17 +4773,13 @@ cricket::VideoChannel* SdpOfferAnswerHandler::CreateVideoChannel(
if (!channel_manager()->media_engine()) if (!channel_manager()->media_engine())
return nullptr; 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 // TODO(bugs.webrtc.org/11992): CreateVideoChannel internally switches to the
// worker thread. We shouldn't be using the `call_ptr_` hack here but simply // 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). // be on the worker thread and use `call_` (update upstream code).
return channel_manager()->CreateVideoChannel( return channel_manager()->CreateVideoChannel(
pc_->call_ptr(), pc_->configuration()->media_config, rtp_transport, pc_->call_ptr(), pc_->configuration()->media_config, signaling_thread(),
signaling_thread(), mid, pc_->SrtpRequired(), pc_->GetCryptoOptions(), mid, pc_->SrtpRequired(), pc_->GetCryptoOptions(), &ssrc_generator_,
&ssrc_generator_, video_options(), video_options(), video_bitrate_allocator_factory_.get());
video_bitrate_allocator_factory_.get());
} }
bool SdpOfferAnswerHandler::CreateDataChannel(const std::string& mid) { 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 // so if ownership of the channel object lies with the transceiver, we could
// un-set the channel pointer and uninitialize/destruct the channel object // un-set the channel pointer and uninitialize/destruct the channel object
// at the same time, rather than in separate steps. // 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 // TODO(tommi): All channel objects end up getting deleted on the
// worker thread (ideally should be on the network thread but the // worker thread (ideally should be on the network thread but the
// MediaChannel objects are tied to the worker. Can the teardown be done // MediaChannel objects are tied to the worker. Can the teardown be done

View File

@ -208,7 +208,8 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase {
webrtc::CryptoOptions(), &ssrc_generator_, transport_name); webrtc::CryptoOptions(), &ssrc_generator_, transport_name);
GetOrCreateFirstTransceiverOfType(cricket::MEDIA_TYPE_AUDIO) GetOrCreateFirstTransceiverOfType(cricket::MEDIA_TYPE_AUDIO)
->internal() ->internal()
->SetChannel(voice_channel_.get()); ->SetChannel(voice_channel_.get(),
[](const std::string&) { return nullptr; });
return voice_media_channel_ptr; return voice_media_channel_ptr;
} }
@ -225,7 +226,8 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase {
webrtc::CryptoOptions(), &ssrc_generator_, transport_name); webrtc::CryptoOptions(), &ssrc_generator_, transport_name);
GetOrCreateFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO) GetOrCreateFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO)
->internal() ->internal()
->SetChannel(video_channel_.get()); ->SetChannel(video_channel_.get(),
[](const std::string&) { return nullptr; });
return video_media_channel_ptr; return video_media_channel_ptr;
} }