From e952f34fea8489fa26ef30d29cb87118c5e6a707 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Wed, 22 Nov 2023 18:28:14 +0100 Subject: [PATCH] RTCP `CompoundPacket`: Use a single DLRR block to hold all ssrc infos (#1237) --- CHANGELOG.md | 1 + worker/include/RTC/RTCP/CompoundPacket.hpp | 17 ++++-- .../include/RTC/RTCP/XrDelaySinceLastRr.hpp | 13 +++++ worker/include/RTC/RtpStreamSend.hpp | 2 +- worker/src/RTC/PipeConsumer.cpp | 13 ++--- worker/src/RTC/RTCP/CompoundPacket.cpp | 54 ++++++++++--------- worker/src/RTC/RtpStreamSend.cpp | 2 +- worker/src/RTC/SimpleConsumer.cpp | 12 +---- worker/src/RTC/SimulcastConsumer.cpp | 12 +---- worker/src/RTC/SvcConsumer.cpp | 12 +---- 10 files changed, 68 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b49660845b..2317eb66b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### NEXT * Fix RTCP SDES packet size calculation ([PR #1236](https://github.com/versatica/mediasoup/pull/1236) based on PR [PR #1234](https://github.com/versatica/mediasoup/pull/1234) by @ybybwdwd). +* RTCP Compound Packet: Use a single DLRR report to hold all ssrc info sub-blocks ([PR #1237](https://github.com/versatica/mediasoup/pull/1237)). ### 3.13.4 diff --git a/worker/include/RTC/RTCP/CompoundPacket.hpp b/worker/include/RTC/RTCP/CompoundPacket.hpp index 3e377d394a..228ba06d73 100644 --- a/worker/include/RTC/RTCP/CompoundPacket.hpp +++ b/worker/include/RTC/RTCP/CompoundPacket.hpp @@ -48,14 +48,16 @@ namespace RTC // Adds the given data and returns true if there is enough space to hold it, // false otherwise. bool Add( - SenderReport* senderReport, SdesChunk* sdesChunk, DelaySinceLastRr* delaySinceLastRrReport); + SenderReport* senderReport, + SdesChunk* sdesChunk, + DelaySinceLastRr::SsrcInfo* delaySinceLastRrSsrcInfo); // RTCP additions per Consumer (pipe). // Adds the given data and returns true if there is enough space to hold it, // false otherwise. bool Add( std::vector& senderReports, std::vector& sdesChunks, - std::vector& delaySinceLastRrReports); + std::vector& delaySinceLastRrSsrcInfos); // RTCP additions per Producer. // Adds the given data and returns true if there is enough space to hold it, // false otherwise. @@ -63,8 +65,6 @@ namespace RTC void AddSenderReport(SenderReport* report); void AddReceiverReport(ReceiverReport* report); void AddSdesChunk(SdesChunk* chunk); - void AddReceiverReferenceTime(ReceiverReferenceTime* report); - void AddDelaySinceLastRr(DelaySinceLastRr* report); bool HasSenderReport() { return this->senderReportPacket.Begin() != this->senderReportPacket.End(); @@ -77,6 +77,14 @@ namespace RTC [](const ExtendedReportBlock* report) { return report->GetType() == ExtendedReportBlock::Type::RRT; }); } + bool HasDelaySinceLastRr() + { + return std::any_of( + this->xrPacket.Begin(), + this->xrPacket.End(), + [](const ExtendedReportBlock* report) + { return report->GetType() == ExtendedReportBlock::Type::DLRR; }); + } void Serialize(uint8_t* data); private: @@ -85,6 +93,7 @@ namespace RTC ReceiverReportPacket receiverReportPacket; SdesPacket sdesPacket; ExtendedReportPacket xrPacket; + DelaySinceLastRr* delaySinceLastRr{ nullptr }; }; } // namespace RTCP } // namespace RTC diff --git a/worker/include/RTC/RTCP/XrDelaySinceLastRr.hpp b/worker/include/RTC/RTCP/XrDelaySinceLastRr.hpp index b2a7ec2c88..fa77eff277 100644 --- a/worker/include/RTC/RTCP/XrDelaySinceLastRr.hpp +++ b/worker/include/RTC/RTCP/XrDelaySinceLastRr.hpp @@ -119,6 +119,19 @@ namespace RTC { this->ssrcInfos.push_back(ssrcInfo); } + // NOTE: This method not only removes given number of ssrc info sub-blocks + // but also deletes their SsrcInfo instances. + void RemoveLastSsrcInfos(size_t number) + { + while (!this->ssrcInfos.empty() && number-- > 0) + { + auto* ssrcInfo = this->ssrcInfos.back(); + + this->ssrcInfos.pop_back(); + + delete ssrcInfo; + } + } Iterator Begin() { return this->ssrcInfos.begin(); diff --git a/worker/include/RTC/RtpStreamSend.hpp b/worker/include/RTC/RtpStreamSend.hpp index f98d399aca..a5fe733b84 100644 --- a/worker/include/RTC/RtpStreamSend.hpp +++ b/worker/include/RTC/RtpStreamSend.hpp @@ -37,7 +37,7 @@ namespace RTC void ReceiveRtcpReceiverReport(RTC::RTCP::ReceiverReport* report); void ReceiveRtcpXrReceiverReferenceTime(RTC::RTCP::ReceiverReferenceTime* report); RTC::RTCP::SenderReport* GetRtcpSenderReport(uint64_t nowMs); - RTC::RTCP::DelaySinceLastRr::SsrcInfo* GetRtcpXrDelaySinceLastRr(uint64_t nowMs); + RTC::RTCP::DelaySinceLastRr::SsrcInfo* GetRtcpXrDelaySinceLastRrSsrcInfo(uint64_t nowMs); RTC::RTCP::SdesChunk* GetRtcpSdesChunk(); void Pause() override; void Resume() override; diff --git a/worker/src/RTC/PipeConsumer.cpp b/worker/src/RTC/PipeConsumer.cpp index d80cba0ab8..5ac3f65c09 100644 --- a/worker/src/RTC/PipeConsumer.cpp +++ b/worker/src/RTC/PipeConsumer.cpp @@ -345,7 +345,7 @@ namespace RTC std::vector senderReports; std::vector sdesChunks; - std::vector xrReports; + std::vector delaySinceLastRrSsrcInfos; for (auto* rtpStream : this->rtpStreams) { @@ -362,19 +362,16 @@ namespace RTC auto* sdesChunk = rtpStream->GetRtcpSdesChunk(); sdesChunks.push_back(sdesChunk); - auto* dlrr = rtpStream->GetRtcpXrDelaySinceLastRr(nowMs); + auto* delaySinceLastRrSsrcInfo = rtpStream->GetRtcpXrDelaySinceLastRrSsrcInfo(nowMs); - if (dlrr) + if (delaySinceLastRrSsrcInfo) { - auto* report = new RTC::RTCP::DelaySinceLastRr(); - report->AddSsrcInfo(dlrr); - - xrReports.push_back(report); + delaySinceLastRrSsrcInfos.push_back(delaySinceLastRrSsrcInfo); } } // RTCP Compound packet buffer cannot hold the data. - if (!packet->Add(senderReports, sdesChunks, xrReports)) + if (!packet->Add(senderReports, sdesChunks, delaySinceLastRrSsrcInfos)) { return false; } diff --git a/worker/src/RTC/RTCP/CompoundPacket.cpp b/worker/src/RTC/RTCP/CompoundPacket.cpp index c1809e3d11..4429f4d2f6 100644 --- a/worker/src/RTC/RTCP/CompoundPacket.cpp +++ b/worker/src/RTC/RTCP/CompoundPacket.cpp @@ -72,7 +72,9 @@ namespace RTC } bool CompoundPacket::Add( - SenderReport* senderReport, SdesChunk* sdesChunk, DelaySinceLastRr* delaySinceLastRrReport) + SenderReport* senderReport, + SdesChunk* sdesChunk, + DelaySinceLastRr::SsrcInfo* delaySinceLastRrSsrcInfo) { // Add the items into the packet. @@ -86,9 +88,16 @@ namespace RTC this->sdesPacket.AddChunk(sdesChunk); } - if (delaySinceLastRrReport) + if (delaySinceLastRrSsrcInfo) { - this->xrPacket.AddReport(delaySinceLastRrReport); + // Add a DLRR block into the XR packet if no present. + if (!this->delaySinceLastRr) + { + this->delaySinceLastRr = new RTC::RTCP::DelaySinceLastRr(); + this->xrPacket.AddReport(this->delaySinceLastRr); + } + + this->delaySinceLastRr->AddSsrcInfo(delaySinceLastRrSsrcInfo); } // New items can hold in the packet, report it. @@ -112,10 +121,10 @@ namespace RTC delete sdesChunk; } - if (delaySinceLastRrReport) + if (delaySinceLastRrSsrcInfo) { - this->xrPacket.RemoveReport(delaySinceLastRrReport); - delete delaySinceLastRrReport; + // NOTE: This method deletes the removed instances in place. + this->delaySinceLastRr->RemoveLastSsrcInfos(1); } return false; @@ -124,7 +133,7 @@ namespace RTC bool CompoundPacket::Add( std::vector& senderReports, std::vector& sdesChunks, - std::vector& delaySinceLastRrReports) + std::vector& delaySinceLastRrSsrcInfos) { // Add the items into the packet. @@ -138,9 +147,16 @@ namespace RTC this->sdesPacket.AddChunk(chunk); } - for (auto* report : delaySinceLastRrReports) + // Add a DLRR block into the XR packet if no present. + if (!delaySinceLastRrSsrcInfos.empty() && !this->delaySinceLastRr) { - this->xrPacket.AddReport(report); + this->delaySinceLastRr = new RTC::RTCP::DelaySinceLastRr(); + this->xrPacket.AddReport(this->delaySinceLastRr); + } + + for (auto* ssrcInfo : delaySinceLastRrSsrcInfos) + { + this->delaySinceLastRr->AddSsrcInfo(ssrcInfo); } // New items can hold in the packet, report it. @@ -164,10 +180,10 @@ namespace RTC delete chunk; } - for (auto* report : delaySinceLastRrReports) + if (!delaySinceLastRrSsrcInfos.empty()) { - this->xrPacket.RemoveReport(report); - delete report; + // NOTE: This method deletes the instances in place. + this->delaySinceLastRr->RemoveLastSsrcInfos(delaySinceLastRrSsrcInfos.size()); } return false; @@ -266,19 +282,5 @@ namespace RTC this->sdesPacket.AddChunk(chunk); } - - void CompoundPacket::AddReceiverReferenceTime(ReceiverReferenceTime* report) - { - MS_TRACE(); - - this->xrPacket.AddReport(report); - } - - void CompoundPacket::AddDelaySinceLastRr(DelaySinceLastRr* report) - { - MS_TRACE(); - - this->xrPacket.AddReport(report); - } } // namespace RTCP } // namespace RTC diff --git a/worker/src/RTC/RtpStreamSend.cpp b/worker/src/RTC/RtpStreamSend.cpp index a76ce3fce4..191e116236 100644 --- a/worker/src/RTC/RtpStreamSend.cpp +++ b/worker/src/RTC/RtpStreamSend.cpp @@ -274,7 +274,7 @@ namespace RTC return report; } - RTC::RTCP::DelaySinceLastRr::SsrcInfo* RtpStreamSend::GetRtcpXrDelaySinceLastRr(uint64_t nowMs) + RTC::RTCP::DelaySinceLastRr::SsrcInfo* RtpStreamSend::GetRtcpXrDelaySinceLastRrSsrcInfo(uint64_t nowMs) { MS_TRACE(); diff --git a/worker/src/RTC/SimpleConsumer.cpp b/worker/src/RTC/SimpleConsumer.cpp index f7d7a885b1..6488e776d2 100644 --- a/worker/src/RTC/SimpleConsumer.cpp +++ b/worker/src/RTC/SimpleConsumer.cpp @@ -439,18 +439,10 @@ namespace RTC // Build SDES chunk for this sender. auto* sdesChunk = this->rtpStream->GetRtcpSdesChunk(); - RTC::RTCP::DelaySinceLastRr* delaySinceLastRrReport{ nullptr }; - - auto* dlrr = this->rtpStream->GetRtcpXrDelaySinceLastRr(nowMs); - - if (dlrr) - { - delaySinceLastRrReport = new RTC::RTCP::DelaySinceLastRr(); - delaySinceLastRrReport->AddSsrcInfo(dlrr); - } + auto* delaySinceLastRrSsrcInfo = this->rtpStream->GetRtcpXrDelaySinceLastRrSsrcInfo(nowMs); // RTCP Compound packet buffer cannot hold the data. - if (!packet->Add(senderReport, sdesChunk, delaySinceLastRrReport)) + if (!packet->Add(senderReport, sdesChunk, delaySinceLastRrSsrcInfo)) { return false; } diff --git a/worker/src/RTC/SimulcastConsumer.cpp b/worker/src/RTC/SimulcastConsumer.cpp index de0f491904..cbafc1d05d 100644 --- a/worker/src/RTC/SimulcastConsumer.cpp +++ b/worker/src/RTC/SimulcastConsumer.cpp @@ -1081,18 +1081,10 @@ namespace RTC // Build SDES chunk for this sender. auto* sdesChunk = this->rtpStream->GetRtcpSdesChunk(); - RTC::RTCP::DelaySinceLastRr* delaySinceLastRrReport{ nullptr }; - - auto* dlrr = this->rtpStream->GetRtcpXrDelaySinceLastRr(nowMs); - - if (dlrr) - { - delaySinceLastRrReport = new RTC::RTCP::DelaySinceLastRr(); - delaySinceLastRrReport->AddSsrcInfo(dlrr); - } + auto* delaySinceLastRrSsrcInfo = this->rtpStream->GetRtcpXrDelaySinceLastRrSsrcInfo(nowMs); // RTCP Compound packet buffer cannot hold the data. - if (!packet->Add(senderReport, sdesChunk, delaySinceLastRrReport)) + if (!packet->Add(senderReport, sdesChunk, delaySinceLastRrSsrcInfo)) { return false; } diff --git a/worker/src/RTC/SvcConsumer.cpp b/worker/src/RTC/SvcConsumer.cpp index b02183f048..9e86e867b2 100644 --- a/worker/src/RTC/SvcConsumer.cpp +++ b/worker/src/RTC/SvcConsumer.cpp @@ -795,18 +795,10 @@ namespace RTC // Build SDES chunk for this sender. auto* sdesChunk = this->rtpStream->GetRtcpSdesChunk(); - RTC::RTCP::DelaySinceLastRr* delaySinceLastRrReport{ nullptr }; - - auto* dlrr = this->rtpStream->GetRtcpXrDelaySinceLastRr(nowMs); - - if (dlrr) - { - delaySinceLastRrReport = new RTC::RTCP::DelaySinceLastRr(); - delaySinceLastRrReport->AddSsrcInfo(dlrr); - } + auto* delaySinceLastRrSsrcInfo = this->rtpStream->GetRtcpXrDelaySinceLastRrSsrcInfo(nowMs); // RTCP Compound packet buffer cannot hold the data. - if (!packet->Add(senderReport, sdesChunk, delaySinceLastRrReport)) + if (!packet->Add(senderReport, sdesChunk, delaySinceLastRrSsrcInfo)) { return false; }