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.

Sign up for GitHub

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

Jump to bottom

support multi ips #211

Closed
wants to merge 1 commit into from
Closed

support multi ips #211

wants to merge 1 commit into from

Conversation

yangjinecho
Copy link
Contributor

No description provided.

ibc
ibc reviewed Jul 19, 2018
View reviewed changes
worker/include/RTC/TcpServer.hpp

private:
static struct sockaddr_storage sockaddrStorageIPv4;
static struct sockaddr_storage sockaddrStorageIPv6;
static std::map<std::string, struct sockaddr_storage> sockaddrStorageMultiIPv4s;
static std::map<std::string, struct sockaddr_storage> sockaddrStorageMultiIPv6s;
static uint16_t minPort;
static uint16_t maxPort;
static std::unordered_map<uint16_t, bool> availableIPv4Ports;
static std::unordered_map<uint16_t, bool> availableIPv6Ports;
Copy link
Member

Choose a reason for hiding this comment

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

Both availableIPv4Ports and availableIPv6Ports are no longer required, right? (same in the .hpp file).

@ibc
Copy link
Member

ibc commented Jul 19, 2018 •
edited
Loading

Wow! this is an awesome feature and PR!

We'll check it as far as we can. Just a few comments:

  • Could you describe here the changes in the JavaScript side? I mean: which new fields should be given to use a custom IPv4 or IPv6?
  • Also, I like this multi-IP feature and I think that the existing API should change. Instead of providing a default IPv4 and IPv6 in the Server settings, we could just require passing a ipv4 and/or ipv6 in the createTransport() JavaScript API.
    • This would require some changes to JavaScript layer.
  • We need to do something with the rtcAnnouncedIPv4/6 values, since now they may also be different. But this may be part of a new API in which, when createTransport() is called, ipv4 and/or ipv6 and also announcedIpv4 and/or announcedIpv6 are given.
  • What about the PlainRtpTransport.cpp? It should also be able to use a custom IP.

Thanks!

@yangjinecho
Copy link
Contributor Author

yangjinecho commented Jul 20, 2018 •
edited
Loading

Will you modify JavaScript api to adapt multi-IP feature？
We can delete ipv4 and IPv6 in the Server settings.
I have no idea about announcedIpv4 and/or announcedIpv6.

@jmillan
Copy link
Member

jmillan commented Jul 20, 2018 •
edited
Loading

Very nice work @yangjinecho!

@ibc
Copy link
Member

ibc commented Jul 20, 2018

@yangjinecho yes, we can do the JS part (we must think about how to do it). We'll check this PR on next week and update here. Thanks!

@ibc
Copy link
Member

ibc commented Aug 17, 2018

Hi @yangjinecho, it will take us longer than expected before we can work on this PR. Will notify here when we start working on it.

@ibc
Copy link
Member

ibc commented Nov 21, 2018

Hi. Thanks for this PR. However we are gonna delay this feature for v3, as described it the v3 roadmap: #227

@ibc ibc closed this Nov 21, 2018
@yangjinecho yangjinecho deleted the yj branch December 19, 2018 03:15
@ibc
Copy link
Member

ibc commented Jan 5, 2019

Hi @yangjinecho, just wanted to let you know that mediasoup v3 (work in progress) no longer accepts RTC ipv4/ipv6 within the Server settings but, instead, requires passing the desired IP (or IPs) when creating each Transport. For that, I've created a PortManager class that manages all the IPs and ports:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibc ibc ibc left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yangjinecho @ibc @jmillan