From 1c7ecefbe7ed08c583d3c403a6881d18a0d0f1ca Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Wed, 11 Aug 2021 12:38:35 -0700 Subject: [PATCH] Reland "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 This relands commit I41cae74605fecf454900a958776b95607ccf3745, after reverting it in order to merge the revert to M93 (the deadline for which is now exceeded). Original change description: > 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} Bug: webrtc:12837 Change-Id: I30a8f03165ab797ed766b51c4eb15c2a9cecb5ed Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228500 Reviewed-by: Harald Alvestrand Commit-Queue: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#34727} --- pc/jsep_transport_controller.cc | 30 +++++++++++++++++++++--- pc/jsep_transport_controller_unittest.cc | 1 - pc/peer_connection_integrationtest.cc | 14 +++++++++++ pc/rtcp_mux_filter.cc | 3 ++- pc/test/integration_test_helpers.h | 12 ++++++++++ 5 files changed, 55 insertions(+), 5 deletions(-) diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index e977f1b938..2da3f0e0a1 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -551,6 +551,17 @@ RTCError JsepTransportController::ApplyDescription_n( MergeEncryptedHeaderExtensionIdsForBundles(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()`. + HandleRejectedContent(content_info); + } + } + for (const cricket::ContentInfo& content_info : description->contents()) { // Don't create transports for rejected m-lines and bundled m-lines. if (content_info.rejected || @@ -569,9 +580,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()`. - HandleRejectedContent(content_info); continue; } @@ -977,7 +987,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 cfe13d04ff..6fe1ee3bf3 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 31652ac4b4..fc094161af 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/test/integration_test_helpers.h b/pc/test/integration_test_helpers.h index b0e0b34864..c7c17b7fe9 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;