[0.x] janus: avoid RTX payload type collisions

[tmatth]

Previously only collsions with other video payload types was being avoided, this checks additionally for audio and application m-lines.

[tmatth]

For context, before this patch when pulling with pion reading from a streaming plugin mountpoint, I was seeing:

v=0
o=- 1663885678684222 1 IN IP4 192.168.0.91
s=Mountpoint ff5151f5-417a-4454-b794-274cbab82329
t=0 0
a=group:BUNDLE a v
a=ice-options:trickle
a=fingerprint:sha-256 F9:D4:C8:68:EE:6E:2E:94:FF:E2:0E:93:06:C0:A7:DF:95:0D:58:1A:B0:03:AB:97:EC:6D:D5:88:10:98:BD:2D
a=extmap-allow-mixed
a=msid-semantic: WMS janus
m=audio 9 UDP/TLS/RTP/SAVPF 97
c=IN IP4 192.168.0.91
a=sendonly
a=mid:a
a=rtcp-mux
a=ice-ufrag:BUh/
a=ice-pwd:mV5RlSeKTeLZZn+V1Ikxmb
a=ice-options:trickle
a=setup:actpass
a=rtpmap:97 opus/48000/2     <-------------------- ruh.....
a=rtcp-fb:97 transport-cc
a=extmap:4 urn:ietf:params:rtp-hdrext:sdes:mid
a=fmtp:97 sprop-stereo=1
a=msid:janus janusa
a=ssrc:3275894722 cname:janus
a=candidate:1 1 udp 2015363327 192.168.0.91 20554 typ host
a=end-of-candidates
m=video 9 UDP/TLS/RTP/SAVPF 96 97
c=IN IP4 192.168.0.91
a=sendonly
a=mid:v
a=rtcp-mux
a=ice-ufrag:BUh/
a=ice-pwd:mV5RlSeKTeLZZn+V1Ikxmb
a=ice-options:trickle
a=setup:actpass
a=rtpmap:96 H264/90000
a=rtcp-fb:96 ccm fir
a=rtcp-fb:96 nack
a=rtcp-fb:96 nack pli
a=rtcp-fb:96 goog-remb
a=rtcp-fb:96 transport-cc
a=extmap:2 http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time
a=extmap:4 urn:ietf:params:rtp-hdrext:sdes:mid
a=fmtp:96 packetization-mode=1; sprop-parameter-sets=; profile-level-id=42E01F
a=rtpmap:97 rtx/90000               <----------------  ...roh......
a=fmtp:97 apt=96
a=ssrc-group:FID 1718034908 3177769222
a=msid:janus janusv
a=ssrc:1718034908 cname:janus
a=ssrc:3177769222 cname:janus
a=candidate:1 1 udp 2015363327 192.168.0.91 20554 typ host
a=end-of-candidates

and with this patch, I get:

v=0
o=- 1664213651629192 1 IN IP4 192.168.0.91
s=Mountpoint 5304c7c4-85ed-4e4f-b2f4-32d4672bf484
t=0 0
a=group:BUNDLE audio video
a=extmap-allow-mixed
a=msid-semantic: WMS janus
m=audio 9 UDP/TLS/RTP/SAVPF 97
c=IN IP4 192.168.0.91
a=sendonly
a=mid:audio
a=rtcp-mux
a=ice-ufrag:Pvur
a=ice-pwd:bH/y+I92iGWdW3AdS0eFye
a=ice-options:trickle
a=fingerprint:sha-256 5F:09:96:84:65:3F:CF:2D:2B:A4:62:E0:B6:21:DB:B0:AC:2F:8C:8D:0A:AD:CF:8D:7B:E1:88:CD:28:9B:E1:37
a=setup:actpass
a=rtpmap:97 opus/48000/2               <------------- ok
a=fmtp:97 sprop-stereo=1
a=extmap:1 urn:ietf:params:rtp-hdrext:sdes:mid
a=msid:janus janusa0
a=ssrc:1845645613 cname:janus
a=ssrc:1845645613 msid:janus janusa0
a=ssrc:1845645613 mslabel:janus
a=ssrc:1845645613 label:janusa0
a=candidate:1 1 udp 2015363327 192.168.0.91 24972 typ host
a=end-of-candidates
m=video 9 UDP/TLS/RTP/SAVPF 96 98
c=IN IP4 192.168.0.91
a=sendonly
a=mid:video
a=rtcp-mux
a=ice-ufrag:Pvur
a=ice-pwd:bH/y+I92iGWdW3AdS0eFye
a=ice-options:trickle
a=fingerprint:sha-256 5F:09:96:84:65:3F:CF:2D:2B:A4:62:E0:B6:21:DB:B0:AC:2F:8C:8D:0A:AD:CF:8D:7B:E1:88:CD:28:9B:E1:37
a=setup:actpass
a=rtpmap:96 H264/90000
a=fmtp:96 packetization-mode=1; sprop-parameter-sets=Z0LAKtmAUAW7AWoCAgKAAAADAIAAADxHjBk0,aMljLIA=; profile-level-id=42C02A
a=rtcp-fb:96 nack
a=rtcp-fb:96 nack pli
a=rtcp-fb:96 goog-remb
a=extmap:1 urn:ietf:params:rtp-hdrext:sdes:mid
a=extmap:2 http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time
a=rtpmap:98 rtx/90000          <-------------- phew
a=fmtp:98 apt=96
a=ssrc-group:FID 451452291 3917668882
a=msid:janus janusv0
a=ssrc:451452291 cname:janus
a=ssrc:451452291 msid:janus janusv0
a=ssrc:451452291 mslabel:janus
a=ssrc:451452291 label:janusv0
a=ssrc:3917668882 cname:janus
a=ssrc:3917668882 msid:janus janusv0
a=ssrc:3917668882 mslabel:janus
a=ssrc:3917668882 label:janusv0
a=candidate:1 1 udp 2015363327 192.168.0.91 24972 typ host
a=end-of-candidates

Note that 1.x has an equivalent bug, but the context is a bit more complicated so I wanted to get feedback on this PR first.

[tmatth]

OK I can reproduce this with the following change to the conf/janus.plugin.streaming.jcfg example mountpoint (setting the opus payload type to 97, the default of 111 does not demonstrate this bug):

#
# This is an example of an RTP source stream, which is what you'll need
# in the vast majority of cases: here, the Streaming plugin will bind to
# some ports, and expect media to be sent by an external source (e.g.,
# FFmpeg or Gstreamer). This sample listens on 5002 for audio (Opus) and
# 5004 for video (VP8), which is what the sample gstreamer script in the
# plugins/streams folder sends to. Whatever is sent to those ports will
# be the source of a WebRTC broadcast users can subscribe to.
#
rtp-sample: {
    type = "rtp"
    id = 1
    description = "Opus/VP8 live stream coming from external source"
    metadata = "You can use this metadata section to put any info you want!"
    audio = true
    video = true
    audioport = 5002
    audiopt = 97
    audiortpmap = "opus/48000/2"
    videoport = 5004
    videopt = 96
    videortpmap = "VP8/90000"
    secret = "adminpwd"
}

Then we can see the borked SDP using https://github.com/pion/example-webrtc-applications/blob/master/janus-gateway/streaming/main.go with this patch:

diff --git a/janus-gateway/streaming/main.go b/janus-gateway/streaming/main.go
index 6a0e125..c97437b 100644
--- a/janus-gateway/streaming/main.go
+++ b/janus-gateway/streaming/main.go
@@ -94,6 +94,7 @@ func main() {
                        Type: webrtc.SDPTypeOffer,
                        SDP:  msg.Jsep["sdp"].(string),
                }
+               fmt.Println("GOT JSEP", offer.SDP)

                // Create a new RTCPeerConnection
                var peerConnection *webrtc.PeerConnection

which yields:

GOT JSEP v=0
o=- 1664231968142910 1 IN IP4 192.168.0.91
s=Mountpoint 1
t=0 0
a=group:BUNDLE audio video
a=extmap-allow-mixed
a=msid-semantic: WMS janus
m=audio 9 UDP/TLS/RTP/SAVPF 97
c=IN IP4 192.168.0.91
a=sendonly
a=mid:audio
a=rtcp-mux
a=ice-ufrag:LlUE
a=ice-pwd:WoEZI6hAGdeSB+IivHSZNF
a=ice-options:trickle
a=fingerprint:sha-256 8B:DF:49:98:E2:D2:23:6B:42:37:BC:09:26:08:8C:3D:07:B9:2E:49:9F:86:05:BD:01:5B:D5:58:C5:81:FB:66
a=setup:actpass
a=rtpmap:97 opus/48000/2
a=extmap:1 urn:ietf:params:rtp-hdrext:sdes:mid
a=msid:janus janusa0
a=ssrc:3980440877 cname:janus
a=ssrc:3980440877 msid:janus janusa0
a=ssrc:3980440877 mslabel:janus
a=ssrc:3980440877 label:janusa0
a=candidate:1 1 udp 2015363327 192.168.0.91 56486 typ host
a=candidate:2 1 udp 2015364351 172.17.0.1 44752 typ host
a=candidate:3 1 udp 2015363583 10.42.0.0 33735 typ host
a=end-of-candidates
m=video 9 UDP/TLS/RTP/SAVPF 96 97
c=IN IP4 192.168.0.91
a=sendonly
a=mid:video
a=rtcp-mux
a=ice-ufrag:LlUE
a=ice-pwd:WoEZI6hAGdeSB+IivHSZNF
a=ice-options:trickle
a=fingerprint:sha-256 8B:DF:49:98:E2:D2:23:6B:42:37:BC:09:26:08:8C:3D:07:B9:2E:49:9F:86:05:BD:01:5B:D5:58:C5:81:FB:66
a=setup:actpass
a=rtpmap:96 VP8/90000
a=rtcp-fb:96 nack
a=rtcp-fb:96 nack pli
a=rtcp-fb:96 goog-remb
a=extmap:1 urn:ietf:params:rtp-hdrext:sdes:mid
a=extmap:2 http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time
a=rtpmap:97 rtx/90000
a=fmtp:97 apt=96
a=ssrc-group:FID 1032543601 1674899379
a=msid:janus janusv0
a=ssrc:1032543601 cname:janus
a=ssrc:1032543601 msid:janus janusv0
a=ssrc:1032543601 mslabel:janus
a=ssrc:1032543601 label:janusv0
a=ssrc:1674899379 cname:janus
a=ssrc:1674899379 msid:janus janusv0
a=ssrc:1674899379 mslabel:janus
a=ssrc:1674899379 label:janusv0
a=candidate:1 1 udp 2015363327 192.168.0.91 56486 typ host
a=candidate:2 1 udp 2015364351 172.17.0.1 44752 typ host
a=candidate:3 1 udp 2015363583 10.42.0.0 33735 typ host
a=end-of-candidates

Connection State has changed checking 
Connection State has changed connected 

[tmatth]

Thanks for spotting this, and not sure why we didn't catch it before. It's likely 1.x is affected too, since we probably only check payload types per medium but possibly not across medium instances. I'll check and in case come up with a patch for that too.

Yeah I tested and 1.x is affected, but as mentioned it means checking all media which is a bit different than the fix here.

[lminiero]

1.x already has maps to keep track of what's used in a medium instance. I think we can get away with it by having a janus_ice_peerconnection map as well where we can keep track of allocations across all tracks, without needing to traverse all medium instances to see what's taken.

[lminiero]

I initially wanted to keep this open until I could prepare a PR for multistream as well, but that one is taking longer than expected, ando so may take a while: as such, no reason to keep this stuck, so I'll merge :+1: I'll reference it from the new multistream PR when that one is ready.