From 6ad507ac35ce638beddd7ac6687d006995637253 Mon Sep 17 00:00:00 2001 From: "pthatcher@webrtc.org" Date: Mon, 16 Mar 2015 20:19:12 +0000 Subject: [PATCH] Refactor how the TransportChannels are set in the BaseChannel to rely lesson Session, so that in the future we can rely on Transport instead, and also be able to change Transports on the fly for BUNDLE. Also, remove channel_name. It's no longer needed. This is a part of the big BUNDLE implementation at https://webrtc-codereview.appspot.com/45519004/ R=decurtis@webrtc.org Review URL: https://webrtc-codereview.appspot.com/43719004 Cr-Commit-Position: refs/heads/master@{#8741} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8741 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/session/media/channel.cc | 153 +++++++++++------- talk/session/media/channel.h | 13 +- talk/session/media/channelmanager_unittest.cc | 2 +- webrtc/p2p/base/fakesession.h | 3 +- webrtc/p2p/base/session.cc | 20 +-- webrtc/p2p/base/session.h | 5 +- webrtc/p2p/base/transportchannelproxy.cc | 2 - webrtc/p2p/base/transportchannelproxy.h | 3 - 8 files changed, 114 insertions(+), 87 deletions(-) diff --git a/talk/session/media/channel.cc b/talk/session/media/channel.cc index 089ed48d13..3c2c64f880 100644 --- a/talk/session/media/channel.cc +++ b/talk/session/media/channel.cc @@ -187,39 +187,28 @@ BaseChannel::~BaseChannel() { // the media channel may try to send on the dead transport channel. NULLing // is not an effective strategy since the sends will come on another thread. delete media_channel_; - set_rtcp_transport_channel(NULL); - if (transport_channel_ != NULL) - session_->DestroyChannel(content_name_, transport_channel_->component()); + set_transport_channel(nullptr); + set_rtcp_transport_channel(nullptr); LOG(LS_INFO) << "Destroyed channel"; } -bool BaseChannel::Init(TransportChannel* transport_channel, - TransportChannel* rtcp_transport_channel) { - if (transport_channel == NULL) { - return false; - } - if (rtcp() && rtcp_transport_channel == NULL) { - return false; - } - transport_channel_ = transport_channel; - - if (!SetDtlsSrtpCiphers(transport_channel_, false)) { +bool BaseChannel::Init() { + if (!SetTransportChannels(session(), rtcp())) { return false; } - transport_channel_->SignalWritableState.connect( - this, &BaseChannel::OnWritableState); - transport_channel_->SignalReadPacket.connect( - this, &BaseChannel::OnChannelRead); - transport_channel_->SignalReadyToSend.connect( - this, &BaseChannel::OnReadyToSend); + if (!SetDtlsSrtpCiphers(transport_channel(), false)) { + return false; + } + if (rtcp() && !SetDtlsSrtpCiphers(rtcp_transport_channel(), true)) { + return false; + } session_->SignalNewLocalDescription.connect( this, &BaseChannel::OnNewLocalDescription); session_->SignalNewRemoteDescription.connect( this, &BaseChannel::OnNewRemoteDescription); - set_rtcp_transport_channel(rtcp_transport_channel); // Both RTP and RTCP channels are set, we can call SetInterface on // media channel and it can set network options. media_channel_->SetInterface(this); @@ -230,6 +219,90 @@ void BaseChannel::Deinit() { media_channel_->SetInterface(NULL); } +bool BaseChannel::SetTransportChannels(BaseSession* session, bool rtcp) { + return worker_thread_->Invoke(Bind( + &BaseChannel::SetTransportChannels_w, this, session, rtcp)); +} + +bool BaseChannel::SetTransportChannels_w(BaseSession* session, bool rtcp) { + ASSERT(worker_thread_ == rtc::Thread::Current()); + + set_transport_channel(session->CreateChannel( + content_name(), cricket::ICE_CANDIDATE_COMPONENT_RTP)); + if (!transport_channel()) { + return false; + } + if (rtcp) { + set_rtcp_transport_channel(session->CreateChannel( + content_name(), cricket::ICE_CANDIDATE_COMPONENT_RTCP)); + if (!rtcp_transport_channel()) { + return false; + } + } else { + set_rtcp_transport_channel(nullptr); + } + + return true; +} + +void BaseChannel::set_transport_channel(TransportChannel* new_tc) { + ASSERT(worker_thread_ == rtc::Thread::Current()); + + TransportChannel* old_tc = transport_channel_; + + if (old_tc == new_tc) { + return; + } + if (old_tc) { + DisconnectFromTransportChannel(old_tc); + session()->DestroyChannel( + content_name(), cricket::ICE_CANDIDATE_COMPONENT_RTP); + } + + transport_channel_ = new_tc; + + if (new_tc) { + ConnectToTransportChannel(new_tc); + } +} + +void BaseChannel::set_rtcp_transport_channel(TransportChannel* new_tc) { + ASSERT(worker_thread_ == rtc::Thread::Current()); + + TransportChannel* old_tc = rtcp_transport_channel_; + + if (old_tc == new_tc) { + return; + } + if (old_tc) { + DisconnectFromTransportChannel(old_tc); + session()->DestroyChannel( + content_name(), cricket::ICE_CANDIDATE_COMPONENT_RTCP); + } + + rtcp_transport_channel_ = new_tc; + + if (new_tc) { + ConnectToTransportChannel(new_tc); + } +} + +void BaseChannel::ConnectToTransportChannel(TransportChannel* tc) { + ASSERT(worker_thread_ == rtc::Thread::Current()); + + tc->SignalWritableState.connect(this, &BaseChannel::OnWritableState); + tc->SignalReadPacket.connect(this, &BaseChannel::OnChannelRead); + tc->SignalReadyToSend.connect(this, &BaseChannel::OnReadyToSend); +} + +void BaseChannel::DisconnectFromTransportChannel(TransportChannel* tc) { + ASSERT(worker_thread_ == rtc::Thread::Current()); + + tc->SignalWritableState.disconnect(this); + tc->SignalReadPacket.disconnect(this); + tc->SignalReadyToSend.disconnect(this); +} + bool BaseChannel::Enable(bool enable) { worker_thread_->Invoke(Bind( enable ? &BaseChannel::EnableMedia_w : &BaseChannel::DisableMedia_w, @@ -300,26 +373,6 @@ bool BaseChannel::GetConnectionStats(ConnectionInfos* infos) { return transport_channel_->GetStats(infos); } -void BaseChannel::set_rtcp_transport_channel(TransportChannel* channel) { - if (rtcp_transport_channel_ != channel) { - if (rtcp_transport_channel_) { - session_->DestroyChannel( - content_name_, rtcp_transport_channel_->component()); - } - rtcp_transport_channel_ = channel; - if (rtcp_transport_channel_) { - // TODO(juberti): Propagate this error code - VERIFY(SetDtlsSrtpCiphers(rtcp_transport_channel_, true)); - rtcp_transport_channel_->SignalWritableState.connect( - this, &BaseChannel::OnWritableState); - rtcp_transport_channel_->SignalReadPacket.connect( - this, &BaseChannel::OnChannelRead); - rtcp_transport_channel_->SignalReadyToSend.connect( - this, &BaseChannel::OnReadyToSend); - } - } -} - bool BaseChannel::IsReadyToReceive() const { // Receive data if we are enabled and have local content, return enabled() && IsReceiveContentDirection(local_content_direction_); @@ -1271,11 +1324,7 @@ VoiceChannel::~VoiceChannel() { } bool VoiceChannel::Init() { - TransportChannel* rtcp_channel = rtcp() ? session()->CreateChannel( - content_name(), "rtcp", ICE_CANDIDATE_COMPONENT_RTCP) : NULL; - if (!BaseChannel::Init(session()->CreateChannel( - content_name(), "rtp", ICE_CANDIDATE_COMPONENT_RTP), - rtcp_channel)) { + if (!BaseChannel::Init()) { return false; } media_channel()->SignalMediaError.connect( @@ -1665,11 +1714,7 @@ VideoChannel::VideoChannel(rtc::Thread* thread, } bool VideoChannel::Init() { - TransportChannel* rtcp_channel = rtcp() ? session()->CreateChannel( - content_name(), "video_rtcp", ICE_CANDIDATE_COMPONENT_RTCP) : NULL; - if (!BaseChannel::Init(session()->CreateChannel( - content_name(), "video_rtp", ICE_CANDIDATE_COMPONENT_RTP), - rtcp_channel)) { + if (!BaseChannel::Init()) { return false; } media_channel()->SignalMediaError.connect( @@ -2118,11 +2163,7 @@ DataChannel::~DataChannel() { } bool DataChannel::Init() { - TransportChannel* rtcp_channel = rtcp() ? session()->CreateChannel( - content_name(), "data_rtcp", ICE_CANDIDATE_COMPONENT_RTCP) : NULL; - if (!BaseChannel::Init(session()->CreateChannel( - content_name(), "data_rtp", ICE_CANDIDATE_COMPONENT_RTP), - rtcp_channel)) { + if (!BaseChannel::Init()) { return false; } media_channel()->SignalDataReceived.connect( diff --git a/talk/session/media/channel.h b/talk/session/media/channel.h index 7e96fe7466..bc4b6f76aa 100644 --- a/talk/session/media/channel.h +++ b/talk/session/media/channel.h @@ -80,8 +80,7 @@ class BaseChannel MediaChannel* channel, BaseSession* session, const std::string& content_name, bool rtcp); virtual ~BaseChannel(); - bool Init(TransportChannel* transport_channel, - TransportChannel* rtcp_transport_channel); + bool Init(); // Deinit may be called multiple times and is simply ignored if it's alreay // done. void Deinit(); @@ -232,6 +231,13 @@ class BaseChannel protected: MediaEngineInterface* media_engine() const { return media_engine_; } virtual MediaChannel* media_channel() const { return media_channel_; } + // Sets the transport_channel_ and rtcp_transport_channel_. If + // |rtcp| is false, set rtcp_transport_channel_ is set to NULL. Get + // the transport channels from |session|. + // TODO(pthatcher): Pass in a Transport instead of a BaseSession. + bool SetTransportChannels(BaseSession* session, bool rtcp); + bool SetTransportChannels_w(BaseSession* session, bool rtcp); + void set_transport_channel(TransportChannel* transport); void set_rtcp_transport_channel(TransportChannel* transport); bool was_ever_writable() const { return was_ever_writable_; } void set_local_content_direction(MediaContentDirection direction) { @@ -246,6 +252,9 @@ class BaseChannel SrtpFilter* srtp_filter() { return &srtp_filter_; } bool rtcp() const { return rtcp_; } + void ConnectToTransportChannel(TransportChannel* tc); + void DisconnectFromTransportChannel(TransportChannel* tc); + void FlushRtcpMessages(); // NetworkInterface implementation, called by MediaEngine diff --git a/talk/session/media/channelmanager_unittest.cc b/talk/session/media/channelmanager_unittest.cc index 8b5b326686..f02800195a 100644 --- a/talk/session/media/channelmanager_unittest.cc +++ b/talk/session/media/channelmanager_unittest.cc @@ -172,7 +172,7 @@ TEST_F(ChannelManagerTest, NoTransportChannelTest) { // The test is useless unless the session does not fail creating // cricket::TransportChannel. ASSERT_TRUE(session_->CreateChannel( - "audio", "rtp", cricket::ICE_CANDIDATE_COMPONENT_RTP) == NULL); + "audio", cricket::ICE_CANDIDATE_COMPONENT_RTP) == NULL); cricket::VoiceChannel* voice_channel = cm_->CreateVoiceChannel( session_, cricket::CN_AUDIO, false); diff --git a/webrtc/p2p/base/fakesession.h b/webrtc/p2p/base/fakesession.h index f942487c94..5486e466f7 100644 --- a/webrtc/p2p/base/fakesession.h +++ b/webrtc/p2p/base/fakesession.h @@ -465,12 +465,11 @@ class FakeSession : public BaseSession { virtual TransportChannel* CreateChannel( const std::string& content_name, - const std::string& channel_name, int component) { if (fail_create_channel_) { return NULL; } - return BaseSession::CreateChannel(content_name, channel_name, component); + return BaseSession::CreateChannel(content_name, component); } void set_fail_channel_creation(bool fail_channel_creation) { diff --git a/webrtc/p2p/base/session.cc b/webrtc/p2p/base/session.cc index 690cf13b0c..9078cb4139 100644 --- a/webrtc/p2p/base/session.cc +++ b/webrtc/p2p/base/session.cc @@ -46,15 +46,14 @@ TransportChannel* TransportProxy::GetChannel(int component) { return GetChannelProxy(component); } -TransportChannel* TransportProxy::CreateChannel(const std::string& name, - int component) { +TransportChannel* TransportProxy::CreateChannel(int component) { ASSERT(rtc::Thread::Current() == worker_thread_); ASSERT(GetChannel(component) == NULL); ASSERT(!transport_->get()->HasChannel(component)); // We always create a proxy in case we need to change out the transport later. TransportChannelProxy* channel_proxy = - new TransportChannelProxy(content_name(), name, component); + new TransportChannelProxy(content_name(), component); channels_[component] = channel_proxy; // If we're already negotiated, create an impl and hook it up to the proxy @@ -142,18 +141,6 @@ TransportChannelProxy* TransportProxy::GetChannelProxy(int component) const { return (iter != channels_.end()) ? iter->second : NULL; } -TransportChannelProxy* TransportProxy::GetChannelProxyByName( - const std::string& name) const { - for (ChannelMap::const_iterator iter = channels_.begin(); - iter != channels_.end(); - ++iter) { - if (iter->second->name() == name) { - return iter->second; - } - } - return NULL; -} - void TransportProxy::CreateChannelImpl(int component) { worker_thread_->Invoke(Bind( &TransportProxy::CreateChannelImpl_w, this, component)); @@ -474,13 +461,12 @@ bool BaseSession::PushdownRemoteTransportDescription( } TransportChannel* BaseSession::CreateChannel(const std::string& content_name, - const std::string& channel_name, int component) { // We create the proxy "on demand" here because we need to support // creating channels at any time, even before we send or receive // initiate messages, which is before we create the transports. TransportProxy* transproxy = GetOrCreateTransportProxy(content_name); - return transproxy->CreateChannel(channel_name, component); + return transproxy->CreateChannel(component); } TransportChannel* BaseSession::GetChannel(const std::string& content_name, diff --git a/webrtc/p2p/base/session.h b/webrtc/p2p/base/session.h index 329ae2a285..8dffca7771 100644 --- a/webrtc/p2p/base/session.h +++ b/webrtc/p2p/base/session.h @@ -81,8 +81,7 @@ class TransportProxy : public sigslot::has_slots<> { } TransportChannel* GetChannel(int component); - TransportChannel* CreateChannel(const std::string& channel_name, - int component); + TransportChannel* CreateChannel(int component); bool HasChannel(int component); void DestroyChannel(int component); @@ -130,7 +129,6 @@ class TransportProxy : public sigslot::has_slots<> { private: TransportChannelProxy* GetChannelProxy(int component) const; - TransportChannelProxy* GetChannelProxyByName(const std::string& name) const; // Creates a new channel on the Transport which causes the reference // count to increment. @@ -306,7 +304,6 @@ class BaseSession : public sigslot::has_slots<>, // shouldn't be an issue since the main thread will be blocked in // Send when doing so. virtual TransportChannel* CreateChannel(const std::string& content_name, - const std::string& channel_name, int component); // Returns the channel with the given names. diff --git a/webrtc/p2p/base/transportchannelproxy.cc b/webrtc/p2p/base/transportchannelproxy.cc index 511892009e..79772514c6 100644 --- a/webrtc/p2p/base/transportchannelproxy.cc +++ b/webrtc/p2p/base/transportchannelproxy.cc @@ -22,10 +22,8 @@ enum { }; TransportChannelProxy::TransportChannelProxy(const std::string& content_name, - const std::string& name, int component) : TransportChannel(content_name, component), - name_(name), impl_(NULL) { worker_thread_ = rtc::Thread::Current(); } diff --git a/webrtc/p2p/base/transportchannelproxy.h b/webrtc/p2p/base/transportchannelproxy.h index ffdad4326c..23cd20b90e 100644 --- a/webrtc/p2p/base/transportchannelproxy.h +++ b/webrtc/p2p/base/transportchannelproxy.h @@ -34,11 +34,9 @@ class TransportChannelProxy : public TransportChannel, public rtc::MessageHandler { public: TransportChannelProxy(const std::string& content_name, - const std::string& name, int component); virtual ~TransportChannelProxy(); - const std::string& name() const { return name_; } TransportChannelImpl* impl() { return impl_; } virtual TransportChannelState GetState() const; @@ -85,7 +83,6 @@ class TransportChannelProxy : public TransportChannel, typedef std::pair OptionPair; typedef std::vector OptionList; - std::string name_; rtc::Thread* worker_thread_; TransportChannelImpl* impl_; OptionList options_;