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

xmlns of <parameter> inside <source> is wrong #81

Open
jbg opened this issue Sep 6, 2022 · 12 comments
Open

xmlns of <parameter> inside <source> is wrong #81

jbg opened this issue Sep 6, 2022 · 12 comments

Comments

@jbg
Copy link

jbg commented Sep 6, 2022

The <source> element is defined in XEP-0339 and both it and the contained <parameter> are defined in the Jingle SSMA namespace (urn:xmpp:jingle:apps:rtp:ssma:0).

However, Jicofo produces a session-initiate with <source>s containing <parameter>s in the Jingle RTP namespace (urn:xmpp:jingle:apps:rtp:1) instead:

<source xmlns='urn:xmpp:jingle:apps:rtp:ssma:0' ssrc='1351679567' name='jvb-v0'>
  <ssrc-info owner='jvb' xmlns='http://jitsi.org/jitmeet' />
  <parameter xmlns='urn:xmpp:jingle:apps:rtp:1' value='mixedmslabel mixedlabelvideo0' name='msid' />
</source>

Relevant code is here.

Note that Jingle RTP (XEP-0167) does define a <parameter> element too, but despite having the same name it's a different element due to being in a different namespace.

This causes interop problems with other software supporting Jingle, notably xmpp-parsers which gst-meet uses.

@jbg
Copy link
Author

jbg commented Sep 6, 2022

I think this should resolve it: #82

Jicofo will need updating too: https://github.com/jitsi/jicofo/blob/master/src/main/kotlin/org/jitsi/jicofo/conference/source/Source.kt#L44

@saghul
Copy link
Member

saghul commented Sep 6, 2022

Good catch! Have you checked if lib-jitsi-meet checks the NS too?

@jbg
Copy link
Author

jbg commented Sep 6, 2022

It doesn't seem to care about the namespace: https://github.com/jitsi/lib-jitsi-meet/blob/e1ca2f5a7b74ca2443f6cb4948f5e53b78aee1c6/modules/sdp/SDP.js#L759

It also produces <parameter>s in the correct namespace in the other direction.

@saghul
Copy link
Member

saghul commented Sep 6, 2022

Nice!

@jbg
Copy link
Author

jbg commented Sep 6, 2022

Opened jitsi/jicofo#961 for the Jicofo part.

jbg added a commit to avstack/gst-meet that referenced this issue Sep 6, 2022
@damencho
Copy link
Member

damencho commented Sep 6, 2022

What about jigasi?

@jbg
Copy link
Author

jbg commented Sep 10, 2022

I can't find any use of ParameterPacketExtension or SourcePacketExtension in Jigasi, even though it seems like it should be used?

It's used in JVB: https://github.com/jitsi/jitsi-videobridge/blob/e8f546396f37813cf42d4b4e28e39dd340d7caeb/jvb/src/main/kotlin/org/jitsi/videobridge/colibri2/Colibri2ConferenceHandler.kt#L124

I'll prepare a PR for JVB. This would require JVBs to be updated together though, looks like it's used for relay sources. Maybe it's better to somehow make the parsing side lenient so that ParameterPacketExtension can accept both namespaces, at least temporarily? (Though that would require JVBs to first be updated to have that leniency, and then after a delay get the namespace fix...)

@bgrozev
Copy link
Member

bgrozev commented Sep 13, 2022

I'm not sure we need "parameter" in colibri at all. I'm still trying to verify, but don't waste time on a more complex workaround.

@bgrozev
Copy link
Member

bgrozev commented Sep 13, 2022

@jbg actually, why do you need to parse these parameters? As far as I can see "parameter" inside "source" is only used for "msid", and they are not really necessary. Currently our client uses them to set the msid of the associated track, but in the new "ssrc rewriting" mode it just picks "remote-video-1", "remote-video-2", etc.

I can confirm that the use of "parameter" in colibri2 can be removed, so you don't need to worry about the jvb/jicofo compatibility.

@jbg
Copy link
Author

jbg commented Sep 16, 2022

@bgrozev I don't really need the value of the parameter, but the incorrect namespace causes the whole Jingle stanza to fail to parse, forcing us to use a fork of the XMPP library.

@bgrozev
Copy link
Member

bgrozev commented Oct 25, 2022

@jbj, we merged the removal of "parameter" use in colibri2. We can accept your changes if you just update them to not touch colibri (and rebase on top of jitsi/jicofo#997)

@Neustradamus
Copy link

Dear all,

Have you progressed on this problem?

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

5 participants