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); }