search

paullouisageneau / libdatachannel

C/C++ WebRTC network library featuring Data Channels, Media Transport, and WebSockets
https://libdatachannel.org/
Mozilla Public License 2.0
1.73k stars 353 forks source link

RtcpHandler and PeerConnection::forwardMedia #303

Open stazio opened 3 years ago

stazio commented 3 years ago

So I've been noodling on this for a bit and I realized that RtcpHandler doesn't really cut it when it comes to processing the state of the whole transport. I don't know much about TWCC, but by the sound of the title "transport-wide", I think this would be a good example of where RtcpHandler sort-of fails. Additionally, there's a lot of code duplication on my end because essentially the code that runs in forwardMedia, runs again in each instance of my implementation of RtcpHandler to actually process the packets. I think it would be a good idea to make RtcpHandler take the role of forwardMedia and expose a bunch of functions such as processNack or processUnknownPacketType.

The biggest downside of this is that this would be a breaking API change, which is also why I am bringing it up now as it would be better to deal with it now, before too many people start using that.

paullouisageneau commented 3 years ago

I see the issue, I think your idea makes sense. However, I'd like to still allow tracks to run without a handler so forwardMedia should still independently handle the track dispatching.

The API change will mostly consists in moving setRtcpHandler() to PeerConnection, right? If so, please do.

stazio commented 3 years ago

I don't think it's unreasonable to have a default RtcpHandler that is the one that's built in. My biggest complaint is this needlessly complicated double matching parts of a packet thing going on. If you want to be able to support your own non-standard RTCP payload types it's impossible without modifying the library itself.

paullouisageneau commented 3 years ago

Sure, what I meant is that I would still want to be able to run without RTCP handler, in case the user handles RTCP themselves.

stazio commented 3 years ago

Ohhh I understand. I totally misread what you said originally.

stazio commented 3 years ago

Hmm, so I need to be able to call RtcpHandler::outgoing somehow from within the track. If it is owned by PeerConnection now, how should I get access to it?

paullouisageneau commented 3 years ago

I think the easiest way would be to make SrtpTransport have a weak reference on RtcpHandler and call RtcpHandler::outgoing() on send.

stazio commented 3 years ago

So I would struggle quite hard to do this refactor, especially with this new codebase. I really need to add RTX streams so I made stazio:media_distributor to do so. It's using the old codebase so it would need a refactor to work with the new stuff. I don't know if I like it having be done that way -- seems silly to have a track and peer connection handler at the same time.

paullouisageneau commented 3 years ago

Hum indeed, I don't really get the point of introducing a new MediaDistributor (which doesn't actually seem to maintain any particular state), wasn't the original idea about registering the existing RtcpHandler/MediaHandler into PeerConnection?

stazio commented 3 years ago

Yeah,... I need a very quick fix and it seems like a much bigger undertaking than I originally realized.