From d419808e4559142405e2ff017e7797a6c395984f Mon Sep 17 00:00:00 2001 From: Qingsi Wang Date: Tue, 13 Aug 2019 18:29:32 +0000 Subject: [PATCH] Revert "Set the usage pattern bits for adding remote ICE candidates from SDP." This reverts commit 7c6f74ab0344e9c6201de711d54026e9990b8e6c. Reason for revert: Need to merge with stacked changes on bits in a single patch to avoid disruption. Original change's description: > 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} TBR=hta@webrtc.org,qingsi@webrtc.org Change-Id: Ia0d24b345f04e6c83199d7692bb55a440e6ff464 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:10868 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/149023 Reviewed-by: Qingsi Wang Commit-Queue: Qingsi Wang Cr-Commit-Position: refs/heads/master@{#28845} --- pc/peer_connection.cc | 14 ++--- pc/peer_connection_histogram_unittest.cc | 77 ------------------------ 2 files changed, 7 insertions(+), 84 deletions(-) diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 37f19e09a0..4953494d88 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -3695,6 +3695,13 @@ 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); @@ -6337,13 +6344,6 @@ 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 cb8210c3f0..2f997aecbd 100644 --- a/pc/peer_connection_histogram_unittest.cc +++ b/pc/peer_connection_histogram_unittest.cc @@ -18,7 +18,6 @@ #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" @@ -33,7 +32,6 @@ #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" @@ -208,10 +206,6 @@ 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> @@ -669,77 +663,6 @@ 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");