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}
diff --git a/talk/session/media/mediasession.cc b/talk/session/media/mediasession.cc
index 80cc2d7..66d0caf 100644
--- a/talk/session/media/mediasession.cc
+++ b/talk/session/media/mediasession.cc
@@ -907,20 +907,41 @@
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 @@
// 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 @@
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(
diff --git a/talk/session/media/mediasession_unittest.cc b/talk/session/media/mediasession_unittest.cc
index fd0dbb2..ededa8a 100644
--- a/talk/session/media/mediasession_unittest.cc
+++ b/talk/session/media/mediasession_unittest.cc
@@ -147,6 +147,11 @@
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 @@
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 @@
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);
