From ae3df54f671281dd0da30376e39f5117ee237588 Mon Sep 17 00:00:00 2001 From: Amit Hilbuch Date: Mon, 7 Jan 2019 12:13:08 -0800 Subject: [PATCH] Refactoring MID generation to use unique string generator. This changes the MIDs that are generated if calling createOffer twice without setting a local or remote description. Managing the list of seen mids is now deferred to a helper object. This is a reland of 1c376760d83119166407913b965e2e40e9d0c5f6. > Bug: None > Change-Id: I3440d62129884ae49aefd18e03c3a55ae096d923 > Reviewed-on: https://webrtc-review.googlesource.com/c/116021 > Commit-Queue: Amit Hilbuch > Reviewed-by: Steve Anton > Reviewed-by: Seth Hampson > Cr-Commit-Position: refs/heads/master@{#26130} Bug: None Change-Id: Ic8b07a252869f67a476e3af84b8072b7a130f7fd Reviewed-on: https://webrtc-review.googlesource.com/c/116381 Reviewed-by: Steve Anton Commit-Queue: Amit Hilbuch Cr-Commit-Position: refs/heads/master@{#26151} --- pc/peerconnection.cc | 42 ++++++++---------------------- pc/peerconnection.h | 7 ++--- pc/peerconnection_ice_unittest.cc | 9 ++++--- pc/peerconnection_rtp_unittest.cc | 12 ++++----- pc/unique_id_generator.cc | 34 ++++++++++++------------ pc/unique_id_generator.h | 17 +++++++++--- pc/unique_id_generator_unittest.cc | 31 ++++++++++++++++++++++ 7 files changed, 88 insertions(+), 64 deletions(-) diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 7f7cebd9fd..7e6c3dd9d5 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -42,6 +42,7 @@ #include "pc/sctputils.h" #include "pc/sdputils.h" #include "pc/streamcollection.h" +#include "pc/unique_id_generator.h" #include "pc/videocapturertracksource.h" #include "pc/videotrack.h" #include "rtc_base/bind.h" @@ -650,25 +651,6 @@ DataMessageType ToWebrtcDataMessageType(cricket::DataMessageType type) { return DataMessageType::kControl; } -// Find a new MID that is not already in |used_mids|, then add it to |used_mids| -// and return a reference to it. -// Generated MIDs should be no more than 3 bytes long to take up less space in -// the RTP packet. -const std::string& AllocateMid(std::set* used_mids) { - RTC_DCHECK(used_mids); - // We're boring: just generate MIDs 0, 1, 2, ... - size_t i = 0; - std::set::iterator it; - bool inserted; - do { - std::string mid = rtc::ToString(i++); - auto insert_result = used_mids->insert(mid); - it = insert_result.first; - inserted = insert_result.second; - } while (!inserted); - return *it; -} - } // namespace // Upon completion, posts a task to execute the callback of the @@ -2275,27 +2257,27 @@ void PeerConnection::FillInMissingRemoteMids( const cricket::ContentInfos& local_contents = (local_description() ? local_description()->description()->contents() : cricket::ContentInfos()); - std::set used_mids = seen_mids_; for (size_t i = 0; i < remote_description->contents().size(); ++i) { cricket::ContentInfo& content = remote_description->contents()[i]; if (!content.name.empty()) { continue; } - absl::string_view new_mid; + std::string new_mid; absl::string_view source_explanation; if (IsUnifiedPlan()) { if (i < local_contents.size()) { new_mid = local_contents[i].name; source_explanation = "from the matching local media section"; } else { - new_mid = AllocateMid(&used_mids); + new_mid = mid_generator_(); source_explanation = "generated just now"; } } else { - new_mid = GetDefaultMidForPlanB(content.media_description()->type()); + new_mid = std::string( + GetDefaultMidForPlanB(content.media_description()->type())); source_explanation = "to match pre-existing behavior"; } - content.name = std::string(new_mid); + content.name = new_mid; remote_description->transport_infos()[i].content_name = std::string(new_mid); RTC_LOG(LS_INFO) << "SetRemoteDescription: Remote media section at i=" << i @@ -2793,7 +2775,7 @@ RTCError PeerConnection::UpdateTransceiversAndDataChannels( for (size_t i = 0; i < new_contents.size(); ++i) { const cricket::ContentInfo& new_content = new_contents[i]; cricket::MediaType media_type = new_content.media_description()->type(); - seen_mids_.insert(new_content.name); + mid_generator_.AddKnownId(new_content.name); if (media_type == cricket::MEDIA_TYPE_AUDIO || media_type == cricket::MEDIA_TYPE_VIDEO) { const cricket::ContentInfo* old_local_content = nullptr; @@ -4075,9 +4057,6 @@ void PeerConnection::GetOptionsForUnifiedPlanOffer( // The mline indices that can be recycled. New transceivers should reuse these // slots first. std::queue recycleable_mline_indices; - // Track the MIDs used in previous offer/answer exchanges and the current - // offer so that new, unique MIDs are generated. - std::set used_mids = seen_mids_; // First, go through each media section that exists in either the local or // remote description and generate a media section in this offer for the // associated transceiver. If a media section can be recycled, generate a @@ -4135,6 +4114,7 @@ void PeerConnection::GetOptionsForUnifiedPlanOffer( } } } + // Next, look for transceivers that are newly added (that is, are not stopped // and not associated). Reuse media sections marked as recyclable first, // otherwise append to the end of the offer. New media sections should be @@ -4149,12 +4129,12 @@ void PeerConnection::GetOptionsForUnifiedPlanOffer( recycleable_mline_indices.pop(); session_options->media_description_options[mline_index] = GetMediaDescriptionOptionsForTransceiver(transceiver, - AllocateMid(&used_mids)); + mid_generator_()); } else { mline_index = session_options->media_description_options.size(); session_options->media_description_options.push_back( GetMediaDescriptionOptionsForTransceiver(transceiver, - AllocateMid(&used_mids))); + mid_generator_())); } // See comment above for why CreateOffer changes the transceiver's state. transceiver->internal()->set_mline_index(mline_index); @@ -4163,7 +4143,7 @@ void PeerConnection::GetOptionsForUnifiedPlanOffer( // does not already exist. if (!GetDataMid() && HasDataChannels()) { session_options->media_description_options.push_back( - GetMediaDescriptionOptionsForActiveData(AllocateMid(&used_mids))); + GetMediaDescriptionOptionsForActiveData(mid_generator_())); } } diff --git a/pc/peerconnection.h b/pc/peerconnection.h index e596f001ae..c663520490 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -28,6 +28,7 @@ #include "pc/rtptransceiver.h" #include "pc/statscollector.h" #include "pc/streamcollection.h" +#include "pc/unique_id_generator.h" #include "pc/webrtcsessiondescriptionfactory.h" namespace webrtc { @@ -1057,9 +1058,9 @@ class PeerConnection : public PeerConnectionInternal, // to support legacy endpoints that do not support the a=msid attribute (as // opposed to streamless tracks with "a=msid:-"). rtc::scoped_refptr missing_msid_default_stream_; - // MIDs that have been seen either by SetLocalDescription or - // SetRemoteDescription over the life of the PeerConnection. - std::set seen_mids_; + // MIDs will be generated using this generator which will keep track of + // all the MIDs that have been seen over the life of the PeerConnection. + UniqueStringGenerator mid_generator_; SessionError session_error_ = SessionError::kNone; std::string session_error_desc_; diff --git a/pc/peerconnection_ice_unittest.cc b/pc/peerconnection_ice_unittest.cc index 928549f380..619705b712 100644 --- a/pc/peerconnection_ice_unittest.cc +++ b/pc/peerconnection_ice_unittest.cc @@ -756,7 +756,7 @@ TEST_P(PeerConnectionIceTest, IceRestartOfferClearsExistingCandidate) { auto caller = CreatePeerConnectionWithAudioVideo(); auto callee = CreatePeerConnectionWithAudioVideo(); - auto offer = caller->CreateOffer(); + auto offer = caller->CreateOfferAndSetAsLocal(); cricket::Candidate candidate = CreateLocalUdpCandidate(kCallerAddress); AddCandidateToFirstTransport(&candidate, offer.get()); @@ -764,7 +764,8 @@ TEST_P(PeerConnectionIceTest, IceRestartOfferClearsExistingCandidate) { RTCOfferAnswerOptions options; options.ice_restart = true; - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer(options))); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(options))); EXPECT_EQ(0u, callee->GetIceCandidatesFromRemoteDescription().size()); } @@ -776,7 +777,7 @@ TEST_P(PeerConnectionIceTest, auto caller = CreatePeerConnectionWithAudioVideo(); auto callee = CreatePeerConnectionWithAudioVideo(); - auto offer = caller->CreateOffer(); + auto offer = caller->CreateOfferAndSetAsLocal(); cricket::Candidate old_candidate = CreateLocalUdpCandidate(kFirstCallerAddress); AddCandidateToFirstTransport(&old_candidate, offer.get()); @@ -785,7 +786,7 @@ TEST_P(PeerConnectionIceTest, RTCOfferAnswerOptions options; options.ice_restart = true; - auto restart_offer = caller->CreateOffer(options); + auto restart_offer = caller->CreateOfferAndSetAsLocal(options); cricket::Candidate new_candidate = CreateLocalUdpCandidate(kRestartedCallerAddress); AddCandidateToFirstTransport(&new_candidate, restart_offer.get()); diff --git a/pc/peerconnection_rtp_unittest.cc b/pc/peerconnection_rtp_unittest.cc index 1b78ec296e..7d49fecd21 100644 --- a/pc/peerconnection_rtp_unittest.cc +++ b/pc/peerconnection_rtp_unittest.cc @@ -426,10 +426,10 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, caller->AddAudioTrack("audio_track", {}); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); EXPECT_EQ(1u, callee->observer()->add_track_events_.size()); } @@ -443,13 +443,13 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, auto sender = caller->AddAudioTrack("audio_track", {}); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); EXPECT_EQ(0u, callee->observer()->remove_track_events_.size()); caller->pc()->RemoveTrack(sender); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); EXPECT_EQ(1u, callee->observer()->add_track_events_.size()); EXPECT_EQ(1u, callee->observer()->remove_track_events_.size()); } @@ -480,14 +480,14 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, ChangeMsidWhileReceiving) { auto caller = CreatePeerConnection(); caller->AddAudioTrack("audio_track", {"stream1"}); auto callee = CreatePeerConnection(); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_EQ(1u, callee->observer()->on_track_transceivers_.size()); auto transceiver = callee->observer()->on_track_transceivers_[0]; ASSERT_EQ(1u, transceiver->receiver()->streams().size()); EXPECT_EQ("stream1", transceiver->receiver()->streams()[0]->id()); - ASSERT_TRUE(callee->SetLocalDescription(callee->CreateAnswer())); + ASSERT_TRUE(callee->CreateAnswerAndSetAsLocal()); // Change the stream ID in the offer. // TODO(https://crbug.com/webrtc/10129): When RtpSenderInterface::SetStreams diff --git a/pc/unique_id_generator.cc b/pc/unique_id_generator.cc index b381d44e22..6927bbeadc 100644 --- a/pc/unique_id_generator.cc +++ b/pc/unique_id_generator.cc @@ -19,20 +19,6 @@ namespace webrtc { -namespace { -UniqueNumberGenerator CreateUniqueNumberGenerator( - rtc::ArrayView known_ids) { - std::vector known_ints; - for (const std::string& str : known_ids) { - absl::optional value = rtc::StringToNumber(str); - if (value.has_value()) { - known_ints.push_back(value.value()); - } - } - return UniqueNumberGenerator(known_ints); -} -} // namespace - UniqueRandomIdGenerator::UniqueRandomIdGenerator() : known_ids_() {} UniqueRandomIdGenerator::UniqueRandomIdGenerator( rtc::ArrayView known_ids) @@ -50,10 +36,17 @@ uint32_t UniqueRandomIdGenerator::GenerateId() { } } +void UniqueRandomIdGenerator::AddKnownId(uint32_t value) { + known_ids_.insert(value); +} + UniqueStringGenerator::UniqueStringGenerator() : unique_number_generator_() {} UniqueStringGenerator::UniqueStringGenerator( - rtc::ArrayView known_ids) - : unique_number_generator_(CreateUniqueNumberGenerator(known_ids)) {} + rtc::ArrayView known_ids) { + for (const std::string& str : known_ids) { + AddKnownId(str); + } +} UniqueStringGenerator::~UniqueStringGenerator() = default; @@ -61,4 +54,13 @@ std::string UniqueStringGenerator::GenerateString() { return rtc::ToString(unique_number_generator_.GenerateNumber()); } +void UniqueStringGenerator::AddKnownId(const std::string& value) { + absl::optional int_value = rtc::StringToNumber(value); + // The underlying generator works for uint32_t values, so if the provided + // value is not a uint32_t it will never be generated anyway. + if (int_value.has_value()) { + unique_number_generator_.AddKnownId(int_value.value()); + } +} + } // namespace webrtc diff --git a/pc/unique_id_generator.h b/pc/unique_id_generator.h index 15ba79446b..cd6e085db0 100644 --- a/pc/unique_id_generator.h +++ b/pc/unique_id_generator.h @@ -42,13 +42,12 @@ class UniqueNumberGenerator { TIntegral GenerateNumber(); TIntegral operator()() { return GenerateNumber(); } + // Adds an id that this generator should no longer generate. + void AddKnownId(TIntegral value); + private: static_assert(std::is_integral::value, "Must be integral type."); TIntegral counter_; - // This class can be further optimized by removing the known_ids_ set when - // the generator was created without a sequence of ids to ignore. - // In such a case, the implementation uses a counter which is sufficient to - // prevent repetitions of the generated values. std::set known_ids_; }; @@ -71,6 +70,9 @@ class UniqueRandomIdGenerator { uint32_t GenerateId(); uint32_t operator()() { return GenerateId(); } + // Adds an id that this generator should no longer generate. + void AddKnownId(uint32_t value); + private: std::set known_ids_; }; @@ -90,6 +92,9 @@ class UniqueStringGenerator { std::string GenerateString(); std::string operator()() { return GenerateString(); } + // Adds an id that this generator should no longer generate. + void AddKnownId(const std::string& value); + private: // This implementation will be simple and will generate "0", "1", ... UniqueNumberGenerator unique_number_generator_; @@ -117,6 +122,10 @@ TIntegral UniqueNumberGenerator::GenerateNumber() { } } +template +void UniqueNumberGenerator::AddKnownId(TIntegral value) { + known_ids_.insert(value); +} } // namespace webrtc #endif // PC_UNIQUE_ID_GENERATOR_H_ diff --git a/pc/unique_id_generator_unittest.cc b/pc/unique_id_generator_unittest.cc index 86d831b9ae..f3cba7675b 100644 --- a/pc/unique_id_generator_unittest.cc +++ b/pc/unique_id_generator_unittest.cc @@ -76,4 +76,35 @@ TYPED_TEST(UniqueIdGeneratorTest, KnownElementsAreNotGenerated) { EXPECT_THAT(intersection, IsEmpty()); } +TYPED_TEST(UniqueIdGeneratorTest, AddedElementsAreNotGenerated) { + typedef TypeParam Generator; + const size_t num_elements = 100; + rtc::InitRandom(0); + Generator generator1; + std::vector known_values; + for (size_t i = 0; i < num_elements; i++) { + known_values.push_back(generator1()); + } + EXPECT_EQ(num_elements, known_values.size()); + + rtc::InitRandom(0); + Generator generator2; + + for (typename Generator::value_type value : known_values) { + generator2.AddKnownId(value); + } + + std::vector values; + for (size_t i = 0; i < num_elements; i++) { + values.push_back(generator2()); + } + EXPECT_THAT(values, ::testing::SizeIs(num_elements)); + std::sort(values.begin(), values.end()); + std::sort(known_values.begin(), known_values.end()); + std::vector intersection; + std::set_intersection(values.begin(), values.end(), known_values.begin(), + known_values.end(), std::back_inserter(intersection)); + EXPECT_THAT(intersection, IsEmpty()); +} + } // namespace webrtc