Support active TCP candidates (RFC6544) #565

ashellunts · 2023-05-09T19:18:58Z

This PR implements support for active TCP candidates as specified in RFC6544

Open issue: local active TCP candidate can be connected only with 1 remote.

codecov · 2023-05-09T20:53:03Z

Codecov Report

Patch coverage: 66.99% and project coverage change: +0.59 🎉

Comparison is base (898746c) 77.84% compared to head (5b2e30d) 78.44%.

❗ Current head 5b2e30d differs from pull request most recent head 72773df. Consider uploading reports for the commit 72773df to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #565      +/-   ##
==========================================
+ Coverage   77.84%   78.44%   +0.59%     
==========================================
  Files          41       41              
  Lines        4215     4272      +57     
==========================================
+ Hits         3281     3351      +70     
+ Misses        726      714      -12     
+ Partials      208      207       -1

Flag	Coverage Δ
go	`78.44% <66.99%> (+0.59%)`	⬆️
wasm	`22.80% <9.70%> (+0.04%)`	⬆️

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

Impacted Files	Coverage Δ
agent_config.go	`83.14% <40.00%> (+1.00%)`	⬆️
candidate_base.go	`86.47% <62.50%> (+0.15%)`	⬆️
gather.go	`66.54% <63.15%> (-0.25%)`	⬇️
agent.go	`82.16% <70.73%> (+0.12%)`	⬆️
candidatetype.go	`79.48% <80.00%> (+0.69%)`	⬆️
candidate_peer_reflexive.go	`81.81% <100.00%> (+0.56%)`	⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

agent.go

agent_config.go

agent_active_tcp_test.go

gather.go

stv0g · 2023-05-10T09:31:55Z

Hi @ashellunts, its really cool to see some progress here :) 🥳

ashellunts · 2023-05-10T09:59:25Z

Another thing I am thinking. Would it be a good idea to test in different network conditions using vnet? @stv0g

stv0g · 2023-05-10T10:05:48Z

@ashellunts Unfortuantely, transport/vnet has no TCP support yet: pion/transport#27

I also dont expect this to land soon. Hence, we should proceed without vnet tests for now..

Sean-Der · 2023-05-10T16:56:20Z

Amazing work @ashellunts so happy to see this land. Huge thank you @stv0g for being a great reviewer :)

Passive requires using a Mux. I think that makes sense. TCP is only preferable in restrictive environments. Those environments usually only allow a few TCP ports.

I think we can put this behind Type+NetworkTypes in Config! If Host+TCP is enabled we should have Active TCP.

My only call out is we should prefer Host+Srflx UDP over TCP. TCP I think is preferable to Relay IMO

I would love to see this land ASAP! If it has bugs/TODOs we can always fix those :)

stv0g · 2023-05-10T17:15:51Z

Hi @Sean-Der,

thanks for your feedback!

TCP is only preferable in restrictive environments. Those environments usually only allow a few TCP ports.

I assumed that we will see the passive side usually deployed at a Pion agent running at some public server.
In that case the active agents would be running in a restricted network.

My only call out is we should prefer Host+Srflx UDP over TCP. TCP I think is preferable to Relay IMO

Good point. Actually RFC6544 Section 4.2 specifies this behaviour:

The transport protocol itself is a criteria for choosing one candidate over another.
If a particular media stream can run over UDP or TCP, the UDP candidates might be preferred over the TCP candidates.
This allows ICE to use the lower latency UDP connectivity if it exists but fallback to TCP if UDP doesn't work.

(The RFC continues to elaborate here...)

I would love to see this land ASAP! If it has bugs/TODOs we can always fix those :)

I agree :) We just need to get the API right :-S

ashellunts · 2023-05-10T17:38:38Z

I think we can put this behind Type+NetworkTypes in Config! If Host+TCP is enabled we should have Active TCP.

Currently passive candidates are collected when Host+TCP is used. Do you suggest to enable active ones always together with passive ones, when Host+TCP is enabled?

Does it make sense to have option to choose from following cases:

only passive ones (as it is right now); this one allows to disable active candidates so no changes in behavior for library clients;
only active ones; for client-side applications;
both active and passive; not sure use case here. @stv0g you had some ideas regarding this. Is it server to server communication?

Sean-Der · 2023-05-11T00:00:29Z

Currently passive candidates are collected when Host+TCP is used. Do you suggest to enable active ones always together with passive ones, when Host+TCP is enabled?

I think we should do this.

Enable Active by default. If AgentConfig has ICECandidateTypeHost and NetworkTypeTCP (the default values) we should connect to remote Passives.

This will have zero effect on most users today. If the remote only publishes UDP candidates no TCP connections will be made. If the remote publishes a TCP candidate we will attempt a connection.

Leave Passive disabled (same behavior we have now). Don't modify any passive UDP code and leave UDP Mux etc.. as it is.

Passive candidates require special configuration. You usually want them on :80 or :443 and you want all your ICE Agents sharing that. If we just listen on random TCP ports I don't think we are giving users the best experience possible.

I also believe that listening on a new random TCP Port would be confusing to users.

stv0g · 2023-05-11T06:35:17Z

Enable Active by default.

👍

Leave Passive disabled (same behavior we have now).

👍

stv0g · 2023-05-11T06:37:59Z

Both active and passive; not sure use case here. @stv0g you had some ideas regarding this. Is it server to server communication?

I think also local LAN connectivity can profit from TCP candidates if requested.
Using TCP candidates can have also its advantages based on the use case (e.g. file transfers instead of video streaming).

And while pion/ice is mainly used for pion/webrtc, WebRTC is not the only use case (I have others :D)

ashellunts · 2023-05-11T08:52:07Z

OK, let me try to summarize:

ActiveTCP flag is removed, so no API changes needed.
If ICECandidateTypeHost and NetworkTypeTCP is set, active TCP candidate is generated.
If (ICECandidateTypeHost and NetworkTypeTCP is set) AND TCPMux is set, both active and passive candidates are generated.

I have checked: UDP host+srflx has less priority than TCP host+prflx (active/passive pair).
Priority calculation should be updated...

Sean-Der · 2023-05-11T15:30:10Z

@ashellunts summary sounds great to me! If you have a PR that does that would approve.

@stv0g totally agree. If we prioritize WebRTC it makes everything worse :(

We should write a ‘best configuration guide’. So developers know the best way to build their systems for connectivity success. If they are doing cross subnet TCP they probably want on a known part. Things like that could help a lot .

stv0g

Hey, good work :)

I think we are not yet following the RFC in terms of type and local preferences.

agent.go

agent_config.go

candidate_base.go

stv0g · 2023-05-12T17:00:41Z

candidatetype.go

@@ -38,7 +38,21 @@ func (c CandidateType) String() string {
 // The RECOMMENDED values are 126 for host candidates, 100
 // for server reflexive candidates, 110 for peer reflexive candidates,
 // and 0 for relayed candidates.
-func (c CandidateType) Preference() uint16 {
+func (c CandidateType) Preference(networkType NetworkType) uint16 {


I dont think this exactly reflects the recommendation of RFC6544 Section 4.2:

When both UDP and TCP candidates are offered for the same media stream, and one transport protocol should be preferred over the other, the type preferences for the preferred transport protocol candidates SHOULD be increased and/or the type preferences for the other transport protocol candidates SHOULD be decreased.

So I think we should have a user-configurable offset which is added/substracted from the type-dependent preference.

How much the values should be increased or decreased depends on whether it is more important to choose a certain transport protocol or a certain candidate type. If the candidate type is more important (e.g., even if UDP is preferred, TCP host candidates are preferred over UDP server reflexive candidates) changing type preference values by one for the other transport protocol candidates is enough. On the other hand, if the transport protocol is more important (e.g., any UDP candidate is preferred over any TCP candidate), all the preferred transport protocol candidates SHOULD have type preference higher than the other transport protocol candidates. However, it is RECOMMENDED that the relayed candidates are still preferred lower than the other candidate types. For RTP-based media streams, it is RECOMMENDED that UDP candidates are preferred over TCP candidates.

That would mean we need to change existing default type priority for UDP.
Because currently host UDP type priority is 126, max value. So we can't increase it anymore.
If we want to use offsets, we need to make lower.

That may affect existing applications. Personally I would not want to affect existing library clients if they don't use TCP at all.

See also 92be181

Though we may use offset to reduce TCP priorities and not change UDP ones. That would conform to the standard.

stv0g · 2023-05-12T17:10:51Z

gather.go

@@ -225,7 +199,7 @@ func (a *Agent) gatherCandidatesLocal(ctx context.Context, networkTypes []Networ
 					Address:   address,
 					Port:      connAndPort.port,
 					Component: ComponentRTP,
-					TCPType:   tcpType,
+					TCPType:   connAndPort.tcpType,


Can we rename this simply to cfg? And the corresponding conns to cfgs?

I prefer more verbose naming. What about connectionConfiguration?

agent.go

ashellunts · 2023-05-13T12:59:30Z

I have added tests that check if both TCP/UDP are enabled, UDP is selected pair but the test fails sometime. Not sure why... any ideas? @stv0g

Looks like if TCP is selected at first, later it is not changed to UDP (I have added sleep and selected candidate does not change), though UDP pair priority is higher.

ashellunts · 2023-05-13T18:32:01Z

Looks like it is expected behavior.

If I add timeout for host candidate, UDP is always selected.

Sean-Der

Fantastic work! I am all for merging this @ashellunts

We should then cut a release and reach out to the user that was looking to use Active TCP. Tell me if there is anything I can do to help :)

ashellunts · 2023-05-15T08:54:26Z

Added user option to configure relative offset of TCP candidate type priority. By default it is set to 27 so that UDP+srlfx > TCP+host.

Sean-Der · 2023-05-15T13:44:34Z

@ashellunts mind just squashing into one commit? Just to make debugging easier!

really excited to see this long standing issue resolved:)

By default TCP candidate type priority is UDP one minus 27 (except relay), so that UDP+srlfx priority > TCP+host priority. That priority offset can be configured using AgentConfig. Ipv6 TCP candidates are also supported. Open issue: local active TCP candidate can be connected only with 1 remote passive candidate.

ashellunts · 2023-05-15T14:01:38Z

Sure, I like to make smaller commits during review phases, so that reviewer can see difference pushed.
Currently there is 1 open issue: local active TCP candidate can be connected only with 1 remote passive candidate.

really excited to see this long standing issue resolved:)

Yes, I am also glad to finish the feature, especially when there are people waiting for it.

Off topic: what percentage in codecov/patch report means?

agent_config.go

ashellunts mentioned this pull request May 9, 2023

Implement Active TCP-ICE #394

Closed

ashellunts changed the title ~~Active tcp~~ Active TCP mode May 9, 2023

ashellunts requested review from stv0g and Sean-Der May 9, 2023 19:20

ashellunts force-pushed the active-tcp branch from 5a6444b to 1edec30 Compare May 10, 2023 06:41

ashellunts marked this pull request as ready for review May 10, 2023 06:42

ashellunts force-pushed the active-tcp branch 2 times, most recently from 5e6c8d0 to 9afe3ad Compare May 10, 2023 07:00

stv0g requested changes May 10, 2023

View reviewed changes

stv0g mentioned this pull request May 10, 2023

Active TCP candidates? #562

Closed

stv0g changed the title ~~Active TCP mode~~ Support active TCP candidates (RFC6544) May 10, 2023

ashellunts force-pushed the active-tcp branch from 9afe3ad to 272ebee Compare May 10, 2023 17:38

ashellunts force-pushed the active-tcp branch 3 times, most recently from 459f146 to e4e2848 Compare May 12, 2023 12:13

ashellunts requested a review from stv0g May 12, 2023 12:56

stv0g requested changes May 12, 2023

View reviewed changes

ashellunts force-pushed the active-tcp branch 2 times, most recently from dbdde52 to 268fff8 Compare May 13, 2023 17:05

ashellunts requested a review from stv0g May 13, 2023 21:25

Sean-Der approved these changes May 15, 2023

View reviewed changes

ashellunts force-pushed the active-tcp branch from de5dd00 to e61e110 Compare May 15, 2023 08:58

Fix error messages

886f123

ashellunts force-pushed the active-tcp branch from e61e110 to 5b2e30d Compare May 15, 2023 08:58

ashellunts force-pushed the active-tcp branch from 5b2e30d to 72773df Compare May 15, 2023 13:53

stv0g approved these changes May 15, 2023

View reviewed changes

agent_config.go Show resolved Hide resolved

stv0g merged commit 1d502ca into master May 15, 2023

stv0g deleted the active-tcp branch May 15, 2023 20:41

stv0g mentioned this pull request May 15, 2023

Minor style fixes for active TCP candidate support #568

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support active TCP candidates (RFC6544) #565

Support active TCP candidates (RFC6544) #565

ashellunts commented May 9, 2023 •

edited

Loading

codecov bot commented May 9, 2023 •

edited

Loading

stv0g commented May 10, 2023

ashellunts commented May 10, 2023

stv0g commented May 10, 2023

Sean-Der commented May 10, 2023

stv0g commented May 10, 2023

ashellunts commented May 10, 2023 •

edited

Loading

Sean-Der commented May 11, 2023

stv0g commented May 11, 2023

stv0g commented May 11, 2023

ashellunts commented May 11, 2023 •

edited

Loading

Sean-Der commented May 11, 2023

stv0g left a comment

stv0g May 12, 2023

ashellunts May 13, 2023

ashellunts May 13, 2023

ashellunts May 13, 2023

stv0g May 12, 2023

ashellunts May 13, 2023

ashellunts commented May 13, 2023

ashellunts commented May 13, 2023

Sean-Der left a comment

ashellunts commented May 15, 2023

Sean-Der commented May 15, 2023

ashellunts commented May 15, 2023

Support active TCP candidates (RFC6544) #565

Support active TCP candidates (RFC6544) #565

Conversation

ashellunts commented May 9, 2023 • edited Loading

codecov bot commented May 9, 2023 • edited Loading

Codecov Report

stv0g commented May 10, 2023

ashellunts commented May 10, 2023

stv0g commented May 10, 2023

Sean-Der commented May 10, 2023

stv0g commented May 10, 2023

ashellunts commented May 10, 2023 • edited Loading

Sean-Der commented May 11, 2023

stv0g commented May 11, 2023

stv0g commented May 11, 2023

ashellunts commented May 11, 2023 • edited Loading

Sean-Der commented May 11, 2023

stv0g left a comment

Choose a reason for hiding this comment

stv0g May 12, 2023

Choose a reason for hiding this comment

ashellunts May 13, 2023

Choose a reason for hiding this comment

ashellunts May 13, 2023

Choose a reason for hiding this comment

ashellunts May 13, 2023

Choose a reason for hiding this comment

stv0g May 12, 2023

Choose a reason for hiding this comment

ashellunts May 13, 2023

Choose a reason for hiding this comment

ashellunts commented May 13, 2023

ashellunts commented May 13, 2023

Sean-Der left a comment

Choose a reason for hiding this comment

ashellunts commented May 15, 2023

Sean-Der commented May 15, 2023

ashellunts commented May 15, 2023

ashellunts commented May 9, 2023 •

edited

Loading

codecov bot commented May 9, 2023 •

edited

Loading

ashellunts commented May 10, 2023 •

edited

Loading

ashellunts commented May 11, 2023 •

edited

Loading