Do not clone audio RTP packets...

[jmillan]

.., nor create a shared_ptr for each of them.

Audio packets do not need to be referenced in the future as video ones
(for retransmission).

[jmillan]

This PR avoids the overhead of cloning and creating a std::shared_ptr for audio RTP packets since we don't need it. There's no need to access audio RTP packets in the future as it is for video retransmissions.

Internal API changes:

• Consumer::SendRtpPacket(RTC::RtpPacket* packet, std::shared_ptr<RTC::RtpPacket> sharedPacket)
• RtpStreamSend::ReceivePacket(RTC::RtpPacket* packet, std::shared_ptr<RTC::RtpPacket> sharedPacket)

One of which arguments is nullptr:

• packet is nullptr for video
• sharedPacket is nullptr for audio

I started creating two separated methods with one argument each. That required having a private method that would handle both in order to avoid duplicating code. Such approach makes the PR totally unreadable.

[jmillan]

Guys, rust is failing to build mediasoup. Does anyone have an idea? I'm building it correctly locally (macos)

[nazar-pc]

There was new Rust release yesterday, probably related to that.

It is not a compilation, rather it is a clippy lint that checks for good code practices. It has a link to the documentation, just apply the fix suggested there (you can reproduce lint error locally with cargo clippy).

[jmillan]

Ha,

git commit -am"rust: properly use $crate in a macro definition"

[jmillan]

Note: This is a bit convoluted, making Consumer's and RtpStreamSend decide wether they should use the raw or the shared pointer. It also adds few assert that were not there.

I'm going to rework this a bit and do some performance test, with the idea of keeping the current signatures (RtpPacket* packet, shared_ptr<RtpPacket> sharedPacket), being packet always the original packet and sharedPacket only a pointer container (what it really is) that will be filled by the first RtpStreamSend that needs it a clone. Indeed this is how it's done in the original PR.

[ibc]

Is really that important from perf point of view to avoid wrapping audio packets in a shared pointer?

[ibc]

Indeed I feel this PR is too complex and it makes hard to know when we have to operate with packets and shared pointers.

[jmillan]

Is really that important from perf point of view to avoid wrapping audio packets in a shared pointer?

We can't know until we test it.

Indeed I feel this PR is too complex and it makes hard to know when we have to operate with packets and shared pointers.

Exactly this is what I say I'm going to avoid.

[jmillan]

Indeed I feel this PR is too complex and it makes hard to know when we have to operate with packets and shared pointers.

So the raw pointer is the only thing Consumers need to deal with. shared pointer should only be needed by RtpStreamSend in order to increment the ref count if needed.

[jmillan]

Having the following test, for 10M iterations:

nullptr && initialized shared_ptr:

	for (size_t i = 0; i < iterations; i++)
	{
		// Create packet.
		auto* packet = RtpPacket::Parse(rtpBuffer1, 1500);
		packet->SetSsrc(1111);

                 // Wrap it into a shared pointer.
		std::shared_ptr<RtpPacket> sharedPacket(packet);

		stream->ReceivePacket(nullptr, sharedPacket);
	}

raw && empty shared_ptr duration:

	for (size_t i = 0; i < iterations; i++)
	{
		// Create packet.
		auto* packet = RtpPacket::Parse(rtpBuffer1, 1500);
		packet->SetSsrc(1111);
                 
                 // Create an empty shared pointer.
		std::shared_ptr<RtpPacket> sharedPacket;

		stream->ReceivePacket(packet, sharedPacket);
	}

Here the time results:

Is really that important from perf point of view to avoid wrapping audio packets in a shared pointer?

Avoiding the initialisation of a shared pointer when no needed comes with ~= 25% performance gain. IMHO we should go for it, we will get that performance gain for audio.

As indicated previously I would always pass a raw pointer (as we always did) and an empty shared pointer. Such shared pointer will be initialized by the first RtpStreamSend than needs it (video stream with nack enabled) and will be there usable for the rest of RtpStreamSend instances.

Comments @ibc?

[ibc]

If the shared pointer is gonna be created by the first RtpStreamSend that needs it, how does the receiving Transport or Router knows whether it should delete the original packet or not? Or will it delete is always?

[jmillan]

If the shared pointer is gonna be created by the first RtpStreamSend that needs it,

No, the sharer pointer will be created by Router, not initialized (as in test raw && empty shared_ptr duration ), and it will be initialised sharedPacket.reset(packet->Clone()) by the first RtpStreamSend that needs it. Consumers will work with the raw packet solely, as always, and pass the sharedPacket to RtpStreamSend without touching it.

how does the receiving Transport or Router knows whether it should delete the original packet or not? Or will it delete is always?

The original packet is deleted as always, by Transport. The shared one is automatically handled.

[jmillan]

sharedPacket will be created empty by Router and passed down by reference.

[ibc]

Ok, let's see how it looks.

[jmillan]

Ok, let's see how it looks.

I hope the code is clear enough:

• Router creates an empty shared_ptr and passes it down along with the RtpPacket
• Consumers know nothing about the shared_ptr.., they just relay it to RtpStreamSend
• The first RtpStreamSend who needs to ref count the shared pointer, if any, (for later reference), will clone the original packet and ref count it.
• The rest of RtpStreamSend who need to ref count the shared pointer will just increase the counter, as it's already cloned and has it's own buffer.

[ibc]

I like it

[ibc]

Tested with real packet loss?

[jmillan]

Tested with real packet loss?

Yes, tested in a real environment and working as expected.

[ibc]

Please convert into PR