From 80ac24dd36b2df1c0faee5ae490b9a74db9c7464 Mon Sep 17 00:00:00 2001 From: danilchap Date: Mon, 31 Oct 2016 08:40:47 -0700 Subject: [PATCH] Allow max 1 block per type in RTCP Extended Reports Design of individual block in ExtendedReports packet suggest there is no point to have more than one block per type. This CL reduce complexity of having several blocks of the same type in same report. BUG=webrtc:5260 Review-Url: https://codereview.webrtc.org/2378113002 Cr-Commit-Position: refs/heads/master@{#14855} --- .../rtp_rtcp/source/rtcp_packet/dlrr.cc | 18 --- .../rtp_rtcp/source/rtcp_packet/dlrr.h | 11 +- .../source/rtcp_packet/dlrr_unittest.cc | 20 +-- .../source/rtcp_packet/extended_reports.cc | 83 +++++------ .../source/rtcp_packet/extended_reports.h | 31 ++-- .../rtcp_packet/extended_reports_unittest.cc | 140 ++++-------------- .../modules/rtp_rtcp/source/rtcp_receiver.cc | 10 +- .../rtp_rtcp/source/rtcp_receiver_unittest.cc | 59 +++----- webrtc/modules/rtp_rtcp/source/rtcp_sender.cc | 11 +- .../rtp_rtcp/source/rtcp_sender_unittest.cc | 64 ++++---- webrtc/test/rtcp_packet_parser.h | 3 - 11 files changed, 154 insertions(+), 296 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/dlrr.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/dlrr.cc index 97516f1468..cc44886966 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/dlrr.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/dlrr.cc @@ -81,23 +81,5 @@ void Dlrr::Create(uint8_t* buffer) const { RTC_DCHECK_EQ(buffer + BlockLength(), write_at); } -bool Dlrr::AddDlrrItem(const ReceiveTimeInfo& block) { - if (sub_blocks_.size() >= kMaxNumberOfDlrrItems) { - LOG(LS_WARNING) << "Max DLRR items reached."; - return false; - } - sub_blocks_.push_back(block); - return true; -} - -bool Dlrr::AddDlrrItem(uint32_t ssrc, - uint32_t last_rr, - uint32_t delay_last_rr) { - ReceiveTimeInfo block; - block.ssrc = ssrc; - block.last_rr = last_rr; - block.delay_since_last_rr = delay_last_rr; - return AddDlrrItem(block); -} } // namespace rtcp } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/dlrr.h b/webrtc/modules/rtp_rtcp/source/rtcp_packet/dlrr.h index c2a4b8fffc..c5f5c23bf1 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/dlrr.h +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/dlrr.h @@ -32,7 +32,6 @@ struct ReceiveTimeInfo { class Dlrr { public: static const uint8_t kBlockType = 5; - static const size_t kMaxNumberOfDlrrItems = 100; Dlrr() {} Dlrr(const Dlrr& other) = default; @@ -40,6 +39,9 @@ class Dlrr { Dlrr& operator=(const Dlrr& other) = default; + // Dlrr without items treated same as no dlrr block. + explicit operator bool() const { return !sub_blocks_.empty(); } + // Second parameter is value read from block header, // i.e. size of block in 32bits excluding block header itself. bool Parse(const uint8_t* buffer, uint16_t block_length_32bits); @@ -49,9 +51,10 @@ class Dlrr { // Consumes BlockLength() bytes. void Create(uint8_t* buffer) const; - // Max 100 DLRR Items can be added per DLRR report block. - bool AddDlrrItem(const ReceiveTimeInfo& time_info); - bool AddDlrrItem(uint32_t ssrc, uint32_t last_rr, uint32_t delay_last_rr); + void ClearItems() { sub_blocks_.clear(); } + void AddDlrrItem(const ReceiveTimeInfo& time_info) { + sub_blocks_.push_back(time_info); + } const std::vector& sub_blocks() const { return sub_blocks_; } diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/dlrr_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/dlrr_unittest.cc index 161974da9e..6144ab7f36 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/dlrr_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/dlrr_unittest.cc @@ -34,7 +34,7 @@ TEST(RtcpPacketDlrrTest, Empty) { TEST(RtcpPacketDlrrTest, Create) { Dlrr dlrr; - EXPECT_TRUE(dlrr.AddDlrrItem(kSsrc, kLastRR, kDelay)); + dlrr.AddDlrrItem(ReceiveTimeInfo(kSsrc, kLastRR, kDelay)); ASSERT_EQ(kBlockSizeBytes, dlrr.BlockLength()); uint8_t buffer[kBlockSizeBytes]; @@ -69,23 +69,15 @@ TEST(RtcpPacketDlrrTest, ParseFailsOnBadSize) { } } -TEST(RtcpPacketDlrrTest, FailsOnTooManySubBlocks) { - Dlrr dlrr; - for (size_t i = 1; i <= Dlrr::kMaxNumberOfDlrrItems; ++i) { - EXPECT_TRUE(dlrr.AddDlrrItem(kSsrc + i, kLastRR + i, kDelay + i)); - } - EXPECT_FALSE(dlrr.AddDlrrItem(kSsrc, kLastRR, kDelay)); -} - -TEST(RtcpPacketDlrrTest, CreateAndParseMaxSubBlocks) { +TEST(RtcpPacketDlrrTest, CreateAndParseManySubBlocks) { const size_t kBufferSize = 0x1000; // More than enough. + const size_t kManyDlrrItems = 50; uint8_t buffer[kBufferSize]; // Create. Dlrr dlrr; - for (size_t i = 1; i <= Dlrr::kMaxNumberOfDlrrItems; ++i) { - EXPECT_TRUE(dlrr.AddDlrrItem(kSsrc + i, kLastRR + i, kDelay + i)); - } + for (size_t i = 1; i <= kManyDlrrItems; ++i) + dlrr.AddDlrrItem(ReceiveTimeInfo(kSsrc + i, kLastRR + i, kDelay + i)); size_t used_buffer_size = dlrr.BlockLength(); ASSERT_LE(used_buffer_size, kBufferSize); dlrr.Create(buffer); @@ -95,6 +87,6 @@ TEST(RtcpPacketDlrrTest, CreateAndParseMaxSubBlocks) { uint16_t block_length = ByteReader::ReadBigEndian(&buffer[2]); EXPECT_EQ(used_buffer_size, (block_length + 1) * 4u); EXPECT_TRUE(parsed.Parse(buffer, block_length)); - EXPECT_TRUE(parsed.sub_blocks().size() == Dlrr::kMaxNumberOfDlrrItems); + EXPECT_EQ(kManyDlrrItems, parsed.sub_blocks().size()); } } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports.cc index 0199b5276a..5775dd4ffc 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports.cc @@ -52,9 +52,9 @@ bool ExtendedReports::Parse(const CommonHeader& packet) { } sender_ssrc_ = ByteReader::ReadBigEndian(packet.payload()); - rrtr_blocks_.clear(); - dlrr_blocks_.clear(); - voip_metric_blocks_.clear(); + rrtr_block_.reset(); + dlrr_block_.ClearItems(); + voip_metric_block_.reset(); const uint8_t* current_block = packet.payload() + kXrBaseLength; const uint8_t* const packet_end = @@ -91,31 +91,20 @@ bool ExtendedReports::Parse(const CommonHeader& packet) { return true; } -bool ExtendedReports::AddRrtr(const Rrtr& rrtr) { - if (rrtr_blocks_.size() >= kMaxNumberOfRrtrBlocks) { - LOG(LS_WARNING) << "Max RRTR blocks reached."; - return false; - } - rrtr_blocks_.push_back(rrtr); - return true; +void ExtendedReports::SetRrtr(const Rrtr& rrtr) { + if (rrtr_block_) + LOG(LS_WARNING) << "Rrtr already set, overwriting."; + rrtr_block_.emplace(rrtr); } -bool ExtendedReports::AddDlrr(const Dlrr& dlrr) { - if (dlrr_blocks_.size() >= kMaxNumberOfDlrrBlocks) { - LOG(LS_WARNING) << "Max DLRR blocks reached."; - return false; - } - dlrr_blocks_.push_back(dlrr); - return true; +void ExtendedReports::AddDlrrItem(const ReceiveTimeInfo& time_info) { + dlrr_block_.AddDlrrItem(time_info); } -bool ExtendedReports::AddVoipMetric(const VoipMetric& voip_metric) { - if (voip_metric_blocks_.size() >= kMaxNumberOfVoipMetricBlocks) { - LOG(LS_WARNING) << "Max Voip Metric blocks reached."; - return false; - } - voip_metric_blocks_.push_back(voip_metric); - return true; +void ExtendedReports::SetVoipMetric(const VoipMetric& voip_metric) { + if (voip_metric_block_) + LOG(LS_WARNING) << "Voip metric already set, overwriting."; + voip_metric_block_.emplace(voip_metric); } bool ExtendedReports::Create(uint8_t* packet, @@ -131,30 +120,22 @@ bool ExtendedReports::Create(uint8_t* packet, CreateHeader(kReserved, kPacketType, HeaderLength(), packet, index); ByteWriter::WriteBigEndian(packet + *index, sender_ssrc_); *index += sizeof(uint32_t); - for (const Rrtr& block : rrtr_blocks_) { - block.Create(packet + *index); + if (rrtr_block_) { + rrtr_block_->Create(packet + *index); *index += Rrtr::kLength; } - for (const Dlrr& block : dlrr_blocks_) { - block.Create(packet + *index); - *index += block.BlockLength(); + if (dlrr_block_) { + dlrr_block_.Create(packet + *index); + *index += dlrr_block_.BlockLength(); } - for (const VoipMetric& block : voip_metric_blocks_) { - block.Create(packet + *index); + if (voip_metric_block_) { + voip_metric_block_->Create(packet + *index); *index += VoipMetric::kLength; } RTC_CHECK_EQ(*index, index_end); return true; } -size_t ExtendedReports::DlrrLength() const { - size_t length = 0; - for (const Dlrr& block : dlrr_blocks_) { - length += block.BlockLength(); - } - return length; -} - void ExtendedReports::ParseRrtrBlock(const uint8_t* block, uint16_t block_length) { if (block_length != Rrtr::kBlockLength) { @@ -162,16 +143,21 @@ void ExtendedReports::ParseRrtrBlock(const uint8_t* block, << " Should be " << Rrtr::kBlockLength; return; } - rrtr_blocks_.push_back(Rrtr()); - rrtr_blocks_.back().Parse(block); + if (rrtr_block_) { + LOG(LS_WARNING) << "Two rrtr blocks found in same Extended Report packet"; + return; + } + rrtr_block_.emplace(); + rrtr_block_->Parse(block); } void ExtendedReports::ParseDlrrBlock(const uint8_t* block, uint16_t block_length) { - dlrr_blocks_.push_back(Dlrr()); - if (!dlrr_blocks_.back().Parse(block, block_length)) { - dlrr_blocks_.pop_back(); + if (dlrr_block_) { + LOG(LS_WARNING) << "Two Dlrr blocks found in same Extended Report packet"; + return; } + dlrr_block_.Parse(block, block_length); } void ExtendedReports::ParseVoipMetricBlock(const uint8_t* block, @@ -181,8 +167,13 @@ void ExtendedReports::ParseVoipMetricBlock(const uint8_t* block, << " Should be " << VoipMetric::kBlockLength; return; } - voip_metric_blocks_.push_back(VoipMetric()); - voip_metric_blocks_.back().Parse(block); + if (voip_metric_block_) { + LOG(LS_WARNING) + << "Two Voip Metric blocks found in same Extended Report packet"; + return; + } + voip_metric_block_.emplace(); + voip_metric_block_->Parse(block); } } // namespace rtcp } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports.h b/webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports.h index 7b6c54452f..c433477707 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports.h +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports.h @@ -14,6 +14,7 @@ #include #include "webrtc/base/constructormagic.h" +#include "webrtc/base/optional.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/dlrr.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/rrtr.h" @@ -36,16 +37,15 @@ class ExtendedReports : public RtcpPacket { void SetSenderSsrc(uint32_t ssrc) { sender_ssrc_ = ssrc; } - // Max 50 items of each of {Rrtr, Dlrr, VoipMetric} allowed per Xr. - bool AddRrtr(const Rrtr& rrtr); - bool AddDlrr(const Dlrr& dlrr); - bool AddVoipMetric(const VoipMetric& voip_metric); + void SetRrtr(const Rrtr& rrtr); + void AddDlrrItem(const ReceiveTimeInfo& time_info); + void SetVoipMetric(const VoipMetric& voip_metric); uint32_t sender_ssrc() const { return sender_ssrc_; } - const std::vector& rrtrs() const { return rrtr_blocks_; } - const std::vector& dlrrs() const { return dlrr_blocks_; } - const std::vector& voip_metrics() const { - return voip_metric_blocks_; + const rtc::Optional& rrtr() const { return rrtr_block_; } + const Dlrr& dlrr() const { return dlrr_block_; } + const rtc::Optional& voip_metric() const { + return voip_metric_block_; } protected: @@ -55,9 +55,6 @@ class ExtendedReports : public RtcpPacket { RtcpPacket::PacketReadyCallback* callback) const override; private: - static const size_t kMaxNumberOfRrtrBlocks = 50; - static const size_t kMaxNumberOfDlrrBlocks = 50; - static const size_t kMaxNumberOfVoipMetricBlocks = 50; static constexpr size_t kXrBaseLength = 4; size_t BlockLength() const override { @@ -65,10 +62,10 @@ class ExtendedReports : public RtcpPacket { VoipMetricLength(); } - size_t RrtrLength() const { return Rrtr::kLength * rrtr_blocks_.size(); } - size_t DlrrLength() const; + size_t RrtrLength() const { return rrtr_block_ ? Rrtr::kLength : 0; } + size_t DlrrLength() const { return dlrr_block_.BlockLength(); } size_t VoipMetricLength() const { - return VoipMetric::kLength * voip_metric_blocks_.size(); + return voip_metric_block_ ? VoipMetric::kLength : 0; } void ParseRrtrBlock(const uint8_t* block, uint16_t block_length); @@ -76,9 +73,9 @@ class ExtendedReports : public RtcpPacket { void ParseVoipMetricBlock(const uint8_t* block, uint16_t block_length); uint32_t sender_ssrc_; - std::vector rrtr_blocks_; - std::vector dlrr_blocks_; - std::vector voip_metric_blocks_; + rtc::Optional rrtr_block_; + Dlrr dlrr_block_; // Dlrr without items treated same as no dlrr block. + rtc::Optional voip_metric_block_; RTC_DISALLOW_COPY_AND_ASSIGN(ExtendedReports); }; diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports_unittest.cc index 3c5025caaa..905462ace9 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports_unittest.cc @@ -17,7 +17,6 @@ using testing::ElementsAre; using testing::ElementsAreArray; -using testing::IsEmpty; using testing::make_tuple; using webrtc::rtcp::Dlrr; using webrtc::rtcp::ExtendedReports; @@ -62,10 +61,6 @@ bool operator==(const ReceiveTimeInfo& time1, const ReceiveTimeInfo& time2) { time1.delay_since_last_rr == time2.delay_since_last_rr; } -bool operator==(const Dlrr& dlrr1, const Dlrr& dlrr2) { - return dlrr1.sub_blocks() == dlrr2.sub_blocks(); -} - bool operator==(const VoipMetric& metric1, const VoipMetric& metric2) { return metric1.ssrc() == metric2.ssrc() && metric1.voip_metric() == metric2.voip_metric(); @@ -162,16 +157,16 @@ TEST_F(RtcpPacketExtendedReportsTest, ParseWithoutReportBlocks) { ExtendedReports parsed; EXPECT_TRUE(test::ParseSinglePacket(kEmptyPacket, &parsed)); EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); - EXPECT_THAT(parsed.rrtrs(), IsEmpty()); - EXPECT_THAT(parsed.dlrrs(), IsEmpty()); - EXPECT_THAT(parsed.voip_metrics(), IsEmpty()); + EXPECT_FALSE(parsed.rrtr()); + EXPECT_FALSE(parsed.dlrr()); + EXPECT_FALSE(parsed.voip_metric()); } -TEST_F(RtcpPacketExtendedReportsTest, CreateAndParseWithOneRrtrBlock) { - Rrtr rrtr = Rand(); +TEST_F(RtcpPacketExtendedReportsTest, CreateAndParseWithRrtrBlock) { + const Rrtr kRrtr = Rand(); ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); - EXPECT_TRUE(xr.AddRrtr(rrtr)); + xr.SetRrtr(kRrtr); rtc::Buffer packet = xr.Build(); ExtendedReports mparsed; @@ -179,33 +174,14 @@ TEST_F(RtcpPacketExtendedReportsTest, CreateAndParseWithOneRrtrBlock) { const ExtendedReports& parsed = mparsed; EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); - EXPECT_THAT(parsed.rrtrs(), ElementsAre(rrtr)); -} - -TEST_F(RtcpPacketExtendedReportsTest, CreateAndParseWithTwoRrtrBlocks) { - Rrtr rrtr1 = Rand(); - Rrtr rrtr2 = Rand(); - ExtendedReports xr; - xr.SetSenderSsrc(kSenderSsrc); - EXPECT_TRUE(xr.AddRrtr(rrtr1)); - EXPECT_TRUE(xr.AddRrtr(rrtr2)); - - rtc::Buffer packet = xr.Build(); - - ExtendedReports mparsed; - EXPECT_TRUE(test::ParseSinglePacket(packet, &mparsed)); - const ExtendedReports& parsed = mparsed; - - EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); - EXPECT_THAT(parsed.rrtrs(), ElementsAre(rrtr1, rrtr2)); + EXPECT_EQ(kRrtr, parsed.rrtr()); } TEST_F(RtcpPacketExtendedReportsTest, CreateAndParseWithDlrrWithOneSubBlock) { - Dlrr dlrr; - EXPECT_TRUE(dlrr.AddDlrrItem(Rand())); + const ReceiveTimeInfo kTimeInfo = Rand(); ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); - EXPECT_TRUE(xr.AddDlrr(dlrr)); + xr.AddDlrrItem(kTimeInfo); rtc::Buffer packet = xr.Build(); @@ -214,16 +190,16 @@ TEST_F(RtcpPacketExtendedReportsTest, CreateAndParseWithDlrrWithOneSubBlock) { const ExtendedReports& parsed = mparsed; EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); - EXPECT_THAT(parsed.dlrrs(), ElementsAre(dlrr)); + EXPECT_THAT(parsed.dlrr().sub_blocks(), ElementsAre(kTimeInfo)); } TEST_F(RtcpPacketExtendedReportsTest, CreateAndParseWithDlrrWithTwoSubBlocks) { - Dlrr dlrr; - EXPECT_TRUE(dlrr.AddDlrrItem(Rand())); - EXPECT_TRUE(dlrr.AddDlrrItem(Rand())); + const ReceiveTimeInfo kTimeInfo1 = Rand(); + const ReceiveTimeInfo kTimeInfo2 = Rand(); ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); - EXPECT_TRUE(xr.AddDlrr(dlrr)); + xr.AddDlrrItem(kTimeInfo1); + xr.AddDlrrItem(kTimeInfo2); rtc::Buffer packet = xr.Build(); @@ -232,35 +208,15 @@ TEST_F(RtcpPacketExtendedReportsTest, CreateAndParseWithDlrrWithTwoSubBlocks) { const ExtendedReports& parsed = mparsed; EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); - EXPECT_THAT(parsed.dlrrs(), ElementsAre(dlrr)); -} - -TEST_F(RtcpPacketExtendedReportsTest, CreateAndParseWithTwoDlrrBlocks) { - Dlrr dlrr1; - EXPECT_TRUE(dlrr1.AddDlrrItem(Rand())); - Dlrr dlrr2; - EXPECT_TRUE(dlrr2.AddDlrrItem(Rand())); - ExtendedReports xr; - xr.SetSenderSsrc(kSenderSsrc); - EXPECT_TRUE(xr.AddDlrr(dlrr1)); - EXPECT_TRUE(xr.AddDlrr(dlrr2)); - - rtc::Buffer packet = xr.Build(); - - ExtendedReports mparsed; - EXPECT_TRUE(test::ParseSinglePacket(packet, &mparsed)); - const ExtendedReports& parsed = mparsed; - - EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); - EXPECT_THAT(parsed.dlrrs(), ElementsAre(dlrr1, dlrr2)); + EXPECT_THAT(parsed.dlrr().sub_blocks(), ElementsAre(kTimeInfo1, kTimeInfo2)); } TEST_F(RtcpPacketExtendedReportsTest, CreateAndParseWithVoipMetric) { - VoipMetric voip_metric = Rand(); + const VoipMetric kVoipMetric = Rand(); ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); - EXPECT_TRUE(xr.AddVoipMetric(voip_metric)); + xr.SetVoipMetric(kVoipMetric); rtc::Buffer packet = xr.Build(); @@ -269,19 +225,19 @@ TEST_F(RtcpPacketExtendedReportsTest, CreateAndParseWithVoipMetric) { const ExtendedReports& parsed = mparsed; EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); - EXPECT_THAT(parsed.voip_metrics(), ElementsAre(voip_metric)); + EXPECT_EQ(kVoipMetric, parsed.voip_metric()); } TEST_F(RtcpPacketExtendedReportsTest, CreateAndParseWithMultipleReportBlocks) { - Rrtr rrtr = Rand(); - Dlrr dlrr; - EXPECT_TRUE(dlrr.AddDlrrItem(Rand())); - VoipMetric metric = Rand(); + const Rrtr kRrtr = Rand(); + const ReceiveTimeInfo kTimeInfo = Rand(); + const VoipMetric kVoipMetric = Rand(); + ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); - EXPECT_TRUE(xr.AddRrtr(rrtr)); - EXPECT_TRUE(xr.AddDlrr(dlrr)); - EXPECT_TRUE(xr.AddVoipMetric(metric)); + xr.SetRrtr(kRrtr); + xr.AddDlrrItem(kTimeInfo); + xr.SetVoipMetric(kVoipMetric); rtc::Buffer packet = xr.Build(); @@ -290,49 +246,9 @@ TEST_F(RtcpPacketExtendedReportsTest, CreateAndParseWithMultipleReportBlocks) { const ExtendedReports& parsed = mparsed; EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); - EXPECT_THAT(parsed.rrtrs(), ElementsAre(rrtr)); - EXPECT_THAT(parsed.dlrrs(), ElementsAre(dlrr)); - EXPECT_THAT(parsed.voip_metrics(), ElementsAre(metric)); + EXPECT_EQ(kRrtr, parsed.rrtr()); + EXPECT_THAT(parsed.dlrr().sub_blocks(), ElementsAre(kTimeInfo)); + EXPECT_EQ(kVoipMetric, parsed.voip_metric()); } -TEST_F(RtcpPacketExtendedReportsTest, DlrrWithoutItemNotIncludedInPacket) { - Rrtr rrtr = Rand(); - Dlrr dlrr; - VoipMetric metric = Rand(); - ExtendedReports xr; - xr.SetSenderSsrc(kSenderSsrc); - EXPECT_TRUE(xr.AddRrtr(rrtr)); - EXPECT_TRUE(xr.AddDlrr(dlrr)); - EXPECT_TRUE(xr.AddVoipMetric(metric)); - - rtc::Buffer packet = xr.Build(); - - ExtendedReports mparsed; - EXPECT_TRUE(test::ParseSinglePacket(packet, &mparsed)); - const ExtendedReports& parsed = mparsed; - - EXPECT_THAT(parsed.rrtrs(), ElementsAre(rrtr)); - EXPECT_THAT(parsed.dlrrs(), IsEmpty()); - EXPECT_THAT(parsed.voip_metrics(), ElementsAre(metric)); -} - -TEST_F(RtcpPacketExtendedReportsTest, WithTooManyBlocks) { - const size_t kMaxBlocks = 50; - ExtendedReports xr; - - Rrtr rrtr = Rand(); - for (size_t i = 0; i < kMaxBlocks; ++i) - EXPECT_TRUE(xr.AddRrtr(rrtr)); - EXPECT_FALSE(xr.AddRrtr(rrtr)); - - Dlrr dlrr; - for (size_t i = 0; i < kMaxBlocks; ++i) - EXPECT_TRUE(xr.AddDlrr(dlrr)); - EXPECT_FALSE(xr.AddDlrr(dlrr)); - - VoipMetric voip_metric = Rand(); - for (size_t i = 0; i < kMaxBlocks; ++i) - EXPECT_TRUE(xr.AddVoipMetric(voip_metric)); - EXPECT_FALSE(xr.AddVoipMetric(voip_metric)); -} } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc index db11d26bc6..c78637abf3 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -688,13 +688,11 @@ void RTCPReceiver::HandleXr(const CommonHeader& rtcp_block, return; } - for (const rtcp::Rrtr& rrtr : xr.rrtrs()) - HandleXrReceiveReferenceTime(xr.sender_ssrc(), rrtr); + if (xr.rrtr()) + HandleXrReceiveReferenceTime(xr.sender_ssrc(), *xr.rrtr()); - for (const rtcp::Dlrr& dlrr : xr.dlrrs()) { - for (const rtcp::ReceiveTimeInfo& time_info : dlrr.sub_blocks()) - HandleXrDlrrReportBlock(time_info); - } + for (const rtcp::ReceiveTimeInfo& time_info : xr.dlrr().sub_blocks()) + HandleXrDlrrReportBlock(time_info); } void RTCPReceiver::HandleXrReceiveReferenceTime( diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc index e16673c18e..c98400f3fb 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc @@ -53,6 +53,7 @@ using ::testing::SizeIs; using ::testing::StrEq; using ::testing::StrictMock; using ::testing::UnorderedElementsAre; +using rtcp::ReceiveTimeInfo; class MockRtcpPacketTypeCounterObserver : public RtcpPacketTypeCounterObserver { public: @@ -698,7 +699,7 @@ TEST_F(RtcpReceiverTest, InjectExtendedReportsVoipPacket) { voip_metric.SetVoipMetric(metric); rtcp::ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); - xr.AddVoipMetric(voip_metric); + xr.SetVoipMetric(voip_metric); InjectRtcpPacket(xr); } @@ -708,7 +709,7 @@ TEST_F(RtcpReceiverTest, ExtendedReportsVoipPacketNotToUsIgnored) { voip_metric.SetMediaSsrc(kNotToUsSsrc); rtcp::ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); - xr.AddVoipMetric(voip_metric); + xr.SetVoipMetric(voip_metric); InjectRtcpPacket(xr); } @@ -719,9 +720,9 @@ TEST_F(RtcpReceiverTest, InjectExtendedReportsReceiverReferenceTimePacket) { rrtr.SetNtp(kNtp); rtcp::ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); - xr.AddRrtr(rrtr); + xr.SetRrtr(rrtr); - rtcp::ReceiveTimeInfo rrtime; + ReceiveTimeInfo rrtime; EXPECT_FALSE(rtcp_receiver_.LastReceivedXrReferenceTimeInfo(&rrtime)); InjectRtcpPacket(xr); @@ -740,11 +741,9 @@ TEST_F(RtcpReceiverTest, ExtendedReportsDlrrPacketNotToUsIgnored) { // Allow calculate rtt using dlrr/rrtr, simulating media receiver side. rtcp_receiver_.SetRtcpXrRrtrStatus(true); - rtcp::Dlrr dlrr; - dlrr.AddDlrrItem(kNotToUsSsrc, 0x12345, 0x67890); rtcp::ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); - xr.AddDlrr(dlrr); + xr.AddDlrrItem(ReceiveTimeInfo(kNotToUsSsrc, 0x12345, 0x67890)); InjectRtcpPacket(xr); @@ -759,11 +758,9 @@ TEST_F(RtcpReceiverTest, InjectExtendedReportsDlrrPacketWithSubBlock) { int64_t rtt_ms = 0; EXPECT_FALSE(rtcp_receiver_.GetAndResetXrRrRtt(&rtt_ms)); - rtcp::Dlrr dlrr; - dlrr.AddDlrrItem(kReceiverMainSsrc, kLastRR, kDelay); rtcp::ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); - xr.AddDlrr(dlrr); + xr.AddDlrrItem(ReceiveTimeInfo(kReceiverMainSsrc, kLastRR, kDelay)); InjectRtcpPacket(xr); @@ -780,11 +777,9 @@ TEST_F(RtcpReceiverTest, InjectExtendedReportsDlrrPacketWithMultipleSubBlocks) { rtcp::ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); - rtcp::Dlrr dlrr; - dlrr.AddDlrrItem(kReceiverMainSsrc, kLastRR, kDelay); - dlrr.AddDlrrItem(kReceiverMainSsrc + 1, 0x12345, 0x67890); - dlrr.AddDlrrItem(kReceiverMainSsrc + 2, 0x12345, 0x67890); - xr.AddDlrr(dlrr); + xr.AddDlrrItem(ReceiveTimeInfo(kReceiverMainSsrc, kLastRR, kDelay)); + xr.AddDlrrItem(ReceiveTimeInfo(kReceiverMainSsrc + 1, 0x12345, 0x67890)); + xr.AddDlrrItem(ReceiveTimeInfo(kReceiverMainSsrc + 2, 0x12345, 0x67890)); InjectRtcpPacket(xr); @@ -799,19 +794,17 @@ TEST_F(RtcpReceiverTest, InjectExtendedReportsPacketWithMultipleReportBlocks) { rtcp_receiver_.SetRtcpXrRrtrStatus(true); rtcp::Rrtr rrtr; - rtcp::Dlrr dlrr; - dlrr.AddDlrrItem(kReceiverMainSsrc, 0x12345, 0x67890); rtcp::VoipMetric metric; metric.SetMediaSsrc(kReceiverMainSsrc); rtcp::ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); - xr.AddRrtr(rrtr); - xr.AddDlrr(dlrr); - xr.AddVoipMetric(metric); + xr.SetRrtr(rrtr); + xr.AddDlrrItem(ReceiveTimeInfo(kReceiverMainSsrc, 0x12345, 0x67890)); + xr.SetVoipMetric(metric); InjectRtcpPacket(xr); - rtcp::ReceiveTimeInfo rrtime; + ReceiveTimeInfo rrtime; EXPECT_TRUE(rtcp_receiver_.LastReceivedXrReferenceTimeInfo(&rrtime)); int64_t rtt_ms = 0; EXPECT_TRUE(rtcp_receiver_.GetAndResetXrRrRtt(&rtt_ms)); @@ -821,15 +814,13 @@ TEST_F(RtcpReceiverTest, InjectExtendedReportsPacketWithUnknownReportBlock) { rtcp_receiver_.SetRtcpXrRrtrStatus(true); rtcp::Rrtr rrtr; - rtcp::Dlrr dlrr; - dlrr.AddDlrrItem(kReceiverMainSsrc, 0x12345, 0x67890); rtcp::VoipMetric metric; metric.SetMediaSsrc(kReceiverMainSsrc); rtcp::ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); - xr.AddRrtr(rrtr); - xr.AddDlrr(dlrr); - xr.AddVoipMetric(metric); + xr.SetRrtr(rrtr); + xr.AddDlrrItem(ReceiveTimeInfo(kReceiverMainSsrc, 0x12345, 0x67890)); + xr.SetVoipMetric(metric); rtc::Buffer packet = xr.Build(); // Modify the DLRR block to have an unsupported block type, from 5 to 6. @@ -838,7 +829,7 @@ TEST_F(RtcpReceiverTest, InjectExtendedReportsPacketWithUnknownReportBlock) { InjectRtcpPacket(packet); // Validate Rrtr was received and processed. - rtcp::ReceiveTimeInfo rrtime; + ReceiveTimeInfo rrtime; EXPECT_TRUE(rtcp_receiver_.LastReceivedXrReferenceTimeInfo(&rrtime)); // Validate Dlrr report wasn't processed. int64_t rtt_ms = 0; @@ -862,11 +853,9 @@ TEST_F(RtcpReceiverTest, RttCalculatedAfterExtendedReportsDlrr) { uint32_t sent_ntp = CompactNtp(now); system_clock_.AdvanceTimeMilliseconds(kRttMs + kDelayMs); - rtcp::Dlrr dlrr; - dlrr.AddDlrrItem(kReceiverMainSsrc, sent_ntp, kDelayNtp); rtcp::ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); - xr.AddDlrr(dlrr); + xr.AddDlrrItem(ReceiveTimeInfo(kReceiverMainSsrc, sent_ntp, kDelayNtp)); InjectRtcpPacket(xr); @@ -885,11 +874,9 @@ TEST_F(RtcpReceiverTest, XrDlrrCalculatesNegativeRttAsOne) { system_clock_.AdvanceTimeMilliseconds(kRttMs + kDelayMs); rtcp_receiver_.SetRtcpXrRrtrStatus(true); - rtcp::Dlrr dlrr; - dlrr.AddDlrrItem(kReceiverMainSsrc, sent_ntp, kDelayNtp); rtcp::ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); - xr.AddDlrr(dlrr); + xr.AddDlrrItem(ReceiveTimeInfo(kReceiverMainSsrc, sent_ntp, kDelayNtp)); InjectRtcpPacket(xr); @@ -899,7 +886,7 @@ TEST_F(RtcpReceiverTest, XrDlrrCalculatesNegativeRttAsOne) { } TEST_F(RtcpReceiverTest, LastReceivedXrReferenceTimeInfoInitiallyFalse) { - rtcp::ReceiveTimeInfo info; + ReceiveTimeInfo info; EXPECT_FALSE(rtcp_receiver_.LastReceivedXrReferenceTimeInfo(&info)); } @@ -911,11 +898,11 @@ TEST_F(RtcpReceiverTest, GetLastReceivedExtendedReportsReferenceTimeInfo) { rrtr.SetNtp(kNtp); rtcp::ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); - xr.AddRrtr(rrtr); + xr.SetRrtr(rrtr); InjectRtcpPacket(xr); - rtcp::ReceiveTimeInfo info; + ReceiveTimeInfo info; EXPECT_TRUE(rtcp_receiver_.LastReceivedXrReferenceTimeInfo(&info)); EXPECT_EQ(kSenderSsrc, info.ssrc); EXPECT_EQ(kNtpMid, info.last_rr); diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc index 8fe3334789..e6cf669522 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc @@ -701,7 +701,7 @@ std::unique_ptr RTCPSender::BuildReceiverReferenceTime( rtcp::Rrtr rrtr; rrtr.SetNtp(NtpTime(ctx.ntp_sec_, ctx.ntp_frac_)); - xr->AddRrtr(rrtr); + xr->SetRrtr(rrtr); // TODO(sprang): Merge XR report sending to contain all of RRTR, DLRR, VOIP? @@ -712,17 +712,12 @@ std::unique_ptr RTCPSender::BuildDlrr( const RtcpContext& ctx) { rtcp::ExtendedReports* xr = new rtcp::ExtendedReports(); xr->SetSenderSsrc(ssrc_); - - rtcp::Dlrr dlrr; RTC_DCHECK(ctx.feedback_state_.has_last_xr_rr); - dlrr.AddDlrrItem(ctx.feedback_state_.last_xr_rr); - - xr->AddDlrr(dlrr); + xr->AddDlrrItem(ctx.feedback_state_.last_xr_rr); return std::unique_ptr(xr); } -// TODO(sprang): Add a unit test for this, or remove if the code isn't used. std::unique_ptr RTCPSender::BuildVoIPMetric( const RtcpContext& context) { rtcp::ExtendedReports* xr = new rtcp::ExtendedReports(); @@ -732,7 +727,7 @@ std::unique_ptr RTCPSender::BuildVoIPMetric( voip.SetMediaSsrc(remote_ssrc_); voip.SetVoipMetric(xr_voip_metric_); - xr->AddVoipMetric(voip); + xr->SetVoipMetric(voip); return std::unique_ptr(xr); } diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index d348d3a94a..235c84b09e 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -606,28 +606,29 @@ TEST_F(RtcpSenderTest, SendXrWithVoipMetric) { EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpXrVoipMetric)); EXPECT_EQ(1, parser()->xr()->num_packets()); EXPECT_EQ(kSenderSsrc, parser()->xr()->sender_ssrc()); - EXPECT_EQ(1U, parser()->xr()->voip_metrics().size()); - EXPECT_EQ(kRemoteSsrc, parser()->xr()->voip_metrics()[0].ssrc()); - EXPECT_EQ(metric.lossRate, parser()->voip_metric()->lossRate); - EXPECT_EQ(metric.discardRate, parser()->voip_metric()->discardRate); - EXPECT_EQ(metric.burstDensity, parser()->voip_metric()->burstDensity); - EXPECT_EQ(metric.gapDensity, parser()->voip_metric()->gapDensity); - EXPECT_EQ(metric.burstDuration, parser()->voip_metric()->burstDuration); - EXPECT_EQ(metric.gapDuration, parser()->voip_metric()->gapDuration); - EXPECT_EQ(metric.roundTripDelay, parser()->voip_metric()->roundTripDelay); - EXPECT_EQ(metric.endSystemDelay, parser()->voip_metric()->endSystemDelay); - EXPECT_EQ(metric.signalLevel, parser()->voip_metric()->signalLevel); - EXPECT_EQ(metric.noiseLevel, parser()->voip_metric()->noiseLevel); - EXPECT_EQ(metric.RERL, parser()->voip_metric()->RERL); - EXPECT_EQ(metric.Gmin, parser()->voip_metric()->Gmin); - EXPECT_EQ(metric.Rfactor, parser()->voip_metric()->Rfactor); - EXPECT_EQ(metric.extRfactor, parser()->voip_metric()->extRfactor); - EXPECT_EQ(metric.MOSLQ, parser()->voip_metric()->MOSLQ); - EXPECT_EQ(metric.MOSCQ, parser()->voip_metric()->MOSCQ); - EXPECT_EQ(metric.RXconfig, parser()->voip_metric()->RXconfig); - EXPECT_EQ(metric.JBnominal, parser()->voip_metric()->JBnominal); - EXPECT_EQ(metric.JBmax, parser()->voip_metric()->JBmax); - EXPECT_EQ(metric.JBabsMax, parser()->voip_metric()->JBabsMax); + ASSERT_TRUE(parser()->xr()->voip_metric()); + EXPECT_EQ(kRemoteSsrc, parser()->xr()->voip_metric()->ssrc()); + const auto& parsed_metric = parser()->xr()->voip_metric()->voip_metric(); + EXPECT_EQ(metric.lossRate, parsed_metric.lossRate); + EXPECT_EQ(metric.discardRate, parsed_metric.discardRate); + EXPECT_EQ(metric.burstDensity, parsed_metric.burstDensity); + EXPECT_EQ(metric.gapDensity, parsed_metric.gapDensity); + EXPECT_EQ(metric.burstDuration, parsed_metric.burstDuration); + EXPECT_EQ(metric.gapDuration, parsed_metric.gapDuration); + EXPECT_EQ(metric.roundTripDelay, parsed_metric.roundTripDelay); + EXPECT_EQ(metric.endSystemDelay, parsed_metric.endSystemDelay); + EXPECT_EQ(metric.signalLevel, parsed_metric.signalLevel); + EXPECT_EQ(metric.noiseLevel, parsed_metric.noiseLevel); + EXPECT_EQ(metric.RERL, parsed_metric.RERL); + EXPECT_EQ(metric.Gmin, parsed_metric.Gmin); + EXPECT_EQ(metric.Rfactor, parsed_metric.Rfactor); + EXPECT_EQ(metric.extRfactor, parsed_metric.extRfactor); + EXPECT_EQ(metric.MOSLQ, parsed_metric.MOSLQ); + EXPECT_EQ(metric.MOSCQ, parsed_metric.MOSCQ); + EXPECT_EQ(metric.RXconfig, parsed_metric.RXconfig); + EXPECT_EQ(metric.JBnominal, parsed_metric.JBnominal); + EXPECT_EQ(metric.JBmax, parsed_metric.JBmax); + EXPECT_EQ(metric.JBabsMax, parsed_metric.JBabsMax); } TEST_F(RtcpSenderTest, SendXrWithDlrr) { @@ -642,13 +643,11 @@ TEST_F(RtcpSenderTest, SendXrWithDlrr) { EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state, kRtcpReport)); EXPECT_EQ(1, parser()->xr()->num_packets()); EXPECT_EQ(kSenderSsrc, parser()->xr()->sender_ssrc()); - EXPECT_EQ(1U, parser()->xr()->dlrrs().size()); - EXPECT_EQ(1U, parser()->xr()->dlrrs()[0].sub_blocks().size()); - EXPECT_EQ(last_xr_rr.ssrc, parser()->xr()->dlrrs()[0].sub_blocks()[0].ssrc); - EXPECT_EQ(last_xr_rr.last_rr, - parser()->xr()->dlrrs()[0].sub_blocks()[0].last_rr); + EXPECT_EQ(1U, parser()->xr()->dlrr().sub_blocks().size()); + EXPECT_EQ(last_xr_rr.ssrc, parser()->xr()->dlrr().sub_blocks()[0].ssrc); + EXPECT_EQ(last_xr_rr.last_rr, parser()->xr()->dlrr().sub_blocks()[0].last_rr); EXPECT_EQ(last_xr_rr.delay_since_last_rr, - parser()->xr()->dlrrs()[0].sub_blocks()[0].delay_since_last_rr); + parser()->xr()->dlrr().sub_blocks()[0].delay_since_last_rr); } TEST_F(RtcpSenderTest, SendXrWithRrtr) { @@ -661,10 +660,11 @@ TEST_F(RtcpSenderTest, SendXrWithRrtr) { EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpReport)); EXPECT_EQ(1, parser()->xr()->num_packets()); EXPECT_EQ(kSenderSsrc, parser()->xr()->sender_ssrc()); - EXPECT_EQ(0U, parser()->xr()->dlrrs().size()); - EXPECT_EQ(1U, parser()->xr()->rrtrs().size()); - EXPECT_EQ(ntp_secs, parser()->xr()->rrtrs()[0].ntp().seconds()); - EXPECT_EQ(ntp_frac, parser()->xr()->rrtrs()[0].ntp().fractions()); + EXPECT_FALSE(parser()->xr()->dlrr()); + EXPECT_FALSE(parser()->xr()->voip_metric()); + ASSERT_TRUE(parser()->xr()->rrtr()); + EXPECT_EQ(ntp_secs, parser()->xr()->rrtr()->ntp().seconds()); + EXPECT_EQ(ntp_frac, parser()->xr()->rrtr()->ntp().fractions()); } TEST_F(RtcpSenderTest, TestNoXrRrtrSentIfSending) { diff --git a/webrtc/test/rtcp_packet_parser.h b/webrtc/test/rtcp_packet_parser.h index 4fa1405e7b..4a8eca0d67 100644 --- a/webrtc/test/rtcp_packet_parser.h +++ b/webrtc/test/rtcp_packet_parser.h @@ -92,9 +92,6 @@ class RtcpPacketParser { PacketCounter* transport_feedback() { return &transport_feedback_; } - const RTCPVoIPMetric* voip_metric() { - return &xr_.voip_metrics()[0].voip_metric(); - } private: PacketCounter app_;