From f8187e0a82130f93cf01953deafd63ea427af022 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Mon, 26 Apr 2021 21:04:26 +0200 Subject: [PATCH] [Unified Plan] Support multiple BUNDLE groups. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In this CL, JsepTransportController and MediaSessionDescriptionFactory are updated not to assume that there only exists at most a single BUNDLE group but a list of N groups. This makes it possible to create multiple BUNDLE groups by having multiple "a=group:BUNDLE" lines in the SDP. This makes it possible to have some m= sections in one group and some other m= sections in another group. For example, you could group all audio m= sections in one group and all video m= sections in another group. This enables "send all audio tracks on one transport and all video tracks on another transport" in Unified Plan. This is something that was possible in Plan B because all ssrcs in the same m= section were implicitly bundled together forming a group of audio m= section and video m= section (even without use of the BUNDLE tag). PeerConnection will never create multiple BUNDLE groups by default, but upon setting SDP with multiple BUNDLE groups the PeerConnection will accept them if configured to accept BUNDLE. This makes it possible to accept an SFU's BUNDLE offer without having to SDP munge the answer. C++ unit tests are added. This fix has also been verified manually on: https://jsfiddle.net/henbos/to89L6ce/43/ Without fix: 0+2 get bundled, 1+3 don't get bundled. With fix: 0+2 get bundled in first group, 1+3 get bundled in second group. Bug: webrtc:10208 Change-Id: Iaf451fa5459c484730c8018274166ef154b19af8 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/214487 Reviewed-by: Taylor Reviewed-by: Harald Alvestrand Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/master@{#33838} --- pc/jsep_transport_controller.cc | 293 +++++++++---- pc/jsep_transport_controller.h | 27 +- pc/jsep_transport_controller_unittest.cc | 508 +++++++++++++++++++++++ pc/media_session.cc | 100 +++-- pc/media_session_unittest.cc | 60 +++ pc/peer_connection.cc | 15 +- pc/peer_connection.h | 5 +- pc/peer_connection_bundle_unittest.cc | 52 +++ pc/sdp_offer_answer.cc | 185 +++++---- pc/sdp_offer_answer.h | 50 ++- pc/session_description.cc | 11 + pc/session_description.h | 2 + pc/webrtc_sdp.cc | 8 +- pc/webrtc_sdp_unittest.cc | 16 +- 14 files changed, 1077 insertions(+), 255 deletions(-) diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 312b1280b1..372f4f69aa 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -36,6 +36,19 @@ using webrtc::SdpType; namespace webrtc { +namespace { + +bool IsBundledButNotFirstMid( + const std::map& bundle_groups_by_mid, + const std::string& mid) { + auto it = bundle_groups_by_mid.find(mid); + if (it == bundle_groups_by_mid.end()) + return false; + return mid != *it->second->FirstContentName(); +} + +} // namespace + JsepTransportController::JsepTransportController( rtc::Thread* network_thread, cricket::PortAllocator* port_allocator, @@ -534,21 +547,32 @@ RTCError JsepTransportController::ApplyDescription_n( } RTCError error; - error = ValidateAndMaybeUpdateBundleGroup(local, type, description); + error = ValidateAndMaybeUpdateBundleGroups(local, type, description); if (!error.ok()) { return error; } + // Established BUNDLE groups by MID. + std::map + established_bundle_groups_by_mid; + 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(); + } + } - std::vector merged_encrypted_extension_ids; - if (bundle_group_) { - merged_encrypted_extension_ids = - MergeEncryptedHeaderExtensionIdsForBundle(description); + std::map> + merged_encrypted_extension_ids_by_bundle; + if (!bundle_groups_.empty()) { + merged_encrypted_extension_ids_by_bundle = + MergeEncryptedHeaderExtensionIdsForBundles( + established_bundle_groups_by_mid, description); } for (const cricket::ContentInfo& content_info : description->contents()) { - // Don't create transports for rejected m-lines and bundled m-lines." + // Don't create transports for rejected m-lines and bundled m-lines. if (content_info.rejected || - (IsBundled(content_info.name) && content_info.name != *bundled_mid())) { + IsBundledButNotFirstMid(established_bundle_groups_by_mid, + content_info.name)) { continue; } error = MaybeCreateJsepTransport(local, content_info, *description); @@ -564,14 +588,24 @@ RTCError JsepTransportController::ApplyDescription_n( const cricket::TransportInfo& transport_info = description->transport_infos()[i]; if (content_info.rejected) { - HandleRejectedContent(content_info, description); + // This may cause groups to be removed from |bundle_groups_| and + // |established_bundle_groups_by_mid|. + HandleRejectedContent(content_info, established_bundle_groups_by_mid); continue; } - if (IsBundled(content_info.name) && content_info.name != *bundled_mid()) { - if (!HandleBundledContent(content_info)) { + auto it = established_bundle_groups_by_mid.find(content_info.name); + const cricket::ContentGroup* established_bundle_group = + it != established_bundle_groups_by_mid.end() ? it->second : nullptr; + + // For bundle members that are not BUNDLE-tagged (not first in the group), + // configure their transport to be the same as the BUNDLE-tagged transport. + if (established_bundle_group && + content_info.name != *established_bundle_group->FirstContentName()) { + if (!HandleBundledContent(content_info, *established_bundle_group)) { return RTCError(RTCErrorType::INVALID_PARAMETER, - "Failed to process the bundled m= section with mid='" + + "Failed to process the bundled m= section with " + "mid='" + content_info.name + "'."); } continue; @@ -583,8 +617,13 @@ RTCError JsepTransportController::ApplyDescription_n( } std::vector extension_ids; - if (bundled_mid() && content_info.name == *bundled_mid()) { - extension_ids = merged_encrypted_extension_ids; + // Is BUNDLE-tagged (first in the group)? + if (established_bundle_group && + content_info.name == *established_bundle_group->FirstContentName()) { + auto it = merged_encrypted_extension_ids_by_bundle.find( + established_bundle_group); + RTC_DCHECK(it != merged_encrypted_extension_ids_by_bundle.end()); + extension_ids = it->second; } else { extension_ids = GetEncryptedHeaderExtensionIds(content_info); } @@ -622,51 +661,98 @@ RTCError JsepTransportController::ApplyDescription_n( return RTCError::OK(); } -RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroup( +RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroups( bool local, SdpType type, const cricket::SessionDescription* description) { RTC_DCHECK(description); - const cricket::ContentGroup* new_bundle_group = - description->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); - // The BUNDLE group containing a MID that no m= section has is invalid. - if (new_bundle_group) { + std::vector new_bundle_groups = + description->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE); + // Verify |new_bundle_groups|. + std::map new_bundle_groups_by_mid; + for (const cricket::ContentGroup* new_bundle_group : new_bundle_groups) { for (const std::string& content_name : new_bundle_group->content_names()) { + // The BUNDLE group must not contain a MID that is a member of a different + // BUNDLE group, or that contains the same MID multiple times. + if (new_bundle_groups_by_mid.find(content_name) != + new_bundle_groups_by_mid.end()) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "A BUNDLE group contains a MID='" + content_name + + "' that is already in a BUNDLE group."); + } + new_bundle_groups_by_mid.insert( + std::make_pair(content_name, new_bundle_group)); + // The BUNDLE group must not contain a MID that no m= section has. if (!description->GetContentByName(content_name)) { return RTCError(RTCErrorType::INVALID_PARAMETER, - "The BUNDLE group contains MID='" + content_name + + "A BUNDLE group contains a MID='" + content_name + "' matching no m= section."); } } } if (type == SdpType::kAnswer) { - const cricket::ContentGroup* offered_bundle_group = - local ? remote_desc_->GetGroupByName(cricket::GROUP_TYPE_BUNDLE) - : local_desc_->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); + std::vector offered_bundle_groups = + local ? remote_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE) + : local_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE); - if (new_bundle_group) { - // The BUNDLE group in answer should be a subset of offered group. + std::map + offered_bundle_groups_by_mid; + for (const cricket::ContentGroup* offered_bundle_group : + offered_bundle_groups) { + for (const std::string& content_name : + offered_bundle_group->content_names()) { + offered_bundle_groups_by_mid[content_name] = offered_bundle_group; + } + } + + std::map + new_bundle_groups_by_offered_bundle_groups; + for (const cricket::ContentGroup* new_bundle_group : new_bundle_groups) { + if (!new_bundle_group->FirstContentName()) { + // Empty groups could be a subset of any group. + continue; + } + // The group in the answer (new_bundle_group) must have a corresponding + // group in the offer (original_group), because the answer groups may only + // be subsets of the offer groups. + auto it = offered_bundle_groups_by_mid.find( + *new_bundle_group->FirstContentName()); + if (it == offered_bundle_groups_by_mid.end()) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "A BUNDLE group was added in the answer that did not " + "exist in the offer."); + } + const cricket::ContentGroup* offered_bundle_group = it->second; + if (new_bundle_groups_by_offered_bundle_groups.find( + offered_bundle_group) != + new_bundle_groups_by_offered_bundle_groups.end()) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "A MID in the answer has changed group."); + } + new_bundle_groups_by_offered_bundle_groups.insert( + std::make_pair(offered_bundle_group, new_bundle_group)); for (const std::string& content_name : new_bundle_group->content_names()) { - if (!offered_bundle_group || - !offered_bundle_group->HasContentName(content_name)) { + it = offered_bundle_groups_by_mid.find(content_name); + // The BUNDLE group in answer should be a subset of offered group. + if (it == offered_bundle_groups_by_mid.end() || + it->second != offered_bundle_group) { return RTCError(RTCErrorType::INVALID_PARAMETER, - "The BUNDLE group in answer contains a MID='" + + "A BUNDLE group in answer contains a MID='" + content_name + - "' that was " - "not in the offered group."); + "' that was not in the offered group."); } } } - if (bundle_group_) { - for (const std::string& content_name : bundle_group_->content_names()) { + for (const auto& bundle_group : bundle_groups_) { + for (const std::string& content_name : bundle_group->content_names()) { // An answer that removes m= sections from pre-negotiated BUNDLE group // without rejecting it, is invalid. - if (!new_bundle_group || - !new_bundle_group->HasContentName(content_name)) { + auto it = new_bundle_groups_by_mid.find(content_name); + if (it == new_bundle_groups_by_mid.end()) { auto* content_info = description->GetContentByName(content_name); if (!content_info || !content_info->rejected) { return RTCError(RTCErrorType::INVALID_PARAMETER, @@ -687,33 +773,39 @@ RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroup( } if (ShouldUpdateBundleGroup(type, description)) { - bundle_group_ = *new_bundle_group; - } - - if (!bundled_mid()) { - return RTCError::OK(); - } - - auto bundled_content = description->GetContentByName(*bundled_mid()); - if (!bundled_content) { - return RTCError( - RTCErrorType::INVALID_PARAMETER, - "An m= section associated with the BUNDLE-tag doesn't exist."); - } - - // If the |bundled_content| is rejected, other contents in the bundle group - // should be rejected. - if (bundled_content->rejected) { - for (const auto& content_name : bundle_group_->content_names()) { - auto other_content = description->GetContentByName(content_name); - if (!other_content->rejected) { - return RTCError(RTCErrorType::INVALID_PARAMETER, - "The m= section with mid='" + content_name + - "' should be rejected."); - } + bundle_groups_.clear(); + for (const cricket::ContentGroup* new_bundle_group : new_bundle_groups) { + bundle_groups_.push_back( + std::make_unique(*new_bundle_group)); } } + for (const auto& bundle_group : bundle_groups_) { + if (!bundle_group->FirstContentName()) + continue; + + // The first MID in a BUNDLE group is BUNDLE-tagged. + auto bundled_content = + description->GetContentByName(*bundle_group->FirstContentName()); + if (!bundled_content) { + return RTCError( + RTCErrorType::INVALID_PARAMETER, + "An m= section associated with the BUNDLE-tag doesn't exist."); + } + + // If the |bundled_content| is rejected, other contents in the bundle group + // must also be rejected. + if (bundled_content->rejected) { + for (const auto& content_name : bundle_group->content_names()) { + auto other_content = description->GetContentByName(content_name); + if (!other_content->rejected) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "The m= section with mid='" + content_name + + "' should be rejected."); + } + } + } + } return RTCError::OK(); } @@ -733,30 +825,49 @@ RTCError JsepTransportController::ValidateContent( void JsepTransportController::HandleRejectedContent( const cricket::ContentInfo& content_info, - const cricket::SessionDescription* description) { + std::map& + established_bundle_groups_by_mid) { // If the content is rejected, let the // BaseChannel/SctpTransport change the RtpTransport/DtlsTransport first, // then destroy the cricket::JsepTransport. - RemoveTransportForMid(content_info.name); - if (content_info.name == bundled_mid()) { - for (const auto& content_name : bundle_group_->content_names()) { + auto it = established_bundle_groups_by_mid.find(content_info.name); + cricket::ContentGroup* bundle_group = + it != established_bundle_groups_by_mid.end() ? it->second : nullptr; + if (bundle_group && !bundle_group->content_names().empty() && + content_info.name == *bundle_group->FirstContentName()) { + // Rejecting a BUNDLE group's first mid means we are rejecting the entire + // group. + for (const auto& content_name : bundle_group->content_names()) { RemoveTransportForMid(content_name); + // We are about to delete this BUNDLE group, erase all mappings to it. + it = established_bundle_groups_by_mid.find(content_name); + RTC_DCHECK(it != established_bundle_groups_by_mid.end()); + established_bundle_groups_by_mid.erase(it); } - bundle_group_.reset(); - } else if (IsBundled(content_info.name)) { - // Remove the rejected content from the |bundle_group_|. - bundle_group_->RemoveContentName(content_info.name); - // Reset the bundle group if nothing left. - if (!bundle_group_->FirstContentName()) { - bundle_group_.reset(); + // Delete the BUNDLE group. + auto bundle_group_it = std::find_if( + bundle_groups_.begin(), bundle_groups_.end(), + [bundle_group](std::unique_ptr& group) { + return bundle_group == group.get(); + }); + RTC_DCHECK(bundle_group_it != bundle_groups_.end()); + bundle_groups_.erase(bundle_group_it); + } else { + RemoveTransportForMid(content_info.name); + if (bundle_group) { + // Remove the rejected content from the |bundle_group|. + bundle_group->RemoveContentName(content_info.name); } } MaybeDestroyJsepTransport(content_info.name); } bool JsepTransportController::HandleBundledContent( - const cricket::ContentInfo& content_info) { - auto jsep_transport = GetJsepTransportByName(*bundled_mid()); + const cricket::ContentInfo& content_info, + const cricket::ContentGroup& bundle_group) { + RTC_DCHECK(bundle_group.FirstContentName()); + auto jsep_transport = + GetJsepTransportByName(*bundle_group.FirstContentName()); RTC_DCHECK(jsep_transport); // If the content is bundled, let the // BaseChannel/SctpTransport change the RtpTransport/DtlsTransport first, @@ -837,11 +948,11 @@ bool JsepTransportController::ShouldUpdateBundleGroup( } RTC_DCHECK(local_desc_ && remote_desc_); - const cricket::ContentGroup* local_bundle = - local_desc_->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); - const cricket::ContentGroup* remote_bundle = - remote_desc_->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); - return local_bundle && remote_bundle; + std::vector local_bundles = + local_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE); + std::vector remote_bundles = + remote_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE); + return !local_bundles.empty() && !remote_bundles.empty(); } std::vector JsepTransportController::GetEncryptedHeaderExtensionIds( @@ -865,26 +976,32 @@ std::vector JsepTransportController::GetEncryptedHeaderExtensionIds( return encrypted_header_extension_ids; } -std::vector -JsepTransportController::MergeEncryptedHeaderExtensionIdsForBundle( +std::map> +JsepTransportController::MergeEncryptedHeaderExtensionIdsForBundles( + const std::map& bundle_groups_by_mid, const cricket::SessionDescription* description) { RTC_DCHECK(description); - RTC_DCHECK(bundle_group_); - - std::vector merged_ids; + RTC_DCHECK(!bundle_groups_.empty()); + std::map> + merged_encrypted_extension_ids_by_bundle; // Union the encrypted header IDs in the group when bundle is enabled. for (const cricket::ContentInfo& content_info : description->contents()) { - if (bundle_group_->HasContentName(content_info.name)) { - std::vector extension_ids = - GetEncryptedHeaderExtensionIds(content_info); - for (int id : extension_ids) { - if (!absl::c_linear_search(merged_ids, id)) { - merged_ids.push_back(id); - } + auto it = bundle_groups_by_mid.find(content_info.name); + if (it == bundle_groups_by_mid.end()) + continue; + // Get or create list of IDs for the BUNDLE group. + std::vector& merged_ids = + merged_encrypted_extension_ids_by_bundle[it->second]; + // Add IDs not already in the list. + std::vector extension_ids = + GetEncryptedHeaderExtensionIds(content_info); + for (int id : extension_ids) { + if (!absl::c_linear_search(merged_ids, id)) { + merged_ids.push_back(id); } } } - return merged_ids; + return merged_encrypted_extension_ids_by_bundle; } int JsepTransportController::GetRtpAbsSendTimeHeaderExtensionId( diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index 568058571f..e3c1187fb4 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h @@ -321,16 +321,18 @@ class JsepTransportController : public sigslot::has_slots<> { SdpType type, const cricket::SessionDescription* description) RTC_RUN_ON(network_thread_); - RTCError ValidateAndMaybeUpdateBundleGroup( + RTCError ValidateAndMaybeUpdateBundleGroups( bool local, SdpType type, const cricket::SessionDescription* description); RTCError ValidateContent(const cricket::ContentInfo& content_info); void HandleRejectedContent(const cricket::ContentInfo& content_info, - const cricket::SessionDescription* description) + std::map& + established_bundle_groups_by_mid) RTC_RUN_ON(network_thread_); - bool HandleBundledContent(const cricket::ContentInfo& content_info) + bool HandleBundledContent(const cricket::ContentInfo& content_info, + const cricket::ContentGroup& bundle_group) RTC_RUN_ON(network_thread_); bool SetTransportForMid(const std::string& mid, @@ -343,22 +345,12 @@ class JsepTransportController : public sigslot::has_slots<> { const std::vector& encrypted_extension_ids, int rtp_abs_sendtime_extn_id); - absl::optional bundled_mid() const { - absl::optional bundled_mid; - if (bundle_group_ && bundle_group_->FirstContentName()) { - bundled_mid = *(bundle_group_->FirstContentName()); - } - return bundled_mid; - } - - bool IsBundled(const std::string& mid) const { - return bundle_group_ && bundle_group_->HasContentName(mid); - } - bool ShouldUpdateBundleGroup(SdpType type, const cricket::SessionDescription* description); - std::vector MergeEncryptedHeaderExtensionIdsForBundle( + std::map> + MergeEncryptedHeaderExtensionIdsForBundles( + const std::map& bundle_groups_by_mid, const cricket::SessionDescription* description); std::vector GetEncryptedHeaderExtensionIds( const cricket::ContentInfo& content_info); @@ -491,7 +483,8 @@ class JsepTransportController : public sigslot::has_slots<> { const cricket::SessionDescription* remote_desc_ = nullptr; absl::optional initial_offerer_; - absl::optional bundle_group_; + // Use unique_ptr<> to get a stable address. + std::vector> bundle_groups_; cricket::IceConfig ice_config_; cricket::IceRole ice_role_ = cricket::ICEROLE_CONTROLLING; diff --git a/pc/jsep_transport_controller_unittest.cc b/pc/jsep_transport_controller_unittest.cc index 674ac227f9..5c621fdee3 100644 --- a/pc/jsep_transport_controller_unittest.cc +++ b/pc/jsep_transport_controller_unittest.cc @@ -33,6 +33,8 @@ static const char kIceUfrag2[] = "u0002"; static const char kIcePwd2[] = "TESTICEPWD00000000000002"; static const char kIceUfrag3[] = "u0003"; static const char kIcePwd3[] = "TESTICEPWD00000000000003"; +static const char kIceUfrag4[] = "u0004"; +static const char kIcePwd4[] = "TESTICEPWD00000000000004"; static const char kAudioMid1[] = "audio1"; static const char kAudioMid2[] = "audio2"; static const char kVideoMid1[] = "video1"; @@ -1099,6 +1101,512 @@ TEST_F(JsepTransportControllerTest, MultipleMediaSectionsOfSameTypeWithBundle) { ASSERT_TRUE(it2 != changed_dtls_transport_by_mid_.end()); } +TEST_F(JsepTransportControllerTest, MultipleBundleGroups) { + static const char kMid1Audio[] = "1_audio"; + static const char kMid2Video[] = "2_video"; + static const char kMid3Audio[] = "3_audio"; + static const char kMid4Video[] = "4_video"; + + CreateJsepTransportController(JsepTransportController::Config()); + cricket::ContentGroup bundle_group1(cricket::GROUP_TYPE_BUNDLE); + bundle_group1.AddContentName(kMid1Audio); + bundle_group1.AddContentName(kMid2Video); + cricket::ContentGroup bundle_group2(cricket::GROUP_TYPE_BUNDLE); + bundle_group2.AddContentName(kMid3Audio); + bundle_group2.AddContentName(kMid4Video); + + auto local_offer = std::make_unique(); + AddAudioSection(local_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid2Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(local_offer.get(), kMid3Audio, kIceUfrag3, kIcePwd3, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid4Video, kIceUfrag4, kIcePwd4, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + local_offer->AddGroup(bundle_group1); + local_offer->AddGroup(bundle_group2); + + auto remote_answer = std::make_unique(); + AddAudioSection(remote_answer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(remote_answer.get(), kMid2Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(remote_answer.get(), kMid3Audio, kIceUfrag3, kIcePwd3, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(remote_answer.get(), kMid4Video, kIceUfrag4, kIcePwd4, + 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()); + + // Verify that (kMid1Audio,kMid2Video) and (kMid3Audio,kMid4Video) form two + // distinct bundled groups. + auto mid1_transport = transport_controller_->GetRtpTransport(kMid1Audio); + auto mid2_transport = transport_controller_->GetRtpTransport(kMid2Video); + auto mid3_transport = transport_controller_->GetRtpTransport(kMid3Audio); + auto mid4_transport = transport_controller_->GetRtpTransport(kMid4Video); + EXPECT_EQ(mid1_transport, mid2_transport); + EXPECT_EQ(mid3_transport, mid4_transport); + EXPECT_NE(mid1_transport, mid3_transport); + + auto it = changed_rtp_transport_by_mid_.find(kMid1Audio); + ASSERT_TRUE(it != changed_rtp_transport_by_mid_.end()); + EXPECT_EQ(it->second, mid1_transport); + + it = changed_rtp_transport_by_mid_.find(kMid2Video); + ASSERT_TRUE(it != changed_rtp_transport_by_mid_.end()); + EXPECT_EQ(it->second, mid2_transport); + + it = changed_rtp_transport_by_mid_.find(kMid3Audio); + ASSERT_TRUE(it != changed_rtp_transport_by_mid_.end()); + EXPECT_EQ(it->second, mid3_transport); + + it = changed_rtp_transport_by_mid_.find(kMid4Video); + ASSERT_TRUE(it != changed_rtp_transport_by_mid_.end()); + EXPECT_EQ(it->second, mid4_transport); +} + +TEST_F(JsepTransportControllerTest, + MultipleBundleGroupsInOfferButOnlyASingleGroupInAnswer) { + static const char kMid1Audio[] = "1_audio"; + static const char kMid2Video[] = "2_video"; + static const char kMid3Audio[] = "3_audio"; + static const char kMid4Video[] = "4_video"; + + CreateJsepTransportController(JsepTransportController::Config()); + cricket::ContentGroup bundle_group1(cricket::GROUP_TYPE_BUNDLE); + bundle_group1.AddContentName(kMid1Audio); + bundle_group1.AddContentName(kMid2Video); + cricket::ContentGroup bundle_group2(cricket::GROUP_TYPE_BUNDLE); + bundle_group2.AddContentName(kMid3Audio); + bundle_group2.AddContentName(kMid4Video); + + auto local_offer = std::make_unique(); + AddAudioSection(local_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid2Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(local_offer.get(), kMid3Audio, kIceUfrag3, kIcePwd3, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid4Video, kIceUfrag4, kIcePwd4, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + // The offer has both groups. + local_offer->AddGroup(bundle_group1); + local_offer->AddGroup(bundle_group2); + + auto remote_answer = std::make_unique(); + AddAudioSection(remote_answer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(remote_answer.get(), kMid2Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(remote_answer.get(), kMid3Audio, kIceUfrag3, kIcePwd3, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(remote_answer.get(), kMid4Video, kIceUfrag4, kIcePwd4, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + // The answer only has a single group! This is what happens when talking to an + // endpoint that does not have support for multiple BUNDLE groups. + remote_answer->AddGroup(bundle_group1); + + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_offer.get()) + .ok()); + EXPECT_TRUE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get()) + .ok()); + + // Verify that (kMid1Audio,kMid2Video) form a bundle group, but that + // kMid3Audio and kMid4Video are unbundled. + auto mid1_transport = transport_controller_->GetRtpTransport(kMid1Audio); + auto mid2_transport = transport_controller_->GetRtpTransport(kMid2Video); + auto mid3_transport = transport_controller_->GetRtpTransport(kMid3Audio); + auto mid4_transport = transport_controller_->GetRtpTransport(kMid4Video); + EXPECT_EQ(mid1_transport, mid2_transport); + EXPECT_NE(mid3_transport, mid4_transport); + EXPECT_NE(mid1_transport, mid3_transport); + EXPECT_NE(mid1_transport, mid4_transport); +} + +TEST_F(JsepTransportControllerTest, MultipleBundleGroupsIllegallyChangeGroup) { + static const char kMid1Audio[] = "1_audio"; + static const char kMid2Video[] = "2_video"; + static const char kMid3Audio[] = "3_audio"; + static const char kMid4Video[] = "4_video"; + + CreateJsepTransportController(JsepTransportController::Config()); + // Offer groups (kMid1Audio,kMid2Video) and (kMid3Audio,kMid4Video). + cricket::ContentGroup offer_bundle_group1(cricket::GROUP_TYPE_BUNDLE); + offer_bundle_group1.AddContentName(kMid1Audio); + offer_bundle_group1.AddContentName(kMid2Video); + cricket::ContentGroup offer_bundle_group2(cricket::GROUP_TYPE_BUNDLE); + offer_bundle_group2.AddContentName(kMid3Audio); + offer_bundle_group2.AddContentName(kMid4Video); + // Answer groups (kMid1Audio,kMid4Video) and (kMid3Audio,kMid2Video), i.e. the + // second group members have switched places. This should get rejected. + cricket::ContentGroup answer_bundle_group1(cricket::GROUP_TYPE_BUNDLE); + answer_bundle_group1.AddContentName(kMid1Audio); + answer_bundle_group1.AddContentName(kMid4Video); + cricket::ContentGroup answer_bundle_group2(cricket::GROUP_TYPE_BUNDLE); + answer_bundle_group2.AddContentName(kMid3Audio); + answer_bundle_group2.AddContentName(kMid2Video); + + auto local_offer = std::make_unique(); + AddAudioSection(local_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid2Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(local_offer.get(), kMid3Audio, kIceUfrag3, kIcePwd3, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid4Video, kIceUfrag4, kIcePwd4, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + local_offer->AddGroup(offer_bundle_group1); + local_offer->AddGroup(offer_bundle_group2); + + auto remote_answer = std::make_unique(); + AddAudioSection(remote_answer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(remote_answer.get(), kMid2Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(remote_answer.get(), kMid3Audio, kIceUfrag3, kIcePwd3, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(remote_answer.get(), kMid4Video, kIceUfrag4, kIcePwd4, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + remote_answer->AddGroup(answer_bundle_group1); + remote_answer->AddGroup(answer_bundle_group2); + + // Accept offer. + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_offer.get()) + .ok()); + // Reject answer! + EXPECT_FALSE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get()) + .ok()); +} + +TEST_F(JsepTransportControllerTest, MultipleBundleGroupsInvalidSubsets) { + static const char kMid1Audio[] = "1_audio"; + static const char kMid2Video[] = "2_video"; + static const char kMid3Audio[] = "3_audio"; + static const char kMid4Video[] = "4_video"; + + CreateJsepTransportController(JsepTransportController::Config()); + // Offer groups (kMid1Audio,kMid2Video) and (kMid3Audio,kMid4Video). + cricket::ContentGroup offer_bundle_group1(cricket::GROUP_TYPE_BUNDLE); + offer_bundle_group1.AddContentName(kMid1Audio); + offer_bundle_group1.AddContentName(kMid2Video); + cricket::ContentGroup offer_bundle_group2(cricket::GROUP_TYPE_BUNDLE); + offer_bundle_group2.AddContentName(kMid3Audio); + offer_bundle_group2.AddContentName(kMid4Video); + // Answer groups (kMid1Audio) and (kMid2Video), i.e. the second group was + // moved from the first group. This should get rejected. + cricket::ContentGroup answer_bundle_group1(cricket::GROUP_TYPE_BUNDLE); + answer_bundle_group1.AddContentName(kMid1Audio); + cricket::ContentGroup answer_bundle_group2(cricket::GROUP_TYPE_BUNDLE); + answer_bundle_group2.AddContentName(kMid2Video); + + auto local_offer = std::make_unique(); + AddAudioSection(local_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid2Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(local_offer.get(), kMid3Audio, kIceUfrag3, kIcePwd3, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(local_offer.get(), kMid4Video, kIceUfrag4, kIcePwd4, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + local_offer->AddGroup(offer_bundle_group1); + local_offer->AddGroup(offer_bundle_group2); + + auto remote_answer = std::make_unique(); + AddAudioSection(remote_answer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(remote_answer.get(), kMid2Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(remote_answer.get(), kMid3Audio, kIceUfrag3, kIcePwd3, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(remote_answer.get(), kMid4Video, kIceUfrag4, kIcePwd4, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + remote_answer->AddGroup(answer_bundle_group1); + remote_answer->AddGroup(answer_bundle_group2); + + // Accept offer. + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_offer.get()) + .ok()); + // Reject answer! + EXPECT_FALSE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get()) + .ok()); +} + +TEST_F(JsepTransportControllerTest, MultipleBundleGroupsInvalidOverlap) { + static const char kMid1Audio[] = "1_audio"; + static const char kMid2Video[] = "2_video"; + static const char kMid3Audio[] = "3_audio"; + + CreateJsepTransportController(JsepTransportController::Config()); + // Offer groups (kMid1Audio,kMid3Audio) and (kMid2Video,kMid3Audio), i.e. + // kMid3Audio is in both groups - this is illegal. + cricket::ContentGroup offer_bundle_group1(cricket::GROUP_TYPE_BUNDLE); + offer_bundle_group1.AddContentName(kMid1Audio); + offer_bundle_group1.AddContentName(kMid3Audio); + cricket::ContentGroup offer_bundle_group2(cricket::GROUP_TYPE_BUNDLE); + offer_bundle_group2.AddContentName(kMid2Video); + offer_bundle_group2.AddContentName(kMid3Audio); + + auto offer = std::make_unique(); + AddAudioSection(offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddVideoSection(offer.get(), kMid2Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(offer.get(), kMid3Audio, kIceUfrag3, kIcePwd3, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + offer->AddGroup(offer_bundle_group1); + offer->AddGroup(offer_bundle_group2); + + // Reject offer, both if set as local or remote. + EXPECT_FALSE( + transport_controller_->SetLocalDescription(SdpType::kOffer, offer.get()) + .ok()); + EXPECT_FALSE( + transport_controller_->SetRemoteDescription(SdpType::kOffer, offer.get()) + .ok()); +} + +TEST_F(JsepTransportControllerTest, MultipleBundleGroupsUnbundleFirstMid) { + 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()); + // Offer groups (kMid1Audio,kMid2Audio,kMid3Audio) and + // (kMid4Video,kMid5Video,kMid6Video). + cricket::ContentGroup offer_bundle_group1(cricket::GROUP_TYPE_BUNDLE); + offer_bundle_group1.AddContentName(kMid1Audio); + offer_bundle_group1.AddContentName(kMid2Audio); + offer_bundle_group1.AddContentName(kMid3Audio); + cricket::ContentGroup offer_bundle_group2(cricket::GROUP_TYPE_BUNDLE); + offer_bundle_group2.AddContentName(kMid4Video); + offer_bundle_group2.AddContentName(kMid5Video); + offer_bundle_group2.AddContentName(kMid6Video); + // Answer groups (kMid2Audio,kMid3Audio) and (kMid5Video,kMid6Video), i.e. + // we've moved the first MIDs out of the groups. + cricket::ContentGroup answer_bundle_group1(cricket::GROUP_TYPE_BUNDLE); + answer_bundle_group1.AddContentName(kMid2Audio); + answer_bundle_group1.AddContentName(kMid3Audio); + cricket::ContentGroup answer_bundle_group2(cricket::GROUP_TYPE_BUNDLE); + answer_bundle_group2.AddContentName(kMid5Video); + answer_bundle_group2.AddContentName(kMid6Video); + + auto local_offer = std::make_unique(); + AddAudioSection(local_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(local_offer.get(), kMid2Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(local_offer.get(), kMid3Audio, 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); + AddVideoSection(local_offer.get(), kMid6Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + local_offer->AddGroup(offer_bundle_group1); + local_offer->AddGroup(offer_bundle_group2); + + auto remote_answer = std::make_unique(); + AddAudioSection(remote_answer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(remote_answer.get(), kMid2Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(remote_answer.get(), kMid3Audio, 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); + AddVideoSection(remote_answer.get(), kMid6Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + remote_answer->AddGroup(answer_bundle_group1); + remote_answer->AddGroup(answer_bundle_group2); + + 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); + 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, mid2_transport); + EXPECT_EQ(mid2_transport, mid3_transport); + EXPECT_NE(mid4_transport, mid5_transport); + EXPECT_EQ(mid5_transport, mid6_transport); + EXPECT_NE(mid1_transport, mid4_transport); + EXPECT_NE(mid2_transport, mid5_transport); +} + +TEST_F(JsepTransportControllerTest, MultipleBundleGroupsChangeFirstMid) { + 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()); + // Offer groups (kMid1Audio,kMid2Audio,kMid3Audio) and + // (kMid4Video,kMid5Video,kMid6Video). + cricket::ContentGroup offer_bundle_group1(cricket::GROUP_TYPE_BUNDLE); + offer_bundle_group1.AddContentName(kMid1Audio); + offer_bundle_group1.AddContentName(kMid2Audio); + offer_bundle_group1.AddContentName(kMid3Audio); + cricket::ContentGroup offer_bundle_group2(cricket::GROUP_TYPE_BUNDLE); + offer_bundle_group2.AddContentName(kMid4Video); + offer_bundle_group2.AddContentName(kMid5Video); + offer_bundle_group2.AddContentName(kMid6Video); + // Answer groups (kMid2Audio,kMid1Audio,kMid3Audio) and + // (kMid5Video,kMid6Video,kMid4Video), i.e. we've changed which MID is first + // but accept the whole group. + cricket::ContentGroup answer_bundle_group1(cricket::GROUP_TYPE_BUNDLE); + answer_bundle_group1.AddContentName(kMid2Audio); + answer_bundle_group1.AddContentName(kMid1Audio); + answer_bundle_group1.AddContentName(kMid3Audio); + cricket::ContentGroup answer_bundle_group2(cricket::GROUP_TYPE_BUNDLE); + answer_bundle_group2.AddContentName(kMid5Video); + answer_bundle_group2.AddContentName(kMid6Video); + answer_bundle_group2.AddContentName(kMid4Video); + + auto local_offer = std::make_unique(); + AddAudioSection(local_offer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(local_offer.get(), kMid2Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(local_offer.get(), kMid3Audio, 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); + AddVideoSection(local_offer.get(), kMid6Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + local_offer->AddGroup(offer_bundle_group1); + local_offer->AddGroup(offer_bundle_group2); + + auto remote_answer = std::make_unique(); + AddAudioSection(remote_answer.get(), kMid1Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(remote_answer.get(), kMid2Audio, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + AddAudioSection(remote_answer.get(), kMid3Audio, 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); + AddVideoSection(remote_answer.get(), kMid6Video, kIceUfrag2, kIcePwd2, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + remote_answer->AddGroup(answer_bundle_group1); + remote_answer->AddGroup(answer_bundle_group2); + + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_offer.get()) + .ok()); + + // The fact that we accept this answer is actually a bug. If we accept the + // first MID to be in the group, we should also accept that it is the tagged + // one. + // TODO(https://crbug.com/webrtc/12699): When this issue is fixed, change this + // to EXPECT_FALSE and remove the below expectations about transports. + 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); + 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); +} + // Tests that only a subset of all the m= sections are bundled. TEST_F(JsepTransportControllerTest, BundleSubsetOfMediaSections) { CreateJsepTransportController(JsepTransportController::Config()); diff --git a/pc/media_session.cc b/pc/media_session.cc index 2e779bd7b1..c08d5393f3 100644 --- a/pc/media_session.cc +++ b/pc/media_session.cc @@ -1673,10 +1673,19 @@ MediaSessionDescriptionFactory::CreateAnswer( // If the offer supports BUNDLE, and we want to use it too, create a BUNDLE // group in the answer with the appropriate content names. - const ContentGroup* offer_bundle = offer->GetGroupByName(GROUP_TYPE_BUNDLE); - ContentGroup answer_bundle(GROUP_TYPE_BUNDLE); - // Transport info shared by the bundle group. - std::unique_ptr bundle_transport; + std::vector offer_bundles = + offer->GetGroupsByName(GROUP_TYPE_BUNDLE); + // There are as many answer BUNDLE groups as offer BUNDLE groups (even if + // rejected, we respond with an empty group). |offer_bundles|, + // |answer_bundles| and |bundle_transports| share the same size and indices. + std::vector answer_bundles; + std::vector> bundle_transports; + answer_bundles.reserve(offer_bundles.size()); + bundle_transports.reserve(offer_bundles.size()); + for (size_t i = 0; i < offer_bundles.size(); ++i) { + answer_bundles.emplace_back(GROUP_TYPE_BUNDLE); + bundle_transports.emplace_back(nullptr); + } answer->set_extmap_allow_mixed(offer->extmap_allow_mixed()); @@ -1691,6 +1700,18 @@ MediaSessionDescriptionFactory::CreateAnswer( RTC_DCHECK( IsMediaContentOfType(offer_content, media_description_options.type)); RTC_DCHECK(media_description_options.mid == offer_content->name); + // Get the index of the BUNDLE group that this MID belongs to, if any. + absl::optional bundle_index; + for (size_t i = 0; i < offer_bundles.size(); ++i) { + if (offer_bundles[i]->HasContentName(media_description_options.mid)) { + bundle_index = i; + break; + } + } + TransportInfo* bundle_transport = + bundle_index.has_value() ? bundle_transports[bundle_index.value()].get() + : nullptr; + const ContentInfo* current_content = nullptr; if (current_description && msection_index < current_description->contents().size()) { @@ -1703,35 +1724,34 @@ MediaSessionDescriptionFactory::CreateAnswer( case MEDIA_TYPE_AUDIO: if (!AddAudioContentForAnswer( media_description_options, session_options, offer_content, - offer, current_content, current_description, - bundle_transport.get(), answer_audio_codecs, header_extensions, - ¤t_streams, answer.get(), &ice_credentials)) { + offer, current_content, current_description, bundle_transport, + answer_audio_codecs, header_extensions, ¤t_streams, + answer.get(), &ice_credentials)) { return nullptr; } break; case MEDIA_TYPE_VIDEO: if (!AddVideoContentForAnswer( media_description_options, session_options, offer_content, - offer, current_content, current_description, - bundle_transport.get(), answer_video_codecs, header_extensions, - ¤t_streams, answer.get(), &ice_credentials)) { + offer, current_content, current_description, bundle_transport, + answer_video_codecs, header_extensions, ¤t_streams, + answer.get(), &ice_credentials)) { return nullptr; } break; case MEDIA_TYPE_DATA: - if (!AddDataContentForAnswer(media_description_options, session_options, - offer_content, offer, current_content, - current_description, - bundle_transport.get(), ¤t_streams, - answer.get(), &ice_credentials)) { + if (!AddDataContentForAnswer( + media_description_options, session_options, offer_content, + offer, current_content, current_description, bundle_transport, + ¤t_streams, answer.get(), &ice_credentials)) { return nullptr; } break; case MEDIA_TYPE_UNSUPPORTED: if (!AddUnsupportedContentForAnswer( media_description_options, session_options, offer_content, - offer, current_content, current_description, - bundle_transport.get(), answer.get(), &ice_credentials)) { + offer, current_content, current_description, bundle_transport, + answer.get(), &ice_credentials)) { return nullptr; } break; @@ -1742,37 +1762,41 @@ MediaSessionDescriptionFactory::CreateAnswer( // See if we can add the newly generated m= section to the BUNDLE group in // the answer. ContentInfo& added = answer->contents().back(); - if (!added.rejected && session_options.bundle_enabled && offer_bundle && - offer_bundle->HasContentName(added.name)) { - answer_bundle.AddContentName(added.name); - bundle_transport.reset( + if (!added.rejected && session_options.bundle_enabled && + bundle_index.has_value()) { + // The |bundle_index| is for |media_description_options.mid|. + RTC_DCHECK_EQ(media_description_options.mid, added.name); + answer_bundles[bundle_index.value()].AddContentName(added.name); + bundle_transports[bundle_index.value()].reset( new TransportInfo(*answer->GetTransportInfoByName(added.name))); } } - // If a BUNDLE group was offered, put a BUNDLE group in the answer even if - // it's empty. RFC5888 says: + // If BUNDLE group(s) were offered, put the same number of BUNDLE groups in + // the answer even if they're empty. RFC5888 says: // // A SIP entity that receives an offer that contains an "a=group" line // with semantics that are understood MUST return an answer that // contains an "a=group" line with the same semantics. - if (offer_bundle) { - answer->AddGroup(answer_bundle); - } + if (!offer_bundles.empty()) { + for (const ContentGroup& answer_bundle : answer_bundles) { + answer->AddGroup(answer_bundle); - if (answer_bundle.FirstContentName()) { - // Share the same ICE credentials and crypto params across all contents, - // as BUNDLE requires. - if (!UpdateTransportInfoForBundle(answer_bundle, answer.get())) { - RTC_LOG(LS_ERROR) - << "CreateAnswer failed to UpdateTransportInfoForBundle."; - return NULL; - } + if (answer_bundle.FirstContentName()) { + // Share the same ICE credentials and crypto params across all contents, + // as BUNDLE requires. + if (!UpdateTransportInfoForBundle(answer_bundle, answer.get())) { + RTC_LOG(LS_ERROR) + << "CreateAnswer failed to UpdateTransportInfoForBundle."; + return NULL; + } - if (!UpdateCryptoParamsForBundle(answer_bundle, answer.get())) { - RTC_LOG(LS_ERROR) - << "CreateAnswer failed to UpdateCryptoParamsForBundle."; - return NULL; + if (!UpdateCryptoParamsForBundle(answer_bundle, answer.get())) { + RTC_LOG(LS_ERROR) + << "CreateAnswer failed to UpdateCryptoParamsForBundle."; + return NULL; + } + } } } diff --git a/pc/media_session_unittest.cc b/pc/media_session_unittest.cc index 6d914f9b81..099195f501 100644 --- a/pc/media_session_unittest.cc +++ b/pc/media_session_unittest.cc @@ -1036,6 +1036,66 @@ TEST_F(MediaSessionDescriptionFactoryTest, ReAnswerChangedBundleOffererTagged) { EXPECT_TRUE(bundle_group->HasContentName("video")); } +TEST_F(MediaSessionDescriptionFactoryTest, + CreateAnswerForOfferWithMultipleBundleGroups) { + // Create an offer with 4 m= sections, initially without BUNDLE groups. + MediaSessionOptions opts; + opts.bundle_enabled = false; + AddMediaDescriptionOptions(MEDIA_TYPE_AUDIO, "1", + RtpTransceiverDirection::kSendRecv, kActive, + &opts); + AddMediaDescriptionOptions(MEDIA_TYPE_AUDIO, "2", + RtpTransceiverDirection::kSendRecv, kActive, + &opts); + AddMediaDescriptionOptions(MEDIA_TYPE_AUDIO, "3", + RtpTransceiverDirection::kSendRecv, kActive, + &opts); + AddMediaDescriptionOptions(MEDIA_TYPE_AUDIO, "4", + RtpTransceiverDirection::kSendRecv, kActive, + &opts); + std::unique_ptr offer = f1_.CreateOffer(opts, nullptr); + ASSERT_TRUE(offer->groups().empty()); + + // Munge the offer to have two groups. Offers like these cannot be generated + // without munging, but it is valid to receive such offers from remote + // endpoints. + cricket::ContentGroup bundle_group1(cricket::GROUP_TYPE_BUNDLE); + bundle_group1.AddContentName("1"); + bundle_group1.AddContentName("2"); + cricket::ContentGroup bundle_group2(cricket::GROUP_TYPE_BUNDLE); + bundle_group2.AddContentName("3"); + bundle_group2.AddContentName("4"); + offer->AddGroup(bundle_group1); + offer->AddGroup(bundle_group2); + + // If BUNDLE is enabled, the answer to this offer should accept both BUNDLE + // groups. + opts.bundle_enabled = true; + std::unique_ptr answer = + f2_.CreateAnswer(offer.get(), opts, nullptr); + + std::vector answer_groups = + answer->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE); + ASSERT_EQ(answer_groups.size(), 2u); + EXPECT_EQ(answer_groups[0]->content_names().size(), 2u); + EXPECT_TRUE(answer_groups[0]->HasContentName("1")); + EXPECT_TRUE(answer_groups[0]->HasContentName("2")); + EXPECT_EQ(answer_groups[1]->content_names().size(), 2u); + EXPECT_TRUE(answer_groups[1]->HasContentName("3")); + EXPECT_TRUE(answer_groups[1]->HasContentName("4")); + + // If BUNDLE is disabled, the answer to this offer should reject both BUNDLE + // groups. + opts.bundle_enabled = false; + answer = f2_.CreateAnswer(offer.get(), opts, nullptr); + + answer_groups = answer->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE); + // Rejected groups are still listed, but they are empty. + ASSERT_EQ(answer_groups.size(), 2u); + EXPECT_TRUE(answer_groups[0]->content_names().empty()); + EXPECT_TRUE(answer_groups[1]->content_names().empty()); +} + // Test that if the BUNDLE offerer-tagged media section is changed in a reoffer // and there is still a non-rejected media section that was in the initial // offer, then the ICE credentials do not change in the reoffer offerer-tagged diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 9793336d7e..7177764f29 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -2413,21 +2413,20 @@ void PeerConnection::TeardownDataChannelTransport_n() { } // Returns false if bundle is enabled and rtcp_mux is disabled. -bool PeerConnection::ValidateBundleSettings(const SessionDescription* desc) { - bool bundle_enabled = desc->HasGroup(cricket::GROUP_TYPE_BUNDLE); - if (!bundle_enabled) +bool PeerConnection::ValidateBundleSettings( + const SessionDescription* desc, + const std::map& + bundle_groups_by_mid) { + if (bundle_groups_by_mid.empty()) return true; - const cricket::ContentGroup* bundle_group = - desc->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); - RTC_DCHECK(bundle_group != NULL); - const cricket::ContentInfos& contents = desc->contents(); for (cricket::ContentInfos::const_iterator citer = contents.begin(); citer != contents.end(); ++citer) { const cricket::ContentInfo* content = (&*citer); RTC_DCHECK(content != NULL); - if (bundle_group->HasContentName(content->name) && !content->rejected && + auto it = bundle_groups_by_mid.find(content->name); + if (it != bundle_groups_by_mid.end() && !content->rejected && content->type == MediaProtocolType::kRtp) { if (!HasRtcpMuxEnabled(content)) return false; diff --git a/pc/peer_connection.h b/pc/peer_connection.h index d321fd5667..7be137a6a8 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -389,7 +389,10 @@ class PeerConnection : public PeerConnectionInternal, RTC_DCHECK_RUN_ON(signaling_thread()); return is_unified_plan_; } - bool ValidateBundleSettings(const cricket::SessionDescription* desc); + bool ValidateBundleSettings( + const cricket::SessionDescription* desc, + const std::map& + bundle_groups_by_mid); // Returns the MID for the data section associated with the // SCTP data channel, if it has been set. If no data diff --git a/pc/peer_connection_bundle_unittest.cc b/pc/peer_connection_bundle_unittest.cc index a219fa33e4..fa5be62745 100644 --- a/pc/peer_connection_bundle_unittest.cc +++ b/pc/peer_connection_bundle_unittest.cc @@ -886,4 +886,56 @@ TEST_F(PeerConnectionBundleTestUnifiedPlan, EXPECT_TRUE(bundle_group->content_names().empty()); } +TEST_F(PeerConnectionBundleTestUnifiedPlan, MultipleBundleGroups) { + auto caller = CreatePeerConnection(); + caller->AddAudioTrack("0_audio"); + caller->AddAudioTrack("1_audio"); + caller->AddVideoTrack("2_audio"); + caller->AddVideoTrack("3_audio"); + auto callee = CreatePeerConnection(); + + auto offer = caller->CreateOffer(RTCOfferAnswerOptions()); + // Modify the GROUP to have two BUNDLEs. We know that the MIDs will be 0,1,2,4 + // because our implementation has predictable MIDs. + offer->description()->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE); + cricket::ContentGroup bundle_group1(cricket::GROUP_TYPE_BUNDLE); + bundle_group1.AddContentName("0"); + bundle_group1.AddContentName("1"); + cricket::ContentGroup bundle_group2(cricket::GROUP_TYPE_BUNDLE); + bundle_group2.AddContentName("2"); + bundle_group2.AddContentName("3"); + offer->description()->AddGroup(bundle_group1); + offer->description()->AddGroup(bundle_group2); + + EXPECT_TRUE( + caller->SetLocalDescription(CloneSessionDescription(offer.get()))); + callee->SetRemoteDescription(std::move(offer)); + auto answer = callee->CreateAnswer(); + EXPECT_TRUE( + callee->SetLocalDescription(CloneSessionDescription(answer.get()))); + caller->SetRemoteDescription(std::move(answer)); + + // Verify bundling on sender side. + auto senders = caller->pc()->GetSenders(); + ASSERT_EQ(senders.size(), 4u); + auto sender0_transport = senders[0]->dtls_transport(); + auto sender1_transport = senders[1]->dtls_transport(); + auto sender2_transport = senders[2]->dtls_transport(); + auto sender3_transport = senders[3]->dtls_transport(); + EXPECT_EQ(sender0_transport, sender1_transport); + EXPECT_EQ(sender2_transport, sender3_transport); + EXPECT_NE(sender0_transport, sender2_transport); + + // Verify bundling on receiver side. + auto receivers = callee->pc()->GetReceivers(); + ASSERT_EQ(receivers.size(), 4u); + auto receiver0_transport = receivers[0]->dtls_transport(); + auto receiver1_transport = receivers[1]->dtls_transport(); + auto receiver2_transport = receivers[2]->dtls_transport(); + auto receiver3_transport = receivers[3]->dtls_transport(); + EXPECT_EQ(receiver0_transport, receiver1_transport); + EXPECT_EQ(receiver2_transport, receiver3_transport); + EXPECT_NE(receiver0_transport, receiver2_transport); +} + } // namespace webrtc diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 50d6b9a9e6..91ea3794bf 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -164,6 +164,19 @@ void NoteKeyProtocolAndMedia(KeyExchangeProtocolType protocol_type, } } +std::map GetBundleGroupsByMid( + const SessionDescription* desc) { + std::vector bundle_groups = + desc->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE); + std::map bundle_groups_by_mid; + for (const cricket::ContentGroup* bundle_group : bundle_groups) { + for (const std::string& content_name : bundle_group->content_names()) { + bundle_groups_by_mid[content_name] = bundle_group; + } + } + return bundle_groups_by_mid; +} + // Returns true if |new_desc| requests an ICE restart (i.e., new ufrag/pwd). bool CheckForRemoteIceRestart(const SessionDescriptionInterface* old_desc, const SessionDescriptionInterface* new_desc, @@ -334,9 +347,10 @@ bool MediaSectionsHaveSameCount(const SessionDescription& desc1, // needs a ufrag and pwd. Mismatches, such as replying with a DTLS fingerprint // to SDES keys, will be caught in JsepTransport negotiation, and backstopped // by Channel's |srtp_required| check. -RTCError VerifyCrypto(const SessionDescription* desc, bool dtls_enabled) { - const cricket::ContentGroup* bundle = - desc->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); +RTCError VerifyCrypto(const SessionDescription* desc, + bool dtls_enabled, + const std::map& + bundle_groups_by_mid) { for (const cricket::ContentInfo& content_info : desc->contents()) { if (content_info.rejected) { continue; @@ -346,8 +360,10 @@ RTCError VerifyCrypto(const SessionDescription* desc, bool dtls_enabled) { : webrtc::kEnumCounterKeyProtocolSdes, content_info.media_description()->type()); const std::string& mid = content_info.name; - if (bundle && bundle->HasContentName(mid) && - mid != *(bundle->FirstContentName())) { + auto it = bundle_groups_by_mid.find(mid); + const cricket::ContentGroup* bundle = + it != bundle_groups_by_mid.end() ? it->second : nullptr; + if (bundle && mid != *(bundle->FirstContentName())) { // This isn't the first media section in the BUNDLE group, so it's not // required to have crypto attributes, since only the crypto attributes // from the first section actually get used. @@ -384,16 +400,19 @@ RTCError VerifyCrypto(const SessionDescription* desc, bool dtls_enabled) { // Checks that each non-rejected content has ice-ufrag and ice-pwd set, unless // it's in a BUNDLE group, in which case only the BUNDLE-tag section (first // media section/description in the BUNDLE group) needs a ufrag and pwd. -bool VerifyIceUfragPwdPresent(const SessionDescription* desc) { - const cricket::ContentGroup* bundle = - desc->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); +bool VerifyIceUfragPwdPresent( + const SessionDescription* desc, + const std::map& + bundle_groups_by_mid) { for (const cricket::ContentInfo& content_info : desc->contents()) { if (content_info.rejected) { continue; } const std::string& mid = content_info.name; - if (bundle && bundle->HasContentName(mid) && - mid != *(bundle->FirstContentName())) { + auto it = bundle_groups_by_mid.find(mid); + const cricket::ContentGroup* bundle = + it != bundle_groups_by_mid.end() ? it->second : nullptr; + if (bundle && mid != *(bundle->FirstContentName())) { // This isn't the first media section in the BUNDLE group, so it's not // required to have ufrag/password, since only the ufrag/password from // the first section actually get used. @@ -1225,7 +1244,9 @@ void SdpOfferAnswerHandler::SetLocalDescription( } RTCError SdpOfferAnswerHandler::ApplyLocalDescription( - std::unique_ptr desc) { + std::unique_ptr desc, + const std::map& + bundle_groups_by_mid) { RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(desc); @@ -1279,7 +1300,7 @@ RTCError SdpOfferAnswerHandler::ApplyLocalDescription( if (IsUnifiedPlan()) { RTCError error = UpdateTransceiversAndDataChannels( cricket::CS_LOCAL, *local_description(), old_local_description, - remote_description()); + remote_description(), bundle_groups_by_mid); if (!error.ok()) { return error; } @@ -1349,7 +1370,8 @@ RTCError SdpOfferAnswerHandler::ApplyLocalDescription( } error = UpdateSessionState(type, cricket::CS_LOCAL, - local_description()->description()); + local_description()->description(), + bundle_groups_by_mid); if (!error.ok()) { return error; } @@ -1511,7 +1533,9 @@ void SdpOfferAnswerHandler::SetRemoteDescription( } RTCError SdpOfferAnswerHandler::ApplyRemoteDescription( - std::unique_ptr desc) { + std::unique_ptr desc, + const std::map& + bundle_groups_by_mid) { RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(desc); @@ -1555,7 +1579,7 @@ RTCError SdpOfferAnswerHandler::ApplyRemoteDescription( if (IsUnifiedPlan()) { RTCError error = UpdateTransceiversAndDataChannels( cricket::CS_REMOTE, *remote_description(), local_description(), - old_remote_description); + old_remote_description, bundle_groups_by_mid); if (!error.ok()) { return error; } @@ -1577,7 +1601,8 @@ RTCError SdpOfferAnswerHandler::ApplyRemoteDescription( // NOTE: Candidates allocation will be initiated only when // SetLocalDescription is called. error = UpdateSessionState(type, cricket::CS_REMOTE, - remote_description()->description()); + remote_description()->description(), + bundle_groups_by_mid); if (!error.ok()) { return error; } @@ -1870,7 +1895,10 @@ void SdpOfferAnswerHandler::DoSetLocalDescription( return; } - RTCError error = ValidateSessionDescription(desc.get(), cricket::CS_LOCAL); + std::map bundle_groups_by_mid = + GetBundleGroupsByMid(desc->description()); + RTCError error = ValidateSessionDescription(desc.get(), cricket::CS_LOCAL, + bundle_groups_by_mid); if (!error.ok()) { std::string error_message = GetSetDescriptionErrorMessage( cricket::CS_LOCAL, desc->GetType(), error); @@ -1884,7 +1912,7 @@ void SdpOfferAnswerHandler::DoSetLocalDescription( // which may destroy it before returning. const SdpType type = desc->GetType(); - error = ApplyLocalDescription(std::move(desc)); + error = ApplyLocalDescription(std::move(desc), bundle_groups_by_mid); // |desc| may be destroyed at this point. if (!error.ok()) { @@ -2130,7 +2158,10 @@ void SdpOfferAnswerHandler::DoSetRemoteDescription( // points. FillInMissingRemoteMids(desc->description()); - RTCError error = ValidateSessionDescription(desc.get(), cricket::CS_REMOTE); + std::map bundle_groups_by_mid = + GetBundleGroupsByMid(desc->description()); + RTCError error = ValidateSessionDescription(desc.get(), cricket::CS_REMOTE, + bundle_groups_by_mid); if (!error.ok()) { std::string error_message = GetSetDescriptionErrorMessage( cricket::CS_REMOTE, desc->GetType(), error); @@ -2144,7 +2175,7 @@ void SdpOfferAnswerHandler::DoSetRemoteDescription( // ApplyRemoteDescription, which may destroy it before returning. const SdpType type = desc->GetType(); - error = ApplyRemoteDescription(std::move(desc)); + error = ApplyRemoteDescription(std::move(desc), bundle_groups_by_mid); // |desc| may be destroyed at this point. if (!error.ok()) { @@ -2436,7 +2467,9 @@ void SdpOfferAnswerHandler::ChangeSignalingState( RTCError SdpOfferAnswerHandler::UpdateSessionState( SdpType type, cricket::ContentSource source, - const cricket::SessionDescription* description) { + const cricket::SessionDescription* description, + const std::map& + bundle_groups_by_mid) { RTC_DCHECK_RUN_ON(signaling_thread()); // If there's already a pending error then no state transition should happen. @@ -2466,7 +2499,7 @@ RTCError SdpOfferAnswerHandler::UpdateSessionState( // Update internal objects according to the session description's media // descriptions. - RTCError error = PushdownMediaDescription(type, source); + RTCError error = PushdownMediaDescription(type, source, bundle_groups_by_mid); if (!error.ok()) { return error; } @@ -2969,7 +3002,9 @@ void SdpOfferAnswerHandler::GenerateNegotiationNeededEvent() { RTCError SdpOfferAnswerHandler::ValidateSessionDescription( const SessionDescriptionInterface* sdesc, - cricket::ContentSource source) { + cricket::ContentSource source, + const std::map& + bundle_groups_by_mid) { if (session_error() != SessionError::kNone) { LOG_AND_RETURN_ERROR(RTCErrorType::INTERNAL_ERROR, GetSessionErrorMsg()); } @@ -2995,20 +3030,21 @@ RTCError SdpOfferAnswerHandler::ValidateSessionDescription( std::string crypto_error; if (webrtc_session_desc_factory_->SdesPolicy() == cricket::SEC_REQUIRED || pc_->dtls_enabled()) { - RTCError crypto_error = - VerifyCrypto(sdesc->description(), pc_->dtls_enabled()); + RTCError crypto_error = VerifyCrypto( + sdesc->description(), pc_->dtls_enabled(), bundle_groups_by_mid); if (!crypto_error.ok()) { return crypto_error; } } // Verify ice-ufrag and ice-pwd. - if (!VerifyIceUfragPwdPresent(sdesc->description())) { + if (!VerifyIceUfragPwdPresent(sdesc->description(), bundle_groups_by_mid)) { LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, kSdpWithoutIceUfragPwd); } - if (!pc_->ValidateBundleSettings(sdesc->description())) { + if (!pc_->ValidateBundleSettings(sdesc->description(), + bundle_groups_by_mid)) { LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, kBundleWithoutRtcpMux); } @@ -3081,18 +3117,23 @@ RTCError SdpOfferAnswerHandler::UpdateTransceiversAndDataChannels( cricket::ContentSource source, const SessionDescriptionInterface& new_session, const SessionDescriptionInterface* old_local_description, - const SessionDescriptionInterface* old_remote_description) { + const SessionDescriptionInterface* old_remote_description, + const std::map& + bundle_groups_by_mid) { RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(IsUnifiedPlan()); - const cricket::ContentGroup* bundle_group = nullptr; if (new_session.GetType() == SdpType::kOffer) { - auto bundle_group_or_error = - GetEarlyBundleGroup(*new_session.description()); - if (!bundle_group_or_error.ok()) { - return bundle_group_or_error.MoveError(); + // If the BUNDLE policy is max-bundle, then we know for sure that all + // transports will be bundled from the start. Return an error if max-bundle + // is specified but the session description does not have a BUNDLE group. + if (pc_->configuration()->bundle_policy == + PeerConnectionInterface::kBundlePolicyMaxBundle && + bundle_groups_by_mid.empty()) { + LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, + "max-bundle configured but session description " + "has no BUNDLE group"); } - bundle_group = bundle_group_or_error.MoveValue(); } const ContentInfos& new_contents = new_session.description()->contents(); @@ -3100,6 +3141,9 @@ RTCError SdpOfferAnswerHandler::UpdateTransceiversAndDataChannels( const cricket::ContentInfo& new_content = new_contents[i]; cricket::MediaType media_type = new_content.media_description()->type(); mid_generator_.AddKnownId(new_content.name); + auto it = bundle_groups_by_mid.find(new_content.name); + const cricket::ContentGroup* bundle_group = + it != bundle_groups_by_mid.end() ? it->second : nullptr; if (media_type == cricket::MEDIA_TYPE_AUDIO || media_type == cricket::MEDIA_TYPE_VIDEO) { const cricket::ContentInfo* old_local_content = nullptr; @@ -3288,22 +3332,6 @@ SdpOfferAnswerHandler::AssociateTransceiver( return std::move(transceiver); } -RTCErrorOr -SdpOfferAnswerHandler::GetEarlyBundleGroup( - const SessionDescription& desc) const { - const cricket::ContentGroup* bundle_group = nullptr; - if (pc_->configuration()->bundle_policy == - PeerConnectionInterface::kBundlePolicyMaxBundle) { - bundle_group = desc.GetGroupByName(cricket::GROUP_TYPE_BUNDLE); - if (!bundle_group) { - LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, - "max-bundle configured but session description " - "has no BUNDLE group"); - } - } - return bundle_group; -} - RTCError SdpOfferAnswerHandler::UpdateTransceiverChannel( rtc::scoped_refptr> transceiver, @@ -4145,14 +4173,16 @@ void SdpOfferAnswerHandler::EnableSending() { RTCError SdpOfferAnswerHandler::PushdownMediaDescription( SdpType type, - cricket::ContentSource source) { + cricket::ContentSource source, + const std::map& + bundle_groups_by_mid) { const SessionDescriptionInterface* sdesc = (source == cricket::CS_LOCAL ? local_description() : remote_description()); RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(sdesc); - if (!UpdatePayloadTypeDemuxingState(source)) { + if (!UpdatePayloadTypeDemuxingState(source, bundle_groups_by_mid)) { // Note that this is never expected to fail, since RtpDemuxer doesn't return // an error when changing payload type demux criteria, which is all this // does. @@ -4737,7 +4767,9 @@ SdpOfferAnswerHandler::GetMediaDescriptionOptionsForRejectedData( } bool SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( - cricket::ContentSource source) { + cricket::ContentSource source, + const std::map& + bundle_groups_by_mid) { RTC_DCHECK_RUN_ON(signaling_thread()); // We may need to delete any created default streams and disable creation of // new ones on the basis of payload type. This is needed to avoid SSRC @@ -4750,19 +4782,24 @@ bool SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( const SessionDescriptionInterface* sdesc = (source == cricket::CS_LOCAL ? local_description() : remote_description()); - const cricket::ContentGroup* bundle_group = - sdesc->description()->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); - std::set audio_payload_types; - std::set video_payload_types; - bool pt_demuxing_enabled_audio = true; - bool pt_demuxing_enabled_video = true; + struct PayloadTypes { + std::set audio_payload_types; + std::set video_payload_types; + bool pt_demuxing_enabled_audio = true; + bool pt_demuxing_enabled_video = true; + }; + std::map payload_types_by_bundle; for (auto& content_info : sdesc->description()->contents()) { + auto it = bundle_groups_by_mid.find(content_info.name); + const cricket::ContentGroup* bundle_group = + it != bundle_groups_by_mid.end() ? it->second : nullptr; // If this m= section isn't bundled, it's safe to demux by payload type // since other m= sections using the same payload type will also be using // different transports. - if (!bundle_group || !bundle_group->HasContentName(content_info.name)) { + if (!bundle_group) { continue; } + PayloadTypes* payload_types = &payload_types_by_bundle[bundle_group]; if (content_info.rejected || (source == cricket::ContentSource::CS_LOCAL && !RtpTransceiverDirectionHasRecv( @@ -4778,12 +4815,12 @@ bool SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( const cricket::AudioContentDescription* audio_desc = content_info.media_description()->as_audio(); for (const cricket::AudioCodec& audio : audio_desc->codecs()) { - if (audio_payload_types.count(audio.id)) { + if (payload_types->audio_payload_types.count(audio.id)) { // Two m= sections are using the same payload type, thus demuxing // by payload type is not possible. - pt_demuxing_enabled_audio = false; + payload_types->pt_demuxing_enabled_audio = false; } - audio_payload_types.insert(audio.id); + payload_types->audio_payload_types.insert(audio.id); } break; } @@ -4791,12 +4828,12 @@ bool SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( const cricket::VideoContentDescription* video_desc = content_info.media_description()->as_video(); for (const cricket::VideoCodec& video : video_desc->codecs()) { - if (video_payload_types.count(video.id)) { + if (payload_types->video_payload_types.count(video.id)) { // Two m= sections are using the same payload type, thus demuxing // by payload type is not possible. - pt_demuxing_enabled_video = false; + payload_types->pt_demuxing_enabled_video = false; } - video_payload_types.insert(video.id); + payload_types->video_payload_types.insert(video.id); } break; } @@ -4829,23 +4866,27 @@ bool SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( return true; } return pc_->worker_thread()->Invoke( - RTC_FROM_HERE, [&channels_to_update, bundle_group, - pt_demuxing_enabled_audio, pt_demuxing_enabled_video]() { + RTC_FROM_HERE, + [&channels_to_update, &bundle_groups_by_mid, &payload_types_by_bundle]() { for (const auto& it : channels_to_update) { RtpTransceiverDirection local_direction = it.first; cricket::ChannelInterface* channel = it.second; cricket::MediaType media_type = channel->media_type(); - bool in_bundle_group = (bundle_group && bundle_group->HasContentName( - channel->content_name())); + auto bundle_it = bundle_groups_by_mid.find(channel->content_name()); + const cricket::ContentGroup* bundle_group = + bundle_it != bundle_groups_by_mid.end() ? bundle_it->second + : nullptr; if (media_type == cricket::MediaType::MEDIA_TYPE_AUDIO) { if (!channel->SetPayloadTypeDemuxingEnabled( - (!in_bundle_group || pt_demuxing_enabled_audio) && + (!bundle_group || payload_types_by_bundle[bundle_group] + .pt_demuxing_enabled_audio) && RtpTransceiverDirectionHasRecv(local_direction))) { return false; } } else if (media_type == cricket::MediaType::MEDIA_TYPE_VIDEO) { if (!channel->SetPayloadTypeDemuxingEnabled( - (!in_bundle_group || pt_demuxing_enabled_video) && + (!bundle_group || payload_types_by_bundle[bundle_group] + .pt_demuxing_enabled_video) && RtpTransceiverDirectionHasRecv(local_direction))) { return false; } diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index a913a9bad8..1ef124baec 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -227,9 +227,13 @@ class SdpOfferAnswerHandler : public SdpStateProvider, // Synchronous implementations of SetLocalDescription/SetRemoteDescription // that return an RTCError instead of invoking a callback. RTCError ApplyLocalDescription( - std::unique_ptr desc); + std::unique_ptr desc, + const std::map& + bundle_groups_by_mid); RTCError ApplyRemoteDescription( - std::unique_ptr desc); + std::unique_ptr desc, + const std::map& + bundle_groups_by_mid); // Implementation of the offer/answer exchange operations. These are chained // onto the |operations_chain_| when the public CreateOffer(), CreateAnswer(), @@ -251,9 +255,12 @@ class SdpOfferAnswerHandler : public SdpStateProvider, void ChangeSignalingState( PeerConnectionInterface::SignalingState signaling_state); - RTCError UpdateSessionState(SdpType type, - cricket::ContentSource source, - const cricket::SessionDescription* description); + RTCError UpdateSessionState( + SdpType type, + cricket::ContentSource source, + const cricket::SessionDescription* description, + const std::map& + bundle_groups_by_mid); bool IsUnifiedPlan() const RTC_RUN_ON(signaling_thread()); @@ -286,9 +293,11 @@ class SdpOfferAnswerHandler : public SdpStateProvider, bool CheckIfNegotiationIsNeeded(); void GenerateNegotiationNeededEvent(); // Helper method which verifies SDP. - RTCError ValidateSessionDescription(const SessionDescriptionInterface* sdesc, - cricket::ContentSource source) - RTC_RUN_ON(signaling_thread()); + RTCError ValidateSessionDescription( + const SessionDescriptionInterface* sdesc, + cricket::ContentSource source, + const std::map& + bundle_groups_by_mid) RTC_RUN_ON(signaling_thread()); // Updates the local RtpTransceivers according to the JSEP rules. Called as // part of setting the local/remote description. @@ -296,7 +305,9 @@ class SdpOfferAnswerHandler : public SdpStateProvider, cricket::ContentSource source, const SessionDescriptionInterface& new_session, const SessionDescriptionInterface* old_local_description, - const SessionDescriptionInterface* old_remote_description); + const SessionDescriptionInterface* old_remote_description, + const std::map& + bundle_groups_by_mid); // Associate the given transceiver according to the JSEP rules. RTCErrorOr< @@ -317,15 +328,6 @@ class SdpOfferAnswerHandler : public SdpStateProvider, const RtpTransceiver* transceiver, const SessionDescriptionInterface* sdesc) const; - // If the BUNDLE policy is max-bundle, then we know for sure that all - // transports will be bundled from the start. This method returns the BUNDLE - // group if that's the case, or null if BUNDLE will be negotiated later. An - // error is returned if max-bundle is specified but the session description - // does not have a BUNDLE group. - RTCErrorOr GetEarlyBundleGroup( - const cricket::SessionDescription& desc) const - RTC_RUN_ON(signaling_thread()); - // Either creates or destroys the transceiver's BaseChannel according to the // given media section. RTCError UpdateTransceiverChannel( @@ -456,8 +458,11 @@ class SdpOfferAnswerHandler : public SdpStateProvider, void EnableSending(); // Push the media parts of the local or remote session description // down to all of the channels. - RTCError PushdownMediaDescription(SdpType type, - cricket::ContentSource source); + RTCError PushdownMediaDescription( + SdpType type, + cricket::ContentSource source, + const std::map& + bundle_groups_by_mid); RTCError PushdownTransportDescription(cricket::ContentSource source, SdpType type); @@ -544,7 +549,10 @@ class SdpOfferAnswerHandler : public SdpStateProvider, // Based on number of transceivers per media type, enabled or disable // payload type based demuxing in the affected channels. - bool UpdatePayloadTypeDemuxingState(cricket::ContentSource source); + bool UpdatePayloadTypeDemuxingState( + cricket::ContentSource source, + const std::map& + bundle_groups_by_mid); // ================================================================== // Access to pc_ variables diff --git a/pc/session_description.cc b/pc/session_description.cc index 3cb2b6d231..35b732d649 100644 --- a/pc/session_description.cc +++ b/pc/session_description.cc @@ -259,6 +259,17 @@ const ContentGroup* SessionDescription::GetGroupByName( return NULL; } +std::vector SessionDescription::GetGroupsByName( + const std::string& name) const { + std::vector content_groups; + for (const ContentGroup& content_group : content_groups_) { + if (content_group.semantics() == name) { + content_groups.push_back(&content_group); + } + } + return content_groups; +} + ContentInfo::~ContentInfo() { } diff --git a/pc/session_description.h b/pc/session_description.h index fe0fdefd15..96aa996752 100644 --- a/pc/session_description.h +++ b/pc/session_description.h @@ -567,6 +567,8 @@ class SessionDescription { // Group accessors. const ContentGroups& groups() const { return content_groups_; } const ContentGroup* GetGroupByName(const std::string& name) const; + std::vector GetGroupsByName( + const std::string& name) const; bool HasGroup(const std::string& name) const; // Group mutators. diff --git a/pc/webrtc_sdp.cc b/pc/webrtc_sdp.cc index 282c7f705d..379b2f30c2 100644 --- a/pc/webrtc_sdp.cc +++ b/pc/webrtc_sdp.cc @@ -900,11 +900,11 @@ std::string SdpSerialize(const JsepSessionDescription& jdesc) { // Time Description. AddLine(kTimeDescription, &message); - // Group - if (desc->HasGroup(cricket::GROUP_TYPE_BUNDLE)) { + // BUNDLE Groups + std::vector groups = + desc->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE); + for (const cricket::ContentGroup* group : groups) { std::string group_line = kAttrGroup; - const cricket::ContentGroup* group = - desc->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); RTC_DCHECK(group != NULL); for (const std::string& content_name : group->content_names()) { group_line.append(" "); diff --git a/pc/webrtc_sdp_unittest.cc b/pc/webrtc_sdp_unittest.cc index 559b981d59..266fd3dfd6 100644 --- a/pc/webrtc_sdp_unittest.cc +++ b/pc/webrtc_sdp_unittest.cc @@ -2124,17 +2124,21 @@ TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithoutCandidates) { EXPECT_EQ(std::string(kSdpString), message); } -TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithBundle) { - ContentGroup group(cricket::GROUP_TYPE_BUNDLE); - group.AddContentName(kAudioContentName); - group.AddContentName(kVideoContentName); - desc_.AddGroup(group); +TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithBundles) { + ContentGroup group1(cricket::GROUP_TYPE_BUNDLE); + group1.AddContentName(kAudioContentName); + group1.AddContentName(kVideoContentName); + desc_.AddGroup(group1); + ContentGroup group2(cricket::GROUP_TYPE_BUNDLE); + group2.AddContentName(kAudioContentName2); + desc_.AddGroup(group2); ASSERT_TRUE(jdesc_.Initialize(desc_.Clone(), jdesc_.session_id(), jdesc_.session_version())); std::string message = webrtc::SdpSerialize(jdesc_); std::string sdp_with_bundle = kSdpFullString; InjectAfter(kSessionTime, - "a=group:BUNDLE audio_content_name video_content_name\r\n", + "a=group:BUNDLE audio_content_name video_content_name\r\n" + "a=group:BUNDLE audio_content_name_2\r\n", &sdp_with_bundle); EXPECT_EQ(sdp_with_bundle, message); }