Fixing problems with RTP extension ID conflict resolution

If the same extension URI is used for both audio and video (such as
abs-send-time), we should be able to re-use the same ID. A conflict
only exists if two different URIs are attempting to use the same ID.

Review URL: https://codereview.webrtc.org/1286273003

Cr-Commit-Position: refs/heads/master@{#9749}

talk/session/media/mediasession.cc

@@ -907,20 +907,41 @@ static bool FindByUri(const RtpHeaderExtensions& extensions,
   return false;
 }
 
-static void FindAndSetRtpHdrExtUsed(
-  const RtpHeaderExtensions& reference_extensions,
-  RtpHeaderExtensions* offered_extensions,
-  const RtpHeaderExtensions& other_extensions,
-  UsedRtpHeaderExtensionIds* used_extensions) {
-  for (RtpHeaderExtensions::const_iterator it = reference_extensions.begin();
-      it != reference_extensions.end(); ++it) {
-    if (!FindByUri(*offered_extensions, *it, NULL)) {
-      RtpHeaderExtension ext;
-      if (!FindByUri(other_extensions, *it, &ext)) {
-        ext = *it;
-        used_extensions->FindAndSetIdUsed(&ext);
+// Iterates through |offered_extensions|, adding each one to |all_extensions|
+// and |used_ids|, and resolving ID conflicts. If an offered extension has the
+// same URI as one in |all_extensions|, it will re-use the same ID and won't be
+// treated as a conflict.
+static void FindAndSetRtpHdrExtUsed(RtpHeaderExtensions* offered_extensions,
+                                    RtpHeaderExtensions* all_extensions,
+                                    UsedRtpHeaderExtensionIds* used_ids) {
+  for (auto& extension : *offered_extensions) {
+    RtpHeaderExtension existing;
+    if (FindByUri(*all_extensions, extension, &existing)) {
+      extension.id = existing.id;
+    } else {
+      used_ids->FindAndSetIdUsed(&extension);
+      all_extensions->push_back(extension);
+    }
+  }
+}
+
+// Adds |reference_extensions| to |offered_extensions|, while updating
+// |all_extensions| and |used_ids|.
+static void FindRtpHdrExtsToOffer(
+    const RtpHeaderExtensions& reference_extensions,
+    RtpHeaderExtensions* offered_extensions,
+    RtpHeaderExtensions* all_extensions,
+    UsedRtpHeaderExtensionIds* used_ids) {
+  for (auto reference_extension : reference_extensions) {
+    if (!FindByUri(*offered_extensions, reference_extension, NULL)) {
+      RtpHeaderExtension existing;
+      if (FindByUri(*all_extensions, reference_extension, &existing)) {
+        offered_extensions->push_back(existing);
+      } else {
+        used_ids->FindAndSetIdUsed(&reference_extension);
+        all_extensions->push_back(reference_extension);
+        offered_extensions->push_back(reference_extension);
       }
-      offered_extensions->push_back(ext);
     }
   }
 }
@@ -1398,6 +1419,7 @@ void MediaSessionDescriptionFactory::GetRtpHdrExtsToOffer(
   // All header extensions allocated from the same range to avoid potential
   // issues when using BUNDLE.
   UsedRtpHeaderExtensionIds used_ids;
+  RtpHeaderExtensions all_extensions;
   audio_extensions->clear();
   video_extensions->clear();
 
@@ -1410,22 +1432,22 @@ void MediaSessionDescriptionFactory::GetRtpHdrExtsToOffer(
         GetFirstAudioContentDescription(current_description);
     if (audio) {
       *audio_extensions = audio->rtp_header_extensions();
-      used_ids.FindAndSetIdUsed(audio_extensions);
+      FindAndSetRtpHdrExtUsed(audio_extensions, &all_extensions, &used_ids);
     }
     const VideoContentDescription* video =
         GetFirstVideoContentDescription(current_description);
     if (video) {
       *video_extensions = video->rtp_header_extensions();
-      used_ids.FindAndSetIdUsed(video_extensions);
+      FindAndSetRtpHdrExtUsed(video_extensions, &all_extensions, &used_ids);
     }
   }
 
   // Add our default RTP header extensions that are not in
   // |current_description|.
-  FindAndSetRtpHdrExtUsed(audio_rtp_header_extensions(), audio_extensions,
-                          *video_extensions, &used_ids);
-  FindAndSetRtpHdrExtUsed(video_rtp_header_extensions(), video_extensions,
-                          *audio_extensions, &used_ids);
+  FindRtpHdrExtsToOffer(audio_rtp_header_extensions(), audio_extensions,
+                        &all_extensions, &used_ids);
+  FindRtpHdrExtsToOffer(video_rtp_header_extensions(), video_extensions,
+                        &all_extensions, &used_ids);
 }
 
 bool MediaSessionDescriptionFactory::AddTransportOffer(

talk/session/media/mediasession_unittest.cc

@@ -147,6 +147,11 @@ static const RtpHeaderExtension kAudioRtpExtension2[] = {
   RtpHeaderExtension("http://google.com/testing/both_audio_and_video", 7),
 };
 
+static const RtpHeaderExtension kAudioRtpExtension3[] = {
+  RtpHeaderExtension("http://google.com/testing/audio_something", 2),
+  RtpHeaderExtension("http://google.com/testing/both_audio_and_video", 3),
+};
+
 static const RtpHeaderExtension kAudioRtpExtensionAnswer[] = {
   RtpHeaderExtension("urn:ietf:params:rtp-hdrext:ssrc-audio-level", 8),
 };
@@ -162,6 +167,11 @@ static const RtpHeaderExtension kVideoRtpExtension2[] = {
   RtpHeaderExtension("http://google.com/testing/both_audio_and_video", 7),
 };
 
+static const RtpHeaderExtension kVideoRtpExtension3[] = {
+  RtpHeaderExtension("http://google.com/testing/video_something", 4),
+  RtpHeaderExtension("http://google.com/testing/both_audio_and_video", 5),
+};
+
 static const RtpHeaderExtension kVideoRtpExtensionAnswer[] = {
   RtpHeaderExtension("urn:ietf:params:rtp-hdrext:toffset", 14),
 };
@@ -1863,6 +1873,46 @@ TEST_F(MediaSessionDescriptionFactoryTest,
             updated_vcd->rtp_header_extensions());
 }
 
+// Verify that if the same RTP extension URI is used for audio and video, the
+// same ID is used. Also verify that the ID isn't changed when creating an
+// updated offer (this was previously a bug).
+TEST_F(MediaSessionDescriptionFactoryTest,
+       RtpHeaderExtensionIdReused) {
+  MediaSessionOptions opts;
+  opts.recv_audio = true;
+  opts.recv_video = true;
+
+  f1_.set_audio_rtp_header_extensions(MAKE_VECTOR(kAudioRtpExtension3));
+  f1_.set_video_rtp_header_extensions(MAKE_VECTOR(kVideoRtpExtension3));
+
+  rtc::scoped_ptr<SessionDescription> offer(f1_.CreateOffer(opts, NULL));
+
+  // Since the audio extensions used ID 3 for "both_audio_and_video", so should
+  // the video extensions.
+  const RtpHeaderExtension kExpectedVideoRtpExtension[] = {
+    kVideoRtpExtension3[0],
+    kAudioRtpExtension3[1],
+  };
+
+  EXPECT_EQ(MAKE_VECTOR(kAudioRtpExtension3),
+            GetFirstAudioContentDescription(
+                offer.get())->rtp_header_extensions());
+  EXPECT_EQ(MAKE_VECTOR(kExpectedVideoRtpExtension),
+            GetFirstVideoContentDescription(
+                offer.get())->rtp_header_extensions());
+
+  // Nothing should change when creating a new offer
+  rtc::scoped_ptr<SessionDescription> updated_offer(
+      f1_.CreateOffer(opts, offer.get()));
+
+  EXPECT_EQ(MAKE_VECTOR(kAudioRtpExtension3),
+            GetFirstAudioContentDescription(
+                updated_offer.get())->rtp_header_extensions());
+  EXPECT_EQ(MAKE_VECTOR(kExpectedVideoRtpExtension),
+            GetFirstVideoContentDescription(
+                updated_offer.get())->rtp_header_extensions());
+}
+
 TEST(MediaSessionDescription, CopySessionDescription) {
   SessionDescription source;
   cricket::ContentGroup group(cricket::CN_AUDIO);