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

Refactored RTP forwarder internals as a core feature #3155

Merged
merged 2 commits into from
Mar 6, 2023
Merged

Conversation

lminiero
Copy link
Member

As the title says, this PR refactors the internals of the RTP forwarders functionality, moving it away from the plugins where it was implemented (VideoRoom, AudioBridge) and to the core instead. Nothing changes with respect to the APIs and how you use them, they're still created and destroyed as before: it's how they're implemented under the hood that this PR impacts.

The main motivation for this effort is that there was a lot of duplicated code in the two plugins to implement what, apart from a few differences, really was the same thing. Moving the bulk of the code to the core as a new feature allowed us to clean up the AudioBridge and VideoRoom code considerably, since we now just have to invoke a couple of functions to do what before needed hundreds of lines of inline code. Most importantly, now that RTP forwarders are a functionality exposed by the Janus core, this means that it will be much easier to add RTP forwarders to other plugins as well, if needed, since we won't have to duplicate code once more; it also means that, at least in theory, RTP forwarders could be added at the core level too (e.g., for plugin-agnostic monitoring of streams), should a need come for that in the future.

Functionally speaking, as anticipated nothing should change with this patch. The only thing that did indeed change is that, with this new approach, you can't share the same encryption context across multiple RTP forwarders: this was a feature that someone in the past used for optimization purposes (for SRTP forwarders belonging to the same publisher, you encrypted the payload once and sent it to all destinations), but I'm not sure if anyone actually even knew it was a thing; to be honest, the simplification this patch gives us makes me thing it's not a big loss, especially considering it would be rare to have that many forwarders for the same source anyway (and SRTP shouldn't weigh too much anyway).

Apart from this, I tried to ensure that what both VideoRoom and AudioBridge were able to do before is still possible to do now. I've done a few functional tests and they all seemed to be fine. That said, I still need to make tests on some edge cases, mostly the ones involving the different ways you can do simulcast with RTP forwarders. Another important thing to validate before merging is that this won't introduce race conditions: in the VideoRoom, for instance, we had custom references in place when using RTCP support for forwarders, and with the RTCP thread now under the responsibility of the core rather than the plugin, we should double check that this won't cause any issues.

If you're on version 1.x of Janus and you're using RTP forwarders for anything, please do take the time to test this feature and provide feedback. The sooner we know the changes are safe, the sooner we'll be able to merge it.

@lminiero lminiero added the multistream Related to Janus 1.x label Jan 30, 2023
@lminiero lminiero marked this pull request as ready for review January 30, 2023 16:42
@AleXoundOS
Copy link

Will it be possible after this PR is merged to forward incoming RTSP streams in some way (e.g. as 2 RTP streams)?

@lminiero
Copy link
Member Author

Yes, the idea is that in theory it should make it easier to add RTP forwarders to the Streaming plugin too, which means incoming RTP streams could be forwarded externally as well. Of course, that will require a separate effort: this patch only decouples RTP forwarders from plugins, and so only lays the foundation for future work.

@AleXoundOS
Copy link

I haven't tested the changes to spot any regressions. But I can confirm, that adding a primitive RTP forwarding of packets, received from RTSP source, in streaming plugin has become quite simple with this PR.

@lminiero
Copy link
Member Author

lminiero commented Mar 6, 2023

Merging.

@lminiero lminiero merged commit c3c7ffb into master Mar 6, 2023
@lminiero lminiero deleted the rtpfwd-core branch March 6, 2023 16:31
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.

None yet

2 participants