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}

media/engine/webrtc_video_engine.cc

@@ -811,6 +811,8 @@ std::vector<VideoCodec> WebRtcVideoEngine::recv_codecs(bool include_rtx) const {
 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 @@ WebRtcVideoEngine::GetRtpHeaderExtensions() const {
     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;

media/engine/webrtc_voice_engine.cc

@@ -643,6 +643,8 @@ std::vector<webrtc::RtpHeaderExtensionCapability>
 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 @@ WebRtcVoiceEngine::GetRtpHeaderExtensions() const {
     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;
 }

pc/used_ids.h

@@ -147,15 +147,15 @@ class UsedRtpHeaderExtensionIds : public UsedIds<webrtc::RtpExtension> {
 
  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 @@ class UsedRtpHeaderExtensionIds : public UsedIds<webrtc::RtpExtension> {
     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_ >

pc/used_ids_unittest.cc

@@ -119,7 +119,8 @@ TEST_F(UsedRtpHeaderExtensionIdsTest, TwoByteIdsAllowed) {
       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 @@ TEST_F(UsedRtpHeaderExtensionIdsTest, TwoByteIdsAllowed) {
 
   // 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.