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

Add JSEP flag to invert processing order of rid in SDP #2385

Merged
merged 1 commit into from
Oct 8, 2020
Merged

Conversation

lminiero
Copy link
Member

@lminiero lminiero commented Oct 1, 2020

I described in detail the way simulcast works in Janus in this blog post, last year, so I won't spend too many words on that. One thing worth mentioning, though, is that when rid is used, Janus expects a specific order: high substream first, then medium, and finally low. This means that the encoding object should be initialized like this:

parameters.encodings = [
	{ rid: "h", active: true, maxBitrate: 900000 },
	{ rid: "m", active: true, maxBitrate: 300000, scaleResolutionDownBy: 2 },
	{ rid: "l", active: true, maxBitrate: 100000, scaleResolutionDownBy: 4 }
];

which would result in an SDP containing something like this:

a=rid:h send
a=rid:m send
a=rid:l send
a=simulcast: send rid=h;m;l

This assumption on the order is required, because otherwise Janus has no clue on what will constitute a high quality substream, and what would be something else: the name of the rid values are of course useless for that, as they're user-provided and so can be anything. As such, we simply assume that the order will always be high-medium-low, and expect that in the SDP we receive. That said, I've been made aware that there are some implementations that fail to initialize when that's the enforced order, and only work when doing it the other way around, that is low-medium-high. Of course, in that case Janus messes up the mappings, because it ends up mapping the SSRC of what actually is a low quality stream to what it thinks is the high quality one instead, and viceversa, which breaks the automated switches too.

As such, I've come up with this simple PR, which simply just adds a way for you to specify whether the order will be high-medium-low (the default) or low-medium-high, so that Janus can do the mapping properly. Specifically, this can be specified in a new property you can set in the jsep object when sending the offer, called rid_order: the only allowed values are "hml" (which is the default if omitted) and "lmh". Notice that these strings have nothing to do with how you actually called your rid identifiers: "hml" is simply a compact way of saying High-Medium-Low, and "lmh" does the other way around. Trying to pass strings that differ from those patterns will simply result in Janus ignoring them, and so assuming the default "hml" value. I'm not going to add any mapping more complex than that, so don't ask.

I tested this by messing with janus.js to invert the order of encodings, and passing the proper rid_order property, and it seems to be working as expected. I only tested with the EchoTest, though. Feedback welcome.

if(jsep_rids != NULL) {
const char *jsep_rids_value = json_string_value(jsep_rids);
if(jsep_rids_value != NULL) {
if(!strcasecmp(jsep_rids_value, "hml")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: since you've already initialized it to TRUE, this branch has no effect and could be dropped.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's actually there to enforce validation of the field, since we have a warning displayed when the value is unsupported/unknown.

@lminiero
Copy link
Member Author

lminiero commented Oct 8, 2020

Merging.

@lminiero lminiero merged commit c07a727 into master Oct 8, 2020
@lminiero lminiero deleted the rid-order branch October 8, 2020 16:10
PauKerr pushed a commit to caffeinetv/janus-gateway that referenced this pull request Nov 11, 2020
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.

2 participants