admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Revert "Adding test for adding ICE candidate before applying answer."

Browse Source 
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 <steveanton@webrtc.org>
> Commit-Queue: Taylor Brandstetter <deadbeef@webrtc.org>
> 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 <hbos@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22863}
This commit is contained in:
Henrik Boström 2018-04-13 15:22:50 +00:00 committed by Commit Bot
parent fafeac3517
commit 5d8f8fa0b6
3 changed files with 20 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
pc/jseptransport2.cc
View File

@ -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) {

35
pc/peerconnection.cc
View File

@ -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;
}

19
pc/peerconnection_ice_unittest.cc
View File

@ -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);