Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Active TCP-ICE #392

Closed
wants to merge 3 commits into from
Closed

Conversation

dgunay
Copy link

@dgunay dgunay commented Oct 9, 2021

Description

Implementing Active TCP ICE to enable Pion to connect to passive TCP ICE hosts.

Still relatively new to WebRTC and ICE, so if I stray from RFC 6544 or otherwise misunderstand something fundamental about the way this should work please don't hesitate to let me know.

Reference issue

#245 - Continued ICE improvements

@dgunay
Copy link
Author

dgunay commented Oct 11, 2021

@Sean-Der dove into the code a bit and tried to open a TCP connection to the remote. That eventually lead me into the gatherCandidatesLocal function. I have a question (maybe I should just read the RFC front to back):

I see the code structure right now assumes each local candidate opens a PacketConn on some port; section 4.1 of RFC6544:

First, agents SHOULD obtain host candidates as described in
Section 5.1. Then, each agent SHOULD "obtain" (allocate a
placeholder for) an active host candidate for each component of each
TCP-capable media stream on each interface that the host has. The
agent does not yet have to actually allocate a port for these
candidates, but they are used for the creation of the check lists.

candidateBase.start() kicks off recvLoop(), which crashes because I didn't provide a port. Any suggestions on how we can best structure the code to support placeholder active TCP candidates?

I also tend to litter my code with TODOs and FIXMEs so I might have tugged on some threads there that I'm forgetting.

@codecov
Copy link

codecov bot commented Oct 14, 2021

Codecov Report

Merging #392 (238317f) into master (6d30128) will decrease coverage by 53.10%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #392       +/-   ##
===========================================
- Coverage   78.48%   25.38%   -53.11%     
===========================================
  Files          33       33               
  Lines        2598     2616       +18     
===========================================
- Hits         2039      664     -1375     
- Misses        391     1924     +1533     
+ Partials      168       28      -140     
Flag Coverage Δ
go ?
wasm 25.38% <0.00%> (-0.18%) ⬇️

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

Impacted Files Coverage Δ
agent.go 0.00% <0.00%> (-81.72%) ⬇️
agent_config.go 0.00% <ø> (-84.38%) ⬇️
candidate_base.go 55.17% <ø> (-27.59%) ⬇️
gather.go 0.00% <0.00%> (-67.68%) ⬇️
candidate_peer_reflexive.go 0.00% <0.00%> (-84.62%) ⬇️
transport.go 0.00% <0.00%> (-82.70%) ⬇️
agent_stats.go 0.00% <0.00%> (-79.63%) ⬇️
udp_mux.go 0.00% <0.00%> (-77.87%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d30128...238317f. Read the comment docs.

@Sean-Der
Copy link
Member

@dgunay added you to the Pion org so you can push branches/run CI.

I would override recvLoop maybe? Like CandidateRelay does with close. Reading PR now.

@Sean-Der
Copy link
Member

What you will actually want I believe is one CandidateHost that contains a TCP Socket.

When AddICECandidate comes in with a passive candidate you send off a connection request.

You will need a recvLoop per connection attempt. So I think overriding it completely makes sense!

@ashellunts
Copy link
Contributor

Hi @dgunay, if you need helping hands with this, let me know.

I was also planning to work on active tcp, but you already made good progress.

@dgunay
Copy link
Author

dgunay commented Oct 23, 2021

Hi @dgunay, if you need helping hands with this, let me know.

I was also planning to work on active tcp, but you already made good progress.

Sure! I will not be super active until I finish my masters in December, so if you'd like to take this and build on it feel free.

@ashellunts
Copy link
Contributor

Great, I will.

@dgunay
Copy link
Author

dgunay commented Sep 3, 2022

Voluntarily closing this in favor of #394 - hopefully someone else with the desire or the need will help get it across the finish line.

@dgunay dgunay closed this Sep 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants