From 8591eff5209dd05db757ca092b03d9c1aeca788a Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Wed, 11 Aug 2021 14:56:38 -0700 Subject: [PATCH] Reland "Fix bug where we assume new m= sections will always be bundled." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a reland of commit 704a834f685eb96c9fcf891ca345557bef4d138a, after it was reverted in order to merge a CL to M93. Original change's description: > Fix bug where we assume new m= sections will always be bundled. > > A recent change [1] assumes that all new m= sections will share the > first BUNDLE group (if one already exists), which avoids generating > ICE candidates that are ultimately unnecessary. This is fine for JSEP > endpoints, but it breaks the following scenarios for non-JSEP endpoints: > > * Remote offer adding a new m= section that's not part of any BUNDLE > group. > * Remote offer adding an 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 Plan B without bundling. It may try to add a video stream only > for WebRTC to bundle it with audio. > > This is fixed by doing some minor re-factoring, having BundleManager > update the bundle groups at offer time. > > Also: > * Added some additional validation for multiple bundle groups in a > subsequent offer, since that now becomes relevant. > * Improved rollback support, because now rolling back an offer may need > to not only remove mid->transport mappings but alter them. > > [1]: https://webrtc-review.googlesource.com/c/src/+/221601 > > Bug: webrtc:12906, webrtc:12999 > Change-Id: I4c6e7020c0be33a782d3608dee88e4e2fceb1be1 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/225642 > Reviewed-by: Harald Alvestrand > Reviewed-by: Henrik Boström > Commit-Queue: Taylor Brandstetter > Cr-Commit-Position: refs/heads/master@{#34544} Bug: webrtc:12906, webrtc:12999 Change-Id: Id6acab2e2d7430c65f4b6a1d7372388a70cc18ab Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228465 Commit-Queue: Taylor Brandstetter Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#34728} --- pc/jsep_transport_collection.cc | 177 +++++-- pc/jsep_transport_collection.h | 47 +- pc/jsep_transport_controller.cc | 127 +++-- pc/jsep_transport_controller.h | 15 +- pc/jsep_transport_controller_unittest.cc | 627 +++++++++++++++++++++++ pc/peer_connection_bundle_unittest.cc | 59 ++- pc/sdp_offer_answer.cc | 5 +- pc/test/mock_peer_connection_observers.h | 2 - 8 files changed, 952 insertions(+), 107 deletions(-) diff --git a/pc/jsep_transport_collection.cc b/pc/jsep_transport_collection.cc index 02367fdc0e..df44a9c157 100644 --- a/pc/jsep_transport_collection.cc +++ b/pc/jsep_transport_collection.cc @@ -12,6 +12,7 @@ #include #include +#include #include #include @@ -20,21 +21,53 @@ namespace webrtc { -void BundleManager::Update(const cricket::SessionDescription* description) { +void BundleManager::Update(const cricket::SessionDescription* description, + SdpType type) { RTC_DCHECK_RUN_ON(&sequence_checker_); - bundle_groups_.clear(); - for (const cricket::ContentGroup* new_bundle_group : - description->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE)) { - bundle_groups_.push_back( - std::make_unique(*new_bundle_group)); - RTC_DLOG(LS_VERBOSE) << "Establishing bundle group " - << new_bundle_group->ToString(); - } - established_bundle_groups_by_mid_.clear(); - for (const auto& bundle_group : bundle_groups_) { - for (const std::string& content_name : bundle_group->content_names()) { - established_bundle_groups_by_mid_[content_name] = bundle_group.get(); + // Rollbacks should call Rollback, not Update. + RTC_DCHECK(type != SdpType::kRollback); + bool bundle_groups_changed = false; + // TODO(bugs.webrtc.org/3349): Do this for kPrAnswer as well. To make this + // work, we also need to make sure PRANSWERs don't call + // MaybeDestroyJsepTransport, because the final answer may need the destroyed + // transport if it changes the BUNDLE group. + if (bundle_policy_ == PeerConnectionInterface::kBundlePolicyMaxBundle || + type == SdpType::kAnswer) { + // If our policy is "max-bundle" or this is an answer, update all bundle + // groups. + bundle_groups_changed = true; + bundle_groups_.clear(); + for (const cricket::ContentGroup* new_bundle_group : + description->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE)) { + bundle_groups_.push_back( + std::make_unique(*new_bundle_group)); + RTC_DLOG(LS_VERBOSE) << "Establishing bundle group " + << new_bundle_group->ToString(); } + } else if (type == SdpType::kOffer) { + // If this is an offer, update existing bundle groups. + // We do this because as per RFC 8843, section 7.3.2, the answerer cannot + // remove an m= section from an existing BUNDLE group without rejecting it. + // Thus any m= sections added to a BUNDLE group in this offer can + // preemptively start using the bundled transport, as there is no possible + // non-bundled fallback. + for (const cricket::ContentGroup* new_bundle_group : + description->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE)) { + // Attempt to find a matching existing group. + for (const std::string& mid : new_bundle_group->content_names()) { + auto it = established_bundle_groups_by_mid_.find(mid); + if (it != established_bundle_groups_by_mid_.end()) { + *it->second = *new_bundle_group; + bundle_groups_changed = true; + RTC_DLOG(LS_VERBOSE) + << "Establishing bundle group " << new_bundle_group->ToString(); + break; + } + } + } + } + if (bundle_groups_changed) { + RefreshEstablishedBundleGroupsByMid(); } } @@ -92,6 +125,34 @@ void BundleManager::DeleteGroup(const cricket::ContentGroup* bundle_group) { bundle_groups_.erase(bundle_group_it); } +void BundleManager::Rollback() { + RTC_DCHECK_RUN_ON(&sequence_checker_); + bundle_groups_.clear(); + for (const auto& bundle_group : stable_bundle_groups_) { + bundle_groups_.push_back( + std::make_unique(*bundle_group)); + } + RefreshEstablishedBundleGroupsByMid(); +} + +void BundleManager::Commit() { + RTC_DCHECK_RUN_ON(&sequence_checker_); + stable_bundle_groups_.clear(); + for (const auto& bundle_group : bundle_groups_) { + stable_bundle_groups_.push_back( + std::make_unique(*bundle_group)); + } +} + +void BundleManager::RefreshEstablishedBundleGroupsByMid() { + established_bundle_groups_by_mid_.clear(); + for (const auto& bundle_group : bundle_groups_) { + for (const std::string& content_name : bundle_group->content_names()) { + established_bundle_groups_by_mid_[content_name] = bundle_group.get(); + } + } +} + void JsepTransportCollection::RegisterTransport( const std::string& mid, std::unique_ptr transport) { @@ -110,6 +171,17 @@ std::vector JsepTransportCollection::Transports() { return result; } +std::vector +JsepTransportCollection::ActiveTransports() { + RTC_DCHECK_RUN_ON(&sequence_checker_); + std::set transports; + for (const auto& kv : mid_to_transport_) { + transports.insert(kv.second); + } + return std::vector(transports.begin(), + transports.end()); +} + void JsepTransportCollection::DestroyAllTransports() { RTC_DCHECK_RUN_ON(&sequence_checker_); for (const auto& jsep_transport : jsep_transports_by_name_) { @@ -157,8 +229,6 @@ bool JsepTransportCollection::SetTransportForMid( if (it != mid_to_transport_.end() && it->second == jsep_transport) return true; - pending_mids_.push_back(mid); - // The map_change_callback must be called before destroying the // transport, because it removes references to the transport // in the RTP demuxer. @@ -191,17 +261,36 @@ void JsepTransportCollection::RemoveTransportForMid(const std::string& mid) { RTC_DCHECK(IsConsistent()); } -void JsepTransportCollection::RollbackTransports() { +bool JsepTransportCollection::RollbackTransports() { RTC_DCHECK_RUN_ON(&sequence_checker_); - for (auto&& mid : pending_mids_) { - RemoveTransportForMid(mid); + bool ret = true; + // First, remove any new mid->transport mappings. + for (const auto& kv : mid_to_transport_) { + if (stable_mid_to_transport_.count(kv.first) == 0) { + ret = ret && map_change_callback_(kv.first, nullptr); + } } - pending_mids_.clear(); + // Next, restore old mappings. + for (const auto& kv : stable_mid_to_transport_) { + auto it = mid_to_transport_.find(kv.first); + if (it == mid_to_transport_.end() || it->second != kv.second) { + ret = ret && map_change_callback_(kv.first, kv.second); + } + } + mid_to_transport_ = stable_mid_to_transport_; + // Moving a transport back to mid_to_transport_ means it's now included in + // the aggregate state if it wasn't previously. + state_change_callback_(); + DestroyUnusedTransports(); + RTC_DCHECK(IsConsistent()); + return ret; } void JsepTransportCollection::CommitTransports() { RTC_DCHECK_RUN_ON(&sequence_checker_); - pending_mids_.clear(); + stable_mid_to_transport_ = mid_to_transport_; + DestroyUnusedTransports(); + RTC_DCHECK(IsConsistent()); } bool JsepTransportCollection::TransportInUse( @@ -215,14 +304,32 @@ bool JsepTransportCollection::TransportInUse( return false; } +bool JsepTransportCollection::TransportNeededForRollback( + cricket::JsepTransport* jsep_transport) const { + RTC_DCHECK_RUN_ON(&sequence_checker_); + for (const auto& kv : stable_mid_to_transport_) { + if (kv.second == jsep_transport) { + return true; + } + } + return false; +} + void JsepTransportCollection::MaybeDestroyJsepTransport( cricket::JsepTransport* transport) { RTC_DCHECK_RUN_ON(&sequence_checker_); // Don't destroy the JsepTransport if there are still media sections referring - // to it. + // to it, or if it will be needed in case of rollback. if (TransportInUse(transport)) { return; } + // If this transport is needed for rollback, don't destroy it yet, but make + // sure the aggregate state is updated since this transport is no longer + // included in it. + if (TransportNeededForRollback(transport)) { + state_change_callback_(); + return; + } for (const auto& it : jsep_transports_by_name_) { if (it.second.get() == transport) { jsep_transports_by_name_.erase(it.first); @@ -233,21 +340,33 @@ void JsepTransportCollection::MaybeDestroyJsepTransport( RTC_DCHECK(IsConsistent()); } +void JsepTransportCollection::DestroyUnusedTransports() { + RTC_DCHECK_RUN_ON(&sequence_checker_); + bool need_state_change_callback = false; + auto it = jsep_transports_by_name_.begin(); + while (it != jsep_transports_by_name_.end()) { + if (TransportInUse(it->second.get()) || + TransportNeededForRollback(it->second.get())) { + ++it; + } else { + it = jsep_transports_by_name_.erase(it); + need_state_change_callback = true; + } + } + if (need_state_change_callback) { + state_change_callback_(); + } +} + bool JsepTransportCollection::IsConsistent() { RTC_DCHECK_RUN_ON(&sequence_checker_); for (const auto& it : jsep_transports_by_name_) { - if (!TransportInUse(it.second.get())) { + if (!TransportInUse(it.second.get()) && + !TransportNeededForRollback(it.second.get())) { RTC_LOG(LS_ERROR) << "Transport registered with mid " << it.first << " is not in use, transport " << it.second.get(); return false; } - const auto& lookup = mid_to_transport_.find(it.first); - if (lookup->second != it.second.get()) { - // Not an error, but unusual. - RTC_DLOG(LS_INFO) << "Note: Mid " << it.first << " was registered to " - << it.second.get() << " but currently maps to " - << lookup->second; - } } return true; } diff --git a/pc/jsep_transport_collection.h b/pc/jsep_transport_collection.h index 0dd528d348..aa5293475e 100644 --- a/pc/jsep_transport_collection.h +++ b/pc/jsep_transport_collection.h @@ -18,6 +18,7 @@ #include #include +#include "api/peer_connection_interface.h" #include "api/sequence_checker.h" #include "pc/jsep_transport.h" #include "pc/session_description.h" @@ -41,7 +42,8 @@ namespace webrtc { // 6) Change the logic to do what's right. class BundleManager { public: - BundleManager() { + explicit BundleManager(PeerConnectionInterface::BundlePolicy bundle_policy) + : bundle_policy_(bundle_policy) { // Allow constructor to be called on a different thread. sequence_checker_.Detach(); } @@ -58,17 +60,27 @@ class BundleManager { bool IsFirstMidInGroup(const std::string& mid) const; // Update the groups description. This completely replaces the group // description with the one from the SessionDescription. - void Update(const cricket::SessionDescription* description); + void Update(const cricket::SessionDescription* description, SdpType type); // Delete a MID from the group that contains it. void DeleteMid(const cricket::ContentGroup* bundle_group, const std::string& mid); // Delete a group. void DeleteGroup(const cricket::ContentGroup* bundle_group); + // Roll back to previous stable state. + void Rollback(); + // Commit current bundle groups. + void Commit(); private: + // Recalculate established_bundle_groups_by_mid_ from bundle_groups_. + void RefreshEstablishedBundleGroupsByMid() RTC_RUN_ON(sequence_checker_); + RTC_NO_UNIQUE_ADDRESS SequenceChecker sequence_checker_; + PeerConnectionInterface::BundlePolicy bundle_policy_; std::vector> bundle_groups_ RTC_GUARDED_BY(sequence_checker_); + std::vector> stable_bundle_groups_ + RTC_GUARDED_BY(sequence_checker_); std::map established_bundle_groups_by_mid_; }; @@ -91,7 +103,11 @@ class JsepTransportCollection { void RegisterTransport(const std::string& mid, std::unique_ptr transport); + // Returns all transports, including those not currently mapped to any MID + // because they're being kept alive in case of rollback. std::vector Transports(); + // Only returns transports currently mapped to a MID. + std::vector ActiveTransports(); void DestroyAllTransports(); // Lookup a JsepTransport by the MID that was used to register it. cricket::JsepTransport* GetTransportByName(const std::string& mid); @@ -108,17 +124,28 @@ class JsepTransportCollection { // Remove a transport for a MID. This may destroy a transport if it is // no longer in use. void RemoveTransportForMid(const std::string& mid); - // Roll back pending mid-to-transport mappings. - void RollbackTransports(); - // Commit pending mid-transport mappings (rollback is no longer possible). + // Roll back to previous stable mid-to-transport mappings. + bool RollbackTransports(); + // Commit pending mid-transport mappings (rollback is no longer possible), + // and destroy unused transports because we know now we'll never need them + // again. void CommitTransports(); + + private: // Returns true if any mid currently maps to this transport. bool TransportInUse(cricket::JsepTransport* jsep_transport) const; - private: - // Destroy a transport if it's no longer in use. + // Returns true if any mid in the last stable mapping maps to this transport, + // meaning it should be kept alive in case of rollback. + bool TransportNeededForRollback(cricket::JsepTransport* jsep_transport) const; + + // Destroy a transport if it's no longer in use. This includes whether it + // will be needed in case of rollback. void MaybeDestroyJsepTransport(cricket::JsepTransport* transport); + // Destroys all transports that are no longer in use. + void DestroyUnusedTransports(); + bool IsConsistent(); // For testing only: Verify internal structure. RTC_NO_UNIQUE_ADDRESS SequenceChecker sequence_checker_; @@ -130,8 +157,10 @@ class JsepTransportCollection { // (BaseChannel/SctpTransport) and the JsepTransport underneath. std::map mid_to_transport_ RTC_GUARDED_BY(sequence_checker_); - // Keep track of mids that have been mapped to transports. Used for rollback. - std::vector pending_mids_ RTC_GUARDED_BY(sequence_checker_); + // A snapshot of mid_to_transport_ at the last stable state. Used for + // rollback. + std::map stable_mid_to_transport_ + RTC_GUARDED_BY(sequence_checker_); // Callback used to inform subscribers of altered transports. const std::function diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 2da3f0e0a1..bf24990951 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -55,7 +55,8 @@ JsepTransportController::JsepTransportController( UpdateAggregateStates_n(); }), config_(config), - active_reset_srtp_params_(config.active_reset_srtp_params) { + active_reset_srtp_params_(config.active_reset_srtp_params), + bundles_(config.bundle_policy) { // The `transport_observer` is assumed to be non-null. RTC_DCHECK(config_.transport_observer); RTC_DCHECK(config_.rtcp_handler); @@ -374,13 +375,18 @@ void JsepTransportController::SetActiveResetSrtpParams( } } -void JsepTransportController::RollbackTransports() { +RTCError JsepTransportController::RollbackTransports() { if (!network_thread_->IsCurrent()) { - network_thread_->Invoke(RTC_FROM_HERE, [=] { RollbackTransports(); }); - return; + return network_thread_->Invoke( + RTC_FROM_HERE, [=] { return RollbackTransports(); }); } RTC_DCHECK_RUN_ON(network_thread_); - transports_.RollbackTransports(); + bundles_.Rollback(); + if (!transports_.RollbackTransports()) { + LOG_AND_RETURN_ERROR(RTCErrorType::INTERNAL_ERROR, + "Failed to roll back transport state."); + } + return RTCError::OK(); } rtc::scoped_refptr @@ -525,6 +531,23 @@ JsepTransportController::GetDtlsTransports() { return dtls_transports; } +std::vector +JsepTransportController::GetActiveDtlsTransports() { + RTC_DCHECK_RUN_ON(network_thread_); + std::vector dtls_transports; + for (auto jsep_transport : transports_.ActiveTransports()) { + RTC_DCHECK(jsep_transport); + if (jsep_transport->rtp_dtls_transport()) { + dtls_transports.push_back(jsep_transport->rtp_dtls_transport()); + } + + if (jsep_transport->rtcp_dtls_transport()) { + dtls_transports.push_back(jsep_transport->rtcp_dtls_transport()); + } + } + return dtls_transports; +} + RTCError JsepTransportController::ApplyDescription_n( bool local, SdpType type, @@ -551,17 +574,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 || @@ -582,6 +594,8 @@ RTCError JsepTransportController::ApplyDescription_n( description->transport_infos()[i]; if (content_info.rejected) { + // This may cause groups to be removed from |bundles_.bundle_groups()|. + HandleRejectedContent(content_info); continue; } @@ -647,6 +661,7 @@ RTCError JsepTransportController::ApplyDescription_n( } if (type == SdpType::kAnswer) { transports_.CommitTransports(); + bundles_.Commit(); } return RTCError::OK(); } @@ -682,7 +697,44 @@ RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroups( } } - if (type == SdpType::kAnswer) { + if (type == SdpType::kOffer) { + // For an offer, we need to verify that there is not a conflicting mapping + // between existing and new bundle groups. For example, if the existing + // groups are [[1,2],[3,4]] and new are [[1,3],[2,4]] or [[1,2,3,4]], or + // vice versa. Switching things around like this requires a separate offer + // that removes the relevant sections from their group, as per RFC 8843, + // section 7.5.2. + std::map + new_bundle_groups_by_existing_bundle_groups; + std::map + existing_bundle_groups_by_new_bundle_groups; + for (const cricket::ContentGroup* new_bundle_group : new_bundle_groups) { + for (const std::string& mid : new_bundle_group->content_names()) { + cricket::ContentGroup* existing_bundle_group = + bundles_.LookupGroupByMid(mid); + if (!existing_bundle_group) { + continue; + } + auto it = new_bundle_groups_by_existing_bundle_groups.find( + existing_bundle_group); + if (it != new_bundle_groups_by_existing_bundle_groups.end() && + it->second != new_bundle_group) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "MID " + mid + " in the offer has changed group."); + } + new_bundle_groups_by_existing_bundle_groups.insert( + std::make_pair(existing_bundle_group, new_bundle_group)); + it = existing_bundle_groups_by_new_bundle_groups.find(new_bundle_group); + if (it != existing_bundle_groups_by_new_bundle_groups.end() && + it->second != existing_bundle_group) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "MID " + mid + " in the offer has changed group."); + } + existing_bundle_groups_by_new_bundle_groups.insert( + std::make_pair(new_bundle_group, existing_bundle_group)); + } + } + } else if (type == SdpType::kAnswer) { std::vector offered_bundle_groups = local ? remote_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE) : local_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE); @@ -762,9 +814,7 @@ RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroups( "max-bundle is used but no bundle group found."); } - if (ShouldUpdateBundleGroup(type, description)) { - bundles_.Update(description); - } + bundles_.Update(description, type); for (const auto& bundle_group : bundles_.bundle_groups()) { if (!bundle_group->FirstContentName()) @@ -872,26 +922,6 @@ JsepTransportController::CreateJsepTransportDescription( rtp_abs_sendtime_extn_id, transport_info.description); } -bool JsepTransportController::ShouldUpdateBundleGroup( - SdpType type, - const cricket::SessionDescription* description) { - if (config_.bundle_policy == - PeerConnectionInterface::kBundlePolicyMaxBundle) { - return true; - } - - if (type != SdpType::kAnswer) { - return false; - } - - RTC_DCHECK(local_desc_ && remote_desc_); - std::vector local_bundles = - local_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE); - std::vector remote_bundles = - remote_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE); - return !local_bundles.empty() && !remote_bundles.empty(); -} - std::vector JsepTransportController::GetEncryptedHeaderExtensionIds( const cricket::ContentInfo& content_info) { const cricket::MediaContentDescription* content_desc = @@ -987,21 +1017,6 @@ 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()) { @@ -1201,7 +1216,7 @@ void JsepTransportController::OnTransportStateChanged_n( void JsepTransportController::UpdateAggregateStates_n() { TRACE_EVENT0("webrtc", "JsepTransportController::UpdateAggregateStates_n"); - auto dtls_transports = GetDtlsTransports(); + auto dtls_transports = GetActiveDtlsTransports(); cricket::IceConnectionState new_connection_state = cricket::kIceConnectionConnecting; PeerConnectionInterface::IceConnectionState new_ice_connection_state = diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index b8a1fd672c..fb420090d4 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h @@ -224,9 +224,7 @@ class JsepTransportController : public sigslot::has_slots<> { void SetActiveResetSrtpParams(bool active_reset_srtp_params); - // For now the rollback only removes mid to transport mappings - // and deletes unused transports, but doesn't consider anything more complex. - void RollbackTransports(); + RTCError RollbackTransports(); // F: void(const std::string&, const std::vector&) template @@ -342,9 +340,6 @@ class JsepTransportController : public sigslot::has_slots<> { const std::vector& encrypted_extension_ids, int rtp_abs_sendtime_extn_id); - bool ShouldUpdateBundleGroup(SdpType type, - const cricket::SessionDescription* description); - std::map> MergeEncryptedHeaderExtensionIdsForBundles( const cricket::SessionDescription* description); @@ -411,9 +406,13 @@ class JsepTransportController : public sigslot::has_slots<> { cricket::DtlsTransportInternal* rtcp_dtls_transport); // Collect all the DtlsTransports, including RTP and RTCP, from the - // JsepTransports. JsepTransportController can iterate all the DtlsTransports - // and update the aggregate states. + // JsepTransports, including those not mapped to a MID because they are being + // kept alive in case of rollback. std::vector GetDtlsTransports(); + // Same as the above, but doesn't include rollback transports. + // JsepTransportController can iterate all the DtlsTransports and update the + // aggregate states. + std::vector GetActiveDtlsTransports(); // Handlers for signals from Transport. void OnTransportWritableState_n(rtc::PacketTransportInternal* transport) diff --git a/pc/jsep_transport_controller_unittest.cc b/pc/jsep_transport_controller_unittest.cc index 6fe1ee3bf3..4c2ee2a2ae 100644 --- a/pc/jsep_transport_controller_unittest.cc +++ b/pc/jsep_transport_controller_unittest.cc @@ -861,6 +861,67 @@ TEST_F(JsepTransportControllerTest, EXPECT_EQ(2, gathering_state_signal_count_); } +// Test that states immediately return to "new" if all transports are +// discarded. This should happen at offer time, even though the transport +// controller may keep the transport alive in case of rollback. +TEST_F(JsepTransportControllerTest, + IceStatesReturnToNewWhenTransportsDiscarded) { + CreateJsepTransportController(JsepTransportController::Config()); + auto description = std::make_unique(); + AddAudioSection(description.get(), kAudioMid1, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, description.get()) + .ok()); + EXPECT_TRUE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, description.get()) + .ok()); + + // Trigger and verify initial non-new states. + auto fake_audio_dtls = static_cast( + transport_controller_->GetDtlsTransport(kAudioMid1)); + fake_audio_dtls->fake_ice_transport()->MaybeStartGathering(); + fake_audio_dtls->fake_ice_transport()->SetTransportState( + webrtc::IceTransportState::kChecking, + cricket::IceTransportState::STATE_CONNECTING); + EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionChecking, + ice_connection_state_, kTimeout); + EXPECT_EQ(1, ice_connection_state_signal_count_); + EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnecting, + combined_connection_state_, kTimeout); + EXPECT_EQ(1, combined_connection_state_signal_count_); + EXPECT_EQ_WAIT(cricket::kIceGatheringGathering, gathering_state_, kTimeout); + EXPECT_EQ(1, gathering_state_signal_count_); + + // Reject m= section which should disconnect the transport and return states + // to "new". + description->contents()[0].rejected = true; + EXPECT_TRUE(transport_controller_ + ->SetRemoteDescription(SdpType::kOffer, description.get()) + .ok()); + EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionNew, + ice_connection_state_, kTimeout); + EXPECT_EQ(2, ice_connection_state_signal_count_); + EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kNew, + combined_connection_state_, kTimeout); + EXPECT_EQ(2, combined_connection_state_signal_count_); + EXPECT_EQ_WAIT(cricket::kIceGatheringNew, gathering_state_, kTimeout); + EXPECT_EQ(2, gathering_state_signal_count_); + + // For good measure, rollback the offer and verify that states return to + // their previous values. + EXPECT_TRUE(transport_controller_->RollbackTransports().ok()); + EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionChecking, + ice_connection_state_, kTimeout); + EXPECT_EQ(3, ice_connection_state_signal_count_); + EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnecting, + combined_connection_state_, kTimeout); + EXPECT_EQ(3, combined_connection_state_signal_count_); + EXPECT_EQ_WAIT(cricket::kIceGatheringGathering, gathering_state_, kTimeout); + EXPECT_EQ(3, gathering_state_signal_count_); +} + TEST_F(JsepTransportControllerTest, SignalCandidatesGathered) { CreateJsepTransportController(JsepTransportController::Config()); auto description = CreateSessionDescriptionWithBundleGroup(); @@ -1608,6 +1669,356 @@ TEST_F(JsepTransportControllerTest, MultipleBundleGroupsChangeFirstMid) { EXPECT_EQ(mid5_transport, mid6_transport); } +TEST_F(JsepTransportControllerTest, + MultipleBundleGroupsSectionsAddedInSubsequentOffer) { + static const char kMid1Audio[] = "1_audio"; + static const char kMid2Audio[] = "2_audio"; + static const char kMid3Audio[] = "3_audio"; + static const char kMid4Video[] = "4_video"; + static const char kMid5Video[] = "5_video"; + static const char kMid6Video[] = "6_video"; + + CreateJsepTransportController(JsepTransportController::Config()); + // Start by grouping (kMid1Audio,kMid2Audio) and (kMid4Video,kMid4f5Video). + cricket::ContentGroup bundle_group1(cricket::GROUP_TYPE_BUNDLE); + bundle_group1.AddContentName(kMid1Audio); + bundle_group1.AddContentName(kMid2Audio); + cricket::ContentGroup bundle_group2(cricket::GROUP_TYPE_BUNDLE); + bundle_group2.AddContentName(kMid4Video); + bundle_group2.AddContentName(kMid5Video); + + auto local_offer = std::make_unique(); + AddAudioSection(local_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(local_offer.get(), kMid2Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid4Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid5Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + local_offer->AddGroup(bundle_group1); + local_offer->AddGroup(bundle_group2); + + auto remote_answer = std::make_unique(); + AddAudioSection(remote_answer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(remote_answer.get(), kMid2Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(remote_answer.get(), kMid4Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(remote_answer.get(), kMid5Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + remote_answer->AddGroup(bundle_group1); + remote_answer->AddGroup(bundle_group2); + + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_offer.get()) + .ok()); + EXPECT_TRUE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get()) + .ok()); + + // Add kMid3Audio and kMid6Video to the respective audio/video bundle groups. + cricket::ContentGroup new_bundle_group1(cricket::GROUP_TYPE_BUNDLE); + bundle_group1.AddContentName(kMid3Audio); + cricket::ContentGroup new_bundle_group2(cricket::GROUP_TYPE_BUNDLE); + bundle_group2.AddContentName(kMid6Video); + + auto subsequent_offer = std::make_unique(); + AddAudioSection(subsequent_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(subsequent_offer.get(), kMid2Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(subsequent_offer.get(), kMid3Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(subsequent_offer.get(), kMid4Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(subsequent_offer.get(), kMid5Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(subsequent_offer.get(), kMid6Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + subsequent_offer->AddGroup(bundle_group1); + subsequent_offer->AddGroup(bundle_group2); + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, subsequent_offer.get()) + .ok()); + auto mid1_transport = transport_controller_->GetRtpTransport(kMid1Audio); + auto mid2_transport = transport_controller_->GetRtpTransport(kMid2Audio); + auto mid3_transport = transport_controller_->GetRtpTransport(kMid3Audio); + auto mid4_transport = transport_controller_->GetRtpTransport(kMid4Video); + auto mid5_transport = transport_controller_->GetRtpTransport(kMid5Video); + auto mid6_transport = transport_controller_->GetRtpTransport(kMid6Video); + EXPECT_NE(mid1_transport, mid4_transport); + EXPECT_EQ(mid1_transport, mid2_transport); + EXPECT_EQ(mid2_transport, mid3_transport); + EXPECT_EQ(mid4_transport, mid5_transport); + EXPECT_EQ(mid5_transport, mid6_transport); +} + +TEST_F(JsepTransportControllerTest, + MultipleBundleGroupsCombinedInSubsequentOffer) { + static const char kMid1Audio[] = "1_audio"; + static const char kMid2Audio[] = "2_audio"; + static const char kMid3Video[] = "3_video"; + static const char kMid4Video[] = "4_video"; + + CreateJsepTransportController(JsepTransportController::Config()); + // Start by grouping (kMid1Audio,kMid2Audio) and (kMid3Video,kMid4Video). + cricket::ContentGroup bundle_group1(cricket::GROUP_TYPE_BUNDLE); + bundle_group1.AddContentName(kMid1Audio); + bundle_group1.AddContentName(kMid2Audio); + cricket::ContentGroup bundle_group2(cricket::GROUP_TYPE_BUNDLE); + bundle_group2.AddContentName(kMid3Video); + bundle_group2.AddContentName(kMid4Video); + + auto local_offer = std::make_unique(); + AddAudioSection(local_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(local_offer.get(), kMid2Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid3Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid4Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + local_offer->AddGroup(bundle_group1); + local_offer->AddGroup(bundle_group2); + + auto remote_answer = std::make_unique(); + AddAudioSection(remote_answer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(remote_answer.get(), kMid2Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(remote_answer.get(), kMid3Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(remote_answer.get(), kMid4Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + remote_answer->AddGroup(bundle_group1); + remote_answer->AddGroup(bundle_group2); + + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_offer.get()) + .ok()); + EXPECT_TRUE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get()) + .ok()); + + // Switch to grouping (kMid1Audio,kMid2Audio,kMid3Video,kMid4Video). + // This is a illegal without first removing m= sections from their groups. + cricket::ContentGroup new_bundle_group(cricket::GROUP_TYPE_BUNDLE); + new_bundle_group.AddContentName(kMid1Audio); + new_bundle_group.AddContentName(kMid2Audio); + new_bundle_group.AddContentName(kMid3Video); + new_bundle_group.AddContentName(kMid4Video); + + auto subsequent_offer = std::make_unique(); + AddAudioSection(subsequent_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(subsequent_offer.get(), kMid2Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(subsequent_offer.get(), kMid3Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(subsequent_offer.get(), kMid4Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + subsequent_offer->AddGroup(new_bundle_group); + EXPECT_FALSE( + transport_controller_ + ->SetLocalDescription(SdpType::kOffer, subsequent_offer.get()) + .ok()); +} + +TEST_F(JsepTransportControllerTest, + MultipleBundleGroupsSplitInSubsequentOffer) { + static const char kMid1Audio[] = "1_audio"; + static const char kMid2Audio[] = "2_audio"; + static const char kMid3Video[] = "3_video"; + static const char kMid4Video[] = "4_video"; + + CreateJsepTransportController(JsepTransportController::Config()); + // Start by grouping (kMid1Audio,kMid2Audio,kMid3Video,kMid4Video). + cricket::ContentGroup bundle_group(cricket::GROUP_TYPE_BUNDLE); + bundle_group.AddContentName(kMid1Audio); + bundle_group.AddContentName(kMid2Audio); + bundle_group.AddContentName(kMid3Video); + bundle_group.AddContentName(kMid4Video); + + auto local_offer = std::make_unique(); + AddAudioSection(local_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(local_offer.get(), kMid2Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid3Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid4Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + local_offer->AddGroup(bundle_group); + + auto remote_answer = std::make_unique(); + AddAudioSection(remote_answer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(remote_answer.get(), kMid2Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(remote_answer.get(), kMid3Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(remote_answer.get(), kMid4Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + remote_answer->AddGroup(bundle_group); + + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_offer.get()) + .ok()); + EXPECT_TRUE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get()) + .ok()); + + // Switch to grouping (kMid1Audio,kMid2Audio) and (kMid3Video,kMid4Video). + // This is a illegal without first removing m= sections from their groups. + cricket::ContentGroup new_bundle_group1(cricket::GROUP_TYPE_BUNDLE); + new_bundle_group1.AddContentName(kMid1Audio); + new_bundle_group1.AddContentName(kMid2Audio); + cricket::ContentGroup new_bundle_group2(cricket::GROUP_TYPE_BUNDLE); + new_bundle_group2.AddContentName(kMid3Video); + new_bundle_group2.AddContentName(kMid4Video); + + auto subsequent_offer = std::make_unique(); + AddAudioSection(subsequent_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(subsequent_offer.get(), kMid2Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(subsequent_offer.get(), kMid3Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(subsequent_offer.get(), kMid4Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + subsequent_offer->AddGroup(new_bundle_group1); + subsequent_offer->AddGroup(new_bundle_group2); + EXPECT_FALSE( + transport_controller_ + ->SetLocalDescription(SdpType::kOffer, subsequent_offer.get()) + .ok()); +} + +TEST_F(JsepTransportControllerTest, + MultipleBundleGroupsShuffledInSubsequentOffer) { + static const char kMid1Audio[] = "1_audio"; + static const char kMid2Audio[] = "2_audio"; + static const char kMid3Video[] = "3_video"; + static const char kMid4Video[] = "4_video"; + + CreateJsepTransportController(JsepTransportController::Config()); + // Start by grouping (kMid1Audio,kMid2Audio) and (kMid3Video,kMid4Video). + cricket::ContentGroup bundle_group1(cricket::GROUP_TYPE_BUNDLE); + bundle_group1.AddContentName(kMid1Audio); + bundle_group1.AddContentName(kMid2Audio); + cricket::ContentGroup bundle_group2(cricket::GROUP_TYPE_BUNDLE); + bundle_group2.AddContentName(kMid3Video); + bundle_group2.AddContentName(kMid4Video); + + auto local_offer = std::make_unique(); + AddAudioSection(local_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(local_offer.get(), kMid2Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid3Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid4Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + local_offer->AddGroup(bundle_group1); + local_offer->AddGroup(bundle_group2); + + auto remote_answer = std::make_unique(); + AddAudioSection(remote_answer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(remote_answer.get(), kMid2Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(remote_answer.get(), kMid3Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(remote_answer.get(), kMid4Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + remote_answer->AddGroup(bundle_group1); + remote_answer->AddGroup(bundle_group2); + + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_offer.get()) + .ok()); + EXPECT_TRUE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get()) + .ok()); + + // Switch to grouping (kMid1Audio,kMid3Video) and (kMid2Audio,kMid3Video). + // This is a illegal without first removing m= sections from their groups. + cricket::ContentGroup new_bundle_group1(cricket::GROUP_TYPE_BUNDLE); + new_bundle_group1.AddContentName(kMid1Audio); + new_bundle_group1.AddContentName(kMid3Video); + cricket::ContentGroup new_bundle_group2(cricket::GROUP_TYPE_BUNDLE); + new_bundle_group2.AddContentName(kMid2Audio); + new_bundle_group2.AddContentName(kMid4Video); + + auto subsequent_offer = std::make_unique(); + AddAudioSection(subsequent_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(subsequent_offer.get(), kMid2Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(subsequent_offer.get(), kMid3Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(subsequent_offer.get(), kMid4Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + subsequent_offer->AddGroup(new_bundle_group1); + subsequent_offer->AddGroup(new_bundle_group2); + EXPECT_FALSE( + transport_controller_ + ->SetLocalDescription(SdpType::kOffer, subsequent_offer.get()) + .ok()); +} + // Tests that only a subset of all the m= sections are bundled. TEST_F(JsepTransportControllerTest, BundleSubsetOfMediaSections) { CreateJsepTransportController(JsepTransportController::Config()); @@ -2059,4 +2470,220 @@ TEST_F(JsepTransportControllerTest, ChangeTaggedMediaSectionMaxBundle) { .ok()); } +TEST_F(JsepTransportControllerTest, RollbackRestoresRejectedTransport) { + static const char kMid1Audio[] = "1_audio"; + + // Perform initial offer/answer. + CreateJsepTransportController(JsepTransportController::Config()); + auto local_offer = std::make_unique(); + AddAudioSection(local_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + std::unique_ptr remote_answer( + local_offer->Clone()); + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_offer.get()) + .ok()); + EXPECT_TRUE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get()) + .ok()); + + auto mid1_transport = transport_controller_->GetRtpTransport(kMid1Audio); + + // Apply a reoffer which rejects the m= section, causing the transport to be + // set to null. + auto local_reoffer = std::make_unique(); + AddAudioSection(local_reoffer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + local_reoffer->contents()[0].rejected = true; + + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_reoffer.get()) + .ok()); + auto old_mid1_transport = mid1_transport; + mid1_transport = transport_controller_->GetRtpTransport(kMid1Audio); + EXPECT_EQ(nullptr, mid1_transport); + + // Rolling back shouldn't just create a new transport for MID 1, it should + // restore the old transport. + EXPECT_TRUE(transport_controller_->RollbackTransports().ok()); + mid1_transport = transport_controller_->GetRtpTransport(kMid1Audio); + EXPECT_EQ(old_mid1_transport, mid1_transport); +} + +// If an offer with a modified BUNDLE group causes a MID->transport mapping to +// change, rollback should restore the previous mapping. +TEST_F(JsepTransportControllerTest, RollbackRestoresPreviousTransportMapping) { + static const char kMid1Audio[] = "1_audio"; + static const char kMid2Audio[] = "2_audio"; + static const char kMid3Audio[] = "3_audio"; + + // Perform an initial offer/answer to establish a (kMid1Audio,kMid2Audio) + // group. + CreateJsepTransportController(JsepTransportController::Config()); + cricket::ContentGroup bundle_group(cricket::GROUP_TYPE_BUNDLE); + bundle_group.AddContentName(kMid1Audio); + bundle_group.AddContentName(kMid2Audio); + + auto local_offer = std::make_unique(); + AddAudioSection(local_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid2Audio, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(local_offer.get(), kMid3Audio, kIceUfrag3, kIcePwd3, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + local_offer->AddGroup(bundle_group); + + std::unique_ptr remote_answer( + local_offer->Clone()); + + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_offer.get()) + .ok()); + EXPECT_TRUE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get()) + .ok()); + + auto mid1_transport = transport_controller_->GetRtpTransport(kMid1Audio); + auto mid2_transport = transport_controller_->GetRtpTransport(kMid2Audio); + auto mid3_transport = transport_controller_->GetRtpTransport(kMid3Audio); + EXPECT_EQ(mid1_transport, mid2_transport); + EXPECT_NE(mid1_transport, mid3_transport); + + // Apply a reoffer adding kMid3Audio to the group; transport mapping should + // change, even without an answer, since this is an existing group. + bundle_group.AddContentName(kMid3Audio); + auto local_reoffer = std::make_unique(); + AddAudioSection(local_reoffer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_reoffer.get(), kMid2Audio, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(local_reoffer.get(), kMid3Audio, kIceUfrag3, kIcePwd3, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + local_reoffer->AddGroup(bundle_group); + + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_reoffer.get()) + .ok()); + + // Store the old transport pointer and verify that the offer actually changed + // transports. + auto old_mid3_transport = mid3_transport; + mid1_transport = transport_controller_->GetRtpTransport(kMid1Audio); + mid2_transport = transport_controller_->GetRtpTransport(kMid2Audio); + mid3_transport = transport_controller_->GetRtpTransport(kMid3Audio); + EXPECT_EQ(mid1_transport, mid2_transport); + EXPECT_EQ(mid1_transport, mid3_transport); + + // Rolling back shouldn't just create a new transport for MID 3, it should + // restore the old transport. + EXPECT_TRUE(transport_controller_->RollbackTransports().ok()); + mid3_transport = transport_controller_->GetRtpTransport(kMid3Audio); + EXPECT_EQ(old_mid3_transport, mid3_transport); +} + +// Test that if an offer adds a MID to a specific BUNDLE group and is then +// rolled back, it can be added to a different BUNDLE group in a new offer. +// This is effectively testing that rollback resets the BundleManager state. +TEST_F(JsepTransportControllerTest, RollbackAndAddToDifferentBundleGroup) { + static const char kMid1Audio[] = "1_audio"; + static const char kMid2Audio[] = "2_audio"; + static const char kMid3Audio[] = "3_audio"; + + // Perform an initial offer/answer to establish two bundle groups, each with + // one MID. + CreateJsepTransportController(JsepTransportController::Config()); + cricket::ContentGroup bundle_group1(cricket::GROUP_TYPE_BUNDLE); + bundle_group1.AddContentName(kMid1Audio); + cricket::ContentGroup bundle_group2(cricket::GROUP_TYPE_BUNDLE); + bundle_group2.AddContentName(kMid2Audio); + + auto local_offer = std::make_unique(); + AddAudioSection(local_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid2Audio, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + local_offer->AddGroup(bundle_group1); + local_offer->AddGroup(bundle_group2); + + std::unique_ptr remote_answer( + local_offer->Clone()); + + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_offer.get()) + .ok()); + EXPECT_TRUE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get()) + .ok()); + + // Apply an offer that adds kMid3Audio to the first BUNDLE group., + cricket::ContentGroup modified_bundle_group1(cricket::GROUP_TYPE_BUNDLE); + modified_bundle_group1.AddContentName(kMid1Audio); + modified_bundle_group1.AddContentName(kMid3Audio); + auto subsequent_offer_1 = std::make_unique(); + AddAudioSection(subsequent_offer_1.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(subsequent_offer_1.get(), kMid2Audio, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(subsequent_offer_1.get(), kMid3Audio, kIceUfrag3, kIcePwd3, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + subsequent_offer_1->AddGroup(modified_bundle_group1); + subsequent_offer_1->AddGroup(bundle_group2); + + EXPECT_TRUE( + transport_controller_ + ->SetLocalDescription(SdpType::kOffer, subsequent_offer_1.get()) + .ok()); + + auto mid1_transport = transport_controller_->GetRtpTransport(kMid1Audio); + auto mid2_transport = transport_controller_->GetRtpTransport(kMid2Audio); + auto mid3_transport = transport_controller_->GetRtpTransport(kMid3Audio); + EXPECT_NE(mid1_transport, mid2_transport); + EXPECT_EQ(mid1_transport, mid3_transport); + + // Rollback and expect the transport to be reset. + EXPECT_TRUE(transport_controller_->RollbackTransports().ok()); + EXPECT_EQ(nullptr, transport_controller_->GetRtpTransport(kMid3Audio)); + + // Apply an offer that adds kMid3Audio to the second BUNDLE group., + cricket::ContentGroup modified_bundle_group2(cricket::GROUP_TYPE_BUNDLE); + modified_bundle_group2.AddContentName(kMid2Audio); + modified_bundle_group2.AddContentName(kMid3Audio); + auto subsequent_offer_2 = std::make_unique(); + AddAudioSection(subsequent_offer_2.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(subsequent_offer_2.get(), kMid2Audio, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(subsequent_offer_2.get(), kMid3Audio, kIceUfrag3, kIcePwd3, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + subsequent_offer_2->AddGroup(bundle_group1); + subsequent_offer_2->AddGroup(modified_bundle_group2); + + EXPECT_TRUE( + transport_controller_ + ->SetLocalDescription(SdpType::kOffer, subsequent_offer_2.get()) + .ok()); + + mid1_transport = transport_controller_->GetRtpTransport(kMid1Audio); + mid2_transport = transport_controller_->GetRtpTransport(kMid2Audio); + mid3_transport = transport_controller_->GetRtpTransport(kMid3Audio); + EXPECT_NE(mid1_transport, mid2_transport); + EXPECT_EQ(mid2_transport, mid3_transport); +} + } // namespace webrtc diff --git a/pc/peer_connection_bundle_unittest.cc b/pc/peer_connection_bundle_unittest.cc index 08754c6820..be05af920c 100644 --- a/pc/peer_connection_bundle_unittest.cc +++ b/pc/peer_connection_bundle_unittest.cc @@ -909,11 +909,11 @@ TEST_F(PeerConnectionBundleTestUnifiedPlan, MultipleBundleGroups) { EXPECT_TRUE( caller->SetLocalDescription(CloneSessionDescription(offer.get()))); - callee->SetRemoteDescription(std::move(offer)); + EXPECT_TRUE(callee->SetRemoteDescription(std::move(offer))); auto answer = callee->CreateAnswer(); EXPECT_TRUE( callee->SetLocalDescription(CloneSessionDescription(answer.get()))); - caller->SetRemoteDescription(std::move(answer)); + EXPECT_TRUE(caller->SetRemoteDescription(std::move(answer))); // Verify bundling on sender side. auto senders = caller->pc()->GetSenders(); @@ -938,4 +938,59 @@ TEST_F(PeerConnectionBundleTestUnifiedPlan, MultipleBundleGroups) { EXPECT_NE(receiver0_transport, receiver2_transport); } +// Test that, with the "max-compat" bundle policy, it's possible to add an m= +// section that's not part of an existing bundle group. +TEST_F(PeerConnectionBundleTestUnifiedPlan, AddNonBundledSection) { + RTCConfiguration config; + config.bundle_policy = PeerConnectionInterface::kBundlePolicyMaxCompat; + auto caller = CreatePeerConnection(config); + caller->AddAudioTrack("0_audio"); + caller->AddAudioTrack("1_audio"); + auto callee = CreatePeerConnection(config); + + // Establish an existing BUNDLE group. + auto offer = caller->CreateOffer(RTCOfferAnswerOptions()); + EXPECT_TRUE( + caller->SetLocalDescription(CloneSessionDescription(offer.get()))); + EXPECT_TRUE(callee->SetRemoteDescription(std::move(offer))); + auto answer = callee->CreateAnswer(); + EXPECT_TRUE( + callee->SetLocalDescription(CloneSessionDescription(answer.get()))); + EXPECT_TRUE(caller->SetRemoteDescription(std::move(answer))); + + // Add a track but munge SDP so it's not part of the bundle group. + caller->AddAudioTrack("3_audio"); + offer = caller->CreateOffer(RTCOfferAnswerOptions()); + offer->description()->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE); + cricket::ContentGroup bundle_group(cricket::GROUP_TYPE_BUNDLE); + bundle_group.AddContentName("0"); + bundle_group.AddContentName("1"); + offer->description()->AddGroup(bundle_group); + EXPECT_TRUE( + caller->SetLocalDescription(CloneSessionDescription(offer.get()))); + EXPECT_TRUE(callee->SetRemoteDescription(std::move(offer))); + answer = callee->CreateAnswer(); + EXPECT_TRUE( + callee->SetLocalDescription(CloneSessionDescription(answer.get()))); + EXPECT_TRUE(caller->SetRemoteDescription(std::move(answer))); + + // Verify bundling on the sender side. + auto senders = caller->pc()->GetSenders(); + ASSERT_EQ(senders.size(), 3u); + auto sender0_transport = senders[0]->dtls_transport(); + auto sender1_transport = senders[1]->dtls_transport(); + auto sender2_transport = senders[2]->dtls_transport(); + EXPECT_EQ(sender0_transport, sender1_transport); + EXPECT_NE(sender0_transport, sender2_transport); + + // Verify bundling on receiver side. + auto receivers = callee->pc()->GetReceivers(); + ASSERT_EQ(receivers.size(), 3u); + auto receiver0_transport = receivers[0]->dtls_transport(); + auto receiver1_transport = receivers[1]->dtls_transport(); + auto receiver2_transport = receivers[2]->dtls_transport(); + EXPECT_EQ(receiver0_transport, receiver1_transport); + EXPECT_NE(receiver0_transport, receiver2_transport); +} + } // namespace webrtc diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index c167f35f9a..82f13ee957 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -2751,7 +2751,10 @@ RTCError SdpOfferAnswerHandler::Rollback(SdpType desc_type) { transceiver->internal()->set_mid(state.mid()); transceiver->internal()->set_mline_index(state.mline_index()); } - transport_controller()->RollbackTransports(); + RTCError e = transport_controller()->RollbackTransports(); + if (!e.ok()) { + return e; + } transceivers()->DiscardStableStates(); pending_local_description_.reset(); pending_remote_description_.reset(); diff --git a/pc/test/mock_peer_connection_observers.h b/pc/test/mock_peer_connection_observers.h index 413339dbf7..8054e08333 100644 --- a/pc/test/mock_peer_connection_observers.h +++ b/pc/test/mock_peer_connection_observers.h @@ -116,8 +116,6 @@ class MockPeerConnectionObserver : public PeerConnectionObserver { } void OnIceCandidate(const IceCandidateInterface* candidate) override { RTC_DCHECK(pc_); - RTC_DCHECK(PeerConnectionInterface::kIceGatheringNew != - pc_->ice_gathering_state()); candidates_.push_back(std::make_unique( candidate->sdp_mid(), candidate->sdp_mline_index(), candidate->candidate()));