diff --git a/pc/jseptransportcontroller.cc b/pc/jseptransportcontroller.cc index 31d27cae4d..b5ce5fe0a7 100644 --- a/pc/jseptransportcontroller.cc +++ b/pc/jseptransportcontroller.cc @@ -589,7 +589,7 @@ RTCError JsepTransportController::ApplyDescription_n( } std::vector extension_ids; - if (bundle_group_ && content_info.name == *bundled_mid()) { + if (bundled_mid() && content_info.name == *bundled_mid()) { extension_ids = merged_encrypted_extension_ids; } else { extension_ids = GetEncryptedHeaderExtensionIds(content_info); @@ -685,8 +685,9 @@ RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroup( } if (ShouldUpdateBundleGroup(type, description)) { - std::string new_bundled_mid = *(new_bundle_group->FirstContentName()); - if (bundled_mid() && *bundled_mid() != new_bundled_mid) { + const std::string* new_bundled_mid = new_bundle_group->FirstContentName(); + if (bundled_mid() && new_bundled_mid && + *bundled_mid() != *new_bundled_mid) { return RTCError(RTCErrorType::UNSUPPORTED_OPERATION, "Changing the negotiated BUNDLE-tag is not supported."); } diff --git a/pc/jseptransportcontroller.h b/pc/jseptransportcontroller.h index 794175a6e2..6e44534f08 100644 --- a/pc/jseptransportcontroller.h +++ b/pc/jseptransportcontroller.h @@ -201,8 +201,8 @@ class JsepTransportController : public sigslot::has_slots<>, rtc::Optional bundled_mid() const { rtc::Optional bundled_mid; - if (bundle_group_) { - bundled_mid = std::move(*(bundle_group_->FirstContentName())); + if (bundle_group_ && bundle_group_->FirstContentName()) { + bundled_mid = *(bundle_group_->FirstContentName()); } return bundled_mid; } diff --git a/pc/mediasession.cc b/pc/mediasession.cc index 3bf931c613..eba3249d03 100644 --- a/pc/mediasession.cc +++ b/pc/mediasession.cc @@ -1494,10 +1494,17 @@ SessionDescription* MediaSessionDescriptionFactory::CreateAnswer( } } - // Only put BUNDLE group in answer if nonempty. - if (answer_bundle.FirstContentName()) { + // If a BUNDLE group was offered, put a BUNDLE group in the answer even if + // it's 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 (answer_bundle.FirstContentName()) { // Share the same ICE credentials and crypto params across all contents, // as BUNDLE requires. if (!UpdateTransportInfoForBundle(answer_bundle, answer.get())) { diff --git a/pc/peerconnection_bundle_unittest.cc b/pc/peerconnection_bundle_unittest.cc index c71662bda0..a1b69d8ae1 100644 --- a/pc/peerconnection_bundle_unittest.cc +++ b/pc/peerconnection_bundle_unittest.cc @@ -248,6 +248,13 @@ class PeerConnectionBundleTest PeerConnectionBundleTest() : PeerConnectionBundleBaseTest(GetParam()) {} }; +class PeerConnectionBundleTestUnifiedPlan + : public PeerConnectionBundleBaseTest { + protected: + PeerConnectionBundleTestUnifiedPlan() + : PeerConnectionBundleBaseTest(SdpSemantics::kUnifiedPlan) {} +}; + SdpContentMutator RemoveRtcpMux() { return [](cricket::ContentInfo* content, cricket::TransportInfo* transport) { content->media_description()->set_rtcp_mux(false); @@ -804,4 +811,34 @@ INSTANTIATE_TEST_CASE_P(PeerConnectionBundleTest, Values(SdpSemantics::kPlanB, SdpSemantics::kUnifiedPlan)); +// According to RFC5888, if an endpoint understands the semantics of an +// "a=group", it MUST return an answer with that group. So, an empty BUNDLE +// group is valid when the answerer rejects all m= sections (by stopping all +// transceivers), meaning there's nothing to bundle. +// +// Only writing this test for Unified Plan mode, since there's no way to reject +// m= sections in answers for Plan B without SDP munging. +TEST_F(PeerConnectionBundleTestUnifiedPlan, + EmptyBundleGroupCreatedInAnswerWhenAppropriate) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnection(); + + EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + // Stop all transceivers, causing all m= sections to be rejected. + for (const auto& transceiver : callee->pc()->GetTransceivers()) { + transceiver->Stop(); + } + EXPECT_TRUE( + caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); + + // Verify that the answer actually contained an empty bundle group. + const SessionDescriptionInterface* desc = callee->pc()->local_description(); + ASSERT_NE(nullptr, desc); + const cricket::ContentGroup* bundle_group = + desc->description()->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); + ASSERT_NE(nullptr, bundle_group); + EXPECT_TRUE(bundle_group->content_names().empty()); +} + } // namespace webrtc