Fix handling of empty BUNDLE groups.
This CL fixes issues when applying a description with an empty BUNDLE
group (previously it would fail, after recent refactoring it started
crashing).
This CL also will cause an empty BUNDLE group to be generated when it
should be. Namely, when responding to an offer that had a BUNDLE group,
rejecting everything in it.
Bug: chromium:831996
Change-Id: I4e705a328daef4e81f8f1ace6aa73ddfa13c0107
Reviewed-on: https://webrtc-review.googlesource.com/69720
Reviewed-by: Zhi Huang <zhihuang@webrtc.org>
Commit-Queue: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22844}diff --git a/pc/jseptransportcontroller.cc b/pc/jseptransportcontroller.cc
index 31d27ca..b5ce5fe 100644
--- a/pc/jseptransportcontroller.cc
+++ b/pc/jseptransportcontroller.cc
@@ -589,7 +589,7 @@
}
std::vector<int> 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 @@
}
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 794175a..6e44534 100644
--- a/pc/jseptransportcontroller.h
+++ b/pc/jseptransportcontroller.h
@@ -201,8 +201,8 @@
rtc::Optional<std::string> bundled_mid() const {
rtc::Optional<std::string> 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 3bf931c..eba3249 100644
--- a/pc/mediasession.cc
+++ b/pc/mediasession.cc
@@ -1494,10 +1494,17 @@
}
}
- // 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 c71662b..a1b69d8 100644
--- a/pc/peerconnection_bundle_unittest.cc
+++ b/pc/peerconnection_bundle_unittest.cc
@@ -248,6 +248,13 @@
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 @@
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
