From 95e756035eb03971b8c66caa74b444094306562a Mon Sep 17 00:00:00 2001 From: danilchap Date: Mon, 1 Aug 2016 06:51:13 -0700 Subject: [PATCH] [rtcp] Nack::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/2028543002 Cr-Commit-Position: refs/heads/master@{#13586} --- .../rtp_rtcp/source/rtcp_packet/nack.cc | 60 ++++++----- .../rtp_rtcp/source/rtcp_packet/nack.h | 14 ++- .../source/rtcp_packet/nack_unittest.cc | 100 ++++++++---------- 3 files changed, 84 insertions(+), 90 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/nack.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/nack.cc index 8b9b354a06..0c1f577f7b 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/nack.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/nack.cc @@ -15,12 +15,12 @@ #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 { - +constexpr uint8_t Nack::kFeedbackMessageType; +constexpr size_t Nack::kNackItemLength; // RFC 4585: Feedback format. // // Common packet format: @@ -45,20 +45,23 @@ namespace rtcp { // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ // | PID | BLP | // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ -bool Nack::Parse(const RtcpCommonHeader& header, const uint8_t* payload) { - RTC_DCHECK(header.packet_type == kPacketType); - RTC_DCHECK(header.count_or_format == kFeedbackMessageType); +Nack::Nack() {} +Nack::~Nack() {} - if (header.payload_size_bytes < kCommonFeedbackLength + kNackItemLength) { - LOG(LS_WARNING) << "Payload length " << header.payload_size_bytes +bool Nack::Parse(const CommonHeader& packet) { + RTC_DCHECK_EQ(packet.type(), kPacketType); + RTC_DCHECK_EQ(packet.fmt(), kFeedbackMessageType); + + if (packet.payload_size_bytes() < kCommonFeedbackLength + kNackItemLength) { + LOG(LS_WARNING) << "Payload length " << packet.payload_size_bytes() << " is too small for a Nack."; return false; } size_t nack_items = - (header.payload_size_bytes - kCommonFeedbackLength) / kNackItemLength; + (packet.payload_size_bytes() - kCommonFeedbackLength) / kNackItemLength; - ParseCommonFeedback(payload); - const uint8_t* next_nack = payload + kCommonFeedbackLength; + ParseCommonFeedback(packet.payload()); + const uint8_t* next_nack = packet.payload() + kCommonFeedbackLength; packet_ids_.clear(); packed_.resize(nack_items); @@ -78,42 +81,44 @@ bool Nack::Create(uint8_t* packet, RtcpPacket::PacketReadyCallback* callback) const { RTC_DCHECK(!packed_.empty()); // If nack list can't fit in packet, try to fragment. - size_t nack_index = 0; - const size_t kCommonFbFmtLength = kHeaderLength + kCommonFeedbackLength; - do { + constexpr size_t kNackHeaderLength = kHeaderLength + kCommonFeedbackLength; + for (size_t nack_index = 0; nack_index < packed_.size();) { size_t bytes_left_in_buffer = max_length - *index; - if (bytes_left_in_buffer < kCommonFbFmtLength + kNackItemLength) { + if (bytes_left_in_buffer < kNackHeaderLength + kNackItemLength) { if (!OnBufferFull(packet, index, callback)) return false; continue; } size_t num_nack_fields = - std::min((bytes_left_in_buffer - kCommonFbFmtLength) / kNackItemLength, + std::min((bytes_left_in_buffer - kNackHeaderLength) / kNackItemLength, packed_.size() - nack_index); - size_t size_bytes = - (num_nack_fields * kNackItemLength) + kCommonFbFmtLength; - size_t header_length = ((size_bytes + 3) / 4) - 1; // As 32bit words - 1 - CreateHeader(kFeedbackMessageType, kPacketType, header_length, packet, + size_t payload_size_bytes = + kCommonFeedbackLength + (num_nack_fields * kNackItemLength); + size_t payload_size_32bits = + rtc::CheckedDivExact(payload_size_bytes, 4); + CreateHeader(kFeedbackMessageType, kPacketType, payload_size_32bits, packet, index); + CreateCommonFeedback(packet + *index); *index += kCommonFeedbackLength; - size_t end_index = nack_index + num_nack_fields; - for (; nack_index < end_index; ++nack_index) { - const auto& item = packed_[nack_index]; + + size_t nack_end_index = nack_index + num_nack_fields; + for (; nack_index < nack_end_index; ++nack_index) { + const PackedNack& item = packed_[nack_index]; ByteWriter::WriteBigEndian(packet + *index + 0, item.first_pid); ByteWriter::WriteBigEndian(packet + *index + 2, item.bitmask); *index += kNackItemLength; } RTC_DCHECK_LE(*index, max_length); - } while (nack_index < packed_.size()); + } return true; } size_t Nack::BlockLength() const { - return (packed_.size() * kNackItemLength) + kCommonFeedbackLength + - kHeaderLength; + return kHeaderLength + kCommonFeedbackLength + + packed_.size() * kNackItemLength; } void Nack::WithList(const uint16_t* nack_list, size_t length) { @@ -153,9 +158,10 @@ void Nack::Unpack() { for (const PackedNack& item : packed_) { packet_ids_.push_back(item.first_pid); uint16_t pid = item.first_pid + 1; - for (uint16_t bitmask = item.bitmask; bitmask != 0; bitmask >>= 1, ++pid) + for (uint16_t bitmask = item.bitmask; bitmask != 0; bitmask >>= 1, ++pid) { if (bitmask & 1) packet_ids_.push_back(pid); + } } } diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/nack.h b/webrtc/modules/rtp_rtcp/source/rtcp_packet/nack.h index b6acae5aab..83b3ba954b 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/nack.h +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/nack.h @@ -16,21 +16,19 @@ #include "webrtc/base/basictypes.h" #include "webrtc/base/constructormagic.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/rtpfb.h" -#include "webrtc/modules/rtp_rtcp/source/rtcp_utility.h" namespace webrtc { namespace rtcp { +class CommonHeader; class Nack : public Rtpfb { public: - const uint8_t kFeedbackMessageType = 1; - Nack() {} - - virtual ~Nack() {} + static constexpr uint8_t kFeedbackMessageType = 1; + Nack(); + ~Nack() 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 WithList(const uint16_t* nack_list, size_t length); const std::vector& packet_ids() const { return packet_ids_; } @@ -44,7 +42,7 @@ class Nack : public Rtpfb { size_t BlockLength() const override; private: - const size_t kNackItemLength = 4; + static constexpr size_t kNackItemLength = 4; struct PackedNack { uint16_t first_pid; uint16_t bitmask; diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/nack_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/nack_unittest.cc index f32bdc7b86..d335d66bf8 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/nack_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/nack_unittest.cc @@ -12,36 +12,48 @@ #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" - -using ::testing::_; -using ::testing::ElementsAreArray; -using ::testing::Invoke; -using ::testing::UnorderedElementsAreArray; - -using webrtc::rtcp::Nack; -using webrtc::RTCPUtility::RtcpCommonHeader; -using webrtc::RTCPUtility::RtcpParseCommonHeader; +#include "webrtc/test/rtcp_packet_parser.h" namespace webrtc { namespace { -const uint32_t kSenderSsrc = 0x12345678; -const uint32_t kRemoteSsrc = 0x23456789; +using ::testing::_; +using ::testing::ElementsAreArray; +using ::testing::Invoke; +using ::testing::make_tuple; +using ::testing::UnorderedElementsAreArray; +using ::webrtc::rtcp::Nack; -const uint16_t kList[] = {0, 1, 3, 8, 16}; -const size_t kListLength = sizeof(kList) / sizeof(kList[0]); -const uint8_t kPacket[] = {0x81, 205, 0x00, 0x03, 0x12, 0x34, 0x56, 0x78, - 0x23, 0x45, 0x67, 0x89, 0x00, 0x00, 0x80, 0x85}; -const size_t kPacketLength = sizeof(kPacket); +constexpr uint32_t kSenderSsrc = 0x12345678; +constexpr uint32_t kRemoteSsrc = 0x23456789; -const uint16_t kWrapList[] = {0xffdc, 0xffec, 0xfffe, 0xffff, 0x0000, - 0x0001, 0x0003, 0x0014, 0x0064}; -const size_t kWrapListLength = sizeof(kWrapList) / sizeof(kWrapList[0]); -const uint8_t kWrapPacket[] = {0x81, 205, 0x00, 0x06, 0x12, 0x34, 0x56, 0x78, - 0x23, 0x45, 0x67, 0x89, 0xff, 0xdc, 0x80, 0x00, - 0xff, 0xfe, 0x00, 0x17, 0x00, 0x14, 0x00, 0x00, - 0x00, 0x64, 0x00, 0x00}; -const size_t kWrapPacketLength = sizeof(kWrapPacket); +constexpr uint16_t kList[] = {0, 1, 3, 8, 16}; +constexpr size_t kListLength = sizeof(kList) / sizeof(kList[0]); +constexpr uint8_t kVersionBits = 2 << 6; +// clang-format off +constexpr uint8_t kPacket[] = { + kVersionBits | Nack::kFeedbackMessageType, Nack::kPacketType, 0, 3, + 0x12, 0x34, 0x56, 0x78, + 0x23, 0x45, 0x67, 0x89, + 0x00, 0x00, 0x80, 0x85}; + +constexpr uint16_t kWrapList[] = {0xffdc, 0xffec, 0xfffe, 0xffff, 0x0000, + 0x0001, 0x0003, 0x0014, 0x0064}; +constexpr size_t kWrapListLength = sizeof(kWrapList) / sizeof(kWrapList[0]); +constexpr uint8_t kWrapPacket[] = { + kVersionBits | Nack::kFeedbackMessageType, Nack::kPacketType, 0, 6, + 0x12, 0x34, 0x56, 0x78, + 0x23, 0x45, 0x67, 0x89, + 0xff, 0xdc, 0x80, 0x00, + 0xff, 0xfe, 0x00, 0x17, + 0x00, 0x14, 0x00, 0x00, + 0x00, 0x64, 0x00, 0x00}; +constexpr uint8_t kTooSmallPacket[] = { + kVersionBits | Nack::kFeedbackMessageType, Nack::kPacketType, 0, 2, + 0x12, 0x34, 0x56, 0x78, + 0x23, 0x45, 0x67, 0x89}; +// clang-format on +} // namespace TEST(RtcpPacketNackTest, Create) { Nack nack; @@ -51,18 +63,13 @@ TEST(RtcpPacketNackTest, Create) { rtc::Buffer packet = nack.Build(); - EXPECT_EQ(kPacketLength, packet.size()); - EXPECT_EQ(0, memcmp(kPacket, packet.data(), kPacketLength)); + EXPECT_THAT(make_tuple(packet.data(), packet.size()), + ElementsAreArray(kPacket)); } TEST(RtcpPacketNackTest, Parse) { - RtcpCommonHeader header; - EXPECT_TRUE(RtcpParseCommonHeader(kPacket, kPacketLength, &header)); - EXPECT_EQ(kPacketLength, header.BlockSize()); Nack parsed; - - EXPECT_TRUE( - parsed.Parse(header, kPacket + RtcpCommonHeader::kHeaderSizeBytes)); + EXPECT_TRUE(test::ParseSinglePacket(kPacket, &parsed)); const Nack& const_parsed = parsed; EXPECT_EQ(kSenderSsrc, const_parsed.sender_ssrc()); @@ -78,18 +85,13 @@ TEST(RtcpPacketNackTest, CreateWrap) { rtc::Buffer packet = nack.Build(); - EXPECT_EQ(kWrapPacketLength, packet.size()); - EXPECT_EQ(0, memcmp(kWrapPacket, packet.data(), kWrapPacketLength)); + EXPECT_THAT(make_tuple(packet.data(), packet.size()), + ElementsAreArray(kWrapPacket)); } TEST(RtcpPacketNackTest, ParseWrap) { - RtcpCommonHeader header; - EXPECT_TRUE(RtcpParseCommonHeader(kWrapPacket, kWrapPacketLength, &header)); - EXPECT_EQ(kWrapPacketLength, header.BlockSize()); - Nack parsed; - EXPECT_TRUE( - parsed.Parse(header, kWrapPacket + RtcpCommonHeader::kHeaderSizeBytes)); + EXPECT_TRUE(test::ParseSinglePacket(kWrapPacket, &parsed)); EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); EXPECT_EQ(kRemoteSsrc, parsed.media_ssrc()); @@ -109,10 +111,7 @@ TEST(RtcpPacketNackTest, BadOrder) { rtc::Buffer packet = nack.Build(); Nack parsed; - RtcpCommonHeader header; - EXPECT_TRUE(RtcpParseCommonHeader(packet.data(), packet.size(), &header)); - EXPECT_TRUE(parsed.Parse( - header, packet.data() + RtcpCommonHeader::kHeaderSizeBytes)); + EXPECT_TRUE(test::ParseSinglePacket(packet, &parsed)); EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); EXPECT_EQ(kRemoteSsrc, parsed.media_ssrc()); @@ -136,12 +135,8 @@ TEST(RtcpPacketNackTest, CreateFragmented) { public: explicit NackVerifier(std::vector ids) : ids_(ids) {} void operator()(uint8_t* data, size_t length) { - RtcpCommonHeader header; - EXPECT_TRUE(RtcpParseCommonHeader(data, length, &header)); - EXPECT_EQ(length, header.BlockSize()); Nack nack; - EXPECT_TRUE( - nack.Parse(header, data + RtcpCommonHeader::kHeaderSizeBytes)); + EXPECT_TRUE(test::ParseSinglePacket(data, length, &nack)); EXPECT_EQ(kSenderSsrc, nack.sender_ssrc()); EXPECT_EQ(kRemoteSsrc, nack.media_ssrc()); EXPECT_THAT(nack.packet_ids(), ElementsAreArray(ids_)); @@ -176,13 +171,8 @@ TEST(RtcpPacketNackTest, CreateFailsWithTooSmallBuffer) { } TEST(RtcpPacketNackTest, ParseFailsWithTooSmallBuffer) { - RtcpCommonHeader header; - EXPECT_TRUE(RtcpParseCommonHeader(kPacket, kPacketLength, &header)); - header.payload_size_bytes--; // Damage the packet Nack parsed; - EXPECT_FALSE( - parsed.Parse(header, kPacket + RtcpCommonHeader::kHeaderSizeBytes)); + EXPECT_FALSE(test::ParseSinglePacket(kTooSmallPacket, &parsed)); } -} // namespace } // namespace webrtc