diff --git a/webrtc/pc/channel.cc b/webrtc/pc/channel.cc index e464124bd6..1312d63b37 100644 --- a/webrtc/pc/channel.cc +++ b/webrtc/pc/channel.cc @@ -169,22 +169,8 @@ BaseChannel::BaseChannel(rtc::Thread* worker_thread, content_name_(content_name), transport_controller_(transport_controller), - rtcp_transport_enabled_(rtcp), - transport_channel_(nullptr), - rtcp_transport_channel_(nullptr), - rtp_ready_to_send_(false), - rtcp_ready_to_send_(false), - writable_(false), - was_ever_writable_(false), - has_received_packet_(false), - dtls_keyed_(false), - secure_required_(false), - rtp_abs_sendtime_extn_id_(-1), - - media_channel_(media_channel), - enabled_(false), - local_content_direction_(MD_INACTIVE), - remote_content_direction_(MD_INACTIVE) { + rtcp_enabled_(rtcp), + media_channel_(media_channel) { ASSERT(worker_thread_ == rtc::Thread::Current()); if (transport_controller) { RTC_DCHECK_EQ(network_thread, transport_controller->network_thread()); @@ -268,7 +254,7 @@ bool BaseChannel::InitNetwork_n(const std::string* bundle_transport_name) { if (!SetDtlsSrtpCryptoSuites_n(transport_channel_, false)) { return false; } - if (rtcp_transport_enabled() && + if (rtcp_transport_channel_ && !SetDtlsSrtpCryptoSuites_n(rtcp_transport_channel_, true)) { return false; } @@ -308,10 +294,12 @@ bool BaseChannel::SetTransport_n(const std::string& transport_name) { srtp_filter_.ResetParams(); } - // TODO(guoweis): Remove this grossness when we remove non-muxed RTCP. - if (rtcp_transport_enabled()) { + // If this BaseChannel uses RTCP and we haven't fully negotiated RTCP mux, + // we need an RTCP channel. + if (rtcp_enabled_ && !rtcp_mux_filter_.IsFullyActive()) { LOG(LS_INFO) << "Create RTCP TransportChannel for " << content_name() << " on " << transport_name << " transport "; + // TODO(deadbeef): Remove this grossness when we remove non-muxed RTCP. SetRtcpTransportChannel_n( transport_controller_->CreateTransportChannel_n( transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP), @@ -328,12 +316,11 @@ bool BaseChannel::SetTransport_n(const std::string& transport_name) { return false; } - // TODO(guoweis): Remove this grossness when we remove non-muxed RTCP. - if (rtcp_transport_enabled()) { + // TODO(deadbeef): Remove this grossness when we remove non-muxed RTCP. + if (rtcp_transport_channel_) { // We can only update the RTCP ready to send after set_transport_channel has // handled channel writability. - SetReadyToSend( - true, rtcp_transport_channel_ && rtcp_transport_channel_->writable()); + SetReadyToSend(true, rtcp_transport_channel_->writable()); } transport_name_ = transport_name; return true; @@ -1208,7 +1195,6 @@ void BaseChannel::ActivateRtcpMux_n() { if (!rtcp_mux_filter_.IsActive()) { rtcp_mux_filter_.SetActive(); SetRtcpTransportChannel_n(nullptr, true); - rtcp_transport_enabled_ = false; } } @@ -1232,7 +1218,6 @@ bool BaseChannel::SetRtcpMux_n(bool enable, << " by destroying RTCP transport channel for " << transport_name(); SetRtcpTransportChannel_n(nullptr, true); - rtcp_transport_enabled_ = false; } break; case CA_UPDATE: diff --git a/webrtc/pc/channel.h b/webrtc/pc/channel.h index 3c00ee3d4a..eea1d7f0da 100644 --- a/webrtc/pc/channel.h +++ b/webrtc/pc/channel.h @@ -70,6 +70,7 @@ class BaseChannel public MediaChannel::NetworkInterface, public ConnectionStatsGetter { public: + // |rtcp| represents whether or not this channel uses RTCP. BaseChannel(rtc::Thread* worker_thread, rtc::Thread* network_thread, MediaChannel* channel, @@ -197,7 +198,6 @@ class BaseChannel rtc::Thread* signaling_thread() { return transport_controller_->signaling_thread(); } - bool rtcp_transport_enabled() const { return rtcp_transport_enabled_; } void ConnectToTransportChannel(TransportChannel* tc); void DisconnectFromTransportChannel(TransportChannel* tc); @@ -342,34 +342,37 @@ class BaseChannel // Transport related members that should be accessed from network thread. TransportController* const transport_controller_; std::string transport_name_; - bool rtcp_transport_enabled_; - TransportChannel* transport_channel_; + // Is RTCP used at all by this type of channel? + // Expected to be true (as of typing this) for everything except data + // channels. + const bool rtcp_enabled_; + TransportChannel* transport_channel_ = nullptr; std::vector > socket_options_; - TransportChannel* rtcp_transport_channel_; + TransportChannel* rtcp_transport_channel_ = nullptr; std::vector > rtcp_socket_options_; SrtpFilter srtp_filter_; RtcpMuxFilter rtcp_mux_filter_; BundleFilter bundle_filter_; - bool rtp_ready_to_send_; - bool rtcp_ready_to_send_; - bool writable_; - bool was_ever_writable_; - bool has_received_packet_; - bool dtls_keyed_; - bool secure_required_; + bool rtp_ready_to_send_ = false; + bool rtcp_ready_to_send_ = false; + bool writable_ = false; + bool was_ever_writable_ = false; + bool has_received_packet_ = false; + bool dtls_keyed_ = false; + bool secure_required_ = false; rtc::CryptoOptions crypto_options_; - int rtp_abs_sendtime_extn_id_; + int rtp_abs_sendtime_extn_id_ = -1; // MediaChannel related members that should be access from worker thread. MediaChannel* const media_channel_; // Currently enabled_ flag accessed from signaling thread too, but it can // be changed only when signaling thread does sunchronious call to worker // thread, so it should be safe. - bool enabled_; + bool enabled_ = false; std::vector local_streams_; std::vector remote_streams_; - MediaContentDirection local_content_direction_; - MediaContentDirection remote_content_direction_; + MediaContentDirection local_content_direction_ = MD_INACTIVE; + MediaContentDirection remote_content_direction_ = MD_INACTIVE; }; // VoiceChannel is a specialization that adds support for early media, DTMF, diff --git a/webrtc/pc/rtcpmuxfilter.cc b/webrtc/pc/rtcpmuxfilter.cc index 86d4b6cd07..715e1e7798 100644 --- a/webrtc/pc/rtcpmuxfilter.cc +++ b/webrtc/pc/rtcpmuxfilter.cc @@ -17,10 +17,16 @@ namespace cricket { RtcpMuxFilter::RtcpMuxFilter() : state_(ST_INIT), offer_enable_(false) { } +bool RtcpMuxFilter::IsFullyActive() const { + return state_ == ST_ACTIVE; +} + +bool RtcpMuxFilter::IsProvisionallyActive() const { + return state_ == ST_SENTPRANSWER || state_ == ST_RECEIVEDPRANSWER; +} + bool RtcpMuxFilter::IsActive() const { - return state_ == ST_SENTPRANSWER || - state_ == ST_RECEIVEDPRANSWER || - state_ == ST_ACTIVE; + return IsFullyActive() || IsProvisionallyActive(); } void RtcpMuxFilter::SetActive() { diff --git a/webrtc/pc/rtcpmuxfilter.h b/webrtc/pc/rtcpmuxfilter.h index 94dc41d980..91e790d684 100644 --- a/webrtc/pc/rtcpmuxfilter.h +++ b/webrtc/pc/rtcpmuxfilter.h @@ -21,10 +21,20 @@ class RtcpMuxFilter { public: RtcpMuxFilter(); - // Whether the filter is active, i.e. has RTCP mux been properly negotiated. + // Whether RTCP mux has been negotiated with a final answer (not provisional). + bool IsFullyActive() const; + + // Whether RTCP mux has been negotiated with a provisional answer; this means + // a later answer could disable RTCP mux, and so the RTCP transport should + // not be disposed yet. + bool IsProvisionallyActive() const; + + // Whether the filter is active, i.e. has RTCP mux been properly negotiated, + // either with a final or provisional answer. bool IsActive() const; - // Make the filter active, regardless of the current state. + // Make the filter active (fully, not provisionally) regardless of the + // current state. This should be used when an endpoint *requires* RTCP mux. void SetActive(); // Specifies whether the offer indicates the use of RTCP mux. diff --git a/webrtc/pc/rtcpmuxfilter_unittest.cc b/webrtc/pc/rtcpmuxfilter_unittest.cc index e158897db8..b0ba88a983 100644 --- a/webrtc/pc/rtcpmuxfilter_unittest.cc +++ b/webrtc/pc/rtcpmuxfilter_unittest.cc @@ -82,12 +82,18 @@ TEST(RtcpMuxFilterTest, IsActiveSender) { cricket::RtcpMuxFilter filter; // Init state - not active EXPECT_FALSE(filter.IsActive()); + EXPECT_FALSE(filter.IsProvisionallyActive()); + EXPECT_FALSE(filter.IsFullyActive()); // After sent offer, demux should not be active. filter.SetOffer(true, cricket::CS_LOCAL); EXPECT_FALSE(filter.IsActive()); + EXPECT_FALSE(filter.IsProvisionallyActive()); + EXPECT_FALSE(filter.IsFullyActive()); // Remote accepted, filter is now active. filter.SetAnswer(true, cricket::CS_REMOTE); EXPECT_TRUE(filter.IsActive()); + EXPECT_FALSE(filter.IsProvisionallyActive()); + EXPECT_TRUE(filter.IsFullyActive()); } // Test that we can receive provisional answer and final answer. @@ -96,27 +102,39 @@ TEST(RtcpMuxFilterTest, ReceivePrAnswer) { filter.SetOffer(true, cricket::CS_LOCAL); // Received provisional answer with mux enabled. EXPECT_TRUE(filter.SetProvisionalAnswer(true, cricket::CS_REMOTE)); - // We are now active since both sender and receiver support mux. + // We are now provisionally active since both sender and receiver support mux. EXPECT_TRUE(filter.IsActive()); + EXPECT_TRUE(filter.IsProvisionallyActive()); + EXPECT_FALSE(filter.IsFullyActive()); // Received provisional answer with mux disabled. EXPECT_TRUE(filter.SetProvisionalAnswer(false, cricket::CS_REMOTE)); // We are now inactive since the receiver doesn't support mux. EXPECT_FALSE(filter.IsActive()); + EXPECT_FALSE(filter.IsProvisionallyActive()); + EXPECT_FALSE(filter.IsFullyActive()); // Received final answer with mux enabled. EXPECT_TRUE(filter.SetAnswer(true, cricket::CS_REMOTE)); EXPECT_TRUE(filter.IsActive()); + EXPECT_FALSE(filter.IsProvisionallyActive()); + EXPECT_TRUE(filter.IsFullyActive()); } TEST(RtcpMuxFilterTest, IsActiveReceiver) { cricket::RtcpMuxFilter filter; // Init state - not active. EXPECT_FALSE(filter.IsActive()); + EXPECT_FALSE(filter.IsProvisionallyActive()); + EXPECT_FALSE(filter.IsFullyActive()); // After received offer, demux should not be active filter.SetOffer(true, cricket::CS_REMOTE); EXPECT_FALSE(filter.IsActive()); + EXPECT_FALSE(filter.IsProvisionallyActive()); + EXPECT_FALSE(filter.IsFullyActive()); // We accept, filter is now active filter.SetAnswer(true, cricket::CS_LOCAL); EXPECT_TRUE(filter.IsActive()); + EXPECT_FALSE(filter.IsProvisionallyActive()); + EXPECT_TRUE(filter.IsFullyActive()); } // Test that we can send provisional answer and final answer. @@ -126,12 +144,18 @@ TEST(RtcpMuxFilterTest, SendPrAnswer) { // Send provisional answer with mux enabled. EXPECT_TRUE(filter.SetProvisionalAnswer(true, cricket::CS_LOCAL)); EXPECT_TRUE(filter.IsActive()); + EXPECT_TRUE(filter.IsProvisionallyActive()); + EXPECT_FALSE(filter.IsFullyActive()); // Received provisional answer with mux disabled. EXPECT_TRUE(filter.SetProvisionalAnswer(false, cricket::CS_LOCAL)); EXPECT_FALSE(filter.IsActive()); + EXPECT_FALSE(filter.IsProvisionallyActive()); + EXPECT_FALSE(filter.IsFullyActive()); // Send final answer with mux enabled. EXPECT_TRUE(filter.SetAnswer(true, cricket::CS_LOCAL)); EXPECT_TRUE(filter.IsActive()); + EXPECT_FALSE(filter.IsProvisionallyActive()); + EXPECT_TRUE(filter.IsFullyActive()); } // Test that we can enable the filter in an update.