From 7c6f74ab0344e9c6201de711d54026e9990b8e6c Mon Sep 17 00:00:00 2001 From: Qingsi Wang Date: Mon, 12 Aug 2019 18:49:45 -0700 Subject: [PATCH] Set the usage pattern bits for adding remote ICE candidates from SDP. Currently these bits are only set when a remote ICE candidate is successfully added via addIceCandidate. For non-trickled sessions in which the remote candidates are added via the remote description, these bits are lost. This also happens for trickled sessions, though a rare case, when addIceCandidate does not succeed because the peer connection is not ready to add any remote candidate. Bug: webrtc:10868 Change-Id: Ib2f199f9ffc936060473934d25ba397ef31131a3 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/148880 Reviewed-by: Harald Alvestrand Commit-Queue: Qingsi Wang Cr-Commit-Position: refs/heads/master@{#28844} --- pc/peer_connection.cc | 14 ++--- pc/peer_connection_histogram_unittest.cc | 77 ++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 7 deletions(-) diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 4953494d88..37f19e09a0 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -3695,13 +3695,6 @@ bool PeerConnection::AddIceCandidate( if (ready) { bool result = UseCandidate(ice_candidate); if (result) { - NoteUsageEvent(UsageEvent::REMOTE_CANDIDATE_ADDED); - if (ice_candidate->candidate().address().IsUnresolvedIP()) { - NoteUsageEvent(UsageEvent::REMOTE_MDNS_CANDIDATE_ADDED); - } - if (ice_candidate->candidate().address().IsPrivateIP()) { - NoteUsageEvent(UsageEvent::REMOTE_PRIVATE_CANDIDATE_ADDED); - } NoteAddIceCandidateResult(kAddIceCandidateSuccess); } else { NoteAddIceCandidateResult(kAddIceCandidateFailNotUsable); @@ -6344,6 +6337,13 @@ bool PeerConnection::UseCandidate(const IceCandidateInterface* candidate) { RTCError error = transport_controller_->AddRemoteCandidates( result.value()->name, candidates); if (error.ok()) { + NoteUsageEvent(UsageEvent::REMOTE_CANDIDATE_ADDED); + if (candidate->candidate().address().IsUnresolvedIP()) { + NoteUsageEvent(UsageEvent::REMOTE_MDNS_CANDIDATE_ADDED); + } + if (candidate->candidate().address().IsPrivateIP()) { + NoteUsageEvent(UsageEvent::REMOTE_PRIVATE_CANDIDATE_ADDED); + } // Candidates successfully submitted for checking. if (ice_connection_state_ == PeerConnectionInterface::kIceConnectionNew || ice_connection_state_ == diff --git a/pc/peer_connection_histogram_unittest.cc b/pc/peer_connection_histogram_unittest.cc index 2f997aecbd..cb8210c3f0 100644 --- a/pc/peer_connection_histogram_unittest.cc +++ b/pc/peer_connection_histogram_unittest.cc @@ -18,6 +18,7 @@ #include "absl/types/optional.h" #include "api/call/call_factory_interface.h" #include "api/jsep.h" +#include "api/jsep_session_description.h" #include "api/peer_connection_interface.h" #include "api/peer_connection_proxy.h" #include "api/rtc_error.h" @@ -32,6 +33,7 @@ #include "pc/peer_connection_wrapper.h" #include "pc/sdp_utils.h" #include "pc/test/mock_peer_connection_observers.h" +#include "pc/webrtc_sdp.h" #include "rtc_base/arraysize.h" #include "rtc_base/checks.h" #include "rtc_base/fake_mdns_responder.h" @@ -206,6 +208,10 @@ class PeerConnectionWrapperForUsageHistogramTest return true; } + webrtc::PeerConnectionInterface::IceGatheringState ice_gathering_state() { + return pc()->ice_gathering_state(); + } + private: // Candidates that have been sent but not yet configured std::vector> @@ -663,6 +669,77 @@ TEST_F(PeerConnectionUsageHistogramTest, FingerprintWithPrivateIPCallee) { #ifndef WEBRTC_ANDROID #ifdef HAVE_SCTP +// Test that the usage pattern bits for adding remote private or mDNS candidates +// are set when the remote candidates are retrieved from Offer/Answer SDP +// instead of trickled ICE messages. +TEST_F(PeerConnectionUsageHistogramTest, + AddRemoteCandidatesFromRemoteDescription) { + // We construct the following data-channel-only scenario. The caller collects + // private local candidates and appends them in the Offer as in non-trickled + // sessions. The callee collects mDNS candidates. Only Offer is signaled to + // the callee and we expect a connection with prflx candidates. + auto caller = CreatePeerConnectionWithPrivateLocalAddresses(); + auto callee = CreatePeerConnectionWithMdns(RTCConfiguration()); + caller->CreateDataChannel("test_channel"); + ASSERT_TRUE(caller->SetLocalDescription(caller->CreateOffer())); + // Wait until the gathering completes (or at least having gathered one + // candidate) so that the session description would have contained ICE + // candidates. + EXPECT_EQ_WAIT(webrtc::PeerConnectionInterface::kIceGatheringComplete, + caller->ice_gathering_state(), kDefaultTimeout); + // Get the current offer that contains candidates and pass it to the callee. + // + // Note that we cannot use CloneSessionDescription on |cur_offer| to obtain an + // SDP with candidates. The method above does not strictly copy everything, in + // particular, not copying the ICE candidates. + // TODO(qingsi): Technically, this is a bug. Fix it. + auto cur_offer = caller->pc()->local_description(); + ASSERT_TRUE(cur_offer); + std::string sdp_with_candidates_str; + cur_offer->ToString(&sdp_with_candidates_str); + auto offer = absl::make_unique(SdpType::kOffer); + ASSERT_TRUE(SdpDeserialize(sdp_with_candidates_str, offer.get(), + nullptr /* error */)); + ASSERT_TRUE(callee->SetRemoteDescription(std::move(offer))); + + auto answer = callee->CreateAnswer(); + callee->SetLocalDescription(CloneSessionDescription(answer.get())); + caller->SetRemoteDescription(std::move(answer)); + EXPECT_TRUE_WAIT(caller->IsConnected(), kDefaultTimeout); + EXPECT_TRUE_WAIT(callee->IsConnected(), kDefaultTimeout); + // The callee needs to process the open message to have the data channel open. + EXPECT_TRUE_WAIT(callee->observer()->last_datachannel_ != nullptr, + kDefaultTimeout); + caller->pc()->Close(); + callee->pc()->Close(); + + int expected_fingerprint_caller = MakeUsageFingerprint( + {PeerConnection::UsageEvent::DATA_ADDED, + PeerConnection::UsageEvent::SET_LOCAL_DESCRIPTION_CALLED, + PeerConnection::UsageEvent::SET_REMOTE_DESCRIPTION_CALLED, + PeerConnection::UsageEvent::CANDIDATE_COLLECTED, + PeerConnection::UsageEvent::PRIVATE_CANDIDATE_COLLECTED, + PeerConnection::UsageEvent::ICE_STATE_CONNECTED, + PeerConnection::UsageEvent::CLOSE_CALLED}); + + int expected_fingerprint_callee = MakeUsageFingerprint( + {PeerConnection::UsageEvent::DATA_ADDED, + PeerConnection::UsageEvent::SET_LOCAL_DESCRIPTION_CALLED, + PeerConnection::UsageEvent::SET_REMOTE_DESCRIPTION_CALLED, + PeerConnection::UsageEvent::CANDIDATE_COLLECTED, + PeerConnection::UsageEvent::MDNS_CANDIDATE_COLLECTED, + PeerConnection::UsageEvent::REMOTE_CANDIDATE_ADDED, + PeerConnection::UsageEvent::REMOTE_PRIVATE_CANDIDATE_ADDED, + PeerConnection::UsageEvent::ICE_STATE_CONNECTED, + PeerConnection::UsageEvent::CLOSE_CALLED}); + + EXPECT_EQ(2, webrtc::metrics::NumSamples(kUsagePatternMetric)); + EXPECT_EQ(1, webrtc::metrics::NumEvents(kUsagePatternMetric, + expected_fingerprint_caller)); + EXPECT_EQ(1, webrtc::metrics::NumEvents(kUsagePatternMetric, + expected_fingerprint_callee)); +} + TEST_F(PeerConnectionUsageHistogramTest, NotableUsageNoted) { auto caller = CreatePeerConnection(); caller->CreateDataChannel("foo");