diff --git a/webrtc/api/stats/rtcstats_objects.h b/webrtc/api/stats/rtcstats_objects.h index dab947ec4e..29e2d7633c 100644 --- a/webrtc/api/stats/rtcstats_objects.h +++ b/webrtc/api/stats/rtcstats_objects.h @@ -136,7 +136,6 @@ class RTCIceCandidatePairStats final : public RTCStats { // "RTCStatsMember"? RTCStatsMember state; RTCStatsMember priority; - // TODO(hbos): Collect and populate this value. https://bugs.webrtc.org/7062 RTCStatsMember nominated; // TODO(hbos): Collect this the way the spec describes it. We have a value for // it but it is not spec-compliant. https://bugs.webrtc.org/7062 diff --git a/webrtc/p2p/base/jseptransport.cc b/webrtc/p2p/base/jseptransport.cc index 9018a643d8..86dc6a3584 100644 --- a/webrtc/p2p/base/jseptransport.cc +++ b/webrtc/p2p/base/jseptransport.cc @@ -60,7 +60,8 @@ ConnectionInfo::ConnectionInfo() recv_ping_responses(0), key(nullptr), state(IceCandidatePairState::WAITING), - priority(0) {} + priority(0), + nominated(false) {} bool BadTransportDescription(const std::string& desc, std::string* err_desc) { if (err_desc) { diff --git a/webrtc/p2p/base/jseptransport.h b/webrtc/p2p/base/jseptransport.h index 45963e681f..2d68543d5c 100644 --- a/webrtc/p2p/base/jseptransport.h +++ b/webrtc/p2p/base/jseptransport.h @@ -113,6 +113,8 @@ struct ConnectionInfo { IceCandidatePairState state; // https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-priority uint64_t priority; + // https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-nominated + bool nominated; }; // Information about all the connections of a channel. diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc index bf57cf9568..80b09c5034 100644 --- a/webrtc/p2p/base/port.cc +++ b/webrtc/p2p/base/port.cc @@ -1503,6 +1503,7 @@ ConnectionInfo Connection::stats() { stats_.key = this; stats_.state = state_; stats_.priority = priority(); + stats_.nominated = nominated(); return stats_; } diff --git a/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h index f6900210f4..a80fde79a9 100644 --- a/webrtc/p2p/base/port.h +++ b/webrtc/p2p/base/port.h @@ -531,7 +531,14 @@ class Connection : public CandidatePairInterface, void set_nomination(uint32_t value) { nomination_ = value; } uint32_t remote_nomination() const { return remote_nomination_; } - bool nominated() const { return remote_nomination_ > 0; } + // One or several pairs may be nominated based on if Regular or Aggressive + // Nomination is used. https://tools.ietf.org/html/rfc5245#section-8 + // |nominated| is defined both for the controlling or controlled agent based + // on if a nomination has been pinged or acknowledged. The controlled agent + // gets its |remote_nomination_| set when pinged by the controlling agent with + // a nomination value. The controlling agent gets its |acked_nomination_| set + // when receiving a response to a nominating ping. + bool nominated() const { return acked_nomination_ || remote_nomination_; } // Public for unit tests. void set_remote_nomination(uint32_t remote_nomination) { remote_nomination_ = remote_nomination; diff --git a/webrtc/p2p/base/port_unittest.cc b/webrtc/p2p/base/port_unittest.cc index 9714280b93..ecd5c416c7 100644 --- a/webrtc/p2p/base/port_unittest.cc +++ b/webrtc/p2p/base/port_unittest.cc @@ -1788,6 +1788,62 @@ TEST_F(PortTest, TestSendStunMessage) { EXPECT_EQ(2U, retransmit_attr->value()); } +TEST_F(PortTest, TestNomination) { + std::unique_ptr lport( + CreateTestPort(kLocalAddr1, "lfrag", "lpass")); + std::unique_ptr rport( + CreateTestPort(kLocalAddr2, "rfrag", "rpass")); + lport->SetIceRole(cricket::ICEROLE_CONTROLLING); + lport->SetIceTiebreaker(kTiebreaker1); + rport->SetIceRole(cricket::ICEROLE_CONTROLLED); + rport->SetIceTiebreaker(kTiebreaker2); + + lport->PrepareAddress(); + rport->PrepareAddress(); + ASSERT_FALSE(lport->Candidates().empty()); + ASSERT_FALSE(rport->Candidates().empty()); + Connection* lconn = lport->CreateConnection(rport->Candidates()[0], + Port::ORIGIN_MESSAGE); + Connection* rconn = rport->CreateConnection(lport->Candidates()[0], + Port::ORIGIN_MESSAGE); + + // |lconn| is controlling, |rconn| is controlled. + uint32_t nomination = 1234; + lconn->set_nomination(nomination); + + EXPECT_FALSE(lconn->nominated()); + EXPECT_FALSE(rconn->nominated()); + EXPECT_EQ(lconn->nominated(), lconn->stats().nominated); + EXPECT_EQ(rconn->nominated(), rconn->stats().nominated); + + // Send ping (including the nomination value) from |lconn| to |rconn|. This + // should set the remote nomination of |rconn|. + lconn->Ping(0); + ASSERT_TRUE_WAIT(lport->last_stun_msg(), kDefaultTimeout); + ASSERT_TRUE(lport->last_stun_buf()); + rconn->OnReadPacket(lport->last_stun_buf()->data(), + lport->last_stun_buf()->size(), + rtc::PacketTime()); + EXPECT_EQ(nomination, rconn->remote_nomination()); + EXPECT_FALSE(lconn->nominated()); + EXPECT_TRUE(rconn->nominated()); + EXPECT_EQ(lconn->nominated(), lconn->stats().nominated); + EXPECT_EQ(rconn->nominated(), rconn->stats().nominated); + + // This should result in an acknowledgment sent back from |rconn| to |lconn|, + // updating the acknowledged nomination of |lconn|. + ASSERT_TRUE_WAIT(rport->last_stun_msg(), kDefaultTimeout); + ASSERT_TRUE(rport->last_stun_buf()); + lconn->OnReadPacket(rport->last_stun_buf()->data(), + rport->last_stun_buf()->size(), + rtc::PacketTime()); + EXPECT_EQ(nomination, lconn->acked_nomination()); + EXPECT_TRUE(lconn->nominated()); + EXPECT_TRUE(rconn->nominated()); + EXPECT_EQ(lconn->nominated(), lconn->stats().nominated); + EXPECT_EQ(rconn->nominated(), rconn->stats().nominated); +} + TEST_F(PortTest, TestUseCandidateAttribute) { std::unique_ptr lport( CreateTestPort(kLocalAddr1, "lfrag", "lpass")); diff --git a/webrtc/pc/rtcstats_integrationtest.cc b/webrtc/pc/rtcstats_integrationtest.cc index 44a99d4a35..25b31f7dd7 100644 --- a/webrtc/pc/rtcstats_integrationtest.cc +++ b/webrtc/pc/rtcstats_integrationtest.cc @@ -367,7 +367,7 @@ class RTCStatsReportVerifier { candidate_pair.remote_candidate_id, RTCRemoteIceCandidateStats::kType); verifier.TestMemberIsDefined(candidate_pair.state); verifier.TestMemberIsNonNegative(candidate_pair.priority); - verifier.TestMemberIsUndefined(candidate_pair.nominated); + verifier.TestMemberIsDefined(candidate_pair.nominated); verifier.TestMemberIsDefined(candidate_pair.writable); verifier.TestMemberIsUndefined(candidate_pair.readable); verifier.TestMemberIsNonNegative(candidate_pair.bytes_sent); diff --git a/webrtc/pc/rtcstatscollector.cc b/webrtc/pc/rtcstatscollector.cc index 99d7bc1c90..81443bb82c 100644 --- a/webrtc/pc/rtcstatscollector.cc +++ b/webrtc/pc/rtcstatscollector.cc @@ -866,6 +866,7 @@ void RTCStatsCollector::ProduceIceCandidateAndPairStats_n( candidate_pair_stats->state = IceCandidatePairStateToRTCStatsIceCandidatePairState(info.state); candidate_pair_stats->priority = info.priority; + candidate_pair_stats->nominated = info.nominated; // TODO(hbos): This writable is different than the spec. It goes to // false after a certain amount of time without a response passes. // crbug.com/633550 diff --git a/webrtc/pc/rtcstatscollector_unittest.cc b/webrtc/pc/rtcstatscollector_unittest.cc index 3a4033ba27..e698cf513a 100644 --- a/webrtc/pc/rtcstatscollector_unittest.cc +++ b/webrtc/pc/rtcstatscollector_unittest.cc @@ -1249,6 +1249,7 @@ TEST_F(RTCStatsCollectorTest, CollectRTCIceCandidatePairStats) { connection_info.sent_ping_responses = 1000; connection_info.state = cricket::IceCandidatePairState::IN_PROGRESS; connection_info.priority = 5555; + connection_info.nominated = false; cricket::TransportChannelStats transport_channel_stats; transport_channel_stats.component = cricket::ICE_CANDIDATE_COMPONENT_RTP; @@ -1289,6 +1290,7 @@ TEST_F(RTCStatsCollectorTest, CollectRTCIceCandidatePairStats) { "RTCIceCandidate_" + remote_candidate->id(); expected_pair.state = RTCStatsIceCandidatePairState::kInProgress; expected_pair.priority = 5555; + expected_pair.nominated = false; expected_pair.writable = true; expected_pair.bytes_sent = 42; expected_pair.bytes_received = 1234; @@ -1307,6 +1309,22 @@ TEST_F(RTCStatsCollectorTest, CollectRTCIceCandidatePairStats) { report->Get(expected_pair.id())->cast_to()); EXPECT_TRUE(report->Get(*expected_pair.transport_id)); + // Set nominated and "GetStats" again. + session_stats.transport_stats["transport"] + .channel_stats[0] + .connection_infos[0] + .nominated = true; + EXPECT_CALL(*video_media_channel, GetStats(_)) + .WillOnce(DoAll(SetArgPointee<0>(video_media_info), Return(true))); + collector_->ClearCachedStatsReport(); + report = GetStatsReport(); + expected_pair.nominated = true; + ASSERT_TRUE(report->Get(expected_pair.id())); + EXPECT_EQ( + expected_pair, + report->Get(expected_pair.id())->cast_to()); + EXPECT_TRUE(report->Get(*expected_pair.transport_id)); + // Make pair the current pair, clear bandwidth and "GetStats" again. session_stats.transport_stats["transport"] .channel_stats[0]