From 47e38b73bb07e5b76903c767cb4cba0b810a60bc Mon Sep 17 00:00:00 2001 From: Amit Hilbuch Date: Fri, 4 Jan 2019 20:44:23 +0000 Subject: [PATCH] Revert "Refactoring MID generation to use unique string generator." This reverts commit 1c376760d83119166407913b965e2e40e9d0c5f6. Reason for revert: Breaks chromium tests. Will fix those and reland. Original change's description: > Refactoring MID generation to use unique string generator. > > Managing the list of seen mids is now deferred to a helper object. > > 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} TBR=steveanton@webrtc.org,shampson@webrtc.org,amithi@webrtc.org Change-Id: Ifdf12b7cfa95d683927ce3827fe88c74379c9f6b No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: None Reviewed-on: https://webrtc-review.googlesource.com/c/116201 Reviewed-by: Amit Hilbuch Commit-Queue: Amit Hilbuch Cr-Commit-Position: refs/heads/master@{#26139} --- 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, 64 insertions(+), 88 deletions(-) diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 7e6c3dd9d5..7f7cebd9fd 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -42,7 +42,6 @@ #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" @@ -651,6 +650,25 @@ 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 @@ -2257,27 +2275,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; } - std::string new_mid; + absl::string_view 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 = mid_generator_(); + new_mid = AllocateMid(&used_mids); source_explanation = "generated just now"; } } else { - new_mid = std::string( - GetDefaultMidForPlanB(content.media_description()->type())); + new_mid = GetDefaultMidForPlanB(content.media_description()->type()); source_explanation = "to match pre-existing behavior"; } - content.name = new_mid; + content.name = std::string(new_mid); remote_description->transport_infos()[i].content_name = std::string(new_mid); RTC_LOG(LS_INFO) << "SetRemoteDescription: Remote media section at i=" << i @@ -2775,7 +2793,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(); - mid_generator_.AddKnownId(new_content.name); + seen_mids_.insert(new_content.name); if (media_type == cricket::MEDIA_TYPE_AUDIO || media_type == cricket::MEDIA_TYPE_VIDEO) { const cricket::ContentInfo* old_local_content = nullptr; @@ -4057,6 +4075,9 @@ 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 @@ -4114,7 +4135,6 @@ 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 @@ -4129,12 +4149,12 @@ void PeerConnection::GetOptionsForUnifiedPlanOffer( recycleable_mline_indices.pop(); session_options->media_description_options[mline_index] = GetMediaDescriptionOptionsForTransceiver(transceiver, - mid_generator_()); + AllocateMid(&used_mids)); } else { mline_index = session_options->media_description_options.size(); session_options->media_description_options.push_back( GetMediaDescriptionOptionsForTransceiver(transceiver, - mid_generator_())); + AllocateMid(&used_mids))); } // See comment above for why CreateOffer changes the transceiver's state. transceiver->internal()->set_mline_index(mline_index); @@ -4143,7 +4163,7 @@ void PeerConnection::GetOptionsForUnifiedPlanOffer( // does not already exist. if (!GetDataMid() && HasDataChannels()) { session_options->media_description_options.push_back( - GetMediaDescriptionOptionsForActiveData(mid_generator_())); + GetMediaDescriptionOptionsForActiveData(AllocateMid(&used_mids))); } } diff --git a/pc/peerconnection.h b/pc/peerconnection.h index c663520490..e596f001ae 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -28,7 +28,6 @@ #include "pc/rtptransceiver.h" #include "pc/statscollector.h" #include "pc/streamcollection.h" -#include "pc/unique_id_generator.h" #include "pc/webrtcsessiondescriptionfactory.h" namespace webrtc { @@ -1058,9 +1057,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 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_; + // MIDs that have been seen either by SetLocalDescription or + // SetRemoteDescription over the life of the PeerConnection. + std::set seen_mids_; 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 619705b712..928549f380 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->CreateOfferAndSetAsLocal(); + auto offer = caller->CreateOffer(); cricket::Candidate candidate = CreateLocalUdpCandidate(kCallerAddress); AddCandidateToFirstTransport(&candidate, offer.get()); @@ -764,8 +764,7 @@ TEST_P(PeerConnectionIceTest, IceRestartOfferClearsExistingCandidate) { RTCOfferAnswerOptions options; options.ice_restart = true; - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(options))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer(options))); EXPECT_EQ(0u, callee->GetIceCandidatesFromRemoteDescription().size()); } @@ -777,7 +776,7 @@ TEST_P(PeerConnectionIceTest, auto caller = CreatePeerConnectionWithAudioVideo(); auto callee = CreatePeerConnectionWithAudioVideo(); - auto offer = caller->CreateOfferAndSetAsLocal(); + auto offer = caller->CreateOffer(); cricket::Candidate old_candidate = CreateLocalUdpCandidate(kFirstCallerAddress); AddCandidateToFirstTransport(&old_candidate, offer.get()); @@ -786,7 +785,7 @@ TEST_P(PeerConnectionIceTest, RTCOfferAnswerOptions options; options.ice_restart = true; - auto restart_offer = caller->CreateOfferAndSetAsLocal(options); + auto restart_offer = caller->CreateOffer(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 7d49fecd21..1b78ec296e 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->CreateOfferAndSetAsLocal())); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); 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->CreateOfferAndSetAsLocal())); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); 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->CreateOfferAndSetAsLocal())); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); 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->CreateOfferAndSetAsLocal())); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); 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->CreateAnswerAndSetAsLocal()); + ASSERT_TRUE(callee->SetLocalDescription(callee->CreateAnswer())); // 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 6927bbeadc..b381d44e22 100644 --- a/pc/unique_id_generator.cc +++ b/pc/unique_id_generator.cc @@ -19,6 +19,20 @@ 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) @@ -36,17 +50,10 @@ 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) { - for (const std::string& str : known_ids) { - AddKnownId(str); - } -} + rtc::ArrayView known_ids) + : unique_number_generator_(CreateUniqueNumberGenerator(known_ids)) {} UniqueStringGenerator::~UniqueStringGenerator() = default; @@ -54,13 +61,4 @@ 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 cd6e085db0..15ba79446b 100644 --- a/pc/unique_id_generator.h +++ b/pc/unique_id_generator.h @@ -42,12 +42,13 @@ 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_; }; @@ -70,9 +71,6 @@ 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_; }; @@ -92,9 +90,6 @@ 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_; @@ -122,10 +117,6 @@ 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 f3cba7675b..86d831b9ae 100644 --- a/pc/unique_id_generator_unittest.cc +++ b/pc/unique_id_generator_unittest.cc @@ -76,35 +76,4 @@ 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