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}
diff --git a/pc/jsep_transport_collection.cc b/pc/jsep_transport_collection.cc
index ce068d9..98b8cd2 100644
--- a/pc/jsep_transport_collection.cc
+++ b/pc/jsep_transport_collection.cc
@@ -20,21 +20,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 +124,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) {
@@ -157,8 +217,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 +249,33 @@
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_;
+ 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(
@@ -212,6 +286,11 @@
return true;
}
}
+ for (const auto& kv : stable_mid_to_transport_) {
+ if (kv.second == jsep_transport) {
+ return true;
+ }
+ }
return false;
}
@@ -219,7 +298,7 @@
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;
}
@@ -233,6 +312,23 @@
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())) {
+ ++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_) {
@@ -241,13 +337,6 @@
<< " 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..9357037 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_;
};
@@ -108,17 +120,25 @@
// 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();
- // 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 currently maps to this transport, in either the
+ // pending or stable mapping.
+ bool TransportInUse(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 +150,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..8b9596b 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>
@@ -551,17 +557,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 +577,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 +644,7 @@
}
if (type == SdpType::kAnswer) {
transports_.CommitTransports();
+ bundles_.Commit();
}
return RTCError::OK();
}
@@ -682,7 +680,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 +797,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 +905,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 +1000,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()) {
diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h
index 71b01bf..3b20bbb 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);
diff --git a/pc/jsep_transport_controller_unittest.cc b/pc/jsep_transport_controller_unittest.cc
index a06f580..bc7cfeb 100644
--- a/pc/jsep_transport_controller_unittest.cc
+++ b/pc/jsep_transport_controller_unittest.cc
@@ -1608,6 +1608,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 +2409,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();
