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

Firefox throws an exception if you pass a max-bundle sdp to janus and use the answer as provided by janus #2390

Closed
JanFellner opened this issue Oct 12, 2020 · 8 comments

Comments

@JanFellner
Copy link
Contributor

Steps to reproduce:

use firefox (any current version, 82.0b9 (64-bit))
initialize a PC with bundlePolicy "max-bundle" (sdpSemantics "unified-plan")
pass audio and video to the peercon.
create an offer (attached)
pass the offer to janus through a configure command
put the answer into the pc using setRemoteDescription

ff_max_bundle.txt
janusTransport.log

@fippo
Copy link
Contributor

fippo commented Oct 12, 2020

@lminiero I looked at this and it seems that Janus misinterprets Firefox bundle-only in the audio m-line, invents a mid "0" for the bundle group and then puts mid:(null) into the SDP.

The obvious workaround is to not use max-bundle yet but that might become more relevant with unified plan
Now whether Firefox is wise to set bundle-only on the second m-line already... (I am looking at you @nils-ohlmeier)

@nils-ohlmeier
Copy link

How about trying to add audio and video in the order of "audio first, followed by video"? My guess is the Janus might handle that "standard" scenario a little better.

Without looking it up I'm pretty sure that "mid:(null)" is not valid SDP. So Firefox is correct to complain about that.

@fippo if I remember it correctly with max-bundle the browser is required to request as much bundling as possible, which only works if everything besides the initial m-section gets bundled. So I think Firefox behavior is correct here.

@lminiero
Copy link
Member

The order of media is irrelevant, Janus accepts it any way. The issue is that the port in the m-line is 0, which in Janus SDP parsing today means the m-line is disabled.

@fippo
Copy link
Contributor

fippo commented Oct 13, 2020

@nils-ohlmeier true, that is what JSEP says. Even though I would interpret "Only the first m= section will contain transport parameters; all streams other than the first will be marked as bundle-only" on media streams, i.e. audio+video, not rtp streams.

@lminiero
Copy link
Member

lminiero commented Oct 13, 2020

The issue is that the port in the m-line is 0, which in Janus SDP parsing today means the m-line is disabled.

More precisely, we probably stop parsing the m-line earlier because of the port 0, and so don't get to the mid. This might explain why it's set to (null) in the attribute, and why the bundle group looks like this in the answer:

a=group:BUNDLE 0 audio

(audio is the default mid when we generate them ourselves). I'll work on a fix for that, but the end result will still be audio disabled even if the connection succeeds, because of the port being 0.

@lminiero
Copy link
Member

@nils-ohlmeier I can confirm that the cause of the exception is the wrong mid in the group:bundle line, not the (null). Ensuring we only add there the mid values of accepted m-lines got Firefox to work, only for the first m-line though, since as expected Janus is treating the port 0 as a "disabled". I'll work on make it "smarter" and look for bundle-only too, in order to fix the management of max-bundles too.

@lminiero
Copy link
Member

The commit above should fix it (it seems to work fine for me now). Thanks for the heads up on the issue!

@nils-ohlmeier
Copy link

Yeah unfortunately the IETF decided AFAIK that implementations which are aware of bundle need to handle port 0 differently from the old style non bundle implementations. I'm not a big fan o that either, but if one reads through the mailing list discussions about bundle I'm sure there is some sensible reasoning for that (assuming one finds bundling reasonable ;-) ).

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

No branches or pull requests

4 participants