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

IPv6 with IPv4 fast fallback (RFC 6555) for RTMP #9120

Merged
merged 4 commits into from Jul 25, 2023

Conversation

jgh-twitch
Copy link
Contributor

@jgh-twitch jgh-twitch commented Jun 21, 2023

Description

This PR changes the RTMP connection behavior from preferring IPv4 when it is available to attempting the user's operating system's preference first followed by the other address family using the algorithm described in RFC 6555. In practice, unless the user has changed the OS configuration, this means that IPv6 will be attempted first. After the initial connection attempt the algorithm will wait 200ms before attempting an address from the other family (usually IPv4). The first TCP connection that is established is the winner and all other attempts are closed.

I have broken this PR up into three commits for review:

  • b4b5f27 Adds the happy eyeballs algorithm itself
  • 2be2ee7 Adopts the happy eyeballs component into librtmp by replacing the existing connection logic.
  • 9805459 Adds an option in Settings -> Advanced for the user to change the behavior of this algorithm (IPv4+IPv6, IPv4-Only, IPv6-Only)

Why not RFC 8305?

After reviewing RFC 8305 it seems that the simple algorithm described is compatible with RFC 6555 and the additional recommendations are optimizations. For the sake of keeping things simple for now I decided to stick with RFC 6555 and if in the future further optimizations are warranted RFC 8305 can be adopted.

Motivation and Context

The motivation behind this PR is that in many international markets ISPs are increasingly IPv6-Native. Using IPv4-only devices or accessing IPv4-only services requires going through additional translation layers. These translation layers such as 464XLAT or NAT64 can contribute to higher round trip times, packet loss, and ingest starvation. Dual-stack users will therefore benefit from preferring IPv6 in order to avoid these translation layers where possible.

How Has This Been Tested?

I have tested these changes on Windows 10 (Dell XPS laptop), MacOS 13.4 (MBP 16" Core i9), Ubuntu Jammy (Dell XPS Laptop+Virtualbox). Tests were run on T-Mobile's IPv6-native network with 464XLAT CGNAT and Google Fiber.

I tested various combinations of IPv4 and IPv6 availability as well as IPv4-only networks. Additionally I was able to test a broken CGNAT that was breaking the TCP handshake on IPv6, resulting in successful fallback to IPv4.

I don't think I will be able to test all of the creative ways that ISPs can mess up their IPv6 deployments, but it seems to work as expected in the scenarios I was able to test. In cases where it does not work users will have the ability to change their settings to use IPv4-only as before.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@tt2468
Copy link
Member

tt2468 commented Jun 21, 2023

Heads up, the happy-eyeballs dep seems to be missing code license headers. cc @RytoEX for if we should use any specific license for it since it's a dep.

@tt2468 tt2468 added Enhancement Improvement to existing functionality Seeking Testers Build artifacts on CI labels Jun 23, 2023
Copy link
Member

@tt2468 tt2468 left a comment

Choose a reason for hiding this comment

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

Tested successfully on Ubuntu 20.04 with an OPNSense firewall configured with an HE.net IPv6 tunnel. Confirmed that all modes work as expected, with traffic to YouTube ingest using V4/V6 where forced. Only issue to note is that since I'm using a tunnel, it appears that the added latency is causing the hybrid mode to choose IPv4 most of the time. That's probably "working as intended" though, as being IPv4 native means IPv4 is going to be more reliable anyway.

Did a quick look over the code and it seems fine. The method of implementation also looks good.

Only thing I'd be worried about is the potential for someone to force IPv6 without understanding what it does, have it not work, and be left wondering what's wrong. Currently, it just says Failed to connect to server. This should perhaps note that IPv6-only mode is enabled, if it's enabled, and suggest turning it back to hybrid.

EDIT: Same possible situation for IPv4-only

plugins/obs-outputs/CMakeLists.txt Outdated Show resolved Hide resolved
@norihiro
Copy link
Contributor

I have some nitpick comments.

  • The commit title for 016d4fe should start from UI: instead of obs-ui: .
  • The period should be removed from the title of 635f9c7.
  • When changing files under deps, majority of the title is deps/happy-eyeballs: instead of just happy-eyeballs: but I think either is ok.

@tt2468
Copy link
Member

tt2468 commented Jul 17, 2023

Small optional ask: Log somewhere which IP version was chosen after successful connect, for diagnostic reasons (for example, if IPv6 was chosen, and the user is having network stutters, the user may be asked to revert to IPv4 to rule out sub-par V6 routing.) I believe there are existing log messages which this info may be added to appropriately.

@jgh-twitch jgh-twitch force-pushed the happy-eyeballs-rfc-6555 branch 5 times, most recently from 229ca34 to d9c9827 Compare July 21, 2023 01:10
@jgh-twitch
Copy link
Contributor Author

jgh-twitch commented Jul 21, 2023

Did some general cleanup. I have left 8c51b51 for review.

@jgh-twitch jgh-twitch force-pushed the happy-eyeballs-rfc-6555 branch 2 times, most recently from 2b195e3 to 8c51b51 Compare July 21, 2023 02:25
@tytan652
Copy link
Collaborator

tytan652 commented Jul 23, 2023

The UI actually relies on the RTMP output properties function to get IPs.

So I think, the RTMP output properties must implement it too even if we actually don't completely rely on it.

https://github.com/obsproject/obs-studio/blob/master/plugins/obs-outputs/rtmp-stream.c#L1713

@jgh-twitch
Copy link
Contributor Author

The UI actually relies on the RTMP output properties function to get IPs.

So I think, the RTMP output properties must implement it too even if we actually don't completely rely on it.

https://github.com/obsproject/obs-studio/blob/master/plugins/obs-outputs/rtmp-stream.c#L1713

Thanks for the feedback. Just to make sure I understand what you're saying before I make any changes: You would like the values in the IP Address Family dropdown list to be populated from rtmp-stream.c?

@tytan652
Copy link
Collaborator

tytan652 commented Jul 24, 2023

Thanks for the feedback. Just to make sure I understand what you're saying before I make any changes: You would like the values in the IP Address Family dropdown list to be populated from rtmp-stream.c?

Not exactly, the properties system is how output/sources/encoders/services normally expose their "UI".

Actually we use a custom UI for RTMP Network Settings, but the ability to select the IP Address family should be added to rtmp_stream_properties too. It does not need to be populated from.

@Lain-B
Copy link
Collaborator

Lain-B commented Jul 24, 2023

I have some minor readability changes I want to make anyway and I'm going to squash some of it, so I'll fix that up as well. I'll take the rest from here to get it merged.

@jgh-
Copy link
Contributor

jgh- commented Jul 24, 2023

Thank you Lain!

This commit adds a utility to connect to a TCP endpoint that may
be dual-stack IPv4/IPv6 using fast fallback. It will attempt the
prefered address family first, followed by the other 200ms later.
The first to connect will be the socket that is handed back
to the caller.
@Lain-B Lain-B force-pushed the happy-eyeballs-rfc-6555 branch 2 times, most recently from 914e301 to 80f447f Compare July 24, 2023 23:39
This commit adopts the happy eyeballs utility in the RTMP
output module, replacing the existing TCP connection logic.
This commit adds a field in Settings -> Advanced called
'IP Address Family' that allows users to select IPv4 and
IPv6, IPv4 Only, or IPv6 Only.
@Lain-B Lain-B merged commit 4b1ba4e into obsproject:master Jul 25, 2023
14 checks passed
@RytoEX RytoEX added this to the OBS Studio (Next Release) milestone Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvement to existing functionality Seeking Testers Build artifacts on CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants