Reland "Fix bug where we assume new m= sections will always be bundled."
This is a reland of d2b885fd91909f1b17fb11292a8c989d5d883b22, after
making sure transports that are just being kept alive in case of
rollback don't contribute to connection state, which broke a WPT.
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 <hta@webrtc.org>
> Reviewed-by: Henrik Boström <hbos@webrtc.org>
> Commit-Queue: Taylor Brandstetter <deadbeef@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#34544}
Bug: webrtc:12906, webrtc:12999
Change-Id: I68bf988b1918dd2d51de76e53e4fd696fea5a09b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/227120
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34596}
diff --git a/pc/jsep_transport_collection.cc b/pc/jsep_transport_collection.cc
index ce068d9..c7364b0 100644
--- a/pc/jsep_transport_collection.cc
+++ b/pc/jsep_transport_collection.cc
@@ -12,6 +12,7 @@
#include <algorithm>
#include <map>
+#include <set>
#include <type_traits>
#include <utility>
@@ -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<cricket::ContentGroup>(*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<cricket::ContentGroup>(*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 @@
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<cricket::ContentGroup>(*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<cricket::ContentGroup>(*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<cricket::JsepTransport> transport) {
@@ -110,6 +171,17 @@
return result;
}
+std::vector<cricket::JsepTransport*>
+JsepTransportCollection::ActiveTransports() {
+ RTC_DCHECK_RUN_ON(&sequence_checker_);
+ std::set<cricket::JsepTransport*> transports;
+ for (const auto& kv : mid_to_transport_) {
+ transports.insert(kv.second);
+ }
+ return std::vector<cricket::JsepTransport*>(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 @@
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 @@
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 @@
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 @@
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 0dd528d..aa52934 100644
--- a/pc/jsep_transport_collection.h
+++ b/pc/jsep_transport_collection.h
@@ -18,6 +18,7 @@
#include <utility>
#include <vector>
+#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 @@
// 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 @@
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<std::unique_ptr<cricket::ContentGroup>> bundle_groups_
RTC_GUARDED_BY(sequence_checker_);
+ std::vector<std::unique_ptr<cricket::ContentGroup>> stable_bundle_groups_
+ RTC_GUARDED_BY(sequence_checker_);
std::map<std::string, cricket::ContentGroup*>
established_bundle_groups_by_mid_;
};
@@ -91,7 +103,11 @@
void RegisterTransport(const std::string& mid,
std::unique_ptr<cricket::JsepTransport> transport);
+ // Returns all transports, including those not currently mapped to any MID
+ // because they're being kept alive in case of rollback.
std::vector<cricket::JsepTransport*> Transports();
+ // Only returns transports currently mapped to a MID.
+ std::vector<cricket::JsepTransport*> 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 @@
// 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 @@
// (BaseChannel/SctpTransport) and the JsepTransport underneath.
std::map<std::string, cricket::JsepTransport*> mid_to_transport_
RTC_GUARDED_BY(sequence_checker_);
- // Keep track of mids that have been mapped to transports. Used for rollback.
- std::vector<std::string> pending_mids_ RTC_GUARDED_BY(sequence_checker_);
+ // A snapshot of mid_to_transport_ at the last stable state. Used for
+ // rollback.
+ std::map<std::string, cricket::JsepTransport*> stable_mid_to_transport_
+ RTC_GUARDED_BY(sequence_checker_);
// Callback used to inform subscribers of altered transports.
const std::function<bool(const std::string& mid,
cricket::JsepTransport* transport)>
diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc
index 47fddd4..1b554eb 100644
--- a/pc/jsep_transport_controller.cc
+++ b/pc/jsep_transport_controller.cc
@@ -55,7 +55,8 @@
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::RollbackTransports() {
+RTCError JsepTransportController::RollbackTransports() {
if (!network_thread_->IsCurrent()) {
- network_thread_->Invoke<void>(RTC_FROM_HERE, [=] { RollbackTransports(); });
- return;
+ return network_thread_->Invoke<RTCError>(
+ 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<webrtc::IceTransportInterface>
@@ -525,6 +531,23 @@
return dtls_transports;
}
+std::vector<cricket::DtlsTransportInternal*>
+JsepTransportController::GetActiveDtlsTransports() {
+ RTC_DCHECK_RUN_ON(network_thread_);
+ std::vector<cricket::DtlsTransportInternal*> 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 @@
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 @@
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 @@
}
if (type == SdpType::kAnswer) {
transports_.CommitTransports();
+ bundles_.Commit();
}
return RTCError::OK();
}
@@ -682,7 +697,44 @@
}
}
- 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<const cricket::ContentGroup*, const cricket::ContentGroup*>
+ new_bundle_groups_by_existing_bundle_groups;
+ std::map<const cricket::ContentGroup*, const cricket::ContentGroup*>
+ 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<const cricket::ContentGroup*> offered_bundle_groups =
local ? remote_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE)
: local_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE);
@@ -762,9 +814,7 @@
"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 @@
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<const cricket::ContentGroup*> local_bundles =
- local_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE);
- std::vector<const cricket::ContentGroup*> remote_bundles =
- remote_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE);
- return !local_bundles.empty() && !remote_bundles.empty();
-}
-
std::vector<int> JsepTransportController::GetEncryptedHeaderExtensionIds(
const cricket::ContentInfo& content_info) {
const cricket::MediaContentDescription* content_desc =
@@ -987,21 +1017,6 @@
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::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 71b01bf..4161f6a 100644
--- a/pc/jsep_transport_controller.h
+++ b/pc/jsep_transport_controller.h
@@ -224,9 +224,7 @@
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<cricket::Candidate>&)
template <typename F>
@@ -342,9 +340,6 @@
const std::vector<int>& encrypted_extension_ids,
int rtp_abs_sendtime_extn_id);
- bool ShouldUpdateBundleGroup(SdpType type,
- const cricket::SessionDescription* description);
-
std::map<const cricket::ContentGroup*, std::vector<int>>
MergeEncryptedHeaderExtensionIdsForBundles(
const cricket::SessionDescription* description);
@@ -411,9 +406,13 @@
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<cricket::DtlsTransportInternal*> GetDtlsTransports();
+ // Same as the above, but doesn't include rollback transports.
+ // JsepTransportController can iterate all the DtlsTransports and update the
+ // aggregate states.
+ std::vector<cricket::DtlsTransportInternal*> 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 a06f580..88fda75 100644
--- a/pc/jsep_transport_controller_unittest.cc
+++ b/pc/jsep_transport_controller_unittest.cc
@@ -861,6 +861,67 @@
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<cricket::SessionDescription>();
+ 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<FakeDtlsTransport*>(
+ 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 @@
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<cricket::SessionDescription>();
+ 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<cricket::SessionDescription>();
+ 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<cricket::SessionDescription>();
+ 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<cricket::SessionDescription>();
+ 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<cricket::SessionDescription>();
+ 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<cricket::SessionDescription>();
+ 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<cricket::SessionDescription>();
+ 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<cricket::SessionDescription>();
+ 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<cricket::SessionDescription>();
+ 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<cricket::SessionDescription>();
+ 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<cricket::SessionDescription>();
+ 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<cricket::SessionDescription>();
+ 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 @@
.ok());
}
+TEST_F(JsepTransportControllerTest, RollbackRestoresRejectedTransport) {
+ static const char kMid1Audio[] = "1_audio";
+
+ // Perform initial offer/answer.
+ CreateJsepTransportController(JsepTransportController::Config());
+ auto local_offer = std::make_unique<cricket::SessionDescription>();
+ AddAudioSection(local_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1,
+ cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS,
+ nullptr);
+ std::unique_ptr<cricket::SessionDescription> 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<cricket::SessionDescription>();
+ 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<cricket::SessionDescription>();
+ 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<cricket::SessionDescription> 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<cricket::SessionDescription>();
+ 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<cricket::SessionDescription>();
+ 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<cricket::SessionDescription> 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<cricket::SessionDescription>();
+ 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<cricket::SessionDescription>();
+ 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 08754c6..be05af9 100644
--- a/pc/peer_connection_bundle_unittest.cc
+++ b/pc/peer_connection_bundle_unittest.cc
@@ -909,11 +909,11 @@
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 @@
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 533bd84..929736e 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -2737,7 +2737,10 @@
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 413339d..8054e08 100644
--- a/pc/test/mock_peer_connection_observers.h
+++ b/pc/test/mock_peer_connection_observers.h
@@ -116,8 +116,6 @@
}
void OnIceCandidate(const IceCandidateInterface* candidate) override {
RTC_DCHECK(pc_);
- RTC_DCHECK(PeerConnectionInterface::kIceGatheringNew !=
- pc_->ice_gathering_state());
candidates_.push_back(std::make_unique<JsepIceCandidate>(
candidate->sdp_mid(), candidate->sdp_mline_index(),
candidate->candidate()));
