Pushing to RTCTrack can cause panics/drops

[kixelated]

The current interface uses the channels RTCTrack.Samples chan<- media.RTCSample and RTCTrack.RawRTP chan<- *rtp.Packet to send data over the connection.

If the connection is closed, these channels are closed. Sending to a closed channel will cause the code to panic. I've also seen a lot of packet drops because the buffered channel (size 15) is not large enough to handle surges.

I would love to change the API such that you call WriteSample or WritePacket instead of interfacing with channels. I also don't think this should be part of the RTPTrack, but instead the RTCRtpSender returned by AddTrack.

This should also be a blocking call which would remove a goroutine and some context switching. It would go a long way for my project which creates thousands of peer connections.

[rob-deutsch]

For what it's worth; I ran into similar issues with pushing to RTCTrack last week. My app creates a lot of relatively short-lived connections, and I ran into issues keeping everything in sync with RTCRtpSender/RTCTrack channels.

Your suggested fix is actually part of the v2 plans.

[backkem]

As Rob mentioned, we definitely intend to do this. The channels API is there mostly for historical reasons. We've been documenting API breaking changes on the wiki. This point would be covered under: https://github.com/pions/webrtc/wiki/v2.0.0-Release-Notes#go-channels-chan-were-removed-from-the-public-api. Preferably we would match the API used in pions/datachannel and other packages in the org.

[Sean-Der]

100% agree, we have run into lots of issue around this. I am all for a solution, here is what I was thinking so far.

• We should allow for lossy AND lossless connections. Some people might want to throw away packets going in/out. They only want the latest data if latency is a huge concern (Is this true actually true though?)
• WriteSample and WritePacket should be on RTCTrack. RTCRtpSender isn't a spec we control, and the way it is designed is you can swap out audio/video tracks at anytime.

I am all for breaking this for v2.0.0 as long we get feedback from everyone!

[kixelated]

I would propose:

RTCTrack.ReadPacket(context.Context) (*rtp.Packet, error)
RTCTrack.ReadSample(context.Context) (*webrtc.Sample, error)
RTCTrack.WritePacket(context.Context, *rtp.Packet) (error)
RTCTrack.WriteSample(context.Context, *webrtc.Sample) (error)

context support is required for blocking reads and you should use it for writes just in case the UDP/TCP socket blocks.

I kind of want plural methods, like ReadPackets and WritePackets that return an array, but I don't really think there will be that much of a performance or usability benefit.

You can pretty trivially implement the current lossy API with a busy loop that calls Read and writes to a buffered channel (reverse for writing). That's literally what the transceiver does today. But you would now have the option to do more complex logic, like refusing to drop i-frames.

Just make sure that the goroutine that calls these methods reads/writes to the socket directly. The current approach of using a dedicated goroutine for all reads/writes has some performance impact. The caller can always spawn a goroutine themselves if they wish.

[kixelated]

The plural might actually be better, or needs to be added at some point.

I'm profiling my application and 30% of the time is spent in the sendto syscall. We would like to change this to sendmmsg when there are multiple packets to send to the same user. We would just add a sample and it would get split into multiple packets based on the MTU, and all packets sent at the same time. This will reduce the time spent switching between kernel and user-space.

imo:

RTCTrack.WritePacket(context.Context, ...*rtp.Packet) (error)

[backkem]

FWIW, My original API ideas are documented on the wiki: https://github.com/pions/webrtc/wiki/TransportDesign#rtp-refactor

[kixelated]

I profiled my application and 15.63% of time is spent receiving from the channel, 33% is actually encrypting/writing to the socket. The removal of the goroutine and channel would be a huge performance improvement.