Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rtcp enhancements #914

Merged
merged 9 commits into from
Oct 4, 2022
Merged

Rtcp enhancements #914

merged 9 commits into from
Oct 4, 2022

Conversation

jmillan
Copy link
Member

@jmillan jmillan commented Sep 28, 2022

Fixes #907

Enhances RTCP compound packet sending:

  • It does now fill the RTCP compound packet as much as possible before sending:
    • Reduces the number of RTCP packets sent and the overhead given by smaller packets.
    • Reduces RTCP compound packet memory allocations since they are used much less now.

NOTE: Please read the changes commit by commit.

Each RR packet can hold up to 31 reports. Do serialize multiple packets
if the number of reports exceed 31.
Each Sdes packet can hold up to 31 chunks. Do serialize multiple packets
    if the number of reports exceed 31.
So a single serialized packet can be sent out instead of sending multiple
smaller packets.
The method gathers all needed RTCP itself.
Make sure CompoundPacket is filled before sending it.

* Reduces memory allocations for new CompoundPacket instances.
* Reduces the CPU and network overhead compared to sending smaller
  packets.
Set the interval to 1 second with a variation of [1..1.5].

Previously the interval was smaller as the send bandwidth increased
getting too small values for large meetings.
@jmillan jmillan requested a review from ibc September 28, 2022 14:06
@jmillan jmillan merged commit ab9cfb9 into v3 Oct 4, 2022
@jmillan jmillan deleted the rtcp-enhancements branch October 4, 2022 13:29
@ibc
Copy link
Member

ibc commented Oct 4, 2022

This was merged without changes in CHANGELOG :(

@jmillan
Copy link
Member Author

jmillan commented Oct 4, 2022

Changelog updated in the next commit.

Comment on lines +205 to +207
size_t size = (Packet::CommonHeaderSize + 4u /* this->ssrc */) *
((this->GetCount() / MaxReportsPerPacket) + 1);
size += ReceiverReport::HeaderSize * this->GetCount();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain in detail this math?

Copy link
Member Author

@jmillan jmillan Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Receiver Report packet is composed by:

  • RTCP common header (Packet::CommonHeaderSize).
  • SSRC of the packet sender. (4 bytes)
  • Up to 31 reports. (Each report size is ReceiverReport::HeaderSize )

This method returns the size needed to represent all it's content.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Why is (commonHeader size + 4) being multiplied by ((number of reports divided by max reports per packet) + 1)?

Copy link
Member Author

@jmillan jmillan Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are calculating there how many receiver report packets (Common header + SSRC) we need, considering our packet instance packet can hold more than 31 receiver reports and the the maximum reports a serialized receiver report packet can contain is 31 (limit given by the 'count' header field in the RTCP common header).

piranna pushed a commit to dyte-in/mediasoup that referenced this pull request Feb 9, 2023
* RTCP: ReceiverReportPacket, fix serization when report number > 31

Each RR packet can hold up to 31 reports. Do serialize multiple packets
if the number of reports exceed 31.

* RTCP: SdesPacket, fix serization when chunk number > 31

Each Sdes packet can hold up to 31 chunks. Do serialize multiple packets
    if the number of reports exceed 31.

* RTCP: SenderReportPacket, serialize multiple SR packets at once

So a single serialized packet can be sent out instead of sending multiple
smaller packets.

* Consumer: GetRtcp() do not indicate a specific RtpStream

The method gathers all needed RTCP itself.

* RTCP: CompoundPacket, fill it up to the MTU before sending

Make sure CompoundPacket is filled before sending it.

* Reduces memory allocations for new CompoundPacket instances.
* Reduces the CPU and network overhead compared to sending smaller
  packets.

* RTCP: CompoundPacket, serialize it in the corresponding sending method

* Transport: enhance RTCP send interval

Set the interval to 1 second with a variation of [1..1.5].

Previously the interval was smaller as the send bandwidth increased
getting too small values for large meetings.

* RTCP: CompoundPacket, avoid creating unneeded std::vector instances

* RTCP: Sdes, fix typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

An overflow and crash risk in RTC::Transport::SendRtcp
2 participants