From 2f36c2399e6747152643fdc999cecff2767ab112 Mon Sep 17 00:00:00 2001 From: danilchap Date: Tue, 29 Mar 2016 08:02:27 -0700 Subject: [PATCH] [rtcp] SenderReport::Parse updated not to use RTCPUtility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BUG=webrtc:5260 R=åsapersson Review URL: https://codereview.webrtc.org/1825353002 Cr-Commit-Position: refs/heads/master@{#12140} --- .../source/rtcp_packet/sender_report.cc | 16 +-- .../source/rtcp_packet/sender_report.h | 7 +- .../rtcp_packet/sender_report_unittest.cc | 102 +++++++++--------- 3 files changed, 62 insertions(+), 63 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.cc index ab1863ed46..8f90ce6bdd 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.cc @@ -13,8 +13,7 @@ #include "webrtc/base/checks.h" #include "webrtc/base/logging.h" #include "webrtc/modules/rtp_rtcp/source/byte_io.h" - -using webrtc::RTCPUtility::RtcpCommonHeader; +#include "webrtc/modules/rtp_rtcp/source/rtcp_packet/common_header.h" namespace webrtc { namespace rtcp { @@ -43,17 +42,17 @@ SenderReport::SenderReport() sender_packet_count_(0), sender_octet_count_(0) {} -bool SenderReport::Parse(const RtcpCommonHeader& header, - const uint8_t* payload) { - RTC_DCHECK(header.packet_type == kPacketType); +bool SenderReport::Parse(const CommonHeader& packet) { + RTC_DCHECK(packet.type() == kPacketType); - const uint8_t report_block_count = header.count_or_format; - if (header.payload_size_bytes < + const uint8_t report_block_count = packet.count(); + if (packet.payload_size_bytes() < kSenderBaseLength + report_block_count * ReportBlock::kLength) { LOG(LS_WARNING) << "Packet is too small to contain all the data."; return false; } // Read SenderReport header. + const uint8_t* const payload = packet.payload(); sender_ssrc_ = ByteReader::ReadBigEndian(&payload[0]); uint32_t secs = ByteReader::ReadBigEndian(&payload[4]); uint32_t frac = ByteReader::ReadBigEndian(&payload[8]); @@ -69,7 +68,8 @@ bool SenderReport::Parse(const RtcpCommonHeader& header, next_block += ReportBlock::kLength; } // Double check we didn't read beyond provided buffer. - RTC_DCHECK_LE(next_block, payload + header.payload_size_bytes); + RTC_DCHECK_EQ(next_block - payload, + static_cast(packet.payload_size_bytes())); return true; } diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.h b/webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.h index e26911abc3..e11bdb9a94 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.h +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.h @@ -15,22 +15,21 @@ #include "webrtc/modules/rtp_rtcp/source/rtcp_packet.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/report_block.h" -#include "webrtc/modules/rtp_rtcp/source/rtcp_utility.h" #include "webrtc/system_wrappers/include/ntp_time.h" namespace webrtc { namespace rtcp { +class CommonHeader; class SenderReport : public RtcpPacket { public: static const uint8_t kPacketType = 200; SenderReport(); - virtual ~SenderReport() {} + ~SenderReport() override {} // Parse assumes header is already parsed and validated. - bool Parse(const RTCPUtility::RtcpCommonHeader& header, - const uint8_t* payload); // Size of the payload is in the header. + bool Parse(const CommonHeader& packet); void From(uint32_t ssrc) { sender_ssrc_ = ssrc; } void WithNtp(NtpTime ntp) { ntp_ = ntp; } diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc index 646e6849a9..ac7fbcaa64 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc @@ -10,41 +10,34 @@ #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.h" +#include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" +#include "webrtc/test/rtcp_packet_parser.h" + +using testing::ElementsAreArray; +using testing::make_tuple; using webrtc::rtcp::ReportBlock; using webrtc::rtcp::SenderReport; -using webrtc::RTCPUtility::RtcpCommonHeader; -using webrtc::RTCPUtility::RtcpParseCommonHeader; namespace webrtc { +namespace { +const uint32_t kSenderSsrc = 0x12345678; +const uint32_t kRemoteSsrc = 0x23456789; +const NtpTime kNtp(0x11121418, 0x22242628); +const uint32_t kRtpTimestamp = 0x33343536; +const uint32_t kPacketCount = 0x44454647; +const uint32_t kOctetCount = 0x55565758; +const uint8_t kPacket[] = {0x80, 200, 0x00, 0x06, + 0x12, 0x34, 0x56, 0x78, + 0x11, 0x12, 0x14, 0x18, + 0x22, 0x24, 0x26, 0x28, + 0x33, 0x34, 0x35, 0x36, + 0x44, 0x45, 0x46, 0x47, + 0x55, 0x56, 0x57, 0x58}; +} // namespace -class RtcpPacketSenderReportTest : public ::testing::Test { - protected: - const uint32_t kSenderSsrc = 0x12345678; - const uint32_t kRemoteSsrc = 0x23456789; - - void ParsePacket(const rtc::Buffer& packet) { - RtcpCommonHeader header; - EXPECT_TRUE(RtcpParseCommonHeader(packet.data(), packet.size(), &header)); - EXPECT_EQ(packet.size(), header.BlockSize()); - EXPECT_TRUE(parsed_.Parse( - header, packet.data() + RtcpCommonHeader::kHeaderSizeBytes)); - } - - // Only ParsePacket can change parsed, tests should use it in readonly mode. - const SenderReport& parsed() { return parsed_; } - - private: - SenderReport parsed_; -}; - -TEST_F(RtcpPacketSenderReportTest, WithoutReportBlocks) { - const NtpTime kNtp(0x11121418, 0x22242628); - const uint32_t kRtpTimestamp = 0x33343536; - const uint32_t kPacketCount = 0x44454647; - const uint32_t kOctetCount = 0x55565758; - +TEST(RtcpPacketSenderReportTest, CreateWithoutReportBlocks) { SenderReport sr; sr.From(kSenderSsrc); sr.WithNtp(kNtp); @@ -52,18 +45,23 @@ TEST_F(RtcpPacketSenderReportTest, WithoutReportBlocks) { sr.WithPacketCount(kPacketCount); sr.WithOctetCount(kOctetCount); - rtc::Buffer packet = sr.Build(); - ParsePacket(packet); - - EXPECT_EQ(kSenderSsrc, parsed().sender_ssrc()); - EXPECT_EQ(kNtp, parsed().ntp()); - EXPECT_EQ(kRtpTimestamp, parsed().rtp_timestamp()); - EXPECT_EQ(kPacketCount, parsed().sender_packet_count()); - EXPECT_EQ(kOctetCount, parsed().sender_octet_count()); - EXPECT_TRUE(parsed().report_blocks().empty()); + rtc::Buffer raw = sr.Build(); + EXPECT_THAT(make_tuple(raw.data(), raw.size()), ElementsAreArray(kPacket)); } -TEST_F(RtcpPacketSenderReportTest, WithOneReportBlock) { +TEST(RtcpPacketSenderReportTest, ParseWithoutReportBlocks) { + SenderReport parsed; + EXPECT_TRUE(test::ParseSinglePacket(kPacket, &parsed)); + + EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); + EXPECT_EQ(kNtp, parsed.ntp()); + EXPECT_EQ(kRtpTimestamp, parsed.rtp_timestamp()); + EXPECT_EQ(kPacketCount, parsed.sender_packet_count()); + EXPECT_EQ(kOctetCount, parsed.sender_octet_count()); + EXPECT_TRUE(parsed.report_blocks().empty()); +} + +TEST(RtcpPacketSenderReportTest, CreateAndParseWithOneReportBlock) { ReportBlock rb; rb.To(kRemoteSsrc); @@ -71,15 +69,16 @@ TEST_F(RtcpPacketSenderReportTest, WithOneReportBlock) { sr.From(kSenderSsrc); EXPECT_TRUE(sr.WithReportBlock(rb)); - rtc::Buffer packet = sr.Build(); - ParsePacket(packet); + rtc::Buffer raw = sr.Build(); + SenderReport parsed; + EXPECT_TRUE(test::ParseSinglePacket(raw, &parsed)); - EXPECT_EQ(kSenderSsrc, parsed().sender_ssrc()); - EXPECT_EQ(1u, parsed().report_blocks().size()); - EXPECT_EQ(kRemoteSsrc, parsed().report_blocks()[0].source_ssrc()); + EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); + EXPECT_EQ(1u, parsed.report_blocks().size()); + EXPECT_EQ(kRemoteSsrc, parsed.report_blocks()[0].source_ssrc()); } -TEST_F(RtcpPacketSenderReportTest, WithTwoReportBlocks) { +TEST(RtcpPacketSenderReportTest, CreateAndParseWithTwoReportBlocks) { ReportBlock rb1; rb1.To(kRemoteSsrc); ReportBlock rb2; @@ -90,16 +89,17 @@ TEST_F(RtcpPacketSenderReportTest, WithTwoReportBlocks) { EXPECT_TRUE(sr.WithReportBlock(rb1)); EXPECT_TRUE(sr.WithReportBlock(rb2)); - rtc::Buffer packet = sr.Build(); - ParsePacket(packet); + rtc::Buffer raw = sr.Build(); + SenderReport parsed; + EXPECT_TRUE(test::ParseSinglePacket(raw, &parsed)); - EXPECT_EQ(kSenderSsrc, parsed().sender_ssrc()); - EXPECT_EQ(2u, parsed().report_blocks().size()); - EXPECT_EQ(kRemoteSsrc, parsed().report_blocks()[0].source_ssrc()); - EXPECT_EQ(kRemoteSsrc + 1, parsed().report_blocks()[1].source_ssrc()); + EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); + EXPECT_EQ(2u, parsed.report_blocks().size()); + EXPECT_EQ(kRemoteSsrc, parsed.report_blocks()[0].source_ssrc()); + EXPECT_EQ(kRemoteSsrc + 1, parsed.report_blocks()[1].source_ssrc()); } -TEST_F(RtcpPacketSenderReportTest, WithTooManyReportBlocks) { +TEST(RtcpPacketSenderReportTest, CreateWithTooManyReportBlocks) { SenderReport sr; sr.From(kSenderSsrc); const size_t kMaxReportBlocks = (1 << 5) - 1;