-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Auto switch simulcast layer if REMB is too low #2735
Closed
sjkummer
wants to merge
4
commits into
meetecho:abs-send-time
from
sjkummer:auto-switch-simulcast-layer
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
562ba78
POC: automatically switch to lowest simulcast layer after receiving R…
sjkummer e2302e8
introduce min_rec_bitrate_simulcast setting in videoroom for simulcas…
sjkummer 4166d2f
typos
sjkummer 192dd4f
default simulcast_min_bitrate to 256000
sjkummer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
don't you need two values here, one as the minimum bitrate for the low spatial layer and a second one for the mid one?
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.
I also see at least two but i would say all three values might even make more sense.
If we do not reach the lowest bitrate janus could also drop the fps (but i would make that in a second task)
Having the bitrates on hand alows further improvement in the future.
e.g.
min_required_bitrate_vp8_sl_0: 150000
min_required_bitrate_vp8_sl_1: 500000
min_required_bitrate_vp8_sl_2: 1200000
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.
This sounds overly specific: I don't want anything codec specific in there. Besides, I don't think it's a good idea to have generic room settings here, especially if the idea is to match the simulcast targets of publishers, who may be configured differently even when in the same room.
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.
The question is how do you want janus to select the proper layer on the subscriber side if the plugin handle is not aware about the publisher configured bitrates for the layers? You don´t have them in janus as the publisher client has set them on the client side. (I also don´t see them in the general settings, i see them in an api call on the subscriber side)
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.
What sounds more interesting is tracking the bitrate for the different substreams for all publishers, and use those as triggers, which would make it more dynamic as well (even though more complex, as we'd have to make sure substreams that are just starting don't confuse the logic, e.g., because the high substream temporarily results with a lower bitrate than the medium/low one).
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.
We never needed to know them. Having the publisher optionally pass the settings they used via API sounds viable too, even though not necessarily reliable, since those are targets and not the actual bitrate in use.
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.
For the publisher side yeah, currently the browser is handling the layer sending according to available bitrates so no need to know them inside janus. For the subscriber side i think its the easist to start with an api that allows to set them dynamic and see if it works in real live (but yes you are totally right - having them in sync with the publisher side would make the most sense but is from the current perspective way more effort)
We will sync tomorrow @fippo - @sjkummer and me (you are always invited :D)