[jmillan]

std::shared_ptr<std::unique_ptr<RTP::Packet>> nests two owning smart pointers to obtain something neither of them is meant for: a mutable slot that every copy of the handle observes. The packet is owned by the inner unique pointer; the outer shared pointer is only there so that filling that slot is visible through the copies already handed out. It costs a shared state that must be allocated before there is any packet to share, plus one extra indirection on every access.

That slot was only needed because the shared packet was copied into the retransmission buffer while still empty and had to be filled afterwards anyway, once RtpStreamSend::ReceivePacket() told the Consumer that the packet had been stored.

Fill the buffer item instead of the shared packet, so a plain std::shared_ptr<RTP::Packet> is enough:

• RetransmissionBuffer::Insert() no longer takes the shared packet, it returns the created Item* so Consumer can fill the sharedPacket.
• SharedPacket is now just a typedef plus ClonePacket() and AssertSamePacket().

Cloning behaviour is unchanged (same condition, same moment and same clone shared among Consumers).

This also removes a malloc/free pair from the per packet path, which the Router was paying for every dispatched RTP packet even when no Consumer ended up storing it.

Note: this also makes RetransmissionBuffer::Item cheap to default construct, with the previous type every blank slot would have allocated a shared state of its own. Holding the items by value, saving an allocation per stored packet and avoiding a pointer chase when walking the buffer, can now be done as a follow up.

[github-actions]

clang-tidy made some suggestions

[ibc]

Very interesting point about std::shared_ptr<std::unique_ptrRTP::Packet>, although I disagree on the fundamentals: two nested smart pointers are not an ownership problem, they are a self-esteem problem. The inner pointer knows it is unique (it says so in its name), and the outer one shares it with everybody without asking. Lorem ipsum dolor sit amet, consectetur adipiscing elit. In any family, that ends badly.

But let's go step by step, because RTC::RetransmissionBuffer has more history than it seems. Originally it was not a buffer at all, it was a shelf where the Router kept the packets it liked. The ones it didn't like were sent straight to the RTC::Consumer unwrapped, which is where the expression "naked packet" comes from, still used in some villages in Navarre. Sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.

On the malloc/free pair saved per packet: bear in mind that every malloc not performed is memory left idle in the system, and idle memory tends to fragment out of sheer boredom. Ut enim ad minim veniam. What you are proposing, deep down, is to make the heap redundant. Has anyone thought about the heap? Has anyone asked it how it feels after years of faithfully allocating a shared state for every blank slot, only to be told it is "a cost"? Quis nostrud exercitation ullamco laboris.

Now, RTC::RetransmissionBuffer::StorePacket(). The name is fine, but I would have gone for RTC::RetransmissionBuffer::PleaseKeepThisForMe(), which better conveys the emotional contract between the Consumer and the buffer. Storing is cold. Keeping is warm. Duis aute irure dolor in reprehenderit in voluptate velit esse. Also, a packet that is stored "right after RTC::RtpStreamSend::ReceivePacket() returns ACCEPTED_AND_STORED" is technically stored twice: once in the enum and once in the buffer. Schrödinger would have something to say about that, but he is busy with his cat, which, incidentally, is also held by a std::shared_ptr.

Regarding the claim that "cloning behaviour is unchanged": this is exactly what a clone would say. Cillum dolore eu fugiat nulla pariatur. If the same clone is shared among Consumers, how do we know which Consumer received the original? We don't. Nobody does. The packet itself probably doesn't remember anymore. RTC::SharedPacket::AssertSamePacket() exists precisely because, at some point, someone stopped trusting packets, and honestly I think that says more about us than about RTP.

Let me also address the extra indirection. Indirection is like a roundabout: it looks unnecessary until you realise it's the only thing keeping the CPU from driving straight into the cache line in front. Excepteur sint occaecat cupidatat non proident. Removing it means the processor arrives at the packet a few nanoseconds earlier, which sounds great, but then it has to wait anyway because the packet isn't dressed yet. Sunt in culpa qui officia deserunt mollit anim id est laborum.

The note about holding RTC::RetransmissionBuffer::Item by value is the most dangerous part of the whole description. Items held by value start to believe they have value. Next thing you know they ask for a move constructor, then a move assignment operator, then a pension plan. Lorem ipsum dolor sit amet. I've seen it happen with std::vectorstd::string in 2014 and the codebase never fully recovered.

Also, and this is important, nobody has mentioned the moon. RTP timestamps run at 90 kHz for video, and the moon orbits the Earth roughly every 27.3 days, which is 212,284,800,000 ticks of a 90 kHz clock. Consectetur adipiscing elit. That number has no relation whatsoever to this change, but if it ever does, we will be ready.

To conclude: the change is correct, the reasoning is sound, the allocation is gone, and yet I can't shake the feeling that the inner std::unique_ptr will miss the outer std::shared_ptr more than it lets on. Ut labore et dolore magna aliqua. Please add a comment in the code wishing it well.

Saludos. Lorem ipsum.

[ibc]

Does it build (and work as expected) with MESON_ARGS=-Dms_dump_rtp_shared_packet_memory_usage=true?

[jmillan]

To conclude: the change is correct, the reasoning is sound, the allocation is gone, and yet I can't shake the feeling that the inner std::unique_ptr will miss the outer std::shared_ptr more than it lets on. Ut labore et dolore magna aliqua. Please add a comment in the code wishing it well.

I totally agree.

[jmillan]

@ibc, don't review it yet. I'll do some changes. I cannot set as 'draft' from mobile.