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