diff --git a/pc/channel_interface.h b/pc/channel_interface.h index a16a9b753d..a82880b5a6 100644 --- a/pc/channel_interface.h +++ b/pc/channel_interface.h @@ -11,6 +11,7 @@ #ifndef PC_CHANNEL_INTERFACE_H_ #define PC_CHANNEL_INTERFACE_H_ +#include #include #include @@ -42,8 +43,10 @@ struct MediaConfig; // ChannelInterface contains methods common to voice and video channels. // As more methods are added to BaseChannel, they should be included in the // interface as well. +// TODO(bugs.webrtc.org/13931): Merge this class into RtpTransceiver. class ChannelInterface { public: + virtual ~ChannelInterface() = default; virtual cricket::MediaType media_type() const = 0; virtual MediaChannel* media_channel() const = 0; @@ -83,14 +86,11 @@ class ChannelInterface { // * An SrtpTransport for SDES. // * A DtlsSrtpTransport for DTLS-SRTP. virtual bool SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport) = 0; - - protected: - virtual ~ChannelInterface() = default; }; class ChannelFactoryInterface { public: - virtual VideoChannel* CreateVideoChannel( + virtual std::unique_ptr CreateVideoChannel( webrtc::Call* call, const MediaConfig& media_config, const std::string& mid, @@ -100,7 +100,7 @@ class ChannelFactoryInterface { webrtc::VideoBitrateAllocatorFactory* video_bitrate_allocator_factory) = 0; - virtual VoiceChannel* CreateVoiceChannel( + virtual std::unique_ptr CreateVoiceChannel( webrtc::Call* call, const MediaConfig& media_config, const std::string& mid, @@ -108,8 +108,6 @@ class ChannelFactoryInterface { const webrtc::CryptoOptions& crypto_options, const AudioOptions& options) = 0; - virtual void DestroyChannel(ChannelInterface* channel) = 0; - protected: virtual ~ChannelFactoryInterface() = default; }; diff --git a/pc/channel_manager.cc b/pc/channel_manager.cc index 9cdba420c2..6ca7973561 100644 --- a/pc/channel_manager.cc +++ b/pc/channel_manager.cc @@ -63,8 +63,6 @@ ChannelManager::~ChannelManager() { RTC_DCHECK_RUN_ON(signaling_thread_); worker_thread_->Invoke(RTC_FROM_HERE, [&] { RTC_DCHECK_RUN_ON(worker_thread_); - RTC_DCHECK(voice_channels_.empty()); - RTC_DCHECK(video_channels_.empty()); // 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 @@ -151,7 +149,7 @@ ChannelManager::GetSupportedVideoRtpHeaderExtensions() const { return media_engine_->video().GetRtpHeaderExtensions(); } -VoiceChannel* ChannelManager::CreateVoiceChannel( +std::unique_ptr ChannelManager::CreateVoiceChannel( webrtc::Call* call, const MediaConfig& media_config, const std::string& mid, @@ -164,10 +162,11 @@ VoiceChannel* ChannelManager::CreateVoiceChannel( // PeerConnection and add the expectation that we're already on the right // thread. if (!worker_thread_->IsCurrent()) { - return worker_thread_->Invoke(RTC_FROM_HERE, [&] { - return CreateVoiceChannel(call, media_config, mid, srtp_required, - crypto_options, options); - }); + return worker_thread_->Invoke>( + RTC_FROM_HERE, [&] { + return CreateVoiceChannel(call, media_config, mid, srtp_required, + crypto_options, options); + }); } RTC_DCHECK_RUN_ON(worker_thread_); @@ -183,19 +182,10 @@ VoiceChannel* ChannelManager::CreateVoiceChannel( absl::WrapUnique(media_channel), mid, srtp_required, crypto_options, &ssrc_generator_); - VoiceChannel* voice_channel_ptr = voice_channel.get(); - voice_channels_.push_back(std::move(voice_channel)); - return voice_channel_ptr; + return voice_channel; } -void ChannelManager::DestroyVoiceChannel(VoiceChannel* channel) { - TRACE_EVENT0("webrtc", "ChannelManager::DestroyVoiceChannel"); - RTC_DCHECK_RUN_ON(worker_thread_); - voice_channels_.erase(absl::c_find_if( - voice_channels_, [&](const auto& p) { return p.get() == channel; })); -} - -VideoChannel* ChannelManager::CreateVideoChannel( +std::unique_ptr ChannelManager::CreateVideoChannel( webrtc::Call* call, const MediaConfig& media_config, const std::string& mid, @@ -209,11 +199,12 @@ VideoChannel* ChannelManager::CreateVideoChannel( // PeerConnection and add the expectation that we're already on the right // thread. if (!worker_thread_->IsCurrent()) { - return worker_thread_->Invoke(RTC_FROM_HERE, [&] { - return CreateVideoChannel(call, media_config, mid, srtp_required, - crypto_options, options, - video_bitrate_allocator_factory); - }); + return worker_thread_->Invoke>( + RTC_FROM_HERE, [&] { + return CreateVideoChannel(call, media_config, mid, srtp_required, + crypto_options, options, + video_bitrate_allocator_factory); + }); } RTC_DCHECK_RUN_ON(worker_thread_); @@ -230,37 +221,7 @@ VideoChannel* ChannelManager::CreateVideoChannel( absl::WrapUnique(media_channel), mid, srtp_required, crypto_options, &ssrc_generator_); - VideoChannel* video_channel_ptr = video_channel.get(); - video_channels_.push_back(std::move(video_channel)); - return video_channel_ptr; -} - -void ChannelManager::DestroyVideoChannel(VideoChannel* channel) { - TRACE_EVENT0("webrtc", "ChannelManager::DestroyVideoChannel"); - RTC_DCHECK_RUN_ON(worker_thread_); - - video_channels_.erase(absl::c_find_if( - video_channels_, [&](const auto& p) { return p.get() == channel; })); -} - -void ChannelManager::DestroyChannel(ChannelInterface* channel) { - RTC_DCHECK(channel); - - if (!worker_thread_->IsCurrent()) { - // TODO(tommi): Do this asynchronously when we have a way to make sure that - // the call to DestroyChannel runs before ~Call() runs, which today happens - // inside an Invoke from the signaling thread in PeerConnectin::Close(). - worker_thread_->Invoke(RTC_FROM_HERE, - [&] { DestroyChannel(channel); }); - return; - } - - if (channel->media_type() == MEDIA_TYPE_AUDIO) { - DestroyVoiceChannel(static_cast(channel)); - } else { - RTC_DCHECK_EQ(channel->media_type(), MEDIA_TYPE_VIDEO); - DestroyVideoChannel(static_cast(channel)); - } + return video_channel; } bool ChannelManager::StartAecDump(webrtc::FileWrapper file, diff --git a/pc/channel_manager.h b/pc/channel_manager.h index b4de2d1b8f..6fd27edffe 100644 --- a/pc/channel_manager.h +++ b/pc/channel_manager.h @@ -76,21 +76,22 @@ class ChannelManager : public ChannelFactoryInterface { GetSupportedVideoRtpHeaderExtensions() const; // The operations below all occur on the worker thread. - // ChannelManager retains ownership of the created channels, so clients should - // call the appropriate Destroy*Channel method when done. + // The caller is responsible for ensuring that destruction happens + // on the worker thread. // Creates a voice channel, to be associated with the specified session. - VoiceChannel* CreateVoiceChannel(webrtc::Call* call, - const MediaConfig& media_config, - const std::string& mid, - bool srtp_required, - const webrtc::CryptoOptions& crypto_options, - const AudioOptions& options) override; + std::unique_ptr CreateVoiceChannel( + webrtc::Call* call, + const MediaConfig& media_config, + const std::string& mid, + bool srtp_required, + const webrtc::CryptoOptions& crypto_options, + const AudioOptions& options) override; // Creates a video channel, synced with the specified voice channel, and // associated with the specified session. // Version of the above that takes PacketTransportInternal. - VideoChannel* CreateVideoChannel( + std::unique_ptr CreateVideoChannel( webrtc::Call* call, const MediaConfig& media_config, const std::string& mid, @@ -100,8 +101,6 @@ class ChannelManager : public ChannelFactoryInterface { webrtc::VideoBitrateAllocatorFactory* video_bitrate_allocator_factory) override; - void DestroyChannel(ChannelInterface* channel) override; - // Starts AEC dump using existing file, with a specified maximum file size in // bytes. When the limit is reached, logging will stop and the file will be // closed. If max_size_bytes is set to <= 0, no limit will be used. @@ -134,12 +133,6 @@ class ChannelManager : public ChannelFactoryInterface { // and worker threads. See if we can't restrict usage to a single thread. rtc::UniqueRandomIdGenerator ssrc_generator_; - // Vector contents are non-null. - std::vector> voice_channels_ - RTC_GUARDED_BY(worker_thread_); - std::vector> video_channels_ - RTC_GUARDED_BY(worker_thread_); - const bool enable_rtx_; }; diff --git a/pc/channel_manager_unittest.cc b/pc/channel_manager_unittest.cc index ebc3873a27..6d7318b446 100644 --- a/pc/channel_manager_unittest.cc +++ b/pc/channel_manager_unittest.cc @@ -69,19 +69,20 @@ 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(), cricket::CN_AUDIO, - kDefaultSrtpRequired, webrtc::CryptoOptions(), AudioOptions()); + std::unique_ptr voice_channel = + cm_->CreateVoiceChannel(&fake_call_, cricket::MediaConfig(), + cricket::CN_AUDIO, kDefaultSrtpRequired, + webrtc::CryptoOptions(), AudioOptions()); ASSERT_TRUE(voice_channel != nullptr); - cricket::VideoChannel* video_channel = cm_->CreateVideoChannel( - &fake_call_, cricket::MediaConfig(), cricket::CN_VIDEO, - kDefaultSrtpRequired, webrtc::CryptoOptions(), VideoOptions(), - video_bitrate_allocator_factory_.get()); + std::unique_ptr video_channel = + cm_->CreateVideoChannel(&fake_call_, cricket::MediaConfig(), + cricket::CN_VIDEO, kDefaultSrtpRequired, + webrtc::CryptoOptions(), VideoOptions(), + video_bitrate_allocator_factory_.get()); ASSERT_TRUE(video_channel != nullptr); - - cm_->DestroyChannel(video_channel); - cm_->DestroyChannel(voice_channel); + // Destruction is tested by having the owning pointers + // go out of scope. } std::unique_ptr network_; diff --git a/pc/rtp_sender_receiver_unittest.cc b/pc/rtp_sender_receiver_unittest.cc index 077be486a5..5023917c40 100644 --- a/pc/rtp_sender_receiver_unittest.cc +++ b/pc/rtp_sender_receiver_unittest.cc @@ -181,9 +181,6 @@ class RtpSenderReceiverTest voice_channel_->SetRtpTransport(nullptr); video_channel_->SetRtpTransport(nullptr); - - channel_manager_->DestroyChannel(voice_channel_); - channel_manager_->DestroyChannel(video_channel_); } std::unique_ptr CreateDtlsSrtpTransport() { @@ -533,8 +530,8 @@ class RtpSenderReceiverTest cricket::FakeMediaEngine* media_engine_; std::unique_ptr channel_manager_; cricket::FakeCall fake_call_; - cricket::VoiceChannel* voice_channel_; - cricket::VideoChannel* video_channel_; + std::unique_ptr voice_channel_; + std::unique_ptr video_channel_; cricket::FakeVoiceMediaChannel* voice_media_channel_; cricket::FakeVideoMediaChannel* video_media_channel_; rtc::scoped_refptr audio_rtp_sender_; diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index e2e69f6670..d7474185eb 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc @@ -161,7 +161,7 @@ RtpTransceiver::~RtpTransceiver() { } void RtpTransceiver::SetChannel( - cricket::ChannelInterface* channel, + std::unique_ptr channel, std::function transport_lookup) { RTC_DCHECK_RUN_ON(thread_); RTC_DCHECK(channel); @@ -177,6 +177,8 @@ void RtpTransceiver::SetChannel( RTC_DCHECK_EQ(media_type(), channel->media_type()); signaling_thread_safety_ = PendingTaskSafetyFlag::Create(); + std::unique_ptr channel_to_delete; + // An alternative to this, could be to require SetChannel to be called // on the network thread. The channel object operates for the most part // on the network thread, as part of its initialization being on the network @@ -187,7 +189,13 @@ void RtpTransceiver::SetChannel( // helps with keeping the channel implementation requirements being met and // avoids synchronization for accessing the pointer or network related state. channel_manager_->network_thread()->Invoke(RTC_FROM_HERE, [&]() { - channel_ = channel; + if (channel_) { + channel_->SetFirstPacketReceivedCallback(nullptr); + channel_->SetRtpTransport(nullptr); + channel_to_delete = std::move(channel_); + } + + channel_ = std::move(channel); channel_->SetRtpTransport(transport_lookup(channel_->mid())); channel_->SetFirstPacketReceivedCallback( @@ -214,26 +222,24 @@ void RtpTransceiver::ClearChannel() { signaling_thread_safety_->SetNotAlive(); signaling_thread_safety_ = nullptr; } - cricket::ChannelInterface* channel_to_delete = nullptr; + std::unique_ptr channel_to_delete; channel_manager_->network_thread()->Invoke(RTC_FROM_HERE, [&]() { if (channel_) { channel_->SetFirstPacketReceivedCallback(nullptr); channel_->SetRtpTransport(nullptr); - channel_to_delete = channel_; + channel_to_delete = std::move(channel_); } - - channel_ = nullptr; }); RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(1); - PushNewMediaChannelAndDeleteChannel(channel_to_delete); + PushNewMediaChannelAndDeleteChannel(std::move(channel_to_delete)); RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(2); } void RtpTransceiver::PushNewMediaChannelAndDeleteChannel( - cricket::ChannelInterface* channel_to_delete) { + std::unique_ptr channel_to_delete) { // The clumsy combination of pushing down media channel and deleting // the channel is due to the desire to do both things in one Invoke(). if (!channel_to_delete && senders_.empty() && receivers_.empty()) { @@ -253,7 +259,7 @@ void RtpTransceiver::PushNewMediaChannelAndDeleteChannel( // Destroy the channel, if we had one, now _after_ updating the receivers // who might have had references to the previous channel. if (channel_to_delete) { - channel_manager_->DestroyChannel(channel_to_delete); + channel_to_delete.reset(nullptr); } }); } diff --git a/pc/rtp_transceiver.h b/pc/rtp_transceiver.h index a42cae771e..5945d6a525 100644 --- a/pc/rtp_transceiver.h +++ b/pc/rtp_transceiver.h @@ -13,8 +13,8 @@ #include -#include #include +#include #include #include @@ -100,7 +100,7 @@ class RtpTransceiver : public RtpTransceiverInterface, // Returns the Voice/VideoChannel set for this transceiver. May be null if // the transceiver is not in the currently set local/remote description. - cricket::ChannelInterface* channel() const { return channel_; } + cricket::ChannelInterface* channel() const { return channel_.get(); } // Sets the Voice/VideoChannel. The caller must pass in the correct channel // implementation based on the type of the transceiver. The call must @@ -112,12 +112,7 @@ class RtpTransceiver : public RtpTransceiverInterface, // 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 - // `ClearChannel()` is called. - // 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. + // The transceiver takes ownership of `channel`. // // `transport_lookup`: This // callback function will be used to look up the `RtpTransport` object @@ -132,7 +127,7 @@ class RtpTransceiver : public RtpTransceiverInterface, // The callback allows us to combine the transport lookup with network // state initialization of the channel object. // ClearChannel() must be used before calling SetChannel() again. - void SetChannel(cricket::ChannelInterface* channel, + void SetChannel(std::unique_ptr channel, std::function transport_lookup); @@ -285,7 +280,7 @@ class RtpTransceiver : public RtpTransceiverInterface, // Delete a channel, and ensure that references to its media channel // are updated before deleting it. void PushNewMediaChannelAndDeleteChannel( - cricket::ChannelInterface* channel_to_delete); + std::unique_ptr channel_to_delete); // Enforce that this object is created, used and destroyed on one thread. TaskQueueBase* const thread_; @@ -313,7 +308,7 @@ class RtpTransceiver : public RtpTransceiverInterface, // Accessed on both thread_ and the network thread. Considered safe // because all access on the network thread is within an invoke() // from thread_. - cricket::ChannelInterface* channel_ = nullptr; + std::unique_ptr channel_ = nullptr; cricket::ChannelManager* channel_manager_ = nullptr; std::vector codec_preferences_; std::vector header_extensions_to_offer_; diff --git a/pc/rtp_transceiver_unittest.cc b/pc/rtp_transceiver_unittest.cc index 29828a045c..12b706590a 100644 --- a/pc/rtp_transceiver_unittest.cc +++ b/pc/rtp_transceiver_unittest.cc @@ -13,6 +13,7 @@ #include "pc/rtp_transceiver.h" #include +#include #include "absl/strings/string_view.h" #include "absl/types/optional.h" @@ -43,8 +44,6 @@ class ChannelManagerForTest : public cricket::ChannelManager { true, rtc::Thread::Current(), rtc::Thread::Current()) {} - - MOCK_METHOD(void, DestroyChannel, (cricket::ChannelInterface*), (override)); }; } // namespace @@ -54,37 +53,34 @@ TEST(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) { const std::string content_name("my_mid"); auto transceiver = rtc::make_ref_counted( cricket::MediaType::MEDIA_TYPE_AUDIO, &cm); - cricket::MockChannelInterface channel1; - EXPECT_CALL(channel1, media_type()) + auto channel1 = std::make_unique(); + EXPECT_CALL(*channel1, media_type()) .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO)); - EXPECT_CALL(channel1, mid()).WillRepeatedly(ReturnRef(content_name)); - EXPECT_CALL(channel1, SetFirstPacketReceivedCallback(_)); - EXPECT_CALL(channel1, SetRtpTransport(_)).WillRepeatedly(Return(true)); - - transceiver->SetChannel(&channel1, [&](const std::string& mid) { + EXPECT_CALL(*channel1, mid()).WillRepeatedly(ReturnRef(content_name)); + EXPECT_CALL(*channel1, SetFirstPacketReceivedCallback(_)); + EXPECT_CALL(*channel1, SetRtpTransport(_)).WillRepeatedly(Return(true)); + auto channel1_ptr = channel1.get(); + transceiver->SetChannel(std::move(channel1), [&](const std::string& mid) { EXPECT_EQ(mid, content_name); return nullptr; }); - EXPECT_EQ(&channel1, transceiver->channel()); + EXPECT_EQ(channel1_ptr, transceiver->channel()); // Stop the transceiver. transceiver->StopInternal(); - EXPECT_EQ(&channel1, transceiver->channel()); + EXPECT_EQ(channel1_ptr, transceiver->channel()); - cricket::MockChannelInterface channel2; - EXPECT_CALL(channel2, media_type()) + auto channel2 = std::make_unique(); + EXPECT_CALL(*channel2, media_type()) .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO)); // Clear the current channel - required to allow SetChannel() - EXPECT_CALL(channel1, SetFirstPacketReceivedCallback(_)); - EXPECT_CALL(cm, DestroyChannel(&channel1)).WillRepeatedly(testing::Return()); + EXPECT_CALL(*channel1_ptr, SetFirstPacketReceivedCallback(_)); transceiver->ClearChannel(); // Channel can no longer be set, so this call should be a no-op. - transceiver->SetChannel(&channel2, + transceiver->SetChannel(std::move(channel2), [](const std::string&) { return nullptr; }); EXPECT_EQ(nullptr, transceiver->channel()); - - transceiver->ClearChannel(); } // Checks that a channel can be unset on a stopped `RtpTransceiver` @@ -93,24 +89,24 @@ TEST(RtpTransceiverTest, CanUnsetChannelOnStoppedTransceiver) { const std::string content_name("my_mid"); auto transceiver = rtc::make_ref_counted( cricket::MediaType::MEDIA_TYPE_VIDEO, &cm); - cricket::MockChannelInterface channel; - EXPECT_CALL(channel, media_type()) + auto channel = std::make_unique(); + EXPECT_CALL(*channel, media_type()) .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_VIDEO)); - EXPECT_CALL(channel, mid()).WillRepeatedly(ReturnRef(content_name)); - EXPECT_CALL(channel, SetFirstPacketReceivedCallback(_)) + EXPECT_CALL(*channel, mid()).WillRepeatedly(ReturnRef(content_name)); + EXPECT_CALL(*channel, SetFirstPacketReceivedCallback(_)) .WillRepeatedly(testing::Return()); - EXPECT_CALL(channel, SetRtpTransport(_)).WillRepeatedly(Return(true)); - EXPECT_CALL(cm, DestroyChannel(&channel)).WillRepeatedly(testing::Return()); + EXPECT_CALL(*channel, SetRtpTransport(_)).WillRepeatedly(Return(true)); - transceiver->SetChannel(&channel, [&](const std::string& mid) { + auto channel_ptr = channel.get(); + transceiver->SetChannel(std::move(channel), [&](const std::string& mid) { EXPECT_EQ(mid, content_name); return nullptr; }); - EXPECT_EQ(&channel, transceiver->channel()); + EXPECT_EQ(channel_ptr, transceiver->channel()); // Stop the transceiver. transceiver->StopInternal(); - EXPECT_EQ(&channel, transceiver->channel()); + EXPECT_EQ(channel_ptr, transceiver->channel()); // Set the channel to `nullptr`. transceiver->ClearChannel(); @@ -213,11 +209,8 @@ class RtpTransceiverTestForHeaderExtensions : public ::testing::Test { return sender; } - void ClearChannel(cricket::MockChannelInterface& mock_channel) { + void ClearChannel() { EXPECT_CALL(*sender_.get(), SetMediaChannel(_)); - EXPECT_CALL(mock_channel, SetFirstPacketReceivedCallback(_)); - EXPECT_CALL(channel_manager_, DestroyChannel(&mock_channel)) - .WillRepeatedly(testing::Return()); transceiver_->ClearChannel(); } @@ -328,18 +321,20 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, EXPECT_CALL(*sender_.get(), SetMediaChannel(_)); EXPECT_CALL(*sender_.get(), SetTransceiverAsStopped()); EXPECT_CALL(*sender_.get(), Stop()); - cricket::MockChannelInterface mock_channel; - EXPECT_CALL(mock_channel, SetFirstPacketReceivedCallback(_)); - EXPECT_CALL(mock_channel, media_type()) + auto mock_channel = std::make_unique(); + auto mock_channel_ptr = mock_channel.get(); + EXPECT_CALL(*mock_channel, SetFirstPacketReceivedCallback(_)); + 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, mid()).WillRepeatedly(ReturnRef(content_name)); - EXPECT_CALL(mock_channel, SetRtpTransport(_)).WillRepeatedly(Return(true)); - transceiver_->SetChannel(&mock_channel, + EXPECT_CALL(*mock_channel, media_channel()).WillRepeatedly(Return(nullptr)); + EXPECT_CALL(*mock_channel, mid()).WillRepeatedly(ReturnRef(content_name)); + EXPECT_CALL(*mock_channel, SetRtpTransport(_)).WillRepeatedly(Return(true)); + transceiver_->SetChannel(std::move(mock_channel), [](const std::string&) { return nullptr; }); EXPECT_THAT(transceiver_->HeaderExtensionsNegotiated(), ElementsAre()); - ClearChannel(mock_channel); + EXPECT_CALL(*mock_channel_ptr, SetFirstPacketReceivedCallback(_)); + ClearChannel(); } TEST_F(RtpTransceiverTestForHeaderExtensions, ReturnsNegotiatedHdrExts) { @@ -350,13 +345,14 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, ReturnsNegotiatedHdrExts) { EXPECT_CALL(*sender_.get(), SetTransceiverAsStopped()); EXPECT_CALL(*sender_.get(), Stop()); - cricket::MockChannelInterface mock_channel; - EXPECT_CALL(mock_channel, SetFirstPacketReceivedCallback(_)); - EXPECT_CALL(mock_channel, media_type()) + auto mock_channel = std::make_unique(); + auto mock_channel_ptr = mock_channel.get(); + EXPECT_CALL(*mock_channel, SetFirstPacketReceivedCallback(_)); + 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, mid()).WillRepeatedly(ReturnRef(content_name)); - EXPECT_CALL(mock_channel, SetRtpTransport(_)).WillRepeatedly(Return(true)); + EXPECT_CALL(*mock_channel, media_channel()).WillRepeatedly(Return(nullptr)); + EXPECT_CALL(*mock_channel, mid()).WillRepeatedly(ReturnRef(content_name)); + EXPECT_CALL(*mock_channel, SetRtpTransport(_)).WillRepeatedly(Return(true)); cricket::RtpHeaderExtensions extensions = {webrtc::RtpExtension("uri1", 1), webrtc::RtpExtension("uri2", 2)}; @@ -364,7 +360,7 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, ReturnsNegotiatedHdrExts) { description.set_rtp_header_extensions(extensions); transceiver_->OnNegotiationUpdate(SdpType::kAnswer, &description); - transceiver_->SetChannel(&mock_channel, + transceiver_->SetChannel(std::move(mock_channel), [](const std::string&) { return nullptr; }); EXPECT_THAT(transceiver_->HeaderExtensionsNegotiated(), ElementsAre(RtpHeaderExtensionCapability( @@ -372,7 +368,8 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, ReturnsNegotiatedHdrExts) { RtpHeaderExtensionCapability( "uri2", 2, RtpTransceiverDirection::kSendRecv))); - ClearChannel(mock_channel); + EXPECT_CALL(*mock_channel_ptr, SetFirstPacketReceivedCallback(_)); + ClearChannel(); } TEST_F(RtpTransceiverTestForHeaderExtensions, diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 9014852c15..5c1aa85f14 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -3613,22 +3613,24 @@ RTCError SdpOfferAnswerHandler::UpdateTransceiverChannel( } } else { if (!channel) { + std::unique_ptr new_channel; if (transceiver->media_type() == cricket::MEDIA_TYPE_AUDIO) { - channel = CreateVoiceChannel(content.name); + new_channel = CreateVoiceChannel(content.name); } else { RTC_DCHECK_EQ(cricket::MEDIA_TYPE_VIDEO, transceiver->media_type()); - channel = CreateVideoChannel(content.name); + new_channel = CreateVideoChannel(content.name); } - if (!channel) { + if (!new_channel) { return RTCError(RTCErrorType::INTERNAL_ERROR, "Failed to create channel for mid=" + content.name); } // Note: this is a thread hop; the lambda will be executed // on the network thread. - transceiver->internal()->SetChannel(channel, [&](const std::string& mid) { - RTC_DCHECK_RUN_ON(network_thread()); - return transport_controller_n()->GetRtpTransport(mid); - }); + transceiver->internal()->SetChannel( + std::move(new_channel), [&](const std::string& mid) { + RTC_DCHECK_RUN_ON(network_thread()); + return transport_controller_n()->GetRtpTransport(mid); + }); } } return RTCError::OK(); @@ -4801,13 +4803,14 @@ RTCError SdpOfferAnswerHandler::CreateChannels(const SessionDescription& desc) { const cricket::ContentInfo* voice = cricket::GetFirstAudioContent(&desc); if (voice && !voice->rejected && !rtp_manager()->GetAudioTransceiver()->internal()->channel()) { - cricket::VoiceChannel* voice_channel = CreateVoiceChannel(voice->name); + std::unique_ptr voice_channel = + CreateVoiceChannel(voice->name); if (!voice_channel) { return RTCError(RTCErrorType::INTERNAL_ERROR, "Failed to create voice channel."); } rtp_manager()->GetAudioTransceiver()->internal()->SetChannel( - voice_channel, [&](const std::string& mid) { + std::move(voice_channel), [&](const std::string& mid) { RTC_DCHECK_RUN_ON(network_thread()); return transport_controller_n()->GetRtpTransport(mid); }); @@ -4816,13 +4819,14 @@ RTCError SdpOfferAnswerHandler::CreateChannels(const SessionDescription& desc) { const cricket::ContentInfo* video = cricket::GetFirstVideoContent(&desc); if (video && !video->rejected && !rtp_manager()->GetVideoTransceiver()->internal()->channel()) { - cricket::VideoChannel* video_channel = CreateVideoChannel(video->name); + std::unique_ptr video_channel = + CreateVideoChannel(video->name); if (!video_channel) { return RTCError(RTCErrorType::INTERNAL_ERROR, "Failed to create video channel."); } rtp_manager()->GetVideoTransceiver()->internal()->SetChannel( - video_channel, [&](const std::string& mid) { + std::move(video_channel), [&](const std::string& mid) { RTC_DCHECK_RUN_ON(network_thread()); return transport_controller_n()->GetRtpTransport(mid); }); @@ -4841,8 +4845,8 @@ RTCError SdpOfferAnswerHandler::CreateChannels(const SessionDescription& desc) { } // TODO(steveanton): Perhaps this should be managed by the RtpTransceiver. -cricket::VoiceChannel* SdpOfferAnswerHandler::CreateVoiceChannel( - const std::string& mid) { +std::unique_ptr +SdpOfferAnswerHandler::CreateVoiceChannel(const std::string& mid) { TRACE_EVENT0("webrtc", "SdpOfferAnswerHandler::CreateVoiceChannel"); RTC_DCHECK_RUN_ON(signaling_thread()); if (!channel_manager()->media_engine()) @@ -4857,8 +4861,8 @@ cricket::VoiceChannel* SdpOfferAnswerHandler::CreateVoiceChannel( } // TODO(steveanton): Perhaps this should be managed by the RtpTransceiver. -cricket::VideoChannel* SdpOfferAnswerHandler::CreateVideoChannel( - const std::string& mid) { +std::unique_ptr +SdpOfferAnswerHandler::CreateVideoChannel(const std::string& mid) { TRACE_EVENT0("webrtc", "SdpOfferAnswerHandler::CreateVideoChannel"); RTC_DCHECK_RUN_ON(signaling_thread()); if (!channel_manager()->media_engine()) diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index ae5b668415..4463f05ff3 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -528,8 +528,10 @@ class SdpOfferAnswerHandler : public SdpStateProvider, RTCError CreateChannels(const cricket::SessionDescription& desc); // Helper methods to create media channels. - cricket::VoiceChannel* CreateVoiceChannel(const std::string& mid); - cricket::VideoChannel* CreateVideoChannel(const std::string& mid); + std::unique_ptr CreateVoiceChannel( + const std::string& mid); + std::unique_ptr CreateVideoChannel( + const std::string& mid); bool CreateDataChannel(const std::string& mid); // Destroys the RTP data channel transport and/or the SCTP data channel diff --git a/pc/test/fake_peer_connection_for_stats.h b/pc/test/fake_peer_connection_for_stats.h index a263c1eed2..acf8e283a2 100644 --- a/pc/test/fake_peer_connection_for_stats.h +++ b/pc/test/fake_peer_connection_for_stats.h @@ -206,18 +206,17 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase { const std::string& mid, const std::string& transport_name, cricket::VoiceMediaInfo initial_stats = cricket::VoiceMediaInfo()) { - RTC_DCHECK(!voice_channel_); auto voice_media_channel = std::make_unique(network_thread_); auto* voice_media_channel_ptr = voice_media_channel.get(); - voice_channel_ = std::make_unique( + auto voice_channel = std::make_unique( worker_thread_, network_thread_, signaling_thread_, std::move(voice_media_channel), mid, kDefaultSrtpRequired, webrtc::CryptoOptions(), &channel_manager_.ssrc_generator(), transport_name); GetOrCreateFirstTransceiverOfType(cricket::MEDIA_TYPE_AUDIO) ->internal() - ->SetChannel(voice_channel_.get(), + ->SetChannel(std::move(voice_channel), [](const std::string&) { return nullptr; }); voice_media_channel_ptr->SetStats(initial_stats); return voice_media_channel_ptr; @@ -227,18 +226,17 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase { const std::string& mid, const std::string& transport_name, cricket::VideoMediaInfo initial_stats = cricket::VideoMediaInfo()) { - RTC_DCHECK(!video_channel_); auto video_media_channel = std::make_unique(network_thread_); auto video_media_channel_ptr = video_media_channel.get(); - video_channel_ = std::make_unique( + auto video_channel = std::make_unique( worker_thread_, network_thread_, signaling_thread_, std::move(video_media_channel), mid, kDefaultSrtpRequired, webrtc::CryptoOptions(), &channel_manager_.ssrc_generator(), transport_name); GetOrCreateFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO) ->internal() - ->SetChannel(video_channel_.get(), + ->SetChannel(std::move(video_channel), [](const std::string&) { return nullptr; }); video_media_channel_ptr->SetStats(initial_stats); return video_media_channel_ptr; @@ -417,10 +415,6 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase { public: TestChannelManager(rtc::Thread* worker, rtc::Thread* network) : cricket::ChannelManager(nullptr, true, worker, network) {} - - // Override DestroyChannel so that calls from the transceiver won't go to - // the default ChannelManager implementation. - void DestroyChannel(cricket::ChannelInterface*) override {} }; rtc::Thread* const network_thread_; @@ -438,9 +432,6 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase { FakeDataChannelProvider data_channel_provider_; - std::unique_ptr voice_channel_; - std::unique_ptr video_channel_; - std::vector> sctp_data_channels_; std::map transport_stats_by_name_;