RtpStreamSend: Memory optimizations

[jmillan]

Essentially extracted from #675

• RtpStreamSend does not clone each RTP packet coming from Producer.

  • A std::shared_ptr holding the original packet is used by all RtpStreamSend
    instances.

• RtpStreamSend does not clone each RTP packet when sending DTX.

  • The original RTP packet is DTX encoded/decoded when needed.

• RtpStreamSend does not store up to 65536 RTP packets in a std::map.

  • Only a limited range of RTP packets are stored in a std::deque (up to 2000 ms of media).

Memory usage differences using mediasoup-demo with 6 attendees in a room:

• v3: 60MB
• rtpstreamsend-optimizations: 17.5MB.

[jmillan]

@ibc, @nazar-pc,

#675 is extremely big and difficult to review. This is the Nth split I make out of that PR. Let's never do such big PR again with many different kind of changes, it's too time consuming and it takes too much time to get them merged.

AFAIK, after this only the object pool stuff would be missing.

Please, do dedicate some time reviewing. I've tested it and the memory savings big.

I have made some extra enhancements such as not cloning the RTP packet not even for sending DTX.

[jmillan]

Also @nazar-pc , removed the useNack argument from RtpStreamSend constructor since it's present in the already provided params argument.

[jmillan]

Note: I'll write more tests.

[jmillan]

@nazar-pc, could you please look at the Rust CI error?

[jmillan]

@nazar-pc, could you please look at the Rust CI error?

I'm checking

[jmillan]

Rust is failing to link MaxRetransmissionDelay and MinRetransmissionDelay in the GH workflows.

cargo test is properly working for me locally. Any idea?

[ibc]

OMG YES. static members initialization must be done in cpp files.

[jmillan]

OMG YES. static members initialization must be done in cpp files.

Not required for numeric variables as per the spec. Indeed cargo test was working in localhost.

[ibc]

Prove that we initialize any static class numeric variable the way it was done in this PR before.

[jmillan]

Prove that we initialize any static class numeric variable the way it was done in this PR before.

I didn't say otherwise.

[jmillan]

Great problem I've found with the approach in this PR.

A shared_ptr is created in Router out of the original RtpPacket. The payload buffer of it points to a memory pointer of ReadBuffer in UdpSocketHandler or the corresponding in TcpConnectionHandler if we are using TCP.

This means that such payload pointer will get invalidated once the next packet arrives.

This is the reason why the original PR did a RtpPacket::Clone() on the first RtpStreamSend. Instead of that I'll clone the packet in Router , before passing it to the Consumer s, so they have an RtpPacket with it's own memory buffer that can be used safely in the future whenever a retransmission is needed.

[ibc]

A shared_ptr is created in Router out of the original RtpPacket. The payload buffer of it points to a memory pointer of ReadBuffer in UdpSocketHandler or the corresponding in TcpConnectionHandler if we are using TCP.

This means that such payload pointer will get invalidated once the next packet arrives.

Indeed. Just wondering, didn't this bug produce terrible issues already? It shouldn't be hard to reproduce AFAIU.

This is the reason why the original PR did a RtpPacket::Clone() on the first RtpStreamSend. Instead of that I'll clone the packet in Router , before passing it to the Consumer s, so they have an RtpPacket with it's own memory buffer that can be used safely in the future whenever a retransmission is needed.

Why cloning it in Router before passing it to Consumers? It should be cloned when generated in the Producer, right? or even in RtcStreamRecv.

[jmillan]

Why cloning it in Router before passing it to Consumers? It should be cloned when generated in the Producer, right? or even in RtcStreamRecv.

If there is no Consumer there is even no need to clone the packet.

[ibc]

We use RtpObservers as well. Please clone before.

[jmillan]

We use RtpObservers as well. Please clone before.

And do they need to access the RtpPacket once after this libuv iteration?

They process the current packet and don't need to access the packet in the future.

[jmillan]

We can clone the packet before, it's just not needed if there are no Consumers.

[ibc]

We use RtpObservers as well. Please clone before.

And do they need to access the RtpPacket once after this libuv iteration?

They process the current packet and don't need to access the packet in the future.

This is true for now. Will it be forever?

[jmillan]

This is true for now. Will it be forever?

v3 does only clone in RtpStreamSend, thus only if Consumers exists.

I've set an explicit comment in Router, before cloning:

			// Clone the packet so it holds its own buffer, usable for future
			// retransmissions.

If you have a preference to always clone a RtpPacket now I won't oppose. We just can save those cycles when no needed.

[ibc]

Other than pending comment this is great

[nazar-pc]

🎉