admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Revert "Refactoring MID generation to use unique string generator."

Browse Source 
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 <amithi@webrtc.org>
> Reviewed-by: Steve Anton <steveanton@webrtc.org>
> Reviewed-by: Seth Hampson <shampson@webrtc.org>
> 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 <amithi@webrtc.org>
Commit-Queue: Amit Hilbuch <amithi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26139}
This commit is contained in:
Amit Hilbuch 2019-01-04 20:44:23 +00:00 committed by Commit Bot
parent f996c8453e
commit 47e38b73bb
7 changed files with 64 additions and 88 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
pc/peerconnection.cc
View File

@ -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<std::string>* used_mids) {
RTC_DCHECK(used_mids);
// We're boring: just generate MIDs 0, 1, 2, ...
size_t i = 0;
std::set<std::string>::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<std::string> 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<size_t> 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<std::string> 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)));
}
}

7
pc/peerconnection.h
View File

@ -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<MediaStreamInterface> 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<std::string> seen_mids_;
SessionError session_error_ = SessionError::kNone;
std::string session_error_desc_;

9
pc/peerconnection_ice_unittest.cc
View File

@ -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());

12
pc/peerconnection_rtp_unittest.cc
View File

@ -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

34
pc/unique_id_generator.cc
View File

@ -19,6 +19,20 @@
namespace webrtc {
namespace {
UniqueNumberGenerator<uint32_t> CreateUniqueNumberGenerator(
rtc::ArrayView<std::string> known_ids) {
std::vector<uint32_t> known_ints;
for (const std::string& str : known_ids) {
absl::optional<uint32_t> value = rtc::StringToNumber<uint32_t>(str);
if (value.has_value()) {
known_ints.push_back(value.value());
}
}
return UniqueNumberGenerator<uint32_t>(known_ints);
}
} // namespace
UniqueRandomIdGenerator::UniqueRandomIdGenerator() : known_ids_() {}
UniqueRandomIdGenerator::UniqueRandomIdGenerator(
rtc::ArrayView<uint32_t> 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<std::string> known_ids) {
for (const std::string& str : known_ids) {
AddKnownId(str);
}
}
rtc::ArrayView<std::string> 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<uint32_t> int_value = rtc::StringToNumber<uint32_t>(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

17
pc/unique_id_generator.h
View File

@ -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<TIntegral>::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<TIntegral> 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<uint32_t> 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<uint32_t> unique_number_generator_;
@ -122,10 +117,6 @@ TIntegral UniqueNumberGenerator<TIntegral>::GenerateNumber() {
}
}
template <typename TIntegral>
void UniqueNumberGenerator<TIntegral>::AddKnownId(TIntegral value) {
known_ids_.insert(value);
}
} // namespace webrtc
#endif // PC_UNIQUE_ID_GENERATOR_H_

31
pc/unique_id_generator_unittest.cc
View File

@ -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<typename Generator::value_type> 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<typename Generator::value_type> 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<typename Generator::value_type> intersection;
std::set_intersection(values.begin(), values.end(), known_values.begin(),
known_values.end(), std::back_inserter(intersection));
EXPECT_THAT(intersection, IsEmpty());
}
} // namespace webrtc