admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Revert "sdp: measure rtp header extension collisions"

Browse Source 
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}
This commit is contained in:
Andrey Logvin 2022-12-19 09:30:50 +00:00 committed by WebRTC LUCI CQ
parent 6c27d56a2a
commit de57c57e1e
2 changed files with 1 additions and 177 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

54
pc/sdp_offer_answer.cc
View File

@ -491,52 +491,6 @@ RTCError ValidateBundledPayloadTypes(
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 @@ RTCError SdpOfferAnswerHandler::ValidateSessionDescription(
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);

124
pc/sdp_offer_answer_unittest.cc
View File

@ -238,130 +238,6 @@ TEST_F(SdpOfferAnswerTest, BundleCodecCollisionInDifferentBundlesAllowed) {
"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 =