From d215ade504482d3e9b399ee137a3bef103e6c733 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Thu, 12 May 2016 15:25:39 +0200 Subject: [PATCH] [rtcp] Remb::Parse updated not to use RTCPUtility bitrate field changed to 64bit to match Remb packet format BUG=webrtc:5260 R=asapersson@webrtc.org Review URL: https://codereview.webrtc.org/1959023002 . Cr-Commit-Position: refs/heads/master@{#12702} --- .../rtp_rtcp/source/rtcp_packet/remb.cc | 29 ++++++---- .../rtp_rtcp/source/rtcp_packet/remb.h | 18 +++---- .../source/rtcp_packet/remb_unittest.cc | 53 ++++++++++++------- 3 files changed, 61 insertions(+), 39 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/remb.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/remb.cc index 3b33982a83..2f59fbbd55 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/remb.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/remb.cc @@ -13,11 +13,11 @@ #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 Remb::kFeedbackMessageType; // Receiver Estimated Max Bitrate (REMB) (draft-alvestrand-rmcat-remb). // // 0 1 2 3 @@ -36,32 +36,39 @@ namespace rtcp { // 16 | SSRC feedback | // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ // : ... : -bool Remb::Parse(const RtcpCommonHeader& header, const uint8_t* payload) { - RTC_DCHECK(header.packet_type == kPacketType); - RTC_DCHECK(header.count_or_format == kFeedbackMessageType); +bool Remb::Parse(const CommonHeader& packet) { + RTC_DCHECK(packet.type() == kPacketType); + RTC_DCHECK_EQ(packet.fmt(), kFeedbackMessageType); - if (header.payload_size_bytes < 16) { - LOG(LS_WARNING) << "Payload length " << header.payload_size_bytes + if (packet.payload_size_bytes() < 16) { + LOG(LS_WARNING) << "Payload length " << packet.payload_size_bytes() << " is too small for Remb packet."; return false; } + const uint8_t* const payload = packet.payload(); if (kUniqueIdentifier != ByteReader::ReadBigEndian(&payload[8])) { LOG(LS_WARNING) << "REMB identifier not found, not a REMB packet."; return false; } uint8_t number_of_ssrcs = payload[12]; - if (header.payload_size_bytes != + if (packet.payload_size_bytes() != kCommonFeedbackLength + (2 + number_of_ssrcs) * 4) { - LOG(LS_WARNING) << "Payload size " << header.payload_size_bytes + LOG(LS_WARNING) << "Payload size " << packet.payload_size_bytes() << " does not match " << number_of_ssrcs << " ssrcs."; return false; } ParseCommonFeedback(payload); uint8_t exponenta = payload[13] >> 2; - uint32_t mantissa = (static_cast(payload[13] & 0x03) << 16) | + uint64_t mantissa = (static_cast(payload[13] & 0x03) << 16) | ByteReader::ReadBigEndian(&payload[14]); bitrate_bps_ = (mantissa << exponenta); + bool shift_overflow = (bitrate_bps_ >> exponenta) != mantissa; + if (shift_overflow) { + LOG(LS_ERROR) << "Invalid remb bitrate value : " << mantissa + << "*2^" << static_cast(exponenta); + return false; + } const uint8_t* next_ssrc = payload + 16; ssrcs_.clear(); @@ -111,7 +118,7 @@ bool Remb::Create(uint8_t* packet, ByteWriter::WriteBigEndian(packet + *index, kUniqueIdentifier); *index += sizeof(uint32_t); const uint32_t kMaxMantissa = 0x3ffff; // 18 bits. - uint32_t mantissa = bitrate_bps_; + uint64_t mantissa = bitrate_bps_; uint8_t exponenta = 0; while (mantissa > kMaxMantissa) { mantissa >>= 1; diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/remb.h b/webrtc/modules/rtp_rtcp/source/rtcp_packet/remb.h index fa807299b0..9f10921c99 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/remb.h +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/remb.h @@ -16,27 +16,27 @@ #include "webrtc/base/basictypes.h" #include "webrtc/base/constructormagic.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/psfb.h" -#include "webrtc/modules/rtp_rtcp/source/rtcp_utility.h" namespace webrtc { namespace rtcp { +class CommonHeader; + // Receiver Estimated Max Bitrate (REMB) (draft-alvestrand-rmcat-remb). class Remb : public Psfb { public: - static const uint8_t kFeedbackMessageType = 15; + static constexpr uint8_t kFeedbackMessageType = 15; Remb() : bitrate_bps_(0) {} ~Remb() 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); bool AppliesTo(uint32_t ssrc); bool AppliesToMany(const std::vector& ssrcs); - void WithBitrateBps(uint32_t bitrate_bps) { bitrate_bps_ = bitrate_bps; } + void WithBitrateBps(uint64_t bitrate_bps) { bitrate_bps_ = bitrate_bps; } - uint32_t bitrate_bps() const { return bitrate_bps_; } + uint64_t bitrate_bps() const { return bitrate_bps_; } const std::vector& ssrcs() const { return ssrcs_; } protected: @@ -50,14 +50,14 @@ class Remb : public Psfb { } private: - static const size_t kMaxNumberOfSsrcs = 0xff; - static const uint32_t kUniqueIdentifier = 0x52454D42; // 'R' 'E' 'M' 'B'. + static constexpr size_t kMaxNumberOfSsrcs = 0xff; + static constexpr uint32_t kUniqueIdentifier = 0x52454D42; // 'R' 'E' 'M' 'B'. // Media ssrc is unused, shadow base class setter and getter. void To(uint32_t); uint32_t media_ssrc() const; - uint32_t bitrate_bps_; + uint64_t bitrate_bps_; std::vector ssrcs_; RTC_DISALLOW_COPY_AND_ASSIGN(Remb); diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/remb_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/remb_unittest.cc index ee06972e2b..d504143f6f 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/remb_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/remb_unittest.cc @@ -12,32 +12,25 @@ #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::IsEmpty; using testing::make_tuple; using webrtc::rtcp::Remb; -using webrtc::RTCPUtility::RtcpCommonHeader; -using webrtc::RTCPUtility::RtcpParseCommonHeader; namespace webrtc { namespace { - const uint32_t kSenderSsrc = 0x12345678; const uint32_t kRemoteSsrcs[] = {0x23456789, 0x2345678a, 0x2345678b}; const uint32_t kBitrateBps = 0x3fb93 * 2; // 522022; +const uint64_t kBitrateBps64bit = 0x3fb93ULL << 30; const uint8_t kPacket[] = {0x8f, 206, 0x00, 0x07, 0x12, 0x34, 0x56, 0x78, 0x00, 0x00, 0x00, 0x00, 'R', 'E', 'M', 'B', 0x03, 0x07, 0xfb, 0x93, 0x23, 0x45, 0x67, 0x89, 0x23, 0x45, 0x67, 0x8a, 0x23, 0x45, 0x67, 0x8b}; const size_t kPacketLength = sizeof(kPacket); - -bool ParseRemb(const uint8_t* buffer, size_t length, Remb* remb) { - RtcpCommonHeader header; - EXPECT_TRUE(RtcpParseCommonHeader(buffer, length, &header)); - EXPECT_EQ(length, header.BlockSize()); - return remb->Parse(header, buffer + RtcpCommonHeader::kHeaderSizeBytes); -} +} // namespace TEST(RtcpPacketRembTest, Create) { Remb remb; @@ -55,7 +48,7 @@ TEST(RtcpPacketRembTest, Create) { TEST(RtcpPacketRembTest, Parse) { Remb remb; - EXPECT_TRUE(ParseRemb(kPacket, kPacketLength, &remb)); + EXPECT_TRUE(test::ParseSinglePacket(kPacket, &remb)); const Remb& parsed = remb; EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); @@ -70,19 +63,31 @@ TEST(RtcpPacketRembTest, CreateAndParseWithoutSsrcs) { rtc::Buffer packet = remb.Build(); Remb parsed; - EXPECT_TRUE(ParseRemb(packet.data(), packet.size(), &parsed)); + EXPECT_TRUE(test::ParseSinglePacket(packet, &parsed)); EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); EXPECT_EQ(kBitrateBps, parsed.bitrate_bps()); EXPECT_THAT(parsed.ssrcs(), IsEmpty()); } +TEST(RtcpPacketRembTest, CreateAndParse64bitBitrate) { + Remb remb; + remb.WithBitrateBps(kBitrateBps64bit); + rtc::Buffer packet = remb.Build(); + + Remb parsed; + EXPECT_TRUE(test::ParseSinglePacket(packet, &parsed)); + EXPECT_EQ(kBitrateBps64bit, parsed.bitrate_bps()); +} + TEST(RtcpPacketRembTest, ParseFailsOnTooSmallPacketToBeRemb) { - uint8_t packet[kPacketLength]; - memcpy(packet, kPacket, kPacketLength); - packet[3] = 3; // Make it too small. + // Make it too small. + constexpr size_t kTooSmallSize = (1 + 3) * 4; + uint8_t packet[kTooSmallSize]; + memcpy(packet, kPacket, kTooSmallSize); + packet[3] = 3; Remb remb; - EXPECT_FALSE(ParseRemb(packet, (1 + 3) * 4, &remb)); + EXPECT_FALSE(test::ParseSinglePacket(packet, &remb)); } TEST(RtcpPacketRembTest, ParseFailsWhenUniqueIdentifierIsNotRemb) { @@ -91,7 +96,17 @@ TEST(RtcpPacketRembTest, ParseFailsWhenUniqueIdentifierIsNotRemb) { packet[12] = 'N'; // Swap 'R' -> 'N' in the 'REMB' unique identifier. Remb remb; - EXPECT_FALSE(ParseRemb(packet, kPacketLength, &remb)); + EXPECT_FALSE(test::ParseSinglePacket(packet, &remb)); +} + +TEST(RtcpPacketRembTest, ParseFailsWhenBitrateDoNotFitIn64bits) { + uint8_t packet[kPacketLength]; + memcpy(packet, kPacket, kPacketLength); + packet[17] |= 0xfc; // Set exponenta component to maximum of 63. + packet[19] |= 0x02; // Ensure mantissa is at least 2. + + Remb remb; + EXPECT_FALSE(test::ParseSinglePacket(packet, &remb)); } TEST(RtcpPacketRembTest, ParseFailsWhenSsrcCountMismatchLength) { @@ -100,7 +115,7 @@ TEST(RtcpPacketRembTest, ParseFailsWhenSsrcCountMismatchLength) { packet[16]++; // Swap 3 -> 4 in the ssrcs count. Remb remb; - EXPECT_FALSE(ParseRemb(packet, kPacketLength, &remb)); + EXPECT_FALSE(test::ParseSinglePacket(packet, &remb)); } TEST(RtcpPacketRembTest, TooManySsrcs) { @@ -126,5 +141,5 @@ TEST(RtcpPacketRembTest, TooManySsrcsForBatchAssign) { // But not for another one. EXPECT_FALSE(remb.AppliesTo(kRemoteSsrc)); } -} // namespace + } // namespace webrtc