From 1fe119f12f77cc3ede2243f76f4ef4a86af17846 Mon Sep 17 00:00:00 2001 From: Qingsi Wang Date: Fri, 31 May 2019 16:55:33 -0700 Subject: [PATCH] Change the gating of surfacing candidates on ICE transport type change from a field trial to RTCConfiguration. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test coverage is also expanded for the underlying feature. Bug: None Change-Id: Ic9c1362867e4a956c5453be7a9355083b6a442f5 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/138980 Reviewed-by: Steve Anton Reviewed-by: Alex Glaznev Reviewed-by: Kári Helgason Reviewed-by: Amit Hilbuch Commit-Queue: Qingsi Wang Cr-Commit-Position: refs/heads/master@{#28143} --- api/peer_connection_interface.h | 11 ++ p2p/base/ice_transport_internal.h | 7 + p2p/base/p2p_transport_channel.cc | 12 +- p2p/base/p2p_transport_channel_unittest.cc | 68 +++++++-- pc/peer_connection.cc | 5 + pc/peer_connection_integrationtest.cc | 4 +- .../api/org/webrtc/PeerConnection.java | 7 + .../src/org/webrtc/PeerConnectionTest.java | 142 +++++++++++++----- sdk/android/src/jni/pc/peer_connection.cc | 3 + .../api/peerconnection/RTCConfiguration.h | 12 ++ .../api/peerconnection/RTCConfiguration.mm | 7 + 11 files changed, 225 insertions(+), 53 deletions(-) diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h index 1994505242..a9d9785195 100644 --- a/api/peer_connection_interface.h +++ b/api/peer_connection_interface.h @@ -506,6 +506,17 @@ class RTC_EXPORT PeerConnectionInterface : public rtc::RefCountInterface { // re-determining was removed in ICEbis (ICE v2). bool redetermine_role_on_ice_restart = true; + // This flag is only effective when |continual_gathering_policy| is + // GATHER_CONTINUALLY. + // + // If true, after the ICE transport type is changed such that new types of + // ICE candidates are allowed by the new transport type, e.g. from + // IceTransportsType::kRelay to IceTransportsType::kAll, candidates that + // have been gathered by the ICE transport but not matching the previous + // transport type and as a result not observed by PeerConnectionObserver, + // will be surfaced to the observer. + bool surface_ice_candidates_on_ice_transport_type_changed = false; + // The following fields define intervals in milliseconds at which ICE // connectivity checks are sent. // diff --git a/p2p/base/ice_transport_internal.h b/p2p/base/ice_transport_internal.h index 93eec14009..89b2107c5a 100644 --- a/p2p/base/ice_transport_internal.h +++ b/p2p/base/ice_transport_internal.h @@ -91,6 +91,13 @@ struct IceConfig { // candidate pairs will succeed, even before a binding response is received. bool presume_writable_when_fully_relayed = false; + // If true, after the ICE transport type (as the candidate filter used by the + // port allocator) is changed such that new types of ICE candidates are + // allowed by the new filter, e.g. from CF_RELAY to CF_ALL, candidates that + // have been gathered by the ICE transport but filtered out and not signaled + // to the upper layers, will be surfaced. + bool surface_ice_candidates_on_ice_transport_type_changed = false; + // Interval to check on all networks and to perform ICE regathering on any // active network having no connection on it. absl::optional regather_on_failed_networks_interval; diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index 57e0e12734..d469ff7dd6 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -557,6 +557,15 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) { } } + config_.surface_ice_candidates_on_ice_transport_type_changed = + config.surface_ice_candidates_on_ice_transport_type_changed; + if (config_.surface_ice_candidates_on_ice_transport_type_changed && + config_.continual_gathering_policy != GATHER_CONTINUALLY) { + RTC_LOG(LS_WARNING) + << "surface_ice_candidates_on_ice_transport_type_changed is " + "ineffective since we do not gather continually."; + } + if (config_.regather_on_failed_networks_interval != config.regather_on_failed_networks_interval) { config_.regather_on_failed_networks_interval = @@ -1030,10 +1039,11 @@ void P2PTransportChannel::OnUnknownAddress( void P2PTransportChannel::OnCandidateFilterChanged(uint32_t prev_filter, uint32_t cur_filter) { + RTC_DCHECK_RUN_ON(network_thread_); if (prev_filter == cur_filter || allocator_session() == nullptr) { return; } - if (webrtc::field_trial::IsEnabled("WebRTC-GatherOnCandidateFilterChanged")) { + if (config_.surface_ice_candidates_on_ice_transport_type_changed) { allocator_session()->SetCandidateFilter(cur_filter); } } diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index e20f92d1ba..c5bcbdd817 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -4950,8 +4950,6 @@ TEST_F(P2PTransportChannelTest, // removed and are still usable for necessary route switching. TEST_F(P2PTransportChannelTest, SurfaceHostCandidateOnCandidateFilterChangeFromRelayToAll) { - webrtc::test::ScopedFieldTrials field_trials( - "WebRTC-GatherOnCandidateFilterChanged/Enabled/"); rtc::ScopedFakeClock clock; ConfigureEndpoints( @@ -4962,9 +4960,11 @@ TEST_F(P2PTransportChannelTest, auto* ep2 = GetEndpoint(1); ep1->allocator_->SetCandidateFilter(CF_RELAY); ep2->allocator_->SetCandidateFilter(CF_RELAY); - IceConfig continual_gathering_config = - CreateIceConfig(1000, GATHER_CONTINUALLY); - CreateChannels(continual_gathering_config, continual_gathering_config); + // Enable continual gathering and also resurfacing gathered candidates upon + // the candidate filter changed in the ICE configuration. + IceConfig ice_config = CreateIceConfig(1000, GATHER_CONTINUALLY); + ice_config.surface_ice_candidates_on_ice_transport_type_changed = true; + CreateChannels(ice_config, ice_config); ASSERT_TRUE_SIMULATED_WAIT(ep1_ch1()->selected_connection() != nullptr, kDefaultTimeout, clock); ASSERT_TRUE_SIMULATED_WAIT(ep2_ch1()->selected_connection() != nullptr, @@ -5014,8 +5014,6 @@ TEST_F(P2PTransportChannelTest, // changing the candidate filter. TEST_F(P2PTransportChannelTest, SurfaceSrflxCandidateOnCandidateFilterChangeFromRelayToNoHost) { - webrtc::test::ScopedFieldTrials field_trials( - "WebRTC-GatherOnCandidateFilterChanged/Enabled/"); rtc::ScopedFakeClock clock; // We need an actual NAT so that the host candidate is not equivalent to the // srflx candidate; otherwise, the host candidate would still surface even @@ -5032,9 +5030,11 @@ TEST_F(P2PTransportChannelTest, auto* ep2 = GetEndpoint(1); ep1->allocator_->SetCandidateFilter(CF_RELAY); ep2->allocator_->SetCandidateFilter(CF_RELAY); - IceConfig continual_gathering_config = - CreateIceConfig(1000, GATHER_CONTINUALLY); - CreateChannels(continual_gathering_config, continual_gathering_config); + // Enable continual gathering and also resurfacing gathered candidates upon + // the candidate filter changed in the ICE configuration. + IceConfig ice_config = CreateIceConfig(1000, GATHER_CONTINUALLY); + ice_config.surface_ice_candidates_on_ice_transport_type_changed = true; + CreateChannels(ice_config, ice_config); ASSERT_TRUE_SIMULATED_WAIT(ep1_ch1()->selected_connection() != nullptr, kDefaultTimeout, clock); ASSERT_TRUE_SIMULATED_WAIT(ep2_ch1()->selected_connection() != nullptr, @@ -5075,13 +5075,49 @@ TEST_F(P2PTransportChannelTest, ep1_ch1()->selected_connection()->remote_candidate().type()); } +// This is the complement to +// SurfaceHostCandidateOnCandidateFilterChangeFromRelayToAll, and instead of +// gathering continually we only gather once, which makes the config +// |surface_ice_candidates_on_ice_transport_type_changed| ineffective after the +// gathering stopped. +TEST_F(P2PTransportChannelTest, + CannotSurfaceTheNewlyAllowedOnFilterChangeIfNotGatheringContinually) { + rtc::ScopedFakeClock clock; + + ConfigureEndpoints( + OPEN, OPEN, + kDefaultPortAllocatorFlags | PORTALLOCATOR_ENABLE_SHARED_SOCKET, + kDefaultPortAllocatorFlags | PORTALLOCATOR_ENABLE_SHARED_SOCKET); + auto* ep1 = GetEndpoint(0); + auto* ep2 = GetEndpoint(1); + ep1->allocator_->SetCandidateFilter(CF_RELAY); + ep2->allocator_->SetCandidateFilter(CF_RELAY); + // Only gather once. + IceConfig ice_config = CreateIceConfig(1000, GATHER_ONCE); + ice_config.surface_ice_candidates_on_ice_transport_type_changed = true; + CreateChannels(ice_config, ice_config); + ASSERT_TRUE_SIMULATED_WAIT(ep1_ch1()->selected_connection() != nullptr, + kDefaultTimeout, clock); + ASSERT_TRUE_SIMULATED_WAIT(ep2_ch1()->selected_connection() != nullptr, + kDefaultTimeout, clock); + // Loosen the candidate filter at ep1. + ep1->allocator_->SetCandidateFilter(CF_ALL); + // Wait for a period for any potential surfacing of new candidates. + SIMULATED_WAIT(false, kDefaultTimeout, clock); + EXPECT_EQ(RELAY_PORT_TYPE, + ep1_ch1()->selected_connection()->local_candidate().type()); + + // Loosen the candidate filter at ep2. + ep2->allocator_->SetCandidateFilter(CF_ALL); + EXPECT_EQ(RELAY_PORT_TYPE, + ep2_ch1()->selected_connection()->local_candidate().type()); +} + // Test that when the candidate filter is updated to be more restrictive, // candidates that 1) have already been gathered and signaled 2) but no longer // match the filter, are not removed. TEST_F(P2PTransportChannelTest, RestrictingCandidateFilterDoesNotRemoveRegatheredCandidates) { - webrtc::test::ScopedFieldTrials field_trials( - "WebRTC-GatherOnCandidateFilterChanged/Enabled/"); rtc::ScopedFakeClock clock; ConfigureEndpoints( @@ -5092,14 +5128,16 @@ TEST_F(P2PTransportChannelTest, auto* ep2 = GetEndpoint(1); ep1->allocator_->SetCandidateFilter(CF_ALL); ep2->allocator_->SetCandidateFilter(CF_ALL); - IceConfig continual_gathering_config = - CreateIceConfig(1000, GATHER_CONTINUALLY); + // Enable continual gathering and also resurfacing gathered candidates upon + // the candidate filter changed in the ICE configuration. + IceConfig ice_config = CreateIceConfig(1000, GATHER_CONTINUALLY); + ice_config.surface_ice_candidates_on_ice_transport_type_changed = true; // Pause candidates so we can gather all types of candidates. See // P2PTransportChannel::OnConnectionStateChange, where we would stop the // gathering when we have a strongly connected candidate pair. PauseCandidates(0); PauseCandidates(1); - CreateChannels(continual_gathering_config, continual_gathering_config); + CreateChannels(ice_config, ice_config); // We have gathered host, srflx and relay candidates. EXPECT_TRUE_SIMULATED_WAIT(ep1->saved_candidates_.size() == 3u, diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index dcb6c8a67a..0b7c9c8c63 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -749,6 +749,7 @@ bool PeerConnectionInterface::RTCConfiguration::operator==( bool presume_writable_when_fully_relayed; bool enable_ice_renomination; bool redetermine_role_on_ice_restart; + bool surface_ice_candidates_on_ice_transport_type_changed; absl::optional ice_check_interval_strong_connectivity; absl::optional ice_check_interval_weak_connectivity; absl::optional ice_check_min_interval; @@ -804,6 +805,8 @@ bool PeerConnectionInterface::RTCConfiguration::operator==( o.presume_writable_when_fully_relayed && enable_ice_renomination == o.enable_ice_renomination && redetermine_role_on_ice_restart == o.redetermine_role_on_ice_restart && + surface_ice_candidates_on_ice_transport_type_changed == + o.surface_ice_candidates_on_ice_transport_type_changed && ice_check_interval_strong_connectivity == o.ice_check_interval_strong_connectivity && ice_check_interval_weak_connectivity == @@ -5749,6 +5752,8 @@ cricket::IceConfig PeerConnection::ParseIceConfig( ice_config.continual_gathering_policy = gathering_policy; ice_config.presume_writable_when_fully_relayed = config.presume_writable_when_fully_relayed; + ice_config.surface_ice_candidates_on_ice_transport_type_changed = + config.surface_ice_candidates_on_ice_transport_type_changed; ice_config.ice_check_interval_strong_connectivity = config.ice_check_interval_strong_connectivity; ice_config.ice_check_interval_weak_connectivity = diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index 7b3163ba0e..395c43768d 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -5086,8 +5086,6 @@ TEST_P(PeerConnectionIntegrationTest, } TEST_P(PeerConnectionIntegrationTest, RegatherAfterChangingIceTransportType) { - webrtc::test::ScopedFieldTrials field_trials( - "WebRTC-GatherOnCandidateFilterChanged/Enabled/"); static const rtc::SocketAddress turn_server_internal_address{"88.88.88.0", 3478}; static const rtc::SocketAddress turn_server_external_address{"88.88.88.1", 0}; @@ -5103,11 +5101,13 @@ TEST_P(PeerConnectionIntegrationTest, RegatherAfterChangingIceTransportType) { caller_config.servers.push_back(ice_server); caller_config.type = webrtc::PeerConnectionInterface::kRelay; caller_config.continual_gathering_policy = PeerConnection::GATHER_CONTINUALLY; + caller_config.surface_ice_candidates_on_ice_transport_type_changed = true; PeerConnectionInterface::RTCConfiguration callee_config; callee_config.servers.push_back(ice_server); callee_config.type = webrtc::PeerConnectionInterface::kRelay; callee_config.continual_gathering_policy = PeerConnection::GATHER_CONTINUALLY; + callee_config.surface_ice_candidates_on_ice_transport_type_changed = true; ASSERT_TRUE( CreatePeerConnectionWrappersWithConfig(caller_config, callee_config)); diff --git a/sdk/android/api/org/webrtc/PeerConnection.java b/sdk/android/api/org/webrtc/PeerConnection.java index 5f802fb7ca..a44969d99a 100644 --- a/sdk/android/api/org/webrtc/PeerConnection.java +++ b/sdk/android/api/org/webrtc/PeerConnection.java @@ -450,6 +450,7 @@ public class PeerConnection { public int iceCandidatePoolSize; public boolean pruneTurnPorts; public boolean presumeWritableWhenFullyRelayed; + public boolean surfaceIceCandidatesOnIceTransportTypeChanged; // The following fields define intervals in milliseconds at which ICE // connectivity checks are sent. // @@ -552,6 +553,7 @@ public class PeerConnection { iceCandidatePoolSize = 0; pruneTurnPorts = false; presumeWritableWhenFullyRelayed = false; + surfaceIceCandidatesOnIceTransportTypeChanged = false; iceCheckIntervalStrongConnectivityMs = null; iceCheckIntervalWeakConnectivityMs = null; iceCheckMinInterval = null; @@ -658,6 +660,11 @@ public class PeerConnection { return presumeWritableWhenFullyRelayed; } + @CalledByNative("RTCConfiguration") + boolean getSurfaceIceCandidatesOnIceTransportTypeChanged() { + return surfaceIceCandidatesOnIceTransportTypeChanged; + } + @Nullable @CalledByNative("RTCConfiguration") Integer getIceCheckIntervalStrongConnectivity() { diff --git a/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java b/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java index a1a11a5e7d..3836f5057f 100644 --- a/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java +++ b/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java @@ -45,9 +45,11 @@ import org.chromium.base.test.util.DisabledTest; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.webrtc.Logging; import org.webrtc.Metrics.HistogramInfo; import org.webrtc.PeerConnection.IceConnectionState; import org.webrtc.PeerConnection.IceGatheringState; +import org.webrtc.PeerConnection.IceTransportsType; import org.webrtc.PeerConnection.PeerConnectionState; import org.webrtc.PeerConnection.SignalingState; import org.webrtc.PeerConnection.TlsCertPolicy; @@ -59,7 +61,9 @@ import org.webrtc.RtpTransceiver.RtpTransceiverInit; /** End-to-end tests for PeerConnection.java. */ @RunWith(BaseJUnit4ClassRunner.class) public class PeerConnectionTest { - private static final int TIMEOUT_SECONDS = 20; + private static final String TAG = "PeerConnectionTest"; + private static final int DEFAULT_TIMEOUT_SECONDS = 20; + private static final int SHORT_TIMEOUT_SECONDS = 5; private @Nullable TreeSet threadsBeforeTest; @Before @@ -124,6 +128,7 @@ public class PeerConnectionTest { // TODO(bugs.webrtc.org/8491): Remove NoSynchronizedMethodCheck suppression. @SuppressWarnings("NoSynchronizedMethodCheck") public synchronized void onIceCandidate(IceCandidate candidate) { + Logging.d(TAG, "onIceCandidate: " + candidate.toString()); --expectedIceCandidates; // We don't assert expectedIceCandidates >= 0 because it's hard to know @@ -201,7 +206,7 @@ public class PeerConnectionTest { } if (expectedIceConnectionChanges.isEmpty()) { - System.out.println(name + "Got an unexpected ICE connection change " + newState); + Logging.d(TAG, name + "Got an unexpected ICE connection change " + newState); return; } @@ -217,8 +222,7 @@ public class PeerConnectionTest { } if (expectedIceConnectionChanges.isEmpty()) { - System.out.println( - name + "Got an unexpected standardized ICE connection change " + newState); + Logging.d(TAG, name + "Got an unexpected standardized ICE connection change " + newState); return; } @@ -236,7 +240,7 @@ public class PeerConnectionTest { @SuppressWarnings("NoSynchronizedMethodCheck") public synchronized void onConnectionChange(PeerConnectionState newState) { if (expectedConnectionChanges.isEmpty()) { - System.out.println(name + " got an unexpected DTLS connection change " + newState); + Logging.d(TAG, name + " got an unexpected DTLS connection change " + newState); return; } @@ -247,7 +251,7 @@ public class PeerConnectionTest { // TODO(bugs.webrtc.org/8491): Remove NoSynchronizedMethodCheck suppression. @SuppressWarnings("NoSynchronizedMethodCheck") public synchronized void onIceConnectionReceivingChange(boolean receiving) { - System.out.println(name + " got an ICE connection receiving change " + receiving); + Logging.d(TAG, name + " got an ICE connection receiving change " + receiving); } // TODO(bugs.webrtc.org/8491): Remove NoSynchronizedMethodCheck suppression. @@ -267,7 +271,7 @@ public class PeerConnectionTest { return; } if (expectedIceGatheringChanges.isEmpty()) { - System.out.println(name + "Got an unexpected ICE gathering change " + newState); + Logging.d(TAG, name + "Got an unexpected ICE gathering change " + newState); } assertEquals(expectedIceGatheringChanges.remove(), newState); } @@ -530,12 +534,14 @@ public class PeerConnectionTest { TreeSet stillWaitingForExpectations = unsatisfiedExpectations(); while (!stillWaitingForExpectations.isEmpty()) { if (!stillWaitingForExpectations.equals(prev)) { - System.out.println(name + " still waiting at\n " + (new Throwable()).getStackTrace()[1] - + "\n for: " + Arrays.toString(stillWaitingForExpectations.toArray())); + Logging.d(TAG, + name + " still waiting at\n " + (new Throwable()).getStackTrace()[1] + + "\n for: " + Arrays.toString(stillWaitingForExpectations.toArray())); } if (endTime < System.currentTimeMillis()) { - System.out.println(name + " timed out waiting for: " - + Arrays.toString(stillWaitingForExpectations.toArray())); + Logging.d(TAG, + name + " timed out waiting for: " + + Arrays.toString(stillWaitingForExpectations.toArray())); return false; } try { @@ -547,8 +553,8 @@ public class PeerConnectionTest { stillWaitingForExpectations = unsatisfiedExpectations(); } if (prev == null) { - System.out.println( - name + " didn't need to wait at\n " + (new Throwable()).getStackTrace()[1]); + Logging.d( + TAG, name + " didn't need to wait at\n " + (new Throwable()).getStackTrace()[1]); } return true; } @@ -1062,8 +1068,8 @@ public class PeerConnectionTest { offeringPC.addIceCandidate(candidate); } - assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); - assertTrue(answeringExpectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); + assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); + assertTrue(answeringExpectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); assertEquals(PeerConnection.SignalingState.STABLE, offeringPC.signalingState()); assertEquals(PeerConnection.SignalingState.STABLE, answeringPC.signalingState()); @@ -1119,7 +1125,7 @@ public class PeerConnectionTest { DataChannel.Buffer buffer = new DataChannel.Buffer(ByteBuffer.wrap("hello!".getBytes(Charset.forName("UTF-8"))), false); assertTrue(offeringExpectations.dataChannel.send(buffer)); - assertTrue(answeringExpectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); + assertTrue(answeringExpectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); // Construct this binary message two different ways to ensure no // shortcuts are taken. @@ -1131,7 +1137,7 @@ public class PeerConnectionTest { offeringExpectations.expectMessage(expectedBinaryMessage, true); assertTrue(answeringExpectations.dataChannel.send( new DataChannel.Buffer(ByteBuffer.wrap(new byte[] {1, 2, 3, 4, 5}), true))); - assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); + assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); offeringExpectations.expectStateChange(DataChannel.State.CLOSING); answeringExpectations.expectStateChange(DataChannel.State.CLOSING); @@ -1139,8 +1145,8 @@ public class PeerConnectionTest { answeringExpectations.expectStateChange(DataChannel.State.CLOSED); answeringExpectations.dataChannel.close(); offeringExpectations.dataChannel.close(); - assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); - assertTrue(answeringExpectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); + assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); + assertTrue(answeringExpectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); // Test SetBitrate. assertTrue(offeringPC.setBitrate(100000, 5000000, 500000000)); @@ -1279,8 +1285,8 @@ public class PeerConnectionTest { offeringPC.addIceCandidate(candidate); } - assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); - assertTrue(answeringExpectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); + assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); + assertTrue(answeringExpectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); assertEquals(PeerConnection.SignalingState.STABLE, offeringPC.signalingState()); assertEquals(PeerConnection.SignalingState.STABLE, answeringPC.signalingState()); @@ -1291,7 +1297,7 @@ public class PeerConnectionTest { DataChannel.Buffer buffer = new DataChannel.Buffer(ByteBuffer.wrap("hello!".getBytes(Charset.forName("UTF-8"))), false); assertTrue(offeringExpectations.dataChannel.send(buffer)); - assertTrue(answeringExpectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); + assertTrue(answeringExpectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); // Construct this binary message two different ways to ensure no // shortcuts are taken. @@ -1303,7 +1309,7 @@ public class PeerConnectionTest { offeringExpectations.expectMessage(expectedBinaryMessage, true); assertTrue(answeringExpectations.dataChannel.send( new DataChannel.Buffer(ByteBuffer.wrap(new byte[] {1, 2, 3, 4, 5}), true))); - assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); + assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); offeringExpectations.expectStateChange(DataChannel.State.CLOSING); answeringExpectations.expectStateChange(DataChannel.State.CLOSING); @@ -1311,8 +1317,8 @@ public class PeerConnectionTest { answeringExpectations.expectStateChange(DataChannel.State.CLOSED); answeringExpectations.dataChannel.close(); offeringExpectations.dataChannel.close(); - assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); - assertTrue(answeringExpectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); + assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); + assertTrue(answeringExpectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); // Free the Java-land objects and collect them. shutdownPC(offeringPC, offeringExpectations); @@ -1323,6 +1329,72 @@ public class PeerConnectionTest { System.gc(); } + // Tests that ICE candidates that are not allowed by an ICE transport type, thus not being + // signaled to the gathering PeerConnection, can be surfaced via configuration if allowed by the + // new ICE transport type, when RTCConfiguration.surfaceIceCandidatesOnIceTransportTypeChanged is + // true. + @Test + @SmallTest + public void testSurfaceIceCandidatesWhenIceTransportTypeChanged() throws Exception { + // For this test, we only need one PeerConnection to observe the behavior of gathering, and we + // create only the offering PC below. + // + // Allow loopback interfaces too since our Android devices often don't + // have those. + PeerConnectionFactory.Options options = new PeerConnectionFactory.Options(); + options.networkIgnoreMask = 0; + PeerConnectionFactory factory = + PeerConnectionFactory.builder().setOptions(options).createPeerConnectionFactory(); + + PeerConnection.RTCConfiguration rtcConfig = + new PeerConnection.RTCConfiguration(Arrays.asList()); + // NONE would prevent any candidate being signaled to the PC. + rtcConfig.iceTransportsType = PeerConnection.IceTransportsType.NONE; + // We must have the continual gathering enabled to allow the surfacing of candidates on the ICE + // transport type change. + rtcConfig.continualGatheringPolicy = PeerConnection.ContinualGatheringPolicy.GATHER_CONTINUALLY; + rtcConfig.surfaceIceCandidatesOnIceTransportTypeChanged = true; + + ObserverExpectations offeringExpectations = new ObserverExpectations("PCTest:offerer"); + PeerConnection offeringPC = factory.createPeerConnection(rtcConfig, offeringExpectations); + assertNotNull(offeringPC); + + // Create a data channel and set local description to kick off the ICE candidate gathering. + offeringExpectations.expectRenegotiationNeeded(); + DataChannel offeringDC = offeringPC.createDataChannel("offeringDC", new DataChannel.Init()); + assertEquals("offeringDC", offeringDC.label()); + + offeringExpectations.setDataChannel(offeringDC); + SdpObserverLatch sdpLatch = new SdpObserverLatch(); + offeringPC.createOffer(sdpLatch, new MediaConstraints()); + assertTrue(sdpLatch.await()); + SessionDescription offerSdp = sdpLatch.getSdp(); + assertEquals(offerSdp.type, SessionDescription.Type.OFFER); + assertFalse(offerSdp.description.isEmpty()); + + sdpLatch = new SdpObserverLatch(); + offeringExpectations.expectSignalingChange(SignalingState.HAVE_LOCAL_OFFER); + offeringPC.setLocalDescription(sdpLatch, offerSdp); + assertTrue(sdpLatch.await()); + assertNull(sdpLatch.getSdp()); + + assertEquals(offeringPC.getLocalDescription().type, offerSdp.type); + + // Wait until we satisfy all expectations in the setup. + assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); + + // Add the expectation of gathering at least one candidate, which should however fail because of + // the transport type NONE. + offeringExpectations.expectIceCandidates(1); + assertFalse(offeringExpectations.waitForAllExpectationsToBeSatisfied(SHORT_TIMEOUT_SECONDS)); + + // Change the transport type and we should be able to meet the expectation of gathering this + // time. + rtcConfig.iceTransportsType = PeerConnection.IceTransportsType.ALL; + offeringPC.setConfiguration(rtcConfig); + assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); + } + @Test @MediumTest public void testTrackRemovalAndAddition() throws Exception { @@ -1464,8 +1536,8 @@ public class PeerConnectionTest { offeringExpectations.expectFramesDelivered(1); answeringExpectations.expectFramesDelivered(1); - assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); - assertTrue(answeringExpectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); + assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); + assertTrue(answeringExpectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); assertEquals(PeerConnection.SignalingState.STABLE, offeringPC.signalingState()); assertEquals(PeerConnection.SignalingState.STABLE, answeringPC.signalingState()); @@ -1820,36 +1892,36 @@ public class PeerConnectionTest { // Call getStats (old implementation) before shutting down PC. expectations.expectOldStatsCallback(); assertTrue(pc.getStats(expectations, null /* track */)); - assertTrue(expectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); + assertTrue(expectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); // Call the new getStats implementation as well. expectations.expectNewStatsCallback(); pc.getStats(expectations); - assertTrue(expectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); + assertTrue(expectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); expectations.expectIceConnectionChange(IceConnectionState.CLOSED); expectations.expectStandardizedIceConnectionChange(IceConnectionState.CLOSED); expectations.expectConnectionChange(PeerConnectionState.CLOSED); expectations.expectSignalingChange(SignalingState.CLOSED); pc.close(); - assertTrue(expectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); + assertTrue(expectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); // Call getStats (old implementation) after calling close(). Should still // work. expectations.expectOldStatsCallback(); assertTrue(pc.getStats(expectations, null /* track */)); - assertTrue(expectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); + assertTrue(expectations.waitForAllExpectationsToBeSatisfied(DEFAULT_TIMEOUT_SECONDS)); - System.out.println("FYI stats: "); + Logging.d(TAG, "FYI stats: "); int reportIndex = -1; for (StatsReport[] reports : expectations.takeStatsReports()) { - System.out.println(" Report #" + (++reportIndex)); + Logging.d(TAG, " Report #" + (++reportIndex)); for (int i = 0; i < reports.length; ++i) { - System.out.println(" " + reports[i].toString()); + Logging.d(TAG, " " + reports[i].toString()); } } assertEquals(1, reportIndex); - System.out.println("End stats."); + Logging.d(TAG, "End stats."); pc.dispose(); } diff --git a/sdk/android/src/jni/pc/peer_connection.cc b/sdk/android/src/jni/pc/peer_connection.cc index 98c3dd01af..1e3a73d7ee 100644 --- a/sdk/android/src/jni/pc/peer_connection.cc +++ b/sdk/android/src/jni/pc/peer_connection.cc @@ -186,6 +186,9 @@ void JavaToNativeRTCConfiguration( rtc_config->presume_writable_when_fully_relayed = Java_RTCConfiguration_getPresumeWritableWhenFullyRelayed(jni, j_rtc_config); + rtc_config->surface_ice_candidates_on_ice_transport_type_changed = + Java_RTCConfiguration_getSurfaceIceCandidatesOnIceTransportTypeChanged( + jni, j_rtc_config); ScopedJavaLocalRef j_ice_check_interval_strong_connectivity = Java_RTCConfiguration_getIceCheckIntervalStrongConnectivity(jni, j_rtc_config); diff --git a/sdk/objc/api/peerconnection/RTCConfiguration.h b/sdk/objc/api/peerconnection/RTCConfiguration.h index f9e6edfd97..2c32311d9e 100644 --- a/sdk/objc/api/peerconnection/RTCConfiguration.h +++ b/sdk/objc/api/peerconnection/RTCConfiguration.h @@ -140,6 +140,18 @@ RTC_OBJC_EXPORT */ @property(nonatomic, assign) BOOL shouldPresumeWritableWhenFullyRelayed; +/* This flag is only effective when |continualGatheringPolicy| is + * RTCContinualGatheringPolicyGatherContinually. + * + * If YES, after the ICE transport type is changed such that new types of + * ICE candidates are allowed by the new transport type, e.g. from + * RTCIceTransportPolicyRelay to RTCIceTransportPolicyAll, candidates that + * have been gathered by the ICE transport but not matching the previous + * transport type and as a result not observed by PeerConnectionDelegateAdapter, + * will be surfaced to the delegate. + */ +@property(nonatomic, assign) BOOL shouldSurfaceIceCandidatesOnIceTransportTypeChanged; + /** If set to non-nil, controls the minimal interval between consecutive ICE * check packets. */ diff --git a/sdk/objc/api/peerconnection/RTCConfiguration.mm b/sdk/objc/api/peerconnection/RTCConfiguration.mm index 83ae575986..c2ff8bf17a 100644 --- a/sdk/objc/api/peerconnection/RTCConfiguration.mm +++ b/sdk/objc/api/peerconnection/RTCConfiguration.mm @@ -45,6 +45,8 @@ @synthesize shouldPruneTurnPorts = _shouldPruneTurnPorts; @synthesize shouldPresumeWritableWhenFullyRelayed = _shouldPresumeWritableWhenFullyRelayed; +@synthesize shouldSurfaceIceCandidatesOnIceTransportTypeChanged = + _shouldSurfaceIceCandidatesOnIceTransportTypeChanged; @synthesize iceCheckMinInterval = _iceCheckMinInterval; @synthesize iceRegatherIntervalRange = _iceRegatherIntervalRange; @synthesize sdpSemantics = _sdpSemantics; @@ -109,6 +111,8 @@ _shouldPruneTurnPorts = config.prune_turn_ports; _shouldPresumeWritableWhenFullyRelayed = config.presume_writable_when_fully_relayed; + _shouldSurfaceIceCandidatesOnIceTransportTypeChanged = + config.surface_ice_candidates_on_ice_transport_type_changed; if (config.ice_check_min_interval) { _iceCheckMinInterval = [NSNumber numberWithInt:*config.ice_check_min_interval]; @@ -160,6 +164,7 @@ _iceCandidatePoolSize, _shouldPruneTurnPorts, _shouldPresumeWritableWhenFullyRelayed, + _shouldSurfaceIceCandidatesOnIceTransportTypeChanged, _iceCheckMinInterval, _iceRegatherIntervalRange, _disableLinkLocalNetworks, @@ -239,6 +244,8 @@ nativeConfig->prune_turn_ports = _shouldPruneTurnPorts ? true : false; nativeConfig->presume_writable_when_fully_relayed = _shouldPresumeWritableWhenFullyRelayed ? true : false; + nativeConfig->surface_ice_candidates_on_ice_transport_type_changed = + _shouldSurfaceIceCandidatesOnIceTransportTypeChanged ? true : false; if (_iceCheckMinInterval != nil) { nativeConfig->ice_check_min_interval = absl::optional(_iceCheckMinInterval.intValue); }