Revert "sdp: measure rtp header extension collisions"
This reverts commit 6c27d56a2aeb2cff10a216d714552f4970d99d32.
Reason for revert: Breaks C++ 17 compilation (https://ci.chromium.org/ui/p/webrtc/builders/perf/Fuchsia%20Builder/157/overview). While the proposed fix doesn't seem to be trivial and causes some disagreements: https://webrtc-review.googlesource.com/c/src/+/288460
The bot will be added to CQ.
Original change's description:
> sdp: measure rtp header extension collisions
>
> since extension ids are required to be unique in a BUNDLE group:
> https://www.rfc-editor.org/rfc/rfc8843#name-rtp-header-extensions-consi
>
> Measure how much enforcing this would break in UMA first.
>
> BUG=webrtc:14782
>
> Change-Id: Ieaf7a436feea677032499e11ca14973eebda322e
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/288362
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Commit-Queue: Philipp Hancke <phancke@microsoft.com>
> Reviewed-by: Johannes Kron <kron@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#38914}
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Change-Id: I454acc80ac222395acd640dc9f8bcea941855861
Bug: webrtc:14782
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/288382
Commit-Queue: Andrey Logvin <landrey@google.com>
Owners-Override: Jeremy Leconte <jleconte@google.com>
Reviewed-by: Jeremy Leconte <jleconte@webrtc.org>
Owners-Override: Andrey Logvin <landrey@google.com>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Jeremy Leconte <jleconte@google.com>
Cr-Commit-Position: refs/heads/main@{#38915}
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
index d0bd3c8..84a0756 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -491,52 +491,6 @@
return RTCError::OK();
}
-RTCError FindDuplicateHeaderExtensionIds(
- const RtpExtension extension,
- std::map<int, RtpExtension>& id_to_extension) {
- auto existing_extension = id_to_extension.find(extension.id);
- if (existing_extension != id_to_extension.end() &&
- extension != existing_extension->second) {
- return RTCError(
- RTCErrorType::INVALID_PARAMETER,
- "A BUNDLE group contains a codec collision for "
- "header extension id='" +
- rtc::ToString(extension.id) +
- ". The id must be the same across all bundled media descriptions");
- }
- id_to_extension.insert(std::make_pair(extension.id, extension));
- return RTCError::OK();
-}
-
-RTCError ValidateBundledRtpHeaderExtensions(
- const cricket::SessionDescription& description) {
- // https://www.rfc-editor.org/rfc/rfc8843#name-rtp-header-extensions-consi
- // ... the identifier used for a given extension MUST identify the same
- // extension across all the bundled media descriptions.
- std::vector<const cricket::ContentGroup*> bundle_groups =
- description.GetGroupsByName(cricket::GROUP_TYPE_BUNDLE);
- for (const cricket::ContentGroup* bundle_group : bundle_groups) {
- std::map<int, RtpExtension> id_to_extension;
- for (const std::string& content_name : bundle_group->content_names()) {
- const cricket::MediaContentDescription* media_description =
- description.GetContentDescriptionByName(content_name);
- if (!media_description) {
- return RTCError(RTCErrorType::INVALID_PARAMETER,
- "A BUNDLE group contains a MID='" + content_name +
- "' matching no m= section.");
- }
- for (const auto& extension : media_description->rtp_header_extensions()) {
- auto error =
- FindDuplicateHeaderExtensionIds(extension, id_to_extension);
- if (!error.ok()) {
- return error;
- }
- }
- }
- }
- return RTCError::OK();
-}
-
bool IsValidOfferToReceiveMedia(int value) {
typedef PeerConnectionInterface::RTCOfferAnswerOptions Options;
return (value >= Options::kUndefined) &&
@@ -3417,18 +3371,12 @@
return RTCError(RTCErrorType::INVALID_PARAMETER, kSdpWithoutIceUfragPwd);
}
- // Validate that there are no collisions of bundled payload types.
+ // Validate bundle, payload types and that there are no collisions.
error = ValidateBundledPayloadTypes(*sdesc->description());
// TODO(bugs.webrtc.org/14420): actually reject.
RTC_HISTOGRAM_BOOLEAN("WebRTC.PeerConnection.ValidBundledPayloadTypes",
error.ok());
- // Validate that there are no collisions of bundled header extensions ids.
- error = ValidateBundledRtpHeaderExtensions(*sdesc->description());
- // TODO(bugs.webrtc.org/14782): actually reject.
- RTC_HISTOGRAM_BOOLEAN("WebRTC.PeerConnection.ValidBundledExtensionIds",
- error.ok());
-
if (!pc_->ValidateBundleSettings(sdesc->description(),
bundle_groups_by_mid)) {
return RTCError(RTCErrorType::INVALID_PARAMETER, kBundleWithoutRtcpMux);
diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc
index bdd3301..ecac7f8 100644
--- a/pc/sdp_offer_answer_unittest.cc
+++ b/pc/sdp_offer_answer_unittest.cc
@@ -238,130 +238,6 @@
"WebRTC.PeerConnection.ValidBundledPayloadTypes", false));
}
-TEST_F(SdpOfferAnswerTest, BundleMeasuresHeaderExtensionIdCollision) {
- auto pc = CreatePeerConnection();
- std::string sdp =
- "v=0\r\n"
- "o=- 0 3 IN IP4 127.0.0.1\r\n"
- "s=-\r\n"
- "t=0 0\r\n"
- "a=group:BUNDLE 0 1\r\n"
- "a=fingerprint:sha-1 "
- "4A:AD:B9:B1:3F:82:18:3B:54:02:12:DF:3E:5D:49:6B:19:E5:7C:AB\r\n"
- "a=setup:actpass\r\n"
- "a=ice-ufrag:ETEn\r\n"
- "a=ice-pwd:OtSK0WpNtpUjkY4+86js7Z/l\r\n"
- "m=audio 9 UDP/TLS/RTP/SAVPF 111\r\n"
- "c=IN IP4 0.0.0.0\r\n"
- "a=rtcp-mux\r\n"
- "a=sendonly\r\n"
- "a=mid:0\r\n"
- "a=rtpmap:111 opus/48000/2\r\n"
- "a=extmap:3 "
- "http://www.ietf.org/id/"
- "draft-holmer-rmcat-transport-wide-cc-extensions-01\r\n"
- "m=video 9 UDP/TLS/RTP/SAVPF 112\r\n"
- "c=IN IP4 0.0.0.0\r\n"
- "a=rtcp-mux\r\n"
- "a=sendonly\r\n"
- "a=mid:1\r\n"
- "a=rtpmap:112 VP8/90000\r\n"
- "a=extmap:3 "
- "http://www.ietf.org/id/"
- "draft-holmer-rmcat-transport-wide-cc-extensions-01\r\n";
- auto desc = CreateSessionDescription(SdpType::kOffer, sdp);
- ASSERT_NE(desc, nullptr);
- RTCError error;
- pc->SetRemoteDescription(std::move(desc), &error);
- EXPECT_TRUE(error.ok());
- EXPECT_METRIC_EQ(1,
- webrtc::metrics::NumEvents(
- "WebRTC.PeerConnection.ValidBundledExtensionIds", true));
-}
-
-// extmap:3 is used with two different URIs which is not allowed.
-TEST_F(SdpOfferAnswerTest, BundleRejectsHeaderExtensionIdCollision) {
- auto pc = CreatePeerConnection();
- std::string sdp =
- "v=0\r\n"
- "o=- 0 3 IN IP4 127.0.0.1\r\n"
- "s=-\r\n"
- "t=0 0\r\n"
- "a=group:BUNDLE 0 1\r\n"
- "a=fingerprint:sha-1 "
- "4A:AD:B9:B1:3F:82:18:3B:54:02:12:DF:3E:5D:49:6B:19:E5:7C:AB\r\n"
- "a=setup:actpass\r\n"
- "a=ice-ufrag:ETEn\r\n"
- "a=ice-pwd:OtSK0WpNtpUjkY4+86js7Z/l\r\n"
- "m=audio 9 UDP/TLS/RTP/SAVPF 111\r\n"
- "c=IN IP4 0.0.0.0\r\n"
- "a=rtcp-mux\r\n"
- "a=sendonly\r\n"
- "a=mid:0\r\n"
- "a=rtpmap:111 opus/48000/2\r\n"
- "a=extmap:3 "
- "http://www.ietf.org/id/"
- "draft-holmer-rmcat-transport-wide-cc-extensions-01\r\n"
- "m=video 9 UDP/TLS/RTP/SAVPF 112\r\n"
- "c=IN IP4 0.0.0.0\r\n"
- "a=rtcp-mux\r\n"
- "a=sendonly\r\n"
- "a=mid:1\r\n"
- "a=rtpmap:112 VP8/90000\r\n"
- "a=extmap:3 urn:3gpp:video-orientation\r\n";
- auto desc = CreateSessionDescription(SdpType::kOffer, sdp);
- ASSERT_NE(desc, nullptr);
- RTCError error;
- pc->SetRemoteDescription(std::move(desc), &error);
- EXPECT_TRUE(error.ok());
- EXPECT_METRIC_EQ(
- 1, webrtc::metrics::NumEvents(
- "WebRTC.PeerConnection.ValidBundledExtensionIds", false));
-}
-
-// transport-wide cc is negotiated with two different ids 3 and 4.
-// This is not a good idea but tolerable.
-TEST_F(SdpOfferAnswerTest, BundleAcceptsDifferentIdsForSameExtension) {
- auto pc = CreatePeerConnection();
- std::string sdp =
- "v=0\r\n"
- "o=- 0 3 IN IP4 127.0.0.1\r\n"
- "s=-\r\n"
- "t=0 0\r\n"
- "a=group:BUNDLE 0 1\r\n"
- "a=fingerprint:sha-1 "
- "4A:AD:B9:B1:3F:82:18:3B:54:02:12:DF:3E:5D:49:6B:19:E5:7C:AB\r\n"
- "a=setup:actpass\r\n"
- "a=ice-ufrag:ETEn\r\n"
- "a=ice-pwd:OtSK0WpNtpUjkY4+86js7Z/l\r\n"
- "m=audio 9 UDP/TLS/RTP/SAVPF 111\r\n"
- "c=IN IP4 0.0.0.0\r\n"
- "a=rtcp-mux\r\n"
- "a=sendonly\r\n"
- "a=mid:0\r\n"
- "a=rtpmap:111 opus/48000/2\r\n"
- "a=extmap:3 "
- "http://www.ietf.org/id/"
- "draft-holmer-rmcat-transport-wide-cc-extensions-01\r\n"
- "m=video 9 UDP/TLS/RTP/SAVPF 112\r\n"
- "c=IN IP4 0.0.0.0\r\n"
- "a=rtcp-mux\r\n"
- "a=sendonly\r\n"
- "a=mid:1\r\n"
- "a=rtpmap:112 VP8/90000\r\n"
- "a=extmap:4 "
- "http://www.ietf.org/id/"
- "draft-holmer-rmcat-transport-wide-cc-extensions-01\r\n";
- auto desc = CreateSessionDescription(SdpType::kOffer, sdp);
- ASSERT_NE(desc, nullptr);
- RTCError error;
- pc->SetRemoteDescription(std::move(desc), &error);
- EXPECT_TRUE(error.ok());
- EXPECT_METRIC_EQ(1,
- webrtc::metrics::NumEvents(
- "WebRTC.PeerConnection.ValidBundledExtensionIds", true));
-}
-
TEST_F(SdpOfferAnswerTest, LargeMidsAreRejected) {
auto pc = CreatePeerConnection();
std::string sdp =
