Modify Bundle logic to not add & destroy extra transport at add-track

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 <[email protected]> Commit-Queue: Harald Alvestrand <[email protected]> Cr-Commit-Position: refs/heads/master@{#34290}
webrtc-sdk · Jun 15, 2021 · 7a2db8a · 7a2db8a
1 parent e4eb8af
commit 7a2db8a
Show file tree

Hide file tree

Showing 7 changed files with 70 additions and 6 deletions.
diff --git 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
@@ -2051,7 +2051,6 @@ TEST_F(JsepTransportControllerTest, ChangeTaggedMediaSectionMaxBundle) {
   EXPECT_TRUE(transport_controller_
                   ->SetLocalDescription(SdpType::kOffer, local_reoffer.get())
                   .ok());
-
   std::unique_ptr<cricket::SessionDescription> remote_reanswer(
       local_reoffer->Clone());
   EXPECT_TRUE(

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -17,6 +17,7 @@
 
 #include <algorithm>
 #include <functional>
+#include <limits>
 #include <list>
 #include <map>
 #include <memory>
@@ -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<int>::max();
+
   // Variables for tracking delay stats on an audio track
   int audio_packets_stat_ = 0;
   double audio_delay_stat_ = 0.0;