Move ComputeFoundation to Candidate.
Move code from P2PTransportChannel to Candidate, where we set the
foundation value for remote prflx candidates.
Bug: none
Change-Id: I7dbcb85bca35dca7297136b0706092dd8d2b153c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/339902
Reviewed-by: Philipp Hancke <phancke@microsoft.com>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41774}
diff --git a/api/BUILD.gn b/api/BUILD.gn
index 0c982a2..9ced1f2 100644
--- a/api/BUILD.gn
+++ b/api/BUILD.gn
@@ -183,7 +183,9 @@
"candidate.h",
]
deps = [
+ "../p2p:p2p_constants",
"../rtc_base:checks",
+ "../rtc_base:crc32",
"../rtc_base:ip_address",
"../rtc_base:logging",
"../rtc_base:network_constants",
diff --git a/api/candidate.cc b/api/candidate.cc
index 7718f94..06b3156 100644
--- a/api/candidate.cc
+++ b/api/candidate.cc
@@ -11,6 +11,8 @@
#include "api/candidate.h"
#include "absl/base/attributes.h"
+#include "p2p/base/p2p_constants.h"
+#include "rtc_base/crc32.h"
#include "rtc_base/helpers.h"
#include "rtc_base/ip_address.h"
#include "rtc_base/logging.h"
@@ -25,7 +27,7 @@
Candidate::Candidate()
: id_(rtc::CreateRandomString(8)),
- component_(0),
+ component_(ICE_CANDIDATE_COMPONENT_DEFAULT),
priority_(0),
type_(LOCAL_PORT_TYPE),
network_type_(rtc::ADAPTER_TYPE_UNKNOWN),
@@ -209,6 +211,43 @@
return copy;
}
+void Candidate::ComputeFoundation(const rtc::SocketAddress& base_address,
+ uint64_t tie_breaker) {
+ // https://www.rfc-editor.org/rfc/rfc5245#section-4.1.1.3
+ // The foundation is an identifier, scoped within a session. Two candidates
+ // MUST have the same foundation ID when all of the following are true:
+ //
+ // o they are of the same type.
+ // o their bases have the same IP address (the ports can be different).
+ // o for reflexive and relayed candidates, the STUN or TURN servers used to
+ // obtain them have the same IP address.
+ // o they were obtained using the same transport protocol (TCP, UDP, etc.).
+ //
+ // Similarly, two candidates MUST have different foundations if their
+ // types are different, their bases have different IP addresses, the STUN or
+ // TURN servers used to obtain them have different IP addresses, or their
+ // transport protocols are different.
+
+ rtc::StringBuilder sb;
+ sb << type_ << base_address.ipaddr().ToString() << protocol_
+ << relay_protocol_;
+
+ // https://www.rfc-editor.org/rfc/rfc5245#section-5.2
+ // [...] it is possible for both agents to mistakenly believe they are
+ // controlled or controlling. To resolve this, each agent MUST select a random
+ // number, called the tie-breaker, uniformly distributed between 0 and (2**64)
+ // - 1 (that is, a 64-bit positive integer). This number is used in
+ // connectivity checks to detect and repair this case [...]
+ sb << rtc::ToString(tie_breaker);
+ foundation_ = rtc::ToString(rtc::ComputeCrc32(sb.Release()));
+}
+
+void Candidate::ComputePrflxFoundation() {
+ RTC_DCHECK(is_prflx());
+ RTC_DCHECK(!id_.empty());
+ foundation_ = rtc::ToString(rtc::ComputeCrc32(id_));
+}
+
void Candidate::Assign(std::string& s, absl::string_view view) {
// Assigning via a temporary object, like s = std::string(view), results in
// binary size bloat. To avoid that, extract pointer and size from the
diff --git a/api/candidate.h b/api/candidate.h
index 0aa75aa..77774ed 100644
--- a/api/candidate.h
+++ b/api/candidate.h
@@ -74,10 +74,18 @@
void set_component(int component) { component_ = component; }
const std::string& protocol() const { return protocol_; }
+
+ // Valid protocol values are:
+ // UDP_PROTOCOL_NAME, TCP_PROTOCOL_NAME, SSLTCP_PROTOCOL_NAME,
+ // TLS_PROTOCOL_NAME.
void set_protocol(absl::string_view protocol) { Assign(protocol_, protocol); }
// The protocol used to talk to relay.
const std::string& relay_protocol() const { return relay_protocol_; }
+
+ // Valid protocol values are:
+ // UDP_PROTOCOL_NAME, TCP_PROTOCOL_NAME, SSLTCP_PROTOCOL_NAME,
+ // TLS_PROTOCOL_NAME.
void set_relay_protocol(absl::string_view protocol) {
Assign(relay_protocol_, protocol);
}
@@ -169,7 +177,15 @@
uint16_t network_id() const { return network_id_; }
void set_network_id(uint16_t network_id) { network_id_ = network_id; }
+ // From RFC 5245, section-7.2.1.3:
+ // The foundation of the candidate is set to an arbitrary value, different
+ // from the foundation for all other remote candidates.
+ // Note: Use ComputeFoundation to populate this value.
const std::string& foundation() const { return foundation_; }
+
+ // TODO(tommi): Deprecate in favor of ComputeFoundation.
+ // For situations where serializing/deserializing a candidate is needed,
+ // the constructor can be used to inject a value for the foundation.
void set_foundation(absl::string_view foundation) {
Assign(foundation_, foundation);
}
@@ -221,6 +237,25 @@
Candidate ToSanitizedCopy(bool use_hostname_address,
bool filter_related_address) const;
+ // Computes and populates the `foundation()` field.
+ // Foundation: An arbitrary string that is the same for two candidates
+ // that have the same type, base IP address, protocol (UDP, TCP,
+ // etc.), and STUN or TURN server. If any of these are different,
+ // then the foundation will be different. Two candidate pairs with
+ // the same foundation pairs are likely to have similar network
+ // characteristics. Foundations are used in the frozen algorithm.
+ // A session wide (peerconnection) tie-breaker is applied to the foundation,
+ // adds additional randomness and must be the same for all candidates.
+ void ComputeFoundation(const rtc::SocketAddress& base_address,
+ uint64_t tie_breaker);
+
+ // https://www.rfc-editor.org/rfc/rfc5245#section-7.2.1.3
+ // Call to populate the foundation field for a new peer reflexive remote
+ // candidate. The type of the candidate must be "prflx".
+ // The foundation of the candidate is set to an arbitrary value, different
+ // from the foundation for all other remote candidates.
+ void ComputePrflxFoundation();
+
private:
// TODO(bugs.webrtc.org/13220): With C++17, we get a std::string assignment
// operator accepting any object implicitly convertible to std::string_view,
diff --git a/api/candidate_unittest.cc b/api/candidate_unittest.cc
index e52d448..a74afdc 100644
--- a/api/candidate_unittest.cc
+++ b/api/candidate_unittest.cc
@@ -29,9 +29,9 @@
TEST(CandidateTest, Component) {
Candidate c;
- EXPECT_EQ(c.component(), 0);
- c.set_component(ICE_CANDIDATE_COMPONENT_DEFAULT);
EXPECT_EQ(c.component(), ICE_CANDIDATE_COMPONENT_DEFAULT);
+ c.set_component(ICE_CANDIDATE_COMPONENT_RTCP);
+ EXPECT_EQ(c.component(), ICE_CANDIDATE_COMPONENT_RTCP);
}
TEST(CandidateTest, TypeName) {
@@ -53,4 +53,48 @@
EXPECT_EQ(c.type(), RELAY_PORT_TYPE);
}
+TEST(CandidateTest, Foundation) {
+ Candidate c;
+ EXPECT_TRUE(c.foundation().empty());
+ c.set_protocol("udp");
+ c.set_relay_protocol("udp");
+
+ rtc::SocketAddress address("99.99.98.1", 1024);
+ c.set_address(address);
+ c.ComputeFoundation(c.address(), 1);
+ std::string foundation1 = c.foundation();
+ EXPECT_FALSE(foundation1.empty());
+
+ // Change the tiebreaker.
+ c.ComputeFoundation(c.address(), 2);
+ std::string foundation2 = c.foundation();
+ EXPECT_NE(foundation1, foundation2);
+
+ // Provide a different base address.
+ address.SetIP("100.100.100.1");
+ c.ComputeFoundation(address, 1); // Same tiebreaker as for foundation1.
+ foundation2 = c.foundation();
+ EXPECT_NE(foundation1, foundation2);
+
+ // Consistency check (just in case the algorithm ever changes to random!).
+ c.ComputeFoundation(c.address(), 1);
+ foundation2 = c.foundation();
+ EXPECT_EQ(foundation1, foundation2);
+
+ // Changing the protocol should affect the foundation.
+ auto prev_protocol = c.protocol();
+ c.set_protocol("tcp");
+ ASSERT_NE(prev_protocol, c.protocol());
+ c.ComputeFoundation(c.address(), 1);
+ EXPECT_NE(foundation1, c.foundation());
+ c.set_protocol(prev_protocol);
+
+ // Changing the relay protocol should affect the foundation.
+ prev_protocol = c.relay_protocol();
+ c.set_relay_protocol("tcp");
+ ASSERT_NE(prev_protocol, c.relay_protocol());
+ c.ComputeFoundation(c.address(), 1);
+ EXPECT_NE(foundation1, c.foundation());
+}
+
} // namespace cricket
diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn
index 05db81a..1601b49 100644
--- a/p2p/BUILD.gn
+++ b/p2p/BUILD.gn
@@ -586,7 +586,6 @@
"../logging:ice_log",
"../rtc_base:async_packet_socket",
"../rtc_base:checks",
- "../rtc_base:crc32",
"../rtc_base:dscp",
"../rtc_base:event_tracer",
"../rtc_base:ip_address",
@@ -666,7 +665,6 @@
"../rtc_base:byte_buffer",
"../rtc_base:callback_list",
"../rtc_base:checks",
- "../rtc_base:crc32",
"../rtc_base:dscp",
"../rtc_base:event_tracer",
"../rtc_base:ip_address",
diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc
index 93a9a3f..b601940 100644
--- a/p2p/base/connection.cc
+++ b/p2p/base/connection.cc
@@ -1696,9 +1696,8 @@
// Set the related address and foundation attributes before changing the
// address.
local_candidate_.set_related_address(local_candidate_.address());
- local_candidate_.set_foundation(port()->ComputeFoundation(
- PRFLX_PORT_TYPE, local_candidate_.protocol(),
- local_candidate_.relay_protocol(), local_candidate_.address()));
+ local_candidate_.ComputeFoundation(local_candidate_.address(),
+ port_->IceTiebreaker());
local_candidate_.set_priority(priority);
local_candidate_.set_address(addr->GetAddress());
diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc
index eb3553b..b0e19f6 100644
--- a/p2p/base/p2p_transport_channel.cc
+++ b/p2p/base/p2p_transport_channel.cc
@@ -34,7 +34,6 @@
#include "p2p/base/port.h"
#include "p2p/base/wrapping_active_ice_controller.h"
#include "rtc_base/checks.h"
-#include "rtc_base/crc32.h"
#include "rtc_base/experiments/struct_parameters_parser.h"
#include "rtc_base/ip_address.h"
#include "rtc_base/logging.h"
@@ -1082,8 +1081,7 @@
// From RFC 5245, section-7.2.1.3:
// The foundation of the candidate is set to an arbitrary value, different
// from the foundation for all other remote candidates.
- remote_candidate.set_foundation(
- rtc::ToString(rtc::ComputeCrc32(remote_candidate.id())));
+ remote_candidate.ComputePrflxFoundation();
}
// RFC5245, the agent constructs a pair whose local candidate is equal to
diff --git a/p2p/base/port.cc b/p2p/base/port.cc
index f49bfe5..917167c 100644
--- a/p2p/base/port.cc
+++ b/p2p/base/port.cc
@@ -26,7 +26,6 @@
#include "p2p/base/stun_request.h"
#include "rtc_base/byte_buffer.h"
#include "rtc_base/checks.h"
-#include "rtc_base/crc32.h"
#include "rtc_base/helpers.h"
#include "rtc_base/ip_address.h"
#include "rtc_base/logging.h"
@@ -93,17 +92,6 @@
const char TCPTYPE_PASSIVE_STR[] = "passive";
const char TCPTYPE_SIMOPEN_STR[] = "so";
-std::string Port::ComputeFoundation(absl::string_view type,
- absl::string_view protocol,
- absl::string_view relay_protocol,
- const rtc::SocketAddress& base_address) {
- // TODO(bugs.webrtc.org/14605): ensure IceTiebreaker() is set.
- rtc::StringBuilder sb;
- sb << type << base_address.ipaddr().ToString() << protocol << relay_protocol
- << rtc::ToString(IceTiebreaker());
- return rtc::ToString(rtc::ComputeCrc32(sb.Release()));
-}
-
Port::Port(TaskQueueBase* thread,
absl::string_view type,
rtc::PacketSocketFactory* factory,
@@ -260,22 +248,25 @@
bool is_final) {
RTC_DCHECK_RUN_ON(thread_);
- std::string foundation =
- ComputeFoundation(type, protocol, relay_protocol, base_address);
+ // TODO(tommi): Set relay_protocol and optionally provide the base address
+ // to automatically compute the foundation in the ctor? It would be a good
+ // thing for the Candidate class to know the base address and keep it const.
Candidate c(component_, protocol, address, 0U, username_fragment(), password_,
- type, generation_, foundation, network_->id(), network_cost_);
+ type, generation_, "", network_->id(), network_cost_);
+ // Set the relay protocol before computing the foundation field.
+ c.set_relay_protocol(relay_protocol);
+ // TODO(bugs.webrtc.org/14605): ensure IceTiebreaker() is set.
+ c.ComputeFoundation(base_address, tiebreaker_);
+ c.set_priority(
+ c.GetPriority(type_preference, network_->preference(), relay_preference,
+ field_trials_->IsEnabled(
+ "WebRTC-IncreaseIceCandidatePriorityHostSrflx")));
#if RTC_DCHECK_IS_ON
if (protocol == TCP_PROTOCOL_NAME && c.is_local()) {
RTC_DCHECK(!tcptype.empty());
}
#endif
-
- c.set_relay_protocol(relay_protocol);
- c.set_priority(
- c.GetPriority(type_preference, network_->preference(), relay_preference,
- field_trials_->IsEnabled(
- "WebRTC-IncreaseIceCandidatePriorityHostSrflx")));
c.set_tcptype(tcptype);
c.set_network_name(network_->name());
c.set_network_type(network_->type());
diff --git a/p2p/base/port.h b/p2p/base/port.h
index a3214af..4021c25 100644
--- a/p2p/base/port.h
+++ b/p2p/base/port.h
@@ -374,18 +374,6 @@
void GetStunStats(absl::optional<StunStats>* stats) override {}
- // Foundation: An arbitrary string that is the same for two candidates
- // that have the same type, base IP address, protocol (UDP, TCP,
- // etc.), and STUN or TURN server. If any of these are different,
- // then the foundation will be different. Two candidate pairs with
- // the same foundation pairs are likely to have similar network
- // characteristics. Foundations are used in the frozen algorithm.
- std::string ComputeFoundation(
- absl::string_view type,
- absl::string_view protocol,
- absl::string_view relay_protocol,
- const rtc::SocketAddress& base_address) override;
-
protected:
void UpdateNetworkCost() override;
diff --git a/p2p/base/port_interface.h b/p2p/base/port_interface.h
index fb8023b..8a1d18d 100644
--- a/p2p/base/port_interface.h
+++ b/p2p/base/port_interface.h
@@ -167,18 +167,6 @@
// Called when the Connection discovers a local peer reflexive candidate.
virtual void AddPrflxCandidate(const Candidate& local) = 0;
- // Foundation: An arbitrary string that is the same for two candidates
- // that have the same type, base IP address, protocol (UDP, TCP,
- // etc.), and STUN or TURN server. If any of these are different,
- // then the foundation will be different. Two candidate pairs with
- // the same foundation pairs are likely to have similar network
- // characteristics. Foundations are used in the frozen algorithm.
- virtual std::string ComputeFoundation(
- absl::string_view type,
- absl::string_view protocol,
- absl::string_view relay_protocol,
- const rtc::SocketAddress& base_address) = 0;
-
protected:
PortInterface();
virtual void UpdateNetworkCost() = 0;
diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc
index 98cbd2d..351ab6d 100644
--- a/pc/rtc_stats_collector_unittest.cc
+++ b/pc/rtc_stats_collector_unittest.cc
@@ -216,16 +216,13 @@
uint32_t priority,
const rtc::AdapterType underlying_type_for_vpn =
rtc::ADAPTER_TYPE_UNKNOWN) {
- std::unique_ptr<cricket::Candidate> candidate(new cricket::Candidate());
- candidate->set_address(rtc::SocketAddress(hostname, port));
- candidate->set_protocol(protocol);
+ std::unique_ptr<cricket::Candidate> candidate(new cricket::Candidate(
+ cricket::ICE_CANDIDATE_COMPONENT_RTP, protocol,
+ rtc::SocketAddress(hostname, port), priority, "iceusernamefragment",
+ "" /* pwd */, candidate_type, 0 /* generation */, "foundationIsAString"));
+
candidate->set_network_type(adapter_type);
candidate->set_underlying_type_for_vpn(underlying_type_for_vpn);
- candidate->set_type(candidate_type);
- candidate->set_priority(priority);
- // Defaults for testing.
- candidate->set_foundation("foundationIsAString");
- candidate->set_username("iceusernamefragment");
return candidate;
}
@@ -498,6 +495,7 @@
CreateMockSender(media_type, track, ssrc, attachment_id, {});
EXPECT_CALL(*sender, Stop());
EXPECT_CALL(*sender, SetMediaChannel(_));
+ EXPECT_CALL(*sender, SetCodecPreferences(_));
pc_->AddSender(sender);
return sender;
}
@@ -574,6 +572,7 @@
voice_sender_info.local_stats[0].ssrc + 10, local_stream_ids);
EXPECT_CALL(*rtp_sender, SetMediaChannel(_)).WillRepeatedly(Return());
EXPECT_CALL(*rtp_sender, Stop());
+ EXPECT_CALL(*rtp_sender, SetCodecPreferences(_));
pc_->AddSender(rtp_sender);
}
@@ -612,6 +611,7 @@
video_sender_info.local_stats[0].ssrc + 10, local_stream_ids);
EXPECT_CALL(*rtp_sender, SetMediaChannel(_)).WillRepeatedly(Return());
EXPECT_CALL(*rtp_sender, Stop());
+ EXPECT_CALL(*rtp_sender, SetCodecPreferences(_));
pc_->AddSender(rtp_sender);
}
@@ -3092,6 +3092,7 @@
cricket::MEDIA_TYPE_VIDEO, video_track, kSsrc, kAttachmentId, {});
EXPECT_CALL(*sender, Stop());
EXPECT_CALL(*sender, SetMediaChannel(_));
+ EXPECT_CALL(*sender, SetCodecPreferences(_));
pc_->AddSender(sender);
rtc::scoped_refptr<const RTCStatsReport> report = stats_->GetStatsReport();
@@ -3136,6 +3137,7 @@
cricket::MEDIA_TYPE_VIDEO, video_track, kNoSsrc, kAttachmentId, {});
EXPECT_CALL(*sender, Stop());
EXPECT_CALL(*sender, SetMediaChannel(_));
+ EXPECT_CALL(*sender, SetCodecPreferences(_));
pc_->AddSender(sender);
rtc::scoped_refptr<const RTCStatsReport> report = stats_->GetStatsReport();
@@ -3166,6 +3168,7 @@
cricket::MEDIA_TYPE_VIDEO, video_track, kSsrc, kAttachmentId, {});
EXPECT_CALL(*sender, Stop());
EXPECT_CALL(*sender, SetMediaChannel(_));
+ EXPECT_CALL(*sender, SetCodecPreferences(_));
pc_->AddSender(sender);
rtc::scoped_refptr<const RTCStatsReport> report = stats_->GetStatsReport();
@@ -3189,6 +3192,7 @@
cricket::MEDIA_TYPE_AUDIO, /*track=*/nullptr, kSsrc, kAttachmentId, {});
EXPECT_CALL(*sender, Stop());
EXPECT_CALL(*sender, SetMediaChannel(_));
+ EXPECT_CALL(*sender, SetCodecPreferences(_));
pc_->AddSender(sender);
rtc::scoped_refptr<const RTCStatsReport> report = stats_->GetStatsReport();
@@ -3541,6 +3545,7 @@
cricket::MEDIA_TYPE_VIDEO, /*track=*/nullptr, kSsrc, kAttachmentId, {});
EXPECT_CALL(*sender, Stop());
EXPECT_CALL(*sender, SetMediaChannel(_));
+ EXPECT_CALL(*sender, SetCodecPreferences(_));
pc_->AddSender(sender);
rtc::scoped_refptr<const RTCStatsReport> report = stats_->GetStatsReport();
@@ -3662,6 +3667,7 @@
rtc::scoped_refptr<MockRtpSenderInternal> sender =
CreateMockSender(cricket::MEDIA_TYPE_AUDIO, track, 0, 49, {});
EXPECT_CALL(*sender, Stop());
+ EXPECT_CALL(*sender, SetCodecPreferences(_));
pc_->AddSender(sender);
rtc::scoped_refptr<const RTCStatsReport> report = stats_->GetStatsReport();
@@ -3679,11 +3685,11 @@
rtc::scoped_refptr<MockRtpSenderInternal> sender =
CreateMockSender(cricket::MEDIA_TYPE_AUDIO, track, 4711, 49, {});
EXPECT_CALL(*sender, Stop());
+ EXPECT_CALL(*sender, SetCodecPreferences(_));
pc_->AddSender(sender);
// We do not generate any matching voice_sender_info stats.
rtc::scoped_refptr<const RTCStatsReport> report = stats_->GetStatsReport();
-
auto outbound_rtps = report->GetStatsOfType<RTCOutboundRtpStreamStats>();
EXPECT_TRUE(outbound_rtps.empty());
}
