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

Support for multistream PeerConnections (replaces #1459) #2211

Merged
merged 110 commits into from
Feb 11, 2022
Merged

Conversation

lminiero
Copy link
Member

@lminiero lminiero commented Jun 8, 2020

Thanks to our friends at Zextras (thanks guys!), we finally found the time and resources to revive the old multistream effort we started in #1459!

If you've followed its evolution, you'll remember that we actually started working on it a couple of years ago: unfortunately, due to lack of feedback, testing, and more in general us being way too busy with other things, that branch eventually became severely out of sync, and ended up being full of conflicts that were basically unresolvable. For that reason, we tagged that branch as #deprecated#, and stated we'd get back to it sooner or later, to re-apply the same changes starting from the latest Janus instead.

This is exactly what happened here. We went through all the changes we did at the time, rolled up our sleeves, and slowly figured out how they could be re-integrated in the new code base. Actually, this new effort is actually a bit leaner: in fact, the original patch contained a lot of changes that were not really relevant (renaming structures, defines, code style fixes, etc.), that could actually be considered mostly noise in the grand schema of things. As such, I decided to ignore all those changes instead, and just focus on what really needed changing to get all this working, which should make looking at the changes in the code a bit easier (not easy, but definitely easier).

For what concerns plugin APIs and, most importantly, janus.js APIs, the changes are exactly the same as the ones done in old #1459 (apart of course from new things that were added in the meanwhile, and were updated to be used here). While I plan to write a new summary soon, for the time being you can refer to the long description I made on the original PR, as most, if not all, of the concepts explained there apply here too.

Please notice that I won't let this effort die like #1459 did. I encourage you to test this and provide feedback, because this WILL be merged soon, and will be our 1.0.0 version. Considering the disruptive nature of this branch (which is only partly backwards compatible, especially in a few plugins), we plan to create a separate branch for the existing 0.10.x Janus, rather than simply create a tag: in fact, a tag cannot be updated (it's a snapshot), while a separate branch can still receive fixes and optionally enhancements. Please beware, though, that once multistream is merged we'll focus our development efforts on that: we'll still push and accept fixes for bugs on whatever branch 0.10.x will be on, but we won't work on new features there. This should give you one additional motivation in exploring this branch as soon as possible, and ensure that it will work as expected for you.

I made a few tests and all plugins seem to be working as expected. I did notice a few leaks here and there, possibly caused by missing reference counters somewhere, that I plan to address in the next few days. That said, please test and provide feedback!

@lminiero
Copy link
Member Author

lminiero commented Jun 8, 2020

For some more context, you can refer to this talk I made last year at CommCon, where I described the relevant changes:
https://www.youtube.com/watch?v=xbsHYvBjsdY
https://www.slideshare.net/LorenzoMiniero/multistream-in-janus-commcon-2019

@jamken
Copy link

jamken commented Jun 9, 2020

hi,@lminiero
The feature 'multistream in one PC' sounds like a good idea, but it harms the scalability of janus videoroom. In our janus-cloud (https://github.com/OpenSight/janus-cloud) solution, the participants get each stream of the other publishers on an indivudual PC from the different Janus-Servers. Assume that Alice push his stream to JanusA, Bob push hist stream to Janus B, Alice need to pull Bob's stream from JanusB, Bob need to pull Alice's stream from Janus A. So Alice need two PeerConnection, one for JanusA, the other for JanusB, So does Bob. In current implementation of Janus, it's good to support this case, but after 'multistream in one PC', how can Alice get the Bob's stream?
Perhaps this feature 'multistream in one PC' can be a option feature, user can disable/enable it for his situation ( if he want to get the scalability, he can disable this feature and make janus works like before, one stream one pc).

@lminiero
Copy link
Member Author

lminiero commented Jun 9, 2020

@jamken it IS optional. Read the first changes in janus_videoroom.c which is where the updated documentation is.

@petarminchev
Copy link
Contributor

Hi Lorenzo,

First of all, thank you for the hard work and nice feature!

We have a question how multistreams will affect Streaming plugin in the following use-case:

Currently we are using the Streaming plugin and rtp_forward request from VideoRoom to Streaming plugin for horizontal scalability.
We have looked at the documentation, but cannot find a method to add/remove a stream to/from an existing Streaming mountpoint.
The way we understand it is that the subscriber will subscribe to a single Streaming mountpoint with a list of streams inside (each stream inside has a "mid").
There will be a single PeerConnection from the subscriber to that Streaming mountpoint and it will provide all of the streams inside in that single PeerConnection.

The question is what will happen when a new publisher comes to the scene. We would need somehow to add that publisher to the existing streaming mountpoint list.
But we didn't find such a method after looking at the documentation.

How should the above use-case be handled? We don't consider destroying and recreating the streaming mountpoint as a great idea, because it will be disruptive to the subscribers.

Or maybe we misunderstand how the above use-case should be handled in the new multistream approach?

Thank you in advance for any guidance!

@lminiero
Copy link
Member Author

@digitalsamba Streaming mountpoints are static in the current iteration. As such, an approach might be pre-creating mountpoints with enough slots as you'll need, and "fill" them as people come in, without renegotiating. Not ideal, as you'll have to exchange info yourself out-of-band on who's what in that PeerConnection, but that's the only way to do that right now I guess. It's explicitly stated in the presentation I listed above as well that scaling the VideoRoom is a TBD

I don't plan to change the way the new Streaming plugin works right now. One thing we might do in the future is make the VideoRoom itself support remote publishers, but that won't happen here. It's something we'll only look into once this effort has stabilized and it has been merged.

@lnogueir
Copy link

lnogueir commented Jul 5, 2020

When will this be merged and docs get updated? How will this affect videoroom plugin?

@lminiero
Copy link
Member Author

lminiero commented Jul 5, 2020

The sooner you provide feedback, the sooner I'll merge. I can't magically know it works for you if you don't try it and tell me. Check #1459 for docs and changes.

@bwqr
Copy link

bwqr commented Jul 8, 2020

Hi there, I am kind of beginner about webrtc stuffs. I was trying to send both screen and camera on same peer connection and came across with this pr. I want to ask a question about mid and its description.

In order to differentiate the camera and screen, i want to pass descriptions. However I do not know anything about mid which belongs to my track. For example, user firstly publishes its camera which is mostly assigned to mid "0". Then, when user also publishes its screen, via configure request, there are two streams listed with mids "1" and "2". May be I need to add the new track into peer connection via addTrack function. However, again, there is no information about what does this track represents, screen or camera? And other users are not get notified about this track with event which has publishers array, when I have added track with configure request.

Do you have any suggestion about this or have any idea to solve problem? I think I am missing some points about publishing and adding tracks. I don't know if I should open an issue or comment under this pr.

And thanks for the efforts for the whole project and this pr.

@lminiero
Copy link
Member Author

lminiero commented Jul 8, 2020

Do you have any suggestion about this or have any idea to solve problem? I think I am missing some points about publishing and adding tracks. I don't know if I should open an issue or comment under this pr.

You're right, this bit is still partly missing, mostly because the way you add streams has not been properly updated yet. I want to update janus.js so that when you add a new stream (e.g., add webcam video, and audio) you can specify the mid you'd like each track to have: if the mid exists you're replacing a track, otherwise you're adding a new one. This way, you'd have control over that aspect.

Until that happens (I'm busy with other things for the moment) you'll have to take care of the mapping yourself: e.g., negotiate an audio/video publisher as usual (to send audio/video), and then manually add a new track yourself and send a new offer; if you inspect the PeerConnection object ([handle].webrtcStuff.pc) you can find out which mid each outgoing track has.

@bwqr
Copy link

bwqr commented Jul 9, 2020

Thanks for the answer. Specifying the mid foreach track can be really good idea, and if mid already exists, overriding the mid will help the application, because we will not need description anymore. For now, I will try to implement your temporary suggestion.

@kinserlin
Copy link

kinserlin commented Aug 5, 2020

hi.I have deploy the multistream branch to server but serveral problems

<snip>

@lminiero
Copy link
Member Author

lminiero commented Aug 5, 2020

@kinserlin please check the guidelines again, before dropping a megaton of text in here.

@Bug-Fairy
Copy link
Contributor

Firefox seems to be having issues with simulcast on this branch. I'm seeing constant SRTP protect error... srtp_err_status_replay_fail errors for Firefox clients on echotest (and custom plugin). Happens with VP8 when substream 0 or 1 is selected. Localhost server.

@Bug-Fairy
Copy link
Contributor

Is there any specific reason sdp-utils.c has JANUS_BUFSIZE defined as 16384? It's causing SDP parsing to be cutoff in janus core when trying to negotiate multiple streams (5 audio and 5 video in my test case) in one SDP.

@roisasson
Copy link

roisasson commented Aug 12, 2020

This is a great addition to Janus. Thank you for taking it on.
 
I am wondering how multistream will affect synchronization. My understanding is that all MediaStreamTracks in the same MediaStream are synchronized when rendered. The binding between MediaStreams, MediaStreamTracks and the RTP Stream (SSRC) is signaled in SDP using the msid attribute. Janus generates SDP offers that assign all MediaStreamTracks to the same MediaStream with the msid identifier “janus”. This works fine with the VideoRoom plugin today because audio and video Tracks that share the same PeerConnection to a subscriber always originate from the same publisher and typically require synchronization.
 
When using the VideoRoom plugin with multistream, one PeerConnection to a subscriber could carry MediaStreamTracks from different publishers. If they are all assigned the same msid identifier they will be treated as part of the same MediaStream and synchronized when rendered. That might cause media from one publisher to unnecessarily delay the rendering of media from all other publishers. Is my understanding correct? It seems like the msid identifiers have to somehow be configurable. Sorry if I am missing something obvious.

@amnonbb
Copy link
Contributor

amnonbb commented Aug 18, 2020

Is there any specific reason sdp-utils.c has JANUS_BUFSIZE defined as 16384? It's causing SDP parsing to be cutoff in janus core when trying to negotiate multiple streams (5 audio and 5 video in my test case) in one SDP.

I puted 32768 there. Tested with 25 feeds in room

@lminiero
Copy link
Member Author

Is there any specific reason sdp-utils.c has JANUS_BUFSIZE defined as 16384? It's causing SDP parsing to be cutoff in janus core when trying to negotiate multiple streams (5 audio and 5 video in my test case) in one SDP

Simply a value that we thought was large enough. Can't remember if we had a higher value in #1459. Anyway, I have tried a VideoRoom subscription with ~20 videos in and it seemed to work. If you experiment with different values and have proposals, happy to hear them.

@lminiero
Copy link
Member Author

I am wondering how multistream will affect synchronization. [..] It seems like the msid identifiers have to somehow be configurable

Not sure. Making them configurable would mean putting this burden on plugins, making them more complicated, as the core obviously doesn't know the relations between tracks: pretty sure it would have an impact on other parts of the core too (possibly RTCP SDES?). I'd rather not worry about that now, and get everything else working before that and merged, and in case it's an issue fix it later on.

@lminiero
Copy link
Member Author

Firefox seems to be having issues with simulcast on this branch

From what I've read in another issue, Firefox has other issues too. I'll have a look probably next week.

@roisasson
Copy link

I am wondering how multistream will affect synchronization. [..] It seems like the msid identifiers have to somehow be configurable

Not sure. Making them configurable would mean putting this burden on plugins, making them more complicated, as the core obviously doesn't know the relations between tracks: pretty sure it would have an impact on other parts of the core too (possibly RTCP SDES?).

Hi Lorenzo,
 
Complexity is certainly a concern. The scenario I am worried about is, for example, a wireless endpoint with very high jitter publishing audio into a room. That audio stream might delay the playback and rendering of all audio and video streams for all of its subscribers.
 
Just a thought – instead of making the msid identifiers configurable, would it be easier to just copy them over from the incoming SDP offer to the outgoing SDP offer? I believe that would avoid the problem since browsers generate SDP offers with random msid identifiers. Thank you.

@Bug-Fairy
Copy link
Contributor

Is there any specific reason sdp-utils.c has JANUS_BUFSIZE defined as 16384? It's causing SDP parsing to be cutoff in janus core when trying to negotiate multiple streams (5 audio and 5 video in my test case) in one SDP

Simply a value that we thought was large enough. Can't remember if we had a higher value in #1459. Anyway, I have tried a VideoRoom subscription with ~20 videos in and it seemed to work. If you experiment with different values and have proposals, happy to hear them.

I've hit the limit by letting the browser generate the offer. Most of the SDP is taken up by multiple h264 profiles for each video mline. Note that I am not using janus.js but using my own front end.
Did some performance tests and profiling with JANUS_BUFSIZE 65536 and encountered no issues.

@lminiero
Copy link
Member Author

Just noticed I didn't write it here explicitly as well: there is a date for when multistream will be merged, and a strategy for how to deal with those that prefer to keep on using the current 0.x branch. Please see PR #2855 for more information.

@lminiero
Copy link
Member Author

The moment has finally come: merging the multistream branch on the day of Janus' 8th birthday! 🥳 🍰

@lminiero lminiero merged commit 4110eea into master Feb 11, 2022
@lminiero lminiero deleted the multistream branch February 11, 2022 09:02
@lminiero
Copy link
Member Author

For those that are interested, I wrote a blug post to summarize the changes:
https://www.meetecho.com/blog/multistream/

@M0Rf30
Copy link

M0Rf30 commented Feb 11, 2022

The moment has finally come: merging the multistream branch on the day of Janus' 8th birthday! partying_face cake

So great. I also saw your intervention to FOSDEM 2022. Congrats paisà!
AUR package synced to 0.11.8 here

mwalbeck pushed a commit to mwalbeck/docker-janus-gateway that referenced this pull request Mar 16, 2022
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [meetecho/janus-gateway](https://github.com/meetecho/janus-gateway) | major | `v0.11.7` -> `v1.0.0` |

---

### Release Notes

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

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

[Compare Source](meetecho/janus-gateway@v0.12.0...v1.0.0)

-   Refactored Janus to support multistream PeerConnections \[[PR-2211](meetecho/janus-gateway#2211)]
-   Moved all source files under new 'src' folder to unclutter the repo \[[PR-2885](meetecho/janus-gateway#2885)]
-   Fixed definition of trylock wrapper when using pthreads \[[Issue-2894](meetecho/janus-gateway#2894)]
-   Fixed broken RTP when no extensions are negotiated
-   Added checks when inserting RTP extensions to avoid buffer overflows
-   Added missing support for disabled rid simulcast substreams in SDP \[[PR-2888](meetecho/janus-gateway#2888)]
-   Fixed TWCC feedback when simulcast SSRCs are missing (thanks [@&#8203;OxleyS](https://github.com/OxleyS)!) \[[PR-2908](meetecho/janus-gateway#2908)]
-   Added support for playout-delay RTP extension \[[PR-2895](meetecho/janus-gateway#2895)]
-   Fixed partially broken H.264 support when using Firefox in VideoRoom
-   Fixed new VideoRoom rtp_forward API ignoring some properties
-   Fixed deadlock and segfault when stopping Streaming mountpoint recordings \[[Issue-2902](meetecho/janus-gateway#2902)]
-   Fixed RTSP support in Streaming plugin for cameras that expect path-only DESCRIBE requests (thanks [@&#8203;jp-bennett](https://github.com/jp-bennett)!) \[[PR-2909](meetecho/janus-gateway#2909)]
-   Fixed RTP being relayed incorrectly in Lua and Duktape plugins
-   Added Duktape as optional dependency, instead of embedding the engine code \[[PR-2886](meetecho/janus-gateway#2886)]
-   Fixed crash at startup when not able to connect to RabbitMQ server
-   Improved fuzzing and checks on RTP extensions
-   Removed distinction between simulcast and simulcast2 in janus.js \[[PR-2887](meetecho/janus-gateway#2887)]
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

### [`v0.12.0`](meetecho/janus-gateway@v0.11.8...v0.12.0)

[Compare Source](meetecho/janus-gateway@v0.11.8...v0.12.0)

### [`v0.11.8`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v0118---2022-02-11)

[Compare Source](meetecho/janus-gateway@v0.11.7...v0.11.8)

-   Added initial (and limited) integration of RED audio \[[PR-2685](meetecho/janus-gateway#2685)]
-   Added support for Two-Byte header RTP extensions (RFC8285) and, partially, for the new Depencency Descriptor RTP extension (needed for AV1-SVC) \[[PR-2741](meetecho/janus-gateway#2741)]
-   Fixed rare race conditions between sending a packet and closing a connection \[[PR-2869](meetecho/janus-gateway#2869)]
-   Fix last stats before closing PeerConnection not being sent to handlers (thanks [@&#8203;zodiak83](https://github.com/zodiak83)!) \[[PR-2874](meetecho/janus-gateway#2874)]
-   Changed automatic allocation on static loops from round robin to least used \[[PR-2878](meetecho/janus-gateway#2878)]
-   Added new API to bulk start/stop MJR-based recordings in AudioBridge \[[PR-2862](meetecho/janus-gateway#2862)]
-   Fixed broken duration in spatial AudioBridge recordings
-   Fixed broken G.711 RTP forwarding in AudioBridge (thanks [@&#8203;AlexYaremchuk](https://github.com/AlexYaremchuk)!) \[[PR-2875](meetecho/janus-gateway#2875)]
-   Fixed broken recordings in NoSIP plugin
-   Fixed warnings when postprocessing Opus recordings with DTX packets
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

</details>

---

### Configuration

📅 **Schedule**: 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).

Reviewed-on: https://git.walbeck.it/walbeck-it/docker-janus-gateway/pulls/65
Co-authored-by: renovate-bot <[email protected]>
Co-committed-by: renovate-bot <[email protected]>
@tbence94
Copy link
Contributor

This PR broke the previous .mjr file naming conventions by adding another number to the end of the filename.

Previous filenames ended like this:

  • ...-audio.mjr
  • ...-video.mjr

After the PR they end like this:

  • ...-audio-0.mjr
  • ...-video-1.mjr

This can break applications if they expect that the filenames will end in a certain way.
I think this sould be noted in the changelogs somewhere.

Related changes in the code: https://github.com/meetecho/janus-gateway/pull/2211/files#diff-d96a677f8b8c31f15d44bf3a1d83934e5a6b6bdfab05bd70fd7be6119fb26740R7010-R7012

@lminiero
Copy link
Member Author

True, but with all the changes people had to be aware of to use version 1.x, I assumed it wasn't that important 🤭 That said, I'm not sure this belongs in a changelog, since otherwise I'd have to mention any other API change, and they're mostly listed here and the other original PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet