[Merge M93] - Revert "Modify Bundle logic to not add & destroy extra …

…transport at add-track" This reverts commit 7a2db8a. After this commit, the PeerConnection would assume that any new m= sections will be added to the first existing BUNDLE group. This is true of JSEP endpoints (if they don't do SDP munging), but is not necessarily true for non-JSEP endpoints. This breaks the following scenarios: * Remote offer adding a new m= section that's not part of any BUNDLE group. * Remote offer adding a m= section to the second BUNDLE group. The latter is specifically problematic for any application that wants to bundle all audio streams in one group and all video streams in another group when using Unified Plan SDP, to replicate the behavior of using now-deprecated Plan B without bundling. [email protected] Bug: webrtc:12837, webrtc:12906, chromium:1236202 Change-Id: I97a348c96443dee95e2b42792b73ab7b101fd64c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/227681 Reviewed-by: Taylor Brandstetter <[email protected]> Reviewed-by: Harald Alvestrand <[email protected]> Commit-Queue: Taylor Brandstetter <[email protected]> Cr-Commit-Position: refs/branch-heads/4577@{#2} Cr-Branched-From: 5196931-refs/heads/master@{#34463}
webrtc-sdk · Aug 11, 2021 · bc94cec · bc94cec
1 parent fdd8e9c
commit bc94cec
Show file tree

Hide file tree

Showing 5 changed files with 5 additions and 55 deletions.
diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc
@@ -551,17 +551,6 @@ 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 ||
@@ -580,8 +569,9 @@ 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;
     }
 
@@ -987,21 +977,7 @@ 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,6 +2051,7 @@ 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,20 +3639,6 @@ 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,8 +91,7 @@ bool RtcpMuxFilter::SetAnswer(bool answer_enable, ContentSource src) {
   }
 
   if (!ExpectAnswer(src)) {
-    RTC_LOG(LS_ERROR) << "Invalid state for RTCP mux answer, state is "
-                      << state_ << ", source is " << src;
+    RTC_LOG(LS_ERROR) << "Invalid state for RTCP mux answer";
     return false;
   }
 

diff --git a/pc/test/integration_test_helpers.h b/pc/test/integration_test_helpers.h
@@ -17,7 +17,6 @@
 
 #include <algorithm>
 #include <functional>
-#include <limits>
 #include <list>
 #include <map>
 #include <memory>
@@ -705,11 +704,6 @@ 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) {}
@@ -1095,9 +1089,6 @@ 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_) {
@@ -1181,9 +1172,6 @@ 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;