admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Updates rtcp::CompoundPacket to contain unique pointers to packets.

Browse Source 
Currently test code passes pointer to temporary objects, while
RtcpSender passes raw pointers to objects that are then seen as owned,
and will be manually deleted by a overloaded destructor, which is scary
and fragile.

This CL moves all usage to std::unique_ptr<RtcpPacket> instead, which
may create some heap churn in unit tests but that should be fine.

Bug: webrtc:11925
Change-Id: I981bc7ccd6a74115c5a3de64b8427adbf3f16cc7
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/183920
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#32084}
This commit is contained in:
Erik Språng 2020-09-11 15:35:32 +02:00 committed by Commit Bot
parent 1b06876a52
commit de5507d31b
6 changed files with 123 additions and 109 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
modules/rtp_rtcp/source/rtcp_packet/compound_packet.cc
View File

@ -10,6 +10,9 @@
#include "modules/rtp_rtcp/source/rtcp_packet/compound_packet.h"
#include <memory>
#include <utility>
#include "rtc_base/checks.h"
namespace webrtc {
@ -19,16 +22,16 @@ CompoundPacket::CompoundPacket() = default;
CompoundPacket::~CompoundPacket() = default;
void CompoundPacket::Append(RtcpPacket* packet) {
void CompoundPacket::Append(std::unique_ptr<RtcpPacket> packet) {
RTC_CHECK(packet);
appended_packets_.push_back(packet);
appended_packets_.push_back(std::move(packet));
}
bool CompoundPacket::Create(uint8_t* packet,
size_t* index,
size_t max_length,
PacketReadyCallback callback) const {
for (RtcpPacket* appended : appended_packets_) {
for (const auto& appended : appended_packets_) {
if (!appended->Create(packet, index, max_length, callback))
return false;
}
@ -37,7 +40,7 @@ bool CompoundPacket::Create(uint8_t* packet,
size_t CompoundPacket::BlockLength() const {
size_t block_length = 0;
for (RtcpPacket* appended : appended_packets_) {
for (const auto& appended : appended_packets_) {
block_length += appended->BlockLength();
}
return block_length;

12
modules/rtp_rtcp/source/rtcp_packet/compound_packet.h
View File

@ -12,6 +12,7 @@
#ifndef MODULES_RTP_RTCP_SOURCE_RTCP_PACKET_COMPOUND_PACKET_H_
#define MODULES_RTP_RTCP_SOURCE_RTCP_PACKET_COMPOUND_PACKET_H_
#include <memory>
#include <vector>
#include "modules/rtp_rtcp/source/rtcp_packet.h"
@ -25,7 +26,14 @@ class CompoundPacket : public RtcpPacket {
CompoundPacket();
~CompoundPacket() override;
void Append(RtcpPacket* packet);
void Append(std::unique_ptr<RtcpPacket> packet);
// Fallback for call-sites that have not yet migrated to passing a unique_ptr.
// TODO(bugs.webrtc.org/11925): Remove when all usage is gone.
template <typename T>
void Append(T* packet) {
Append(std::make_unique<T>(*packet));
}
// Size of this packet in bytes (i.e. total size of nested packets).
size_t BlockLength() const override;
@ -36,7 +44,7 @@ class CompoundPacket : public RtcpPacket {
PacketReadyCallback callback) const override;
protected:
std::vector<RtcpPacket*> appended_packets_;
std::vector<std::unique_ptr<RtcpPacket>> appended_packets_;
private:
RTC_DISALLOW_COPY_AND_ASSIGN(CompoundPacket);

70
modules/rtp_rtcp/source/rtcp_packet/compound_packet_unittest.cc
View File

@ -10,6 +10,9 @@
#include "modules/rtp_rtcp/source/rtcp_packet/compound_packet.h"
#include <memory>
#include <utility>
#include "modules/rtp_rtcp/source/rtcp_packet.h"
#include "modules/rtp_rtcp/source/rtcp_packet/bye.h"
#include "modules/rtp_rtcp/source/rtcp_packet/fir.h"
@ -38,14 +41,14 @@ const uint8_t kSeqNo = 13;
TEST(RtcpCompoundPacketTest, AppendPacket) {
CompoundPacket compound;
Fir fir;
fir.AddRequestTo(kRemoteSsrc, kSeqNo);
auto fir = std::make_unique<Fir>();
fir->AddRequestTo(kRemoteSsrc, kSeqNo);
ReportBlock rb;
ReceiverReport rr;
rr.SetSenderSsrc(kSenderSsrc);
EXPECT_TRUE(rr.AddReportBlock(rb));
compound.Append(&rr);
compound.Append(&fir);
auto rr = std::make_unique<ReceiverReport>();
rr->SetSenderSsrc(kSenderSsrc);
EXPECT_TRUE(rr->AddReportBlock(rb));
compound.Append(std::move(rr));
compound.Append(std::move(fir));
rtc::Buffer packet = compound.Build();
RtcpPacketParser parser;
@ -58,21 +61,22 @@ TEST(RtcpCompoundPacketTest, AppendPacket) {
TEST(RtcpCompoundPacketTest, AppendPacketWithOwnAppendedPacket) {
CompoundPacket root;
CompoundPacket leaf;
Fir fir;
fir.AddRequestTo(kRemoteSsrc, kSeqNo);
Bye bye;
auto leaf = std::make_unique<CompoundPacket>();
auto fir = std::make_unique<Fir>();
fir->AddRequestTo(kRemoteSsrc, kSeqNo);
auto bye = std::make_unique<Bye>();
ReportBlock rb;
ReceiverReport rr;
EXPECT_TRUE(rr.AddReportBlock(rb));
leaf.Append(&rr);
leaf.Append(&fir);
auto rr = std::make_unique<ReceiverReport>();
EXPECT_TRUE(rr->AddReportBlock(rb));
leaf->Append(std::move(rr));
leaf->Append(std::move(fir));
SenderReport sr;
root.Append(&sr);
root.Append(&bye);
root.Append(&leaf);
auto sr = std::make_unique<SenderReport>();
root.Append(std::move(sr));
root.Append(std::move(bye));
root.Append(std::move(leaf));
rtc::Buffer packet = root.Build();
RtcpPacketParser parser;
@ -86,14 +90,14 @@ TEST(RtcpCompoundPacketTest, AppendPacketWithOwnAppendedPacket) {
TEST(RtcpCompoundPacketTest, BuildWithInputBuffer) {
CompoundPacket compound;
Fir fir;
fir.AddRequestTo(kRemoteSsrc, kSeqNo);
auto fir = std::make_unique<Fir>();
fir->AddRequestTo(kRemoteSsrc, kSeqNo);
ReportBlock rb;
ReceiverReport rr;
rr.SetSenderSsrc(kSenderSsrc);
EXPECT_TRUE(rr.AddReportBlock(rb));
compound.Append(&rr);
compound.Append(&fir);
auto rr = std::make_unique<ReceiverReport>();
rr->SetSenderSsrc(kSenderSsrc);
EXPECT_TRUE(rr->AddReportBlock(rb));
compound.Append(std::move(rr));
compound.Append(std::move(fir));
const size_t kRrLength = 8;
const size_t kReportBlockLength = 24;
@ -115,14 +119,14 @@ TEST(RtcpCompoundPacketTest, BuildWithInputBuffer) {
TEST(RtcpCompoundPacketTest, BuildWithTooSmallBuffer_FragmentedSend) {
CompoundPacket compound;
Fir fir;
fir.AddRequestTo(kRemoteSsrc, kSeqNo);
auto fir = std::make_unique<Fir>();
fir->AddRequestTo(kRemoteSsrc, kSeqNo);
ReportBlock rb;
ReceiverReport rr;
rr.SetSenderSsrc(kSenderSsrc);
EXPECT_TRUE(rr.AddReportBlock(rb));
compound.Append(&rr);
compound.Append(&fir);
auto rr = std::make_unique<ReceiverReport>();
rr->SetSenderSsrc(kSenderSsrc);
EXPECT_TRUE(rr->AddReportBlock(rb));
compound.Append(std::move(rr));
compound.Append(std::move(fir));
const size_t kRrLength = 8;
const size_t kReportBlockLength = 24;

107
modules/rtp_rtcp/source/rtcp_receiver_unittest.cc
View File

@ -11,6 +11,7 @@
#include "modules/rtp_rtcp/source/rtcp_receiver.h"
#include <memory>
#include <utility>
#include "api/array_view.h"
#include "api/units/timestamp.h"
@ -1200,14 +1201,14 @@ TEST(RtcpReceiverTest, TmmbrPacketAccepted) {
receiver.SetRemoteSSRC(kSenderSsrc);
const uint32_t kBitrateBps = 30000;
rtcp::Tmmbr tmmbr;
tmmbr.SetSenderSsrc(kSenderSsrc);
tmmbr.AddTmmbr(rtcp::TmmbItem(kReceiverMainSsrc, kBitrateBps, 0));
rtcp::SenderReport sr;
sr.SetSenderSsrc(kSenderSsrc);
auto tmmbr = std::make_unique<rtcp::Tmmbr>();
tmmbr->SetSenderSsrc(kSenderSsrc);
tmmbr->AddTmmbr(rtcp::TmmbItem(kReceiverMainSsrc, kBitrateBps, 0));
auto sr = std::make_unique<rtcp::SenderReport>();
sr->SetSenderSsrc(kSenderSsrc);
rtcp::CompoundPacket compound;
compound.Append(&sr);
compound.Append(&tmmbr);
compound.Append(std::move(sr));
compound.Append(std::move(tmmbr));
EXPECT_CALL(mocks.rtp_rtcp_impl, OnReceivedRtcpReportBlocks);
EXPECT_CALL(mocks.rtp_rtcp_impl, SetTmmbn(SizeIs(1)));
@ -1228,15 +1229,15 @@ TEST(RtcpReceiverTest, TmmbrPacketNotForUsIgnored) {
receiver.SetRemoteSSRC(kSenderSsrc);
const uint32_t kBitrateBps = 30000;
rtcp::Tmmbr tmmbr;
tmmbr.SetSenderSsrc(kSenderSsrc);
tmmbr.AddTmmbr(rtcp::TmmbItem(kNotToUsSsrc, kBitrateBps, 0));
auto tmmbr = std::make_unique<rtcp::Tmmbr>();
tmmbr->SetSenderSsrc(kSenderSsrc);
tmmbr->AddTmmbr(rtcp::TmmbItem(kNotToUsSsrc, kBitrateBps, 0));
rtcp::SenderReport sr;
sr.SetSenderSsrc(kSenderSsrc);
auto sr = std::make_unique<rtcp::SenderReport>();
sr->SetSenderSsrc(kSenderSsrc);
rtcp::CompoundPacket compound;
compound.Append(&sr);
compound.Append(&tmmbr);
compound.Append(std::move(sr));
compound.Append(std::move(tmmbr));
EXPECT_CALL(mocks.rtp_rtcp_impl, OnReceivedRtcpReportBlocks);
EXPECT_CALL(mocks.bandwidth_observer, OnReceivedRtcpReceiverReport);
@ -1251,14 +1252,14 @@ TEST(RtcpReceiverTest, TmmbrPacketZeroRateIgnored) {
RTCPReceiver receiver(DefaultConfiguration(&mocks), &mocks.rtp_rtcp_impl);
receiver.SetRemoteSSRC(kSenderSsrc);
rtcp::Tmmbr tmmbr;
tmmbr.SetSenderSsrc(kSenderSsrc);
tmmbr.AddTmmbr(rtcp::TmmbItem(kReceiverMainSsrc, 0, 0));
rtcp::SenderReport sr;
sr.SetSenderSsrc(kSenderSsrc);
auto tmmbr = std::make_unique<rtcp::Tmmbr>();
tmmbr->SetSenderSsrc(kSenderSsrc);
tmmbr->AddTmmbr(rtcp::TmmbItem(kReceiverMainSsrc, 0, 0));
auto sr = std::make_unique<rtcp::SenderReport>();
sr->SetSenderSsrc(kSenderSsrc);
rtcp::CompoundPacket compound;
compound.Append(&sr);
compound.Append(&tmmbr);
compound.Append(std::move(sr));
compound.Append(std::move(tmmbr));
EXPECT_CALL(mocks.rtp_rtcp_impl, OnReceivedRtcpReportBlocks);
EXPECT_CALL(mocks.bandwidth_observer, OnReceivedRtcpReceiverReport);
@ -1276,14 +1277,14 @@ TEST(RtcpReceiverTest, TmmbrThreeConstraintsTimeOut) {
// Inject 3 packets "from" kSenderSsrc, kSenderSsrc+1, kSenderSsrc+2.
// The times of arrival are starttime + 0, starttime + 5 and starttime + 10.
for (uint32_t ssrc = kSenderSsrc; ssrc < kSenderSsrc + 3; ++ssrc) {
rtcp::Tmmbr tmmbr;
tmmbr.SetSenderSsrc(ssrc);
tmmbr.AddTmmbr(rtcp::TmmbItem(kReceiverMainSsrc, 30000, 0));
rtcp::SenderReport sr;
sr.SetSenderSsrc(ssrc);
auto tmmbr = std::make_unique<rtcp::Tmmbr>();
tmmbr->SetSenderSsrc(ssrc);
tmmbr->AddTmmbr(rtcp::TmmbItem(kReceiverMainSsrc, 30000, 0));
auto sr = std::make_unique<rtcp::SenderReport>();
sr->SetSenderSsrc(ssrc);
rtcp::CompoundPacket compound;
compound.Append(&sr);
compound.Append(&tmmbr);
compound.Append(std::move(sr));
compound.Append(std::move(tmmbr));
EXPECT_CALL(mocks.rtp_rtcp_impl, OnReceivedRtcpReportBlocks);
EXPECT_CALL(mocks.rtp_rtcp_impl, SetTmmbn);
@ -1598,19 +1599,19 @@ TEST(RtcpReceiverTest, HandlesInvalidTransportFeedback) {
receiver.SetRemoteSSRC(kSenderSsrc);
// Send a compound packet with a TransportFeedback followed by something else.
rtcp::TransportFeedback packet;
packet.SetMediaSsrc(kReceiverMainSsrc);
packet.SetSenderSsrc(kSenderSsrc);
packet.SetBase(1, 1000);
packet.AddReceivedPacket(1, 1000);
auto packet = std::make_unique<rtcp::TransportFeedback>();
packet->SetMediaSsrc(kReceiverMainSsrc);
packet->SetSenderSsrc(kSenderSsrc);
packet->SetBase(1, 1000);
packet->AddReceivedPacket(1, 1000);
static uint32_t kBitrateBps = 50000;
rtcp::Remb remb;
remb.SetSenderSsrc(kSenderSsrc);
remb.SetBitrateBps(kBitrateBps);
auto remb = std::make_unique<rtcp::Remb>();
remb->SetSenderSsrc(kSenderSsrc);
remb->SetBitrateBps(kBitrateBps);
rtcp::CompoundPacket compound;
compound.Append(&packet);
compound.Append(&remb);
compound.Append(std::move(packet));
compound.Append(std::move(remb));
rtc::Buffer built_packet = compound.Build();
// Modify the TransportFeedback packet so that it is invalid.
@ -1639,10 +1640,10 @@ TEST(RtcpReceiverTest, Nack) {
nack_set.insert(std::begin(kNackList1), std::end(kNackList1));
nack_set.insert(std::begin(kNackList23), std::end(kNackList23));
rtcp::Nack nack1;
nack1.SetSenderSsrc(kSenderSsrc);
nack1.SetMediaSsrc(kReceiverMainSsrc);
nack1.SetPacketIds(kNackList1, arraysize(kNackList1));
auto nack1 = std::make_unique<rtcp::Nack>();
nack1->SetSenderSsrc(kSenderSsrc);
nack1->SetMediaSsrc(kReceiverMainSsrc);
nack1->SetPacketIds(kNackList1, arraysize(kNackList1));
EXPECT_CALL(mocks.rtp_rtcp_impl,
OnReceivedNack(ElementsAreArray(kNackList1)));
@ -1653,21 +1654,21 @@ TEST(RtcpReceiverTest, Nack) {
arraysize(kNackList1)),
Field(&RtcpPacketTypeCounter::unique_nack_requests,
arraysize(kNackList1)))));
receiver.IncomingPacket(nack1.Build());
receiver.IncomingPacket(nack1->Build());
rtcp::Nack nack2;
nack2.SetSenderSsrc(kSenderSsrc);
nack2.SetMediaSsrc(kReceiverMainSsrc);
nack2.SetPacketIds(kNackList23, kNackListLength2);
auto nack2 = std::make_unique<rtcp::Nack>();
nack2->SetSenderSsrc(kSenderSsrc);
nack2->SetMediaSsrc(kReceiverMainSsrc);
nack2->SetPacketIds(kNackList23, kNackListLength2);
rtcp::Nack nack3;
nack3.SetSenderSsrc(kSenderSsrc);
nack3.SetMediaSsrc(kReceiverMainSsrc);
nack3.SetPacketIds(kNackList23 + kNackListLength2, kNackListLength3);
auto nack3 = std::make_unique<rtcp::Nack>();
nack3->SetSenderSsrc(kSenderSsrc);
nack3->SetMediaSsrc(kReceiverMainSsrc);
nack3->SetPacketIds(kNackList23 + kNackListLength2, kNackListLength3);
rtcp::CompoundPacket two_nacks;
two_nacks.Append(&nack2);
two_nacks.Append(&nack3);
two_nacks.Append(std::move(nack2));
two_nacks.Append(std::move(nack3));
EXPECT_CALL(mocks.rtp_rtcp_impl,
OnReceivedNack(ElementsAreArray(kNackList23)));

8
modules/rtp_rtcp/source/rtcp_sender.cc
View File

@ -55,10 +55,6 @@ class PacketContainer : public rtcp::CompoundPacket {
public:
PacketContainer(Transport* transport, RtcEventLog* event_log)
: transport_(transport), event_log_(event_log) {}
~PacketContainer() override {
for (RtcpPacket* packet : appended_packets_)
delete packet;
}
size_t SendPackets(size_t max_payload_length) {
size_t bytes_sent = 0;
@ -792,14 +788,14 @@ absl::optional<int32_t> RTCPSender::ComputeCompoundRTCPPacket(
if (builder_it->first == kRtcpBye) {
packet_bye = std::move(packet);
} else {
out_packet->Append(packet.release());
out_packet->Append(std::move(packet));
}
}
}
// Append the BYE now at the end
if (packet_bye) {
out_packet->Append(packet_bye.release());
out_packet->Append(std::move(packet_bye));
}
if (packet_type_counter_observer_ != nullptr) {

24
modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc
View File

@ -10,6 +10,8 @@
#include "modules/rtp_rtcp/source/rtcp_transceiver_impl.h"
#include <memory>
#include <utility>
#include <vector>
#include "absl/memory/memory.h"
@ -677,12 +679,12 @@ TEST(RtcpTransceiverImplTest, CallsObserverOnByeBehindSenderReport) {
rtcp_transceiver.AddMediaReceiverRtcpObserver(kRemoteSsrc, &observer);
CompoundPacket compound;
SenderReport sr;
sr.SetSenderSsrc(kRemoteSsrc);
compound.Append(&sr);
Bye bye;
bye.SetSenderSsrc(kRemoteSsrc);
compound.Append(&bye);
auto sr = std::make_unique<SenderReport>();
sr->SetSenderSsrc(kRemoteSsrc);
compound.Append(std::move(sr));
auto bye = std::make_unique<Bye>();
bye->SetSenderSsrc(kRemoteSsrc);
compound.Append(std::move(bye));
auto raw_packet = compound.Build();
EXPECT_CALL(observer, OnBye(kRemoteSsrc));
@ -698,11 +700,11 @@ TEST(RtcpTransceiverImplTest, CallsObserverOnByeBehindUnknownRtcpPacket) {
CompoundPacket compound;
// Use Application-Defined rtcp packet as unknown.
webrtc::rtcp::App app;
compound.Append(&app);
Bye bye;
bye.SetSenderSsrc(kRemoteSsrc);
compound.Append(&bye);
auto app = std::make_unique<webrtc::rtcp::App>();
compound.Append(std::move(app));
auto bye = std::make_unique<Bye>();
bye->SetSenderSsrc(kRemoteSsrc);
compound.Append(std::move(bye));
auto raw_packet = compound.Build();
EXPECT_CALL(observer, OnBye(kRemoteSsrc));