Dynamically assign ids to header extensions not enabled by default
by creating an id collision and letting UsedIds resolve it.
Also avoid id=15 which is forbidden by
https://www.rfc-editor.org/rfc/rfc8285#section-4.2
so might cause interop issues in offers to implementations
not supporting two-byte extensions.
BUG=webrtc:15378
Change-Id: I27926f065f8e396257294da7acf2be9802169805
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/319280
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#41696}
diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc
index 902b536..a313407 100644
--- a/media/engine/webrtc_video_engine.cc
+++ b/media/engine/webrtc_video_engine.cc
@@ -811,6 +811,8 @@
std::vector<webrtc::RtpHeaderExtensionCapability>
WebRtcVideoEngine::GetRtpHeaderExtensions() const {
std::vector<webrtc::RtpHeaderExtensionCapability> result;
+ // id is *not* incremented for non-default extensions, UsedIds needs to
+ // resolve conflicts.
int id = 1;
for (const auto& uri :
{webrtc::RtpExtension::kTimestampOffsetUri,
@@ -825,27 +827,26 @@
result.emplace_back(uri, id++, webrtc::RtpTransceiverDirection::kSendRecv);
}
for (const auto& uri : {webrtc::RtpExtension::kAbsoluteCaptureTimeUri}) {
- result.emplace_back(uri, id++, webrtc::RtpTransceiverDirection::kStopped);
+ result.emplace_back(uri, id, webrtc::RtpTransceiverDirection::kStopped);
}
- result.emplace_back(webrtc::RtpExtension::kGenericFrameDescriptorUri00, id++,
+ result.emplace_back(webrtc::RtpExtension::kGenericFrameDescriptorUri00, id,
IsEnabled(trials_, "WebRTC-GenericDescriptorAdvertised")
? webrtc::RtpTransceiverDirection::kSendRecv
: webrtc::RtpTransceiverDirection::kStopped);
result.emplace_back(
- webrtc::RtpExtension::kDependencyDescriptorUri, id++,
+ webrtc::RtpExtension::kDependencyDescriptorUri, id,
IsEnabled(trials_, "WebRTC-DependencyDescriptorAdvertised")
? webrtc::RtpTransceiverDirection::kSendRecv
: webrtc::RtpTransceiverDirection::kStopped);
-
result.emplace_back(
- webrtc::RtpExtension::kVideoLayersAllocationUri, id++,
+ webrtc::RtpExtension::kVideoLayersAllocationUri, id,
IsEnabled(trials_, "WebRTC-VideoLayersAllocationAdvertised")
? webrtc::RtpTransceiverDirection::kSendRecv
: webrtc::RtpTransceiverDirection::kStopped);
// VideoFrameTrackingId is a test-only extension.
if (IsEnabled(trials_, "WebRTC-VideoFrameTrackingIdAdvertised")) {
- result.emplace_back(webrtc::RtpExtension::kVideoFrameTrackingIdUri, id++,
+ result.emplace_back(webrtc::RtpExtension::kVideoFrameTrackingIdUri, id,
webrtc::RtpTransceiverDirection::kSendRecv);
}
return result;
diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc
index fcc2703..23a1b19 100644
--- a/media/engine/webrtc_voice_engine.cc
+++ b/media/engine/webrtc_voice_engine.cc
@@ -643,6 +643,8 @@
WebRtcVoiceEngine::GetRtpHeaderExtensions() const {
RTC_DCHECK(signal_thread_checker_.IsCurrent());
std::vector<webrtc::RtpHeaderExtensionCapability> result;
+ // id is *not* incremented for non-default extensions, UsedIds needs to
+ // resolve conflicts.
int id = 1;
for (const auto& uri : {webrtc::RtpExtension::kAudioLevelUri,
webrtc::RtpExtension::kAbsSendTimeUri,
@@ -651,7 +653,7 @@
result.emplace_back(uri, id++, webrtc::RtpTransceiverDirection::kSendRecv);
}
for (const auto& uri : {webrtc::RtpExtension::kAbsoluteCaptureTimeUri}) {
- result.emplace_back(uri, id++, webrtc::RtpTransceiverDirection::kStopped);
+ result.emplace_back(uri, id, webrtc::RtpTransceiverDirection::kStopped);
}
return result;
}
diff --git a/pc/used_ids.h b/pc/used_ids.h
index 6b342cb..42ef00a 100644
--- a/pc/used_ids.h
+++ b/pc/used_ids.h
@@ -147,15 +147,15 @@
private:
// Returns the first unused id in reverse order from the max id of one byte
- // header extensions. This hopefully reduce the risk of more collisions. We
+ // header extensions. This hopefully reduces the risk of more collisions. We
// want to change the default ids as little as possible. If no unused id is
// found and two byte header extensions are enabled (i.e.,
- // `extmap_allow_mixed_` is true), search for unused ids from 15 to 255.
+ // `extmap_allow_mixed_` is true), search for unused ids from 16 to 255.
int FindUnusedId() override {
if (next_extension_id_ <=
webrtc::RtpExtension::kOneByteHeaderExtensionMaxId) {
// First search in reverse order from the max id of one byte header
- // extensions.
+ // extensions (14).
while (IsIdUsed(next_extension_id_) &&
next_extension_id_ >= min_allowed_id_) {
--next_extension_id_;
@@ -165,9 +165,10 @@
if (id_domain_ == IdDomain::kTwoByteAllowed) {
if (next_extension_id_ < min_allowed_id_) {
// We have searched among all one-byte IDs without finding an unused ID,
- // continue at the first two-byte ID.
+ // continue at the first two-byte ID (16; avoid 15 since it is somewhat
+ // special per https://www.rfc-editor.org/rfc/rfc8285#section-4.2
next_extension_id_ =
- webrtc::RtpExtension::kOneByteHeaderExtensionMaxId + 1;
+ webrtc::RtpExtension::kOneByteHeaderExtensionMaxId + 2;
}
if (next_extension_id_ >
diff --git a/pc/used_ids_unittest.cc b/pc/used_ids_unittest.cc
index 6362f27..df3790b 100644
--- a/pc/used_ids_unittest.cc
+++ b/pc/used_ids_unittest.cc
@@ -119,7 +119,8 @@
UsedRtpHeaderExtensionIds::IdDomain::kTwoByteAllowed);
// Fill all one byte IDs.
- for (int i = 1; i < 15; ++i) {
+ for (int i = 1; i <= webrtc::RtpExtension::kOneByteHeaderExtensionMaxId;
+ ++i) {
webrtc::RtpExtension id("", i);
used_ids.FindAndSetIdUsed(&id);
}
@@ -131,11 +132,11 @@
// Expect to reassign to two-byte header extension IDs.
used_ids.FindAndSetIdUsed(&id1_collision);
- EXPECT_EQ(id1_collision.id, 15);
+ EXPECT_EQ(id1_collision.id, 16);
used_ids.FindAndSetIdUsed(&id2_collision);
- EXPECT_EQ(id2_collision.id, 16);
+ EXPECT_EQ(id2_collision.id, 17);
used_ids.FindAndSetIdUsed(&id3_collision);
- EXPECT_EQ(id3_collision.id, 17);
+ EXPECT_EQ(id3_collision.id, 18);
}
// Death tests.
