From 7a2db8acbeec1a1dcb79873d223c1f6a892ef758 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Mon, 14 Jun 2021 15:41:30 +0000 Subject: [PATCH] Modify Bundle logic to not add & destroy extra transport at add-track MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If a bundle is established, then per JSEP, the offerer is required to include the new track in the bundle, and per BUNDLE, the answerer has to either accept the track as part of the bundle or reject the track; it cannot move it out of the group. So we will never need the transport. Bug: webrtc:12837 Change-Id: I41cae74605fecf454900a958776b95607ccf3745 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/221601 Reviewed-by: Henrik Boström Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#34290} --- pc/jsep_transport_controller.cc | 32 +++++++++++++++++++++--- pc/jsep_transport_controller_unittest.cc | 1 - pc/peer_connection_integrationtest.cc | 14 +++++++++++ pc/rtcp_mux_filter.cc | 3 ++- pc/session_description.cc | 12 +++++++++ pc/session_description.h | 2 ++ pc/test/integration_test_helpers.h | 12 +++++++++ 7 files changed, 70 insertions(+), 6 deletions(-) diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 4d06d494fa..e7eff183a7 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -574,6 +574,18 @@ RTCError JsepTransportController::ApplyDescription_n( established_bundle_groups_by_mid, description); } + // Because the creation of transports depends on whether + // certain mids are present, we have to process rejection + // before we try to create transports. + for (size_t i = 0; i < description->contents().size(); ++i) { + const cricket::ContentInfo& content_info = description->contents()[i]; + if (content_info.rejected) { + // This may cause groups to be removed from |bundles_.bundle_groups()| and + // |established_bundle_groups_by_mid|. + HandleRejectedContent(content_info, established_bundle_groups_by_mid); + } + } + for (const cricket::ContentInfo& content_info : description->contents()) { // Don't create transports for rejected m-lines and bundled m-lines. if (content_info.rejected || @@ -593,10 +605,8 @@ RTCError JsepTransportController::ApplyDescription_n( const cricket::ContentInfo& content_info = description->contents()[i]; const cricket::TransportInfo& transport_info = description->transport_infos()[i]; + if (content_info.rejected) { - // This may cause groups to be removed from |bundles_.bundle_groups()| and - // |established_bundle_groups_by_mid|. - HandleRejectedContent(content_info, established_bundle_groups_by_mid); continue; } @@ -1011,7 +1021,21 @@ RTCError JsepTransportController::MaybeCreateJsepTransport( if (transport) { return RTCError::OK(); } - + // If we have agreed to a bundle, the new mid will be added to the bundle + // according to JSEP, and the responder can't move it out of the group + // according to BUNDLE. So don't create a transport. + // The MID will be added to the bundle elsewhere in the code. + if (bundles_.bundle_groups().size() > 0) { + const auto& default_bundle_group = bundles_.bundle_groups()[0]; + if (default_bundle_group->content_names().size() > 0) { + auto bundle_transport = + GetJsepTransportByName(default_bundle_group->content_names()[0]); + if (bundle_transport) { + transports_.SetTransportForMid(content_info.name, bundle_transport); + return RTCError::OK(); + } + } + } const cricket::MediaContentDescription* content_desc = content_info.media_description(); if (certificate_ && !content_desc->cryptos().empty()) { diff --git a/pc/jsep_transport_controller_unittest.cc b/pc/jsep_transport_controller_unittest.cc index 2b261c83c8..a06f5804e4 100644 --- a/pc/jsep_transport_controller_unittest.cc +++ b/pc/jsep_transport_controller_unittest.cc @@ -2051,7 +2051,6 @@ TEST_F(JsepTransportControllerTest, ChangeTaggedMediaSectionMaxBundle) { EXPECT_TRUE(transport_controller_ ->SetLocalDescription(SdpType::kOffer, local_reoffer.get()) .ok()); - std::unique_ptr remote_reanswer( local_reoffer->Clone()); EXPECT_TRUE( diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index dfceacd777..53b674d851 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -3639,6 +3639,20 @@ TEST_P(PeerConnectionIntegrationInteropTest, ASSERT_TRUE(ExpectNewFrames(media_expectations)); } +TEST_P(PeerConnectionIntegrationTest, NewTracksDoNotCauseNewCandidates) { + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + caller()->AddAudioVideoTracks(); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + ASSERT_TRUE_WAIT(DtlsConnected(), kDefaultTimeout); + caller()->ExpectCandidates(0); + callee()->ExpectCandidates(0); + caller()->AddAudioTrack(); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); +} + INSTANTIATE_TEST_SUITE_P( PeerConnectionIntegrationTest, PeerConnectionIntegrationInteropTest, diff --git a/pc/rtcp_mux_filter.cc b/pc/rtcp_mux_filter.cc index a8cf717b28..62adea2243 100644 --- a/pc/rtcp_mux_filter.cc +++ b/pc/rtcp_mux_filter.cc @@ -91,7 +91,8 @@ bool RtcpMuxFilter::SetAnswer(bool answer_enable, ContentSource src) { } if (!ExpectAnswer(src)) { - RTC_LOG(LS_ERROR) << "Invalid state for RTCP mux answer"; + RTC_LOG(LS_ERROR) << "Invalid state for RTCP mux answer, state is " + << state_ << ", source is " << src; return false; } diff --git a/pc/session_description.cc b/pc/session_description.cc index 35b732d649..7b878cbf7b 100644 --- a/pc/session_description.cc +++ b/pc/session_description.cc @@ -85,6 +85,18 @@ bool ContentGroup::RemoveContentName(const std::string& content_name) { return true; } +std::string ContentGroup::ToString() const { + rtc::StringBuilder acc; + acc << semantics_ << "("; + if (!content_names_.empty()) { + for (const auto& name : content_names_) { + acc << name << " "; + } + } + acc << ")"; + return acc.Release(); +} + SessionDescription::SessionDescription() = default; SessionDescription::SessionDescription(const SessionDescription&) = default; diff --git a/pc/session_description.h b/pc/session_description.h index e05aa3ed82..a20caf624a 100644 --- a/pc/session_description.h +++ b/pc/session_description.h @@ -488,6 +488,8 @@ class ContentGroup { bool HasContentName(const std::string& content_name) const; void AddContentName(const std::string& content_name); bool RemoveContentName(const std::string& content_name); + // for debugging + std::string ToString() const; private: std::string semantics_; diff --git a/pc/test/integration_test_helpers.h b/pc/test/integration_test_helpers.h index 117f1b428b..9ec9b0e982 100644 --- a/pc/test/integration_test_helpers.h +++ b/pc/test/integration_test_helpers.h @@ -17,6 +17,7 @@ #include #include +#include #include #include #include @@ -704,6 +705,11 @@ class PeerConnectionIntegrationWrapper : public webrtc::PeerConnectionObserver, audio_concealed_stat_ = *track_stats->concealed_samples; } + // Sets number of candidates expected + void ExpectCandidates(int candidate_count) { + candidates_expected_ = candidate_count; + } + private: explicit PeerConnectionIntegrationWrapper(const std::string& debug_name) : debug_name_(debug_name) {} @@ -1089,6 +1095,9 @@ class PeerConnectionIntegrationWrapper : public webrtc::PeerConnectionObserver, } } + // Check if we expected to have a candidate. + EXPECT_GT(candidates_expected_, 1); + candidates_expected_--; std::string ice_sdp; EXPECT_TRUE(candidate->ToString(&ice_sdp)); if (signaling_message_receiver_ == nullptr || !signal_ice_candidates_) { @@ -1172,6 +1181,9 @@ class PeerConnectionIntegrationWrapper : public webrtc::PeerConnectionObserver, peer_connection_signaling_state_history_; webrtc::FakeRtcEventLogFactory* event_log_factory_; + // Number of ICE candidates expected. The default is no limit. + int candidates_expected_ = std::numeric_limits::max(); + // Variables for tracking delay stats on an audio track int audio_packets_stat_ = 0; double audio_delay_stat_ = 0.0;