jitsi / jitsi-xmpp-extensions

Common library holding all jitsi specific smack xmpp extensions.
Apache License 2.0
15 stars 51 forks source link

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

Open jbg opened 2 years ago

jbg commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

jbg commented 2 years ago

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 commented 2 years ago

Nice!

jbg commented 2 years ago

Opened https://github.com/jitsi/jicofo/pull/961 for the Jicofo part.

damencho commented 2 years ago

What about jigasi?

jbg commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 1 year ago

@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 commented 4 months ago

Dear all,

Have you progressed on this problem?