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 <hta@webrtc.org>
Commit-Queue: Qingsi Wang <qingsi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28844}
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 4953494..37f19e0 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -3695,13 +3695,6 @@
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 @@
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 2f997ae..cb8210c 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 @@
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<std::unique_ptr<webrtc::IceCandidateInterface>>
@@ -663,6 +669,77 @@
#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<JsepSessionDescription>(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");
