From a5b273a635b9876f88430934de19a883a1fb5728 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Thu, 20 Aug 2015 17:30:13 -0700 Subject: [PATCH] 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 | 60 ++++++++++++++------- talk/session/media/mediasession_unittest.cc | 50 +++++++++++++++++ 2 files changed, 91 insertions(+), 19 deletions(-) diff --git a/talk/session/media/mediasession.cc b/talk/session/media/mediasession.cc index 80cc2d7ad7..66d0caf9d7 100644 --- a/talk/session/media/mediasession.cc +++ b/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( diff --git a/talk/session/media/mediasession_unittest.cc b/talk/session/media/mediasession_unittest.cc index fd0dbb2972..ededa8a680 100644 --- a/talk/session/media/mediasession_unittest.cc +++ b/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 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 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);