-
Notifications
You must be signed in to change notification settings - Fork 262
Fixes for Streamers changing IDs when connecting & Updates to SFU behaviour relating to multi streamer changes. #411
Fixes for Streamers changing IDs when connecting & Updates to SFU behaviour relating to multi streamer changes. #411
Conversation
…election option. Fixing browser behaviour when multiple streamers detected (previous failed tests).
Fixes #350 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments and questions
Given there is some changes to fairly load bearing code I am going to ask @Belchy06 to review also. And once reviews have been done I'm gonna ask @MWillWallT to give this a QA pass:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more questions and spotted a doc change that needs to go in too.
…pdating docs to remove PreferSFU (SFU is just selected as a streamer now).
@mcottontensor Michael has given it the green light for QA, I have approved. You may want to consider which branches benefit from the backport using the autobackport tags. |
💔 Some backports could not be created
Note: Successful backport PRs will be merged automatically after passing CI. Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
[UE5.3] Merge pull request #411 from mcottontensor/streamer_ids_fix
[UE5.4] Merge pull request #411 from mcottontensor/streamer_ids_fix
Relevant components:
Problem statement:
Streamer IDs changing:
Streamers need to identify themselves so when they first connect they are assigned a temporary ID. If a player attempts to connect while this temporary name is assigned it can lead to stuck behaviour.
SFUs with multi streamer behaviour:
When the multi streamer change went in the SFU behaviour was kind of hacked into the flow. Only one SFU could be connected at all and it was assumed that there was always only one streamer.
Frontend build scripts smashing windows PATH:
When running the frontend build script, depending on the environment setup you can end up with an incorrect PATH variable. This is due to the script trying to set PATH using the registry variables, but if you manually add or modify PATH in your session before running the script, you will lose them.
Solution
Streamer IDs changing:
Now when streamers connect, they are not "published" in the streamer list until they ID themselves. If they do not ID themselves within a time, they are assigned an ID. This prevents peers connecting to streamers during their ID negotiation.
SFUs with multi streamer behaviour:
There are several new configuration options with the SFU. The SFU ID, which is used to identify the SFU as a streamer on the signalling server. This ID will show up in the streamers list when the SFU is streaming. And the subscribe streamer ID which tells the SFU what streamer ID we are looking to forward. If it cannot find the specified streamer it will request the streamer list on a timeout that is specified in the config until it finds the streamer it's looking for.
Additionally the SFU can now start/stop streaming which will control when the SFU shows up in the streamer list on the signalling server. The SFU will start streaming when it connects to the streamer it's configured to forward, and stop streaming when it loses this connection.
Frontend build scripts smashing windows PATH:
Changed the setting via registry to a simple set OLDPATH and then set PATH to that temp variable.
Documentation
Documentation has been added to the SFU configuration file. Documentation for the streamer ID fix is not needed.
Test Plan and Compatibility
I have manually run through various scenarios of connecting and disconnecting different components in different orders and switching between SFU and DefaultStreamers. We will need to do a usual QA pass over this change.