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

Adding back player limits to streamers in the Signalling Server #427

Merged
merged 8 commits into from
Feb 5, 2025

Conversation

mcottontensor
Copy link
Collaborator

Relevant components:

  • Signalling server
  • Common library
  • Frontend library
  • Frontend UI library
  • Platform scripts
  • SFU

Problem statement:

#270
The player limits on the signalling server side were removed when multi streamers came in. We would like to bring back that functionality.

Solution

This change adds --max_players to the signalling server arguments. This will limit the number of players that can connect to any streamer registered with the signalling server. For eg. with a limit of 10, any streamer connected to the signalling server will be limited to 10 players at a time. This is per streamer, not per signalling server.
The frontend has also been updated to properly signal to the user when a subscription fails like when the player limit is hit.

Documentation

Updated the usage text of the signalling server to show the new argument.
Updated the Common library documentation to document the new signalling message and usage.

@@ -10,7 +10,7 @@
"start": "node ./dist/index.js --serve --console_messages verbose --log_config --https_redirect",
"lint": "eslint ./src --ext .js,.jsx,.ts,.tsx",
"test": "echo \"Error: no test specified\" && exit 1",
"watch": "nodemon -V -d 3 --watch src --watch ../Signalling/dist -e 'ts,js,mjs,cjs,json' --exec 'npm run build && node ./dist/index.js --serve --console_messages verbose --log_config --https_redirect --player_port 1025' --http_root www",
"watch": "nodemon -V -d 3 --watch src --watch ../Signalling/dist -e 'ts,js,mjs,cjs,json' --exec 'npm run build && node ./dist/index.js --serve --console_messages verbose --log_config --https_redirect --player_port 1025' --http_root www --max_players 1",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think don't set this during watch

Copy link
Contributor

@lukehb lukehb left a comment

Choose a reason for hiding this comment

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

Minor changes requested.

@lukehb
Copy link
Contributor

lukehb commented Feb 4, 2025

I am also thinking we should bump signalling protocol version and add a changelog to the top of that md we have the explains the protocol to mention what was modified in each signalling version. If you agree, we may as well start now?

@mcottontensor
Copy link
Collaborator Author

I am also thinking we should bump signalling protocol version and add a changelog to the top of that md we have the explains the protocol to mention what was modified in each signalling version. If you agree, we may as well start now?

MD we have? I've added a new changelog in the protocol folder but we cant really make manual modifications to the files under docs/ because it's all auto generated and cleaned out in the process of building the docs.

@lukehb
Copy link
Contributor

lukehb commented Feb 4, 2025

MD we have?

@mcottontensor https://github.com/EpicGamesExt/PixelStreamingInfrastructure/blob/master/Signalling/docs/Protocol.md

I've added a new changelog in the protocol folder but we cant really make manual modifications to the files under docs/ because it's all auto generated and cleaned out in the process of building the docs.

That works for me!

@mcottontensor
Copy link
Collaborator Author

I find it slightly frustrating that the signalling messages were moved into common but we have the signalling lib as its own package. Feels like things are very separated. I don't know the best solution though.

@mcottontensor mcottontensor merged commit 044742b into EpicGamesExt:master Feb 5, 2025
8 checks passed
@mcottontensor mcottontensor deleted the player_limits branch February 5, 2025 00:34
@myankov153
Copy link

myankov153 commented Feb 5, 2025

Hello there, I would like to ask you a question, when should I expect a new bumped version of @epicgames-ps/lib-pixelstreamingcommon-ue5.5, so I can use subscribeFailed event on my streaming page ?

Thanks in advance!

@mcottontensor
Copy link
Collaborator Author

💚 All backports created successfully

Status Branch Result
UE5.5

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@mcottontensor
Copy link
Collaborator Author

💚 All backports created successfully

Status Branch Result
UE5.5

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

mcottontensor added a commit that referenced this pull request Feb 9, 2025
[UE5.5] Merge pull request #427 from mcottontensor/player_limits
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.

3 participants