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

Avoid RTX payload type collisions in 1.x (see #3078) #3080

Merged
merged 2 commits into from
Oct 3, 2022

Conversation

lminiero
Copy link
Member

This PR aims at providing the same fix @tmatth provided in #3078 for 0.x, that is the fact Janus might generate payload types for RTX that conflict with existing payload types: the reason for this issue was that we only had checks for avoiding conflicts within the context of a specific m-line, without taking into account payload types used in other m-lines though. This patch tries to address that by adding additional maps to the PeerConnection object in the Janus core too, that are updated every time a (re)negotiation takes place to keep track of payload type allocations and mappings with RTX one, when needed.

It seems to be working fine in the few tests I made, and specifically with the mountpoint configuration @tmatth could replicate the problem with, but of course I haven't tested this extensively. As such, please do and provide feedback, so that we can catch potential regressions before we merge this.

@lminiero lminiero added the multistream Related to Janus 1.x label Sep 28, 2022
@tmatth
Copy link
Contributor

tmatth commented Sep 28, 2022

This looks good my testcase at least (rtx is now using payload type 98, vs. payload type 97 for opus and 96 for vp8):

tmatth@bellini:/big-repos/example-webrtc-applications/janus-gateway/streaming$ go run main.go 
GOT JSEP: 
v=0
o=- 1664367867708188 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:1Eir
a=ice-pwd:Rr+UiGQeS8ImfzPe1hAXr9
a=ice-options:trickle
a=fingerprint:sha-256 49:67:B1:03:3A:A5:CF:DC:65:85:04:E4:72:53:E2:16:22:7B:46:94:48:99:52:18:DD:4B:8E:57:14:C1:8E:32
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:610456597 cname:janus
a=ssrc:610456597 msid:janus janusa0
a=ssrc:610456597 mslabel:janus
a=ssrc:610456597 label:janusa0
a=candidate:1 1 udp 2015363327 192.168.0.91 58895 typ host
a=candidate:2 1 udp 2015364351 172.17.0.1 56026 typ host
a=candidate:3 1 udp 2015363583 10.42.0.0 50978 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:1Eir
a=ice-pwd:Rr+UiGQeS8ImfzPe1hAXr9
a=ice-options:trickle
a=fingerprint:sha-256 49:67:B1:03:3A:A5:CF:DC:65:85:04:E4:72:53:E2:16:22:7B:46:94:48:99:52:18:DD:4B:8E:57:14:C1:8E:32
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:98 rtx/90000
a=fmtp:98 apt=96
a=ssrc-group:FID 2688423809 44961990
a=msid:janus janusv0
a=ssrc:2688423809 cname:janus
a=ssrc:2688423809 msid:janus janusv0
a=ssrc:2688423809 mslabel:janus
a=ssrc:2688423809 label:janusv0
a=ssrc:44961990 cname:janus
a=ssrc:44961990 msid:janus janusv0
a=ssrc:44961990 mslabel:janus
a=ssrc:44961990 label:janusv0
a=candidate:1 1 udp 2015363327 192.168.0.91 58895 typ host
a=candidate:2 1 udp 2015364351 172.17.0.1 56026 typ host
a=candidate:3 1 udp 2015363583 10.42.0.0 50978 typ host
a=end-of-candidates

Connection State has changed checking 
Connection State has changed connected 
EventMsg map[result:map[status:started] streaming:event]WebRTCUp type 7683146238909134

Got VP8 track, saving to disk as output.ivf
Got Opus track, saving to disk as output.ogg

@lminiero
Copy link
Member Author

Thanks for double checking! Yep, I tried the exact same mountpoint you shared in your PR, with opus=97 and vp8=96, to make sure 97 wouldn't be used for rtx as well as it did before. I tried the EchoTest too, to test a situation where payload types are originated by the browser, and it seemed fine there as well. I'll have to make some tests with renegotiations, as that may uncover some things we may have to address. In case you manage to also make some other tests yourself, please do keep me posted.

@lminiero
Copy link
Member Author

lminiero commented Oct 3, 2022

Merging.

@lminiero lminiero merged commit 0b5ea05 into master Oct 3, 2022
@lminiero lminiero deleted the rtx-pt-conflicts branch October 3, 2022 13:10
mwalbeck pushed a commit to mwalbeck/docker-janus-gateway that referenced this pull request Oct 4, 2022
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [meetecho/janus-gateway](https://github.com/meetecho/janus-gateway) | minor | `v1.0.4` -> `v1.1.0` |

---

### Release Notes

<details>
<summary>meetecho/janus-gateway</summary>

### [`v1.1.0`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v110---2022-10-03)

[Compare Source](meetecho/janus-gateway@v1.0.4...v1.1.0)

-   Added versioning to .so files \[[PR-3075](meetecho/janus-gateway#3075)]
-   Allow plugins to specify msid in SDPs \[[PR-2998](meetecho/janus-gateway#2998)]
-   Fixed broken RTCP timestamp on 32bit architectures \[[Issue-3045](meetecho/janus-gateway#3045)]
-   Fixed problems compiling against recent versions of libwebsockets \[[Issue-3039](meetecho/janus-gateway#3039)]
-   Updated deprecated DTLS functions to OpenSSL v3.0 [PR-3048](meetecho/janus-gateway#3048)]
-   Switched to SHA256 for signing self signed DTLS certificates (thanks [@&#8203;tgabi333](https://github.com/tgabi333)!) \[[PR-3069](meetecho/janus-gateway#3069)]
-   Started using strnlen to optimize performance of some strlen calls (thanks [@&#8203;tmatth](https://github.com/tmatth)!) \[[PR-3059](meetecho/janus-gateway#3059)]
-   Added checks to avoid RTX payload type collisions \[[PR-3080](meetecho/janus-gateway#3080)]
-   Added new APIs for cascading VideoRoom publishers \[[PR-3014](meetecho/janus-gateway#3014)]
-   Fixed deadlock when using legacy switch in VideoRoom \[[Issue-3066](meetecho/janus-gateway#3066)]
-   Fixed disabled property not being advertized to subscribers when VideoRoom publishers removed tracks
-   Fixed occasional deadlock when using G.711 in the AudioBridge \[[Issue-3062](meetecho/janus-gateway#3062)]
-   Added new way of capturing devices/tracks in janus.js \[[PR-3003](meetecho/janus-gateway#3003)]
-   Removed call to .stop() for remote tracks in demos \[[PR-3056](meetecho/janus-gateway#3056)]
-   Fixed missing message/info/transfer buttons in SIP demo page
-   Fixed postprocessing compilation issue on older FFmpeg versions \[[PR-3064](meetecho/janus-gateway#3064)]
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yMTMuMCIsInVwZGF0ZWRJblZlciI6IjMyLjIxMy4wIn0=-->

Reviewed-on: https://git.walbeck.it/walbeck-it/docker-janus-gateway/pulls/91
Co-authored-by: renovate-bot <bot@walbeck.it>
Co-committed-by: renovate-bot <bot@walbeck.it>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multistream Related to Janus 1.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants