From 0ac50b9dfd7cf4a2d6373e415a815f61a4dbe797 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Wed, 18 May 2022 07:51:34 +0000 Subject: [PATCH] Move ownership of objects from channel_manager to connection_context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a preparatory step in deleting the ChannelManager class. Also delete some declarations whose implementation was previously removed. Bug: webrtc:13931 Change-Id: I8764c00fa696932e79fcfe17550ef2490d6a1ed1 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/262804 Reviewed-by: Henrik Boström Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#36923} --- pc/channel_manager.cc | 30 ++++++--------- pc/channel_manager.h | 25 ++++-------- pc/channel_manager_unittest.cc | 10 +++-- pc/connection_context.cc | 13 ++++++- pc/connection_context.h | 8 ++++ pc/rtp_sender_receiver_unittest.cc | 8 ++-- pc/rtp_transceiver_unittest.cc | 48 ++++++++++++++---------- pc/test/fake_peer_connection_for_stats.h | 9 ++++- 8 files changed, 87 insertions(+), 64 deletions(-) diff --git a/pc/channel_manager.cc b/pc/channel_manager.cc index 4b08843ba3..045d41bfd5 100644 --- a/pc/channel_manager.cc +++ b/pc/channel_manager.cc @@ -27,7 +27,8 @@ namespace cricket { // static std::unique_ptr ChannelManager::Create( - std::unique_ptr media_engine, + MediaEngineInterface* media_engine, + rtc::UniqueRandomIdGenerator* ssrc_generator, bool enable_rtx, rtc::Thread* worker_thread, rtc::Thread* network_thread) { @@ -35,15 +36,16 @@ std::unique_ptr ChannelManager::Create( RTC_DCHECK(worker_thread); return absl::WrapUnique(new ChannelManager( - std::move(media_engine), enable_rtx, worker_thread, network_thread)); + media_engine, ssrc_generator, enable_rtx, worker_thread, network_thread)); } -ChannelManager::ChannelManager( - std::unique_ptr media_engine, - bool enable_rtx, - rtc::Thread* worker_thread, - rtc::Thread* network_thread) - : media_engine_(std::move(media_engine)), +ChannelManager::ChannelManager(MediaEngineInterface* media_engine, + rtc::UniqueRandomIdGenerator* ssrc_generator, + bool enable_rtx, + rtc::Thread* worker_thread, + rtc::Thread* network_thread) + : media_engine_(media_engine), + ssrc_generator_(ssrc_generator), signaling_thread_(rtc::Thread::Current()), worker_thread_(worker_thread), network_thread_(network_thread), @@ -61,14 +63,6 @@ ChannelManager::ChannelManager( ChannelManager::~ChannelManager() { RTC_DCHECK_RUN_ON(signaling_thread_); - worker_thread_->Invoke(RTC_FROM_HERE, [&] { - RTC_DCHECK_RUN_ON(worker_thread_); - // While `media_engine_` is const throughout the ChannelManager's lifetime, - // it requires destruction to happen on the worker thread. Instead of - // marking the pointer as non-const, we live with this const_cast<> in the - // destructor. - const_cast&>(media_engine_).reset(); - }); } void ChannelManager::GetSupportedAudioSendCodecs( @@ -152,7 +146,7 @@ std::unique_ptr ChannelManager::CreateVoiceChannel( auto voice_channel = std::make_unique( worker_thread_, network_thread_, signaling_thread_, absl::WrapUnique(media_channel), mid, srtp_required, crypto_options, - &ssrc_generator_); + ssrc_generator_); return voice_channel; } @@ -191,7 +185,7 @@ std::unique_ptr ChannelManager::CreateVideoChannel( auto video_channel = std::make_unique( worker_thread_, network_thread_, signaling_thread_, absl::WrapUnique(media_channel), mid, srtp_required, crypto_options, - &ssrc_generator_); + ssrc_generator_); return video_channel; } diff --git a/pc/channel_manager.h b/pc/channel_manager.h index 693b552b84..1111002643 100644 --- a/pc/channel_manager.h +++ b/pc/channel_manager.h @@ -50,7 +50,8 @@ class ChannelManager : public ChannelFactoryInterface { // If media_engine is non-nullptr, then the returned ChannelManager instance // will own that reference and media engine initialization static std::unique_ptr Create( - std::unique_ptr media_engine, + MediaEngineInterface* media_engine, + rtc::UniqueRandomIdGenerator* ssrc_generator, bool enable_rtx, rtc::Thread* worker_thread, rtc::Thread* network_thread); @@ -60,8 +61,8 @@ class ChannelManager : public ChannelFactoryInterface { rtc::Thread* worker_thread() const { return worker_thread_; } rtc::Thread* network_thread() const { return network_thread_; } - MediaEngineInterface* media_engine() { return media_engine_.get(); } - rtc::UniqueRandomIdGenerator& ssrc_generator() { return ssrc_generator_; } + MediaEngineInterface* media_engine() { return media_engine_; } + rtc::UniqueRandomIdGenerator& ssrc_generator() { return *ssrc_generator_; } // Retrieves the list of supported audio & video codec types. // Can be called before starting the media engine. @@ -97,29 +98,19 @@ class ChannelManager : public ChannelFactoryInterface { override; protected: - ChannelManager(std::unique_ptr media_engine, + ChannelManager(MediaEngineInterface* media_engine, + rtc::UniqueRandomIdGenerator* ssrc_generator_, bool enable_rtx, rtc::Thread* worker_thread, rtc::Thread* network_thread); - // Destroys a voice channel created by CreateVoiceChannel. - void DestroyVoiceChannel(VoiceChannel* voice_channel); - - // Destroys a video channel created by CreateVideoChannel. - void DestroyVideoChannel(VideoChannel* video_channel); - private: - const std::unique_ptr media_engine_; // Nullable. + MediaEngineInterface* media_engine_; // Nullable. + rtc::UniqueRandomIdGenerator* ssrc_generator_; rtc::Thread* const signaling_thread_; rtc::Thread* const worker_thread_; rtc::Thread* const network_thread_; - // This object should be used to generate any SSRC that is not explicitly - // specified by the user (or by the remote party). - // TODO(bugs.webrtc.org/12666): This variable is used from both the signaling - // and worker threads. See if we can't restrict usage to a single thread. - rtc::UniqueRandomIdGenerator ssrc_generator_; - const bool enable_rtx_; }; diff --git a/pc/channel_manager_unittest.cc b/pc/channel_manager_unittest.cc index 6d7318b446..ccf541ccd2 100644 --- a/pc/channel_manager_unittest.cc +++ b/pc/channel_manager_unittest.cc @@ -58,7 +58,9 @@ class ChannelManagerTest : public ::testing::Test { worker_(rtc::Thread::Current()), video_bitrate_allocator_factory_( webrtc::CreateBuiltinVideoBitrateAllocatorFactory()), - cm_(cricket::ChannelManager::Create(CreateFakeMediaEngine(), + media_engine_(CreateFakeMediaEngine()), + cm_(cricket::ChannelManager::Create(media_engine_.get(), + &ssrc_generator_, false, worker_, network_.get())), @@ -89,6 +91,8 @@ class ChannelManagerTest : public ::testing::Test { rtc::Thread* const worker_; std::unique_ptr video_bitrate_allocator_factory_; + std::unique_ptr media_engine_; + rtc::UniqueRandomIdGenerator ssrc_generator_; std::unique_ptr cm_; cricket::FakeCall fake_call_; webrtc::test::ScopedKeyValueConfig field_trials_; @@ -106,7 +110,7 @@ TEST_F(ChannelManagerTest, SetVideoRtxEnabled) { EXPECT_FALSE(ContainsMatchingCodec(recv_codecs, rtx_codec, &field_trials_)); // Enable and check. - cm_ = cricket::ChannelManager::Create(CreateFakeMediaEngine(), + cm_ = cricket::ChannelManager::Create(media_engine_.get(), &ssrc_generator_, true, worker_, network_.get()); cm_->GetSupportedVideoSendCodecs(&send_codecs); EXPECT_TRUE(ContainsMatchingCodec(send_codecs, rtx_codec, &field_trials_)); @@ -114,7 +118,7 @@ TEST_F(ChannelManagerTest, SetVideoRtxEnabled) { EXPECT_TRUE(ContainsMatchingCodec(recv_codecs, rtx_codec, &field_trials_)); // Disable and check. - cm_ = cricket::ChannelManager::Create(CreateFakeMediaEngine(), + cm_ = cricket::ChannelManager::Create(media_engine_.get(), &ssrc_generator_, false, worker_, network_.get()); cm_->GetSupportedVideoSendCodecs(&send_codecs); EXPECT_FALSE(ContainsMatchingCodec(send_codecs, rtx_codec, &field_trials_)); diff --git a/pc/connection_context.cc b/pc/connection_context.cc index 818283d636..88cc7478af 100644 --- a/pc/connection_context.cc +++ b/pc/connection_context.cc @@ -99,6 +99,7 @@ ConnectionContext::ConnectionContext( wraps_current_thread_)), trials_(dependencies->trials ? std::move(dependencies->trials) : std::make_unique()), + media_engine_(std::move(dependencies->media_engine)), network_monitor_factory_( std::move(dependencies->network_monitor_factory)), call_factory_(std::move(dependencies->call_factory)), @@ -144,8 +145,9 @@ ConnectionContext::ConnectionContext( default_socket_factory_ = std::make_unique(socket_factory); + // TODO(bugs.webrtc.org/13931): Delete ChannelManager when functions gone. channel_manager_ = cricket::ChannelManager::Create( - std::move(dependencies->media_engine), + media_engine_.get(), &ssrc_generator_, /*enable_rtx=*/true, worker_thread(), network_thread()); // Set warning levels on the threads, to give warnings when response @@ -161,6 +163,15 @@ ConnectionContext::ConnectionContext( ConnectionContext::~ConnectionContext() { RTC_DCHECK_RUN_ON(signaling_thread_); channel_manager_.reset(nullptr); + worker_thread_->Invoke(RTC_FROM_HERE, [&] { + RTC_DCHECK_RUN_ON(worker_thread()); + // While `media_engine_` is const throughout the ChannelManager's lifetime, + // it requires destruction to happen on the worker thread. Instead of + // marking the pointer as non-const, we live with this const_cast<> in the + // destructor. + const_cast&>(media_engine_) + .reset(); + }); // Make sure `worker_thread()` and `signaling_thread()` outlive // `default_socket_factory_` and `default_network_manager_`. diff --git a/pc/connection_context.h b/pc/connection_context.h index 43239ffae0..eb3b614223 100644 --- a/pc/connection_context.h +++ b/pc/connection_context.h @@ -70,6 +70,7 @@ class ConnectionContext final } cricket::ChannelManager* channel_manager() const; + cricket::MediaEngineInterface* media_engine() const; rtc::Thread* signaling_thread() { return signaling_thread_; } const rtc::Thread* signaling_thread() const { return signaling_thread_; } @@ -121,6 +122,13 @@ class ConnectionContext final // channel_manager is accessed both on signaling thread and worker thread. // Const after construction, explicitly cleared in destructor. std::unique_ptr channel_manager_; + const std::unique_ptr media_engine_; + + // This object should be used to generate any SSRC that is not explicitly + // specified by the user (or by the remote party). + // TODO(bugs.webrtc.org/12666): This variable is used from both the signaling + // and worker threads. See if we can't restrict usage to a single thread. + rtc::UniqueRandomIdGenerator ssrc_generator_; std::unique_ptr const network_monitor_factory_ RTC_GUARDED_BY(signaling_thread_); std::unique_ptr default_network_manager_ diff --git a/pc/rtp_sender_receiver_unittest.cc b/pc/rtp_sender_receiver_unittest.cc index 5023917c40..e1285feec9 100644 --- a/pc/rtp_sender_receiver_unittest.cc +++ b/pc/rtp_sender_receiver_unittest.cc @@ -114,12 +114,12 @@ class RtpSenderReceiverTest webrtc::CreateBuiltinVideoBitrateAllocatorFactory()), // Create fake media engine/etc. so we can create channels to use to // test RtpSenders/RtpReceivers. - media_engine_(new cricket::FakeMediaEngine()), + media_engine_(std::make_unique()), fake_call_(worker_thread_, network_thread_), local_stream_(MediaStream::Create(kStreamId1)) { worker_thread_->Invoke(RTC_FROM_HERE, [&]() { channel_manager_ = cricket::ChannelManager::Create( - absl::WrapUnique(media_engine_), false, worker_thread_, + media_engine_.get(), &ssrc_generator_, false, worker_thread_, network_thread_); }); @@ -526,8 +526,8 @@ class RtpSenderReceiverTest std::unique_ptr rtp_transport_; std::unique_ptr video_bitrate_allocator_factory_; - // `media_engine_` is actually owned by `channel_manager_`. - cricket::FakeMediaEngine* media_engine_; + std::unique_ptr media_engine_; + rtc::UniqueRandomIdGenerator ssrc_generator_; std::unique_ptr channel_manager_; cricket::FakeCall fake_call_; std::unique_ptr voice_channel_; diff --git a/pc/rtp_transceiver_unittest.cc b/pc/rtp_transceiver_unittest.cc index 3c4b2c93e4..680fe111a1 100644 --- a/pc/rtp_transceiver_unittest.cc +++ b/pc/rtp_transceiver_unittest.cc @@ -38,22 +38,31 @@ using ::testing::ReturnRef; namespace webrtc { namespace { -class ChannelManagerForTest : public cricket::ChannelManager { + +class RtpTransceiverTest : public testing::Test { public: - ChannelManagerForTest() - : cricket::ChannelManager(std::make_unique(), - true, - rtc::Thread::Current(), - rtc::Thread::Current()) {} + RtpTransceiverTest() + : channel_manager_( + cricket::ChannelManager::Create(&fake_media_engine_, + &ssrc_generator_, + true, + rtc::Thread::Current(), + rtc::Thread::Current())) {} + + protected: + cricket::ChannelManager* channel_manager() { return channel_manager_.get(); } + + private: + cricket::FakeMediaEngine fake_media_engine_; + rtc::UniqueRandomIdGenerator ssrc_generator_; + std::unique_ptr channel_manager_; }; -} // namespace // Checks that a channel cannot be set on a stopped `RtpTransceiver`. -TEST(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) { - ChannelManagerForTest cm; +TEST_F(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) { const std::string content_name("my_mid"); auto transceiver = rtc::make_ref_counted( - cricket::MediaType::MEDIA_TYPE_AUDIO, &cm); + cricket::MediaType::MEDIA_TYPE_AUDIO, channel_manager()); auto channel1 = std::make_unique(); EXPECT_CALL(*channel1, media_type()) .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO)); @@ -85,11 +94,10 @@ TEST(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) { } // Checks that a channel can be unset on a stopped `RtpTransceiver` -TEST(RtpTransceiverTest, CanUnsetChannelOnStoppedTransceiver) { - ChannelManagerForTest cm; +TEST_F(RtpTransceiverTest, CanUnsetChannelOnStoppedTransceiver) { const std::string content_name("my_mid"); auto transceiver = rtc::make_ref_counted( - cricket::MediaType::MEDIA_TYPE_VIDEO, &cm); + cricket::MediaType::MEDIA_TYPE_VIDEO, channel_manager()); auto channel = std::make_unique(); EXPECT_CALL(*channel, media_type()) .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_VIDEO)); @@ -114,7 +122,7 @@ TEST(RtpTransceiverTest, CanUnsetChannelOnStoppedTransceiver) { EXPECT_EQ(nullptr, transceiver->channel()); } -class RtpTransceiverUnifiedPlanTest : public ::testing::Test { +class RtpTransceiverUnifiedPlanTest : public RtpTransceiverTest { public: RtpTransceiverUnifiedPlanTest() : transceiver_(rtc::make_ref_counted( @@ -125,8 +133,8 @@ class RtpTransceiverUnifiedPlanTest : public ::testing::Test { rtc::Thread::Current(), rtc::Thread::Current(), receiver_), - &channel_manager_, - channel_manager_.media_engine()->voice().GetRtpHeaderExtensions(), + channel_manager(), + channel_manager()->media_engine()->voice().GetRtpHeaderExtensions(), /* on_negotiation_needed= */ [] {})) {} static rtc::scoped_refptr MockReceiver() { @@ -145,7 +153,6 @@ class RtpTransceiverUnifiedPlanTest : public ::testing::Test { rtc::scoped_refptr receiver_ = MockReceiver(); rtc::scoped_refptr sender_ = MockSender(); - ChannelManagerForTest channel_manager_; rtc::scoped_refptr transceiver_; }; @@ -168,7 +175,7 @@ TEST_F(RtpTransceiverUnifiedPlanTest, StopSetsDirection) { *transceiver_->current_direction()); } -class RtpTransceiverTestForHeaderExtensions : public ::testing::Test { +class RtpTransceiverTestForHeaderExtensions : public RtpTransceiverTest { public: RtpTransceiverTestForHeaderExtensions() : extensions_( @@ -192,7 +199,7 @@ class RtpTransceiverTestForHeaderExtensions : public ::testing::Test { rtc::Thread::Current(), rtc::Thread::Current(), receiver_), - &channel_manager_, + channel_manager(), extensions_, /* on_negotiation_needed= */ [] {})) {} @@ -218,7 +225,6 @@ class RtpTransceiverTestForHeaderExtensions : public ::testing::Test { rtc::scoped_refptr receiver_ = MockReceiver(); rtc::scoped_refptr sender_ = MockSender(); - ChannelManagerForTest channel_manager_; std::vector extensions_; rtc::scoped_refptr transceiver_; }; @@ -404,4 +410,6 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, "uri5", 6, RtpTransceiverDirection::kSendRecv))); } +} // namespace + } // namespace webrtc diff --git a/pc/test/fake_peer_connection_for_stats.h b/pc/test/fake_peer_connection_for_stats.h index d7f97daf76..56aca142ac 100644 --- a/pc/test/fake_peer_connection_for_stats.h +++ b/pc/test/fake_peer_connection_for_stats.h @@ -415,7 +415,14 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase { class TestChannelManager : public cricket::ChannelManager { public: TestChannelManager(rtc::Thread* worker, rtc::Thread* network) - : cricket::ChannelManager(nullptr, true, worker, network) {} + : cricket::ChannelManager(nullptr, + &ssrc_generator_, + true, + worker, + network) {} + + private: + rtc::UniqueRandomIdGenerator ssrc_generator_; }; rtc::Thread* const network_thread_;