Skip to content

Conversation

@Sean-Der
Copy link
Contributor

@Sean-Der Sean-Der commented Jan 22, 2025

TODO

  • Convert length checks to assert
  • Simplify agent<->pfd lookup
  • conn_poll_connect and a callback to notify Agent that candidate is ready
  • Generate active TCP candidate when enabled
  • Ignore remote active TCP candidates

@Sean-Der Sean-Der force-pushed the ice-tcp branch 2 times, most recently from 2e10e4d to eba7075 Compare January 22, 2025 03:23
@Sean-Der

This comment was marked as outdated.

@Sean-Der Sean-Der closed this Jan 22, 2025
@Sean-Der Sean-Der reopened this Jan 22, 2025
@paullouisageneau
Copy link
Owner

paullouisageneau commented Feb 4, 2025

OK, this is complicated because the whole library is designed with the assumption that candidates can only be UDP. It would be way simpler to implement TURN TCP.

I think you can't realistically implement something on the side running in its own thread. It seems to me the best way to implement this is to add optional TCP sockets for each agent to the thread and poll connectivity backends (I don't think mux mode can have TCP candidates as you would have to create new sockets).

TCP sockets would be polled and read by the backend. A lot of functions assume addresses are UDP only, like conn_get_addrs() and agent_conn_recv(), you will probably need to introduce a socket type in addr_record_t to distinguish between UDP and TCP.

You would then need to modify the logic to match candidates pairs properly. You would probably need to introduce a function to order the backend to actively connect to a remote TCP address.

Again, this is complicated and requires fundamental changes, contrary to TURN TCP (as candidates are still UDP in that case).

@Sean-Der

This comment was marked as outdated.

@Sean-Der Sean-Der force-pushed the ice-tcp branch 2 times, most recently from 0e78225 to 759663e Compare March 1, 2025 03:44
@Sean-Der Sean-Der force-pushed the ice-tcp branch 2 times, most recently from 758413a to 4b5850a Compare March 11, 2025 20:44
@Sean-Der

This comment was marked as outdated.

@Sean-Der Sean-Der marked this pull request as ready for review March 29, 2025 03:11
@Sean-Der Sean-Der force-pushed the ice-tcp branch 11 times, most recently from f9e1a3e to e92dcd4 Compare April 2, 2025 02:37
@Sean-Der Sean-Der force-pushed the ice-tcp branch 10 times, most recently from c92e67b to 60aa54f Compare May 15, 2025 15:21
@Sean-Der Sean-Der requested a review from paullouisageneau May 15, 2025 15:30
@Sean-Der Sean-Der force-pushed the ice-tcp branch 4 times, most recently from 089a0d8 to 8a89802 Compare June 14, 2025 03:12

for (int i = 0; i < agent->candidate_pairs_count; ++i) {
ice_candidate_pair_t *pair = &agent->candidate_pairs[i];
if (pair->remote->transport != ICE_CANDIDATE_TRANSPORT_UDP) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you could check if pair->remote->resolved.addr.ss_family == remote->resolved.addr.ss_family to allow one IPv4 and one IPv6 TCP candidate.

return;
}

} else if (conn_impl->next_timestamp <= current_timestamp()) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to handle POLLHUP indicating connection closed and you should also treat POLLERR like connection reset. (They can be set at the same time as POLLIN so it's not an else)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic changed to the following. POLLHUP and POLLERR are now fatal.

      if (pfd->revents & POLLHUP || pfd->revents & POLLERR) {                                                                                                                                                                                        
          agent_conn_fail(agent);                                                                                                                                                                                                                    
          conn_impl->state = CONN_STATE_FINISHED;                                                                                                                                                                                                    
          return;                                                                                                                                                                                                                                    
      }

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with this logic is that when receiving POLLHUP you discard any data sitting in the buffer. While not a big problem in this context, it should be avoided. I'll fix it after merging.

@Sean-Der Sean-Der force-pushed the ice-tcp branch 4 times, most recently from 0509e6a to 0f9ee22 Compare June 20, 2025 16:03
Co-authored-by: Paul-Louis Ageneau <[email protected]>
Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, it looks good. There are a couple things I'll fix later but they don't seem blocking to me. Thank you for the work on this!

@paullouisageneau paullouisageneau merged commit f007d8c into paullouisageneau:master Jul 1, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants