Accept use-candidate for ice-lite #739

boks1971 · 2024-10-30T19:03:46Z

There could be a mismatch between the two ends in candidate priority
when using peer reflexive. It happens in the following scenario

Client has two srflx candidates.
a. The first one gets discovered by LiveKit server as prflx.
b. The second one gets added via ice-trickle first and then
gets a STUN ping. So, it is srflx remote candidate from
server's point-of-view.
This leads to a priority issue.
a. Both candidates have same priority from client's point-of-view
(both are srflx).
b. But, from server's point-of-view, the first candidate has
higher priority (prflx).
The first candidate establishes connectivity and becomes
the selected pair (client is ICE controlling and server is
ICE controlled, server is in ICE lite).
libwebrtc does a sort and switch some time later based on RTT.
As client side has both at same priority, RTT based sorting
could make the second candidate the preferred one.
So, the client sends useCandidate=1 for the second candidate.
pion/ice does not switch because the selected pair is at
higher priority due to prflx candidate.
STUN pings do not happen and the ICE connection eventually fails.

ICE controlled agent should accept use-candidate unconditionally if
it is an ICE lite agent.
Just in case existing behaviour is needed, it can be configured
using EnableUseCandidateCheckPriority.

NOTE: With aggressive nomination, the selected pair could change
a few times, but should eventually settle on what the controlling
side wants.

codecov · 2024-10-30T19:14:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.61%. Comparing base (166b1b7) to head (15ab10a).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #739      +/-   ##
==========================================
+ Coverage   78.53%   78.61%   +0.07%     
==========================================
  Files          41       41              
  Lines        4762     4764       +2     
==========================================
+ Hits         3740     3745       +5     
+ Misses        788      785       -3     
  Partials      234      234

Flag	Coverage Δ
go	`78.61% <100.00%> (+0.07%)`	⬆️
wasm	`22.49% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nils-ohlmeier · 2024-10-30T20:31:53Z

Interesting scenario.
I assume 1.a gets added because the client uses it's host candidate to send a binding request. And 1.b takes longer because of the additional round trip time for the STUN checks to discovers it's srflx and transmitting it to the server.

The part which concerns me here is the proposed solution: I don't think that the right solution here should be to disable peer reflexive altogether. This will slow down ICE connectivity in certain cases.
In 3. you say that Pion in ICE controlled (because it is using ICE lite). So when the ICE controller sends a USE_CANDIDATE Pion needs to follow that. Priorities should only matter for the order in which candidates get tried. The controlled can not use priorities to ignore a USE_CANDIDATE from the controller.

boks1971 · 2024-10-31T01:19:08Z

Interesting scenario. I assume 1.a gets added because the client uses it's host candidate to send a binding request. And 1.b takes longer because of the additional round trip time for the STUN checks to discovers it's srflx and transmitting it to the server.

The part which concerns me here is the proposed solution: I don't think that the right solution here should be to disable peer reflexive altogether. This will slow down ICE connectivity in certain cases. In 3. you say that Pion in ICE controlled (because it is using ICE lite). So when the ICE controller sends a USE_CANDIDATE Pion needs to follow that. Priorities should only matter for the order in which candidates get tried. The controlled can not use priorities to ignore a USE_CANDIDATE from the controller.

Thank you @nils-ohlmeier for the feedback.

That was my original inclination too (accepting USE-CANDIDATE - see a bit of discussion in Pion Slack). But, wanted to do something that did not change existing behaviour. But, after discussion with @Sean-Der in Slack and your feedback here, convinced that it is the right approach. So, I am going to change this PR to accept USE-CANDIDATE (will still have a flag to fall back to older behaviour just in case somebody wants it). Will tag you for review once I finish the changes.

There is this long standing FIrefox issue where it does aggressive nomination even if the peer is ice-lite. I guess in that case the selected pair will change a bunch of times and settle on the final one. Is that correct?

boks1971 · 2024-10-31T02:48:20Z

@nils-ohlmeier Please have a look at this PR when you get a chance and let me know if it looks okay.

cnderrauber · 2024-10-31T03:15:19Z

I think we'd better to make it as a default behavior for controlled ice-lite agent insead of all controlled agent. The failure case is chrome/firefox does renomiantion/aggressive-nomination when remote peer is ice lite, and there is no problem for full agent. So the current behavior of pion comply with the rfc, we don't need to break it for the bad behavior cause by remote peer.

cnderrauber · 2024-10-31T03:19:35Z

Compare priority is ok for both nomiation type in rfc5245:

regular: there are only 1 nomiate so it is ok
aggressive: ICE always selects the highest-priority nominated candidate pair from the valid list as the one used for media , it doesn't mention only controlled needs to compare priority so I think both ends would do the check.

Checked the libwebrtc code, it also compares the nominated connections from remote side when it picks controlled role, the conditions are complex but contains the priority.

boks1971 · 2024-10-31T04:32:17Z

@cnderrauber Thank you for the review. I have updated the code to apply it only for ice-lite agent. Can you please re-review when you get a chance?

cnderrauber

looks good!

cnderrauber · 2024-10-31T05:19:18Z

selection.go

@@ -241,7 +241,7 @@ func (s *controlledSelector) HandleSuccessResponse(m *stun.Message, local, remot
 	s.log.Tracef("Found valid candidate pair: %s", p)
 	if p.nominateOnBindingSuccess {
 		if selectedPair := s.agent.getSelectedPair(); selectedPair == nil ||
-			(selectedPair != p && selectedPair.priority() <= p.priority()) {
+			(selectedPair != p && ((!s.agent.enableUseCandidateCheckPriority && s.agent.lite) || selectedPair.priority() <= p.priority())) {


how about make it a agent method needsToCheckPriorityOnNominated

Good call @cnderrauber . Updated with a helper function.

There could be a mismatch between the two ends in candidate priority when using peer reflexive. It happens in the following scenario 1. Client has two srflx candidates. a. The first one gets discovered by LiveKit server as prflx. b. The second one gets added via ice-trickle first and then gets a STUN ping. So, it is srflx remote candidate from server's point-of-view. 2. This leads to a priority issue. a. Both candidates have same priority from client's point-of-view (both are srflx). b. But, from server's point-of-view, the first candidate has higher priority (prflx). 3. The first candidate establishes connectivity and becomes the selected pair (client is ICE controlling and server is ICE controlled, server is in ICE lite). 4. libwebrtc does a sort and switch some time later based on RTT. As client side has both at same priority, RTT based sorting could make the second candidate the preferred one. So, the client sends useCandidate=1 for the second candidate. pion/ice does not switch because the selected pair is at higher priority due to prflx candidate. 5. STUN pings do not happen and the ICE connection eventually fails. ICE controlled agent should accept use-candidate unconditionally if it is an ICE lite agentt. Just in case existing behaviour is needed, it can be configured using `EnableUseCandidateCheckPriority`. NOTE: With aggressive nomination, the selected pair could change a few times, but should eventually settle on what the controlling side wants.

boks1971 · 2024-10-31T05:35:06Z

@Sean-Der @davidzhao @nils-ohlmeier going to merge this based on discussions in Slack/this PR and @cnderrauber 's review. Want to port this to v2 and start using it in LiveKit project. But, please do review when you get a chance and let me know if you have feedback and I will address it in a subsequent PR.

There could be a mismatch between the two ends in candidate priority when using peer reflexive. It happens in the following scenario 1. Client has two srflx candidates. a. The first one gets discovered by LiveKit server as prflx. b. The second one gets added via ice-trickle first and then gets a STUN ping. So, it is srflx remote candidate from server's point-of-view. 2. This leads to a priority issue. a. Both candidates have same priority from client's point-of-view (both are srflx). b. But, from server's point-of-view, the first candidate has higher priority (prflx). 3. The first candidate establishes connectivity and becomes the selected pair (client is ICE controlling and server is ICE controlled, server is in ICE lite). 4. libwebrtc does a sort and switch some time later based on RTT. As client side has both at same priority, RTT based sorting could make the second candidate the preferred one. So, the client sends useCandidate=1 for the second candidate. pion/ice does not switch because the selected pair is at higher priority due to prflx candidate. 5. STUN pings do not happen and the ICE connection eventually fails. ICE controlled agent should accept use-candidate unconditionally if it is an ICE lite agentt. Just in case existing behaviour is needed, it can be configured using `EnableUseCandidateCheckPriority`. NOTE: With aggressive nomination, the selected pair could change a few times, but should eventually settle on what the controlling side wants.

Sean-Der · 2024-10-31T13:54:52Z

@boks1971 sounds good to me!

What do you need to move to /v3? If I port livekit/livekit would that be a good start?

boks1971 · 2024-10-31T14:06:22Z

Nothing specifically needed @Sean-Der as the default behaviour is the desired behaviour. I cherry-picked the change to v2 and tagged it v2.3.37 and have updated livekit dependencies to use that tag.

boks1971 · 2024-10-31T14:09:32Z

I was debating adding a settings engine setting to enable the priority check (the flag added in this PR) and thread it through. But, as we discussed the default behaviour added in this PR looks like the right one and the flag is just a safety net. So, decided to wait to see if there are any ill effects before adding another things to SettingsEngine.

nils-ohlmeier · 2024-11-01T17:04:25Z

There is this long standing FIrefox issue where it does aggressive nomination even if the peer is ice-lite. I guess in that case the selected pair will change a bunch of times and settle on the final one. Is that correct?

Yeah that one sucks. The problem is that Firefox uses its own ICE stack written in C with lots of pointer magic. Most of the developers don't want to touch it with a ten foot pole. The team is aware of the problem. But my guess is that it is rather unlikely this will get fixed anytime soon.

The best known workaround is to implement on the ice-lite server side to use the candidate pair where it receives traffic from. I'm not sure if a workaround like that should go into Pion to be more compatible with Firefox. @Sean-Der do you have thoughts on this?

boks1971 · 2024-11-01T17:06:50Z

I did that approach in a previous life and it worked well. But, that was a few years ago.

boks1971 requested review from davidzhao, Sean-Der and cnderrauber October 30, 2024 19:04

boks1971 force-pushed the raja_disable_peer_reflexive branch from 8a368bf to fbddc45 Compare October 30, 2024 19:12

boks1971 force-pushed the raja_disable_peer_reflexive branch from fbddc45 to 43f85ec Compare October 31, 2024 02:38

boks1971 changed the title ~~Add option to disble peer reflexive discovery~~ Accept use-candidate Oct 31, 2024

boks1971 force-pushed the raja_disable_peer_reflexive branch from 43f85ec to 9bcc191 Compare October 31, 2024 02:46

boks1971 requested a review from nils-ohlmeier October 31, 2024 02:46

boks1971 force-pushed the raja_disable_peer_reflexive branch from 9bcc191 to 242239b Compare October 31, 2024 04:30

boks1971 changed the title ~~Accept use-candidate~~ Accept use-candidate for ice-lite Oct 31, 2024

cnderrauber approved these changes Oct 31, 2024

View reviewed changes

boks1971 force-pushed the raja_disable_peer_reflexive branch from 242239b to 15ab10a Compare October 31, 2024 05:28

boks1971 merged commit 9407bb0 into master Oct 31, 2024
15 checks passed

boks1971 deleted the raja_disable_peer_reflexive branch October 31, 2024 05:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept use-candidate for ice-lite #739

Accept use-candidate for ice-lite #739

boks1971 commented Oct 30, 2024 •

edited

Loading

codecov bot commented Oct 30, 2024 •

edited

Loading

nils-ohlmeier commented Oct 30, 2024

boks1971 commented Oct 31, 2024

boks1971 commented Oct 31, 2024

cnderrauber commented Oct 31, 2024

cnderrauber commented Oct 31, 2024 •

edited

Loading

boks1971 commented Oct 31, 2024

cnderrauber left a comment

cnderrauber Oct 31, 2024

boks1971 Oct 31, 2024

boks1971 commented Oct 31, 2024

Sean-Der commented Oct 31, 2024

boks1971 commented Oct 31, 2024

boks1971 commented Oct 31, 2024

nils-ohlmeier commented Nov 1, 2024

boks1971 commented Nov 1, 2024

Accept use-candidate for ice-lite #739

Accept use-candidate for ice-lite #739

Conversation

boks1971 commented Oct 30, 2024 • edited Loading

codecov bot commented Oct 30, 2024 • edited Loading

Codecov Report

nils-ohlmeier commented Oct 30, 2024

boks1971 commented Oct 31, 2024

boks1971 commented Oct 31, 2024

cnderrauber commented Oct 31, 2024

cnderrauber commented Oct 31, 2024 • edited Loading

boks1971 commented Oct 31, 2024

cnderrauber left a comment

Choose a reason for hiding this comment

cnderrauber Oct 31, 2024

Choose a reason for hiding this comment

boks1971 Oct 31, 2024

Choose a reason for hiding this comment

boks1971 commented Oct 31, 2024

Sean-Der commented Oct 31, 2024

boks1971 commented Oct 31, 2024

boks1971 commented Oct 31, 2024

nils-ohlmeier commented Nov 1, 2024

boks1971 commented Nov 1, 2024

boks1971 commented Oct 30, 2024 •

edited

Loading

codecov bot commented Oct 30, 2024 •

edited

Loading

cnderrauber commented Oct 31, 2024 •

edited

Loading