From 5d8f8fa0b6816bf3bd0f09e60110dc515403a109 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Fri, 13 Apr 2018 15:22:50 +0000 Subject: [PATCH] Revert "Adding test for adding ICE candidate before applying answer." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit dd59d7049158a25f97ab1c7d381bfb4f8ed127c7. Reason for revert: Speculatively reverting this due to chromium test. The AutoRoller has been turned off for a couple of days due to the M67 branch cut. Did this cause a regression that made it into M67 or are the tests broken? Failed roll: https://chromium-review.googlesource.com/c/chromium/src/+/1011676 Example run: https://ci.chromium.org/p/chromium/builders/luci.chromium.try/mac_chromium_rel_ng/24406 Expected diff: https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=77d652517595443eea13f6ba9aaff67728305213&as=RTCPeerConnection-addIceCandidate-diff.txt Original change's description: > Adding test for adding ICE candidate before applying answer. > > This was working before, but somewhat by accident (because an error > wasn't being surfaced). > > This CL also starts surfacing that error, from > JsepTransportController::AddRemoteCandidates to PeerConnection. > > Bug: None > Change-Id: Ib48c9c00ea2a5baa5f7e3210c5dc7a339498b2d0 > Reviewed-on: https://webrtc-review.googlesource.com/69015 > Reviewed-by: Steve Anton > Commit-Queue: Taylor Brandstetter > Cr-Commit-Position: refs/heads/master@{#22830} TBR=steveanton@webrtc.org,deadbeef@webrtc.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: None Change-Id: I78a1df5d1e38569d02565bf343881420cc171347 Reviewed-on: https://webrtc-review.googlesource.com/69860 Reviewed-by: Henrik Boström Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/master@{#22863} --- pc/jseptransport2.cc | 12 +++-------- pc/peerconnection.cc | 35 +++++++++++++++---------------- pc/peerconnection_ice_unittest.cc | 19 ----------------- 3 files changed, 20 insertions(+), 46 deletions(-) diff --git a/pc/jseptransport2.cc b/pc/jseptransport2.cc index a86a37a452..a4a03487bd 100644 --- a/pc/jseptransport2.cc +++ b/pc/jseptransport2.cc @@ -253,18 +253,12 @@ webrtc::RTCError JsepTransport2::SetRemoteJsepTransportDescription( webrtc::RTCError JsepTransport2::AddRemoteCandidates( const Candidates& candidates) { - // Need a remote description for the remote ICE credentials; can't send - // connectivity checks without them. - // - // Assuming the application's signaling mechanism maintains ordering, it - // shouldn't be possible to hit this error; session descriptions are - // generated before candidates, so they should be received before candidates - // as well. - if (!remote_description_) { + if (!local_description_ || !remote_description_) { return webrtc::RTCError(webrtc::RTCErrorType::INVALID_STATE, mid() + " is not ready to use the remote candidate " - "because the remote description is not set."); + "because the local or remote description is " + "not set."); } for (const cricket::Candidate& candidate : candidates) { diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index b3a2c45606..cce90216c1 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -5340,26 +5340,25 @@ bool PeerConnection::UseCandidate(const IceCandidateInterface* candidate) { // Invoking BaseSession method to handle remote candidates. RTCError error = transport_controller_->AddRemoteCandidates(content.name, candidates); - if (!error.ok()) { + if (error.ok()) { + // Candidates successfully submitted for checking. + if (ice_connection_state_ == PeerConnectionInterface::kIceConnectionNew || + ice_connection_state_ == + PeerConnectionInterface::kIceConnectionDisconnected) { + // If state is New, then the session has just gotten its first remote ICE + // candidates, so go to Checking. + // If state is Disconnected, the session is re-using old candidates or + // receiving additional ones, so go to Checking. + // If state is Connected, stay Connected. + // TODO(bemasc): If state is Connected, and the new candidates are for a + // newly added transport, then the state actually _should_ move to + // checking. Add a way to distinguish that case. + SetIceConnectionState(PeerConnectionInterface::kIceConnectionChecking); + } + // TODO(bemasc): If state is Completed, go back to Connected. + } else if (error.message()) { RTC_LOG(LS_WARNING) << error.message(); - return false; } - - // Candidates successfully submitted for checking. - if (ice_connection_state_ == PeerConnectionInterface::kIceConnectionNew || - ice_connection_state_ == - PeerConnectionInterface::kIceConnectionDisconnected) { - // If state is New, then the session has just gotten its first remote ICE - // candidates, so go to Checking. - // If state is Disconnected, the session is re-using old candidates or - // receiving additional ones, so go to Checking. - // If state is Connected, stay Connected. - // TODO(bemasc): If state is Connected, and the new candidates are for a - // newly added transport, then the state actually _should_ move to - // checking. Add a way to distinguish that case. - SetIceConnectionState(PeerConnectionInterface::kIceConnectionChecking); - } - // TODO(bemasc): If state is Completed, go back to Connected. return true; } diff --git a/pc/peerconnection_ice_unittest.cc b/pc/peerconnection_ice_unittest.cc index c19e31d0a1..e8f450fe9f 100644 --- a/pc/peerconnection_ice_unittest.cc +++ b/pc/peerconnection_ice_unittest.cc @@ -398,25 +398,6 @@ TEST_P(PeerConnectionIceTest, CannotAddCandidateWhenRemoteDescriptionNotSet) { EXPECT_FALSE(caller->pc()->AddIceCandidate(&jsep_candidate)); } -// Should be able to add a candidate immediately after setting a remote offer, -// even if an answer hasn't been created yet. -TEST_P(PeerConnectionIceTest, CanAddCandidateWhenLocalDescriptionNotSet) { - const SocketAddress kCallerAddress("1.1.1.1", 0); - - auto caller = CreatePeerConnectionWithAudioVideo(); - auto callee = CreatePeerConnectionWithAudioVideo(); - caller->network()->AddInterface(kCallerAddress); - - // Apply offer, but not answer. - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - - // Wait for gathering to finish and try adding a candidate from the caller. - EXPECT_TRUE_WAIT(caller->IsIceGatheringDone(), kIceCandidatesTimeout); - ASSERT_LT(0u, caller->observer()->GetCandidatesByMline(0).size()); - EXPECT_TRUE(callee->pc()->AddIceCandidate( - caller->observer()->GetCandidatesByMline(0)[0])); -} - TEST_P(PeerConnectionIceTest, DuplicateIceCandidateIgnoredWhenAdded) { const SocketAddress kCalleeAddress("1.1.1.1", 1111);