Move ownership of the Channel class to RTCRtpTransceiver This makes the channel manager object into a factory, not a manager. Bug: webrtc:13931 Change-Id: I59f7d818a739797a7c0a7a32e6583450834df122 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260467 Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org> Commit-Queue: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/main@{#36718}
diff --git a/pc/channel_interface.h b/pc/channel_interface.h index a16a9b7..a82880b 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 <memory> #include <string> #include <vector> @@ -42,8 +43,10 @@ // 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 @@ // * 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<VideoChannel> CreateVideoChannel( webrtc::Call* call, const MediaConfig& media_config, const std::string& mid, @@ -100,7 +100,7 @@ webrtc::VideoBitrateAllocatorFactory* video_bitrate_allocator_factory) = 0; - virtual VoiceChannel* CreateVoiceChannel( + virtual std::unique_ptr<VoiceChannel> CreateVoiceChannel( webrtc::Call* call, const MediaConfig& media_config, const std::string& mid, @@ -108,8 +108,6 @@ 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 9cdba42..6ca7973 100644 --- a/pc/channel_manager.cc +++ b/pc/channel_manager.cc
@@ -63,8 +63,6 @@ RTC_DCHECK_RUN_ON(signaling_thread_); worker_thread_->Invoke<void>(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 @@ return media_engine_->video().GetRtpHeaderExtensions(); } -VoiceChannel* ChannelManager::CreateVoiceChannel( +std::unique_ptr<VoiceChannel> ChannelManager::CreateVoiceChannel( webrtc::Call* call, const MediaConfig& media_config, const std::string& mid, @@ -164,10 +162,11 @@ // PeerConnection and add the expectation that we're already on the right // thread. if (!worker_thread_->IsCurrent()) { - return worker_thread_->Invoke<VoiceChannel*>(RTC_FROM_HERE, [&] { - return CreateVoiceChannel(call, media_config, mid, srtp_required, - crypto_options, options); - }); + return worker_thread_->Invoke<std::unique_ptr<VoiceChannel>>( + RTC_FROM_HERE, [&] { + return CreateVoiceChannel(call, media_config, mid, srtp_required, + crypto_options, options); + }); } RTC_DCHECK_RUN_ON(worker_thread_); @@ -183,19 +182,10 @@ 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<VideoChannel> ChannelManager::CreateVideoChannel( webrtc::Call* call, const MediaConfig& media_config, const std::string& mid, @@ -209,11 +199,12 @@ // PeerConnection and add the expectation that we're already on the right // thread. if (!worker_thread_->IsCurrent()) { - return worker_thread_->Invoke<VideoChannel*>(RTC_FROM_HERE, [&] { - return CreateVideoChannel(call, media_config, mid, srtp_required, - crypto_options, options, - video_bitrate_allocator_factory); - }); + return worker_thread_->Invoke<std::unique_ptr<VideoChannel>>( + 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 @@ 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<void>(RTC_FROM_HERE, - [&] { DestroyChannel(channel); }); - return; - } - - if (channel->media_type() == MEDIA_TYPE_AUDIO) { - DestroyVoiceChannel(static_cast<VoiceChannel*>(channel)); - } else { - RTC_DCHECK_EQ(channel->media_type(), MEDIA_TYPE_VIDEO); - DestroyVideoChannel(static_cast<VideoChannel*>(channel)); - } + return video_channel; } bool ChannelManager::StartAecDump(webrtc::FileWrapper file,
diff --git a/pc/channel_manager.h b/pc/channel_manager.h index b4de2d1..6fd27ed 100644 --- a/pc/channel_manager.h +++ b/pc/channel_manager.h
@@ -76,21 +76,22 @@ 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<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; // 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<VideoChannel> CreateVideoChannel( webrtc::Call* call, const MediaConfig& media_config, const std::string& mid, @@ -100,8 +101,6 @@ 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 @@ // 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<std::unique_ptr<VoiceChannel>> voice_channels_ - RTC_GUARDED_BY(worker_thread_); - std::vector<std::unique_ptr<VideoChannel>> 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 ebc3873..6d7318b 100644 --- a/pc/channel_manager_unittest.cc +++ b/pc/channel_manager_unittest.cc
@@ -69,19 +69,20 @@ 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<cricket::VoiceChannel> 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<cricket::VideoChannel> 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<rtc::Thread> network_;
diff --git a/pc/rtp_sender_receiver_unittest.cc b/pc/rtp_sender_receiver_unittest.cc index 077be48..5023917 100644 --- a/pc/rtp_sender_receiver_unittest.cc +++ b/pc/rtp_sender_receiver_unittest.cc
@@ -181,9 +181,6 @@ voice_channel_->SetRtpTransport(nullptr); video_channel_->SetRtpTransport(nullptr); - - channel_manager_->DestroyChannel(voice_channel_); - channel_manager_->DestroyChannel(video_channel_); } std::unique_ptr<webrtc::RtpTransportInternal> CreateDtlsSrtpTransport() { @@ -533,8 +530,8 @@ cricket::FakeMediaEngine* media_engine_; std::unique_ptr<cricket::ChannelManager> channel_manager_; cricket::FakeCall fake_call_; - cricket::VoiceChannel* voice_channel_; - cricket::VideoChannel* video_channel_; + std::unique_ptr<cricket::VoiceChannel> voice_channel_; + std::unique_ptr<cricket::VideoChannel> video_channel_; cricket::FakeVoiceMediaChannel* voice_media_channel_; cricket::FakeVideoMediaChannel* video_media_channel_; rtc::scoped_refptr<AudioRtpSender> audio_rtp_sender_;
diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index e2e69f6..d747418 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc
@@ -161,7 +161,7 @@ } void RtpTransceiver::SetChannel( - cricket::ChannelInterface* channel, + std::unique_ptr<cricket::ChannelInterface> channel, std::function<RtpTransportInternal*(const std::string&)> transport_lookup) { RTC_DCHECK_RUN_ON(thread_); RTC_DCHECK(channel); @@ -177,6 +177,8 @@ RTC_DCHECK_EQ(media_type(), channel->media_type()); signaling_thread_safety_ = PendingTaskSafetyFlag::Create(); + std::unique_ptr<cricket::ChannelInterface> 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 @@ // 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<void>(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 @@ signaling_thread_safety_->SetNotAlive(); signaling_thread_safety_ = nullptr; } - cricket::ChannelInterface* channel_to_delete = nullptr; + std::unique_ptr<cricket::ChannelInterface> channel_to_delete; channel_manager_->network_thread()->Invoke<void>(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<cricket::ChannelInterface> 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 @@ // 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 a42cae7..5945d6a 100644 --- a/pc/rtp_transceiver.h +++ b/pc/rtp_transceiver.h
@@ -13,8 +13,8 @@ #include <stddef.h> -#include <algorithm> #include <functional> +#include <memory> #include <string> #include <vector> @@ -100,7 +100,7 @@ // 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 @@ // 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 @@ // 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<cricket::ChannelInterface> channel, std::function<RtpTransportInternal*(const std::string&)> transport_lookup); @@ -285,7 +280,7 @@ // 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<cricket::ChannelInterface> channel_to_delete); // Enforce that this object is created, used and destroyed on one thread. TaskQueueBase* const thread_; @@ -313,7 +308,7 @@ // 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<cricket::ChannelInterface> channel_ = nullptr; cricket::ChannelManager* channel_manager_ = nullptr; std::vector<RtpCodecCapability> codec_preferences_; std::vector<RtpHeaderExtensionCapability> header_extensions_to_offer_;
diff --git a/pc/rtp_transceiver_unittest.cc b/pc/rtp_transceiver_unittest.cc index 29828a0..12b7065 100644 --- a/pc/rtp_transceiver_unittest.cc +++ b/pc/rtp_transceiver_unittest.cc
@@ -13,6 +13,7 @@ #include "pc/rtp_transceiver.h" #include <memory> +#include <utility> #include "absl/strings/string_view.h" #include "absl/types/optional.h" @@ -43,8 +44,6 @@ true, rtc::Thread::Current(), rtc::Thread::Current()) {} - - MOCK_METHOD(void, DestroyChannel, (cricket::ChannelInterface*), (override)); }; } // namespace @@ -54,37 +53,34 @@ const std::string content_name("my_mid"); auto transceiver = rtc::make_ref_counted<RtpTransceiver>( cricket::MediaType::MEDIA_TYPE_AUDIO, &cm); - cricket::MockChannelInterface channel1; - EXPECT_CALL(channel1, media_type()) + auto channel1 = std::make_unique<cricket::MockChannelInterface>(); + 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<cricket::MockChannelInterface>(); + 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 @@ const std::string content_name("my_mid"); auto transceiver = rtc::make_ref_counted<RtpTransceiver>( cricket::MediaType::MEDIA_TYPE_VIDEO, &cm); - cricket::MockChannelInterface channel; - EXPECT_CALL(channel, media_type()) + auto channel = std::make_unique<cricket::MockChannelInterface>(); + 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 @@ 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 @@ 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<cricket::MockChannelInterface>(); + 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 @@ 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<cricket::MockChannelInterface>(); + 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 @@ 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 @@ 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 9014852..5c1aa85 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc
@@ -3613,22 +3613,24 @@ } } else { if (!channel) { + std::unique_ptr<cricket::ChannelInterface> 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 @@ 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<cricket::VoiceChannel> 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 @@ 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<cricket::VideoChannel> 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 @@ } // TODO(steveanton): Perhaps this should be managed by the RtpTransceiver. -cricket::VoiceChannel* SdpOfferAnswerHandler::CreateVoiceChannel( - const std::string& mid) { +std::unique_ptr<cricket::VoiceChannel> +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 @@ } // TODO(steveanton): Perhaps this should be managed by the RtpTransceiver. -cricket::VideoChannel* SdpOfferAnswerHandler::CreateVideoChannel( - const std::string& mid) { +std::unique_ptr<cricket::VideoChannel> +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 ae5b668..4463f05 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h
@@ -528,8 +528,10 @@ 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<cricket::VoiceChannel> CreateVoiceChannel( + const std::string& mid); + std::unique_ptr<cricket::VideoChannel> 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 a263c1e..acf8e28 100644 --- a/pc/test/fake_peer_connection_for_stats.h +++ b/pc/test/fake_peer_connection_for_stats.h
@@ -206,18 +206,17 @@ 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<FakeVoiceMediaChannelForStats>(network_thread_); auto* voice_media_channel_ptr = voice_media_channel.get(); - voice_channel_ = std::make_unique<VoiceChannelForTesting>( + auto voice_channel = std::make_unique<VoiceChannelForTesting>( 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 @@ 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<FakeVideoMediaChannelForStats>(network_thread_); auto video_media_channel_ptr = video_media_channel.get(); - video_channel_ = std::make_unique<VideoChannelForTesting>( + auto video_channel = std::make_unique<VideoChannelForTesting>( 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 @@ 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 @@ FakeDataChannelProvider data_channel_provider_; - std::unique_ptr<cricket::VoiceChannel> voice_channel_; - std::unique_ptr<cricket::VideoChannel> video_channel_; - std::vector<rtc::scoped_refptr<SctpDataChannel>> sctp_data_channels_; std::map<std::string, cricket::TransportStats> transport_stats_by_name_;