From 32e590ec132ae1a22b92529c9ecca9b6563985ca Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Fri, 22 Jan 2016 11:04:56 +0100 Subject: [PATCH] class doesn't rely on structures in RTCPUtility to store data. supports several fci items in same packet. got accessors to read data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BUG=webrtc:5260 R=asapersson@webrtc.org, åsapersson Review URL: https://codereview.webrtc.org/1544403002 . Cr-Commit-Position: refs/heads/master@{#11354} --- webrtc/modules/rtp_rtcp/source/rtcp_packet.h | 5 +- .../rtcp_packet/compound_packet_unittest.cc | 6 + .../rtp_rtcp/source/rtcp_packet/fir.cc | 110 +++++++++------- .../modules/rtp_rtcp/source/rtcp_packet/fir.h | 50 ++++---- .../source/rtcp_packet/fir_unittest.cc | 117 +++++++++++++++--- .../rtp_rtcp/source/rtcp_receiver_unittest.cc | 4 +- webrtc/modules/rtp_rtcp/source/rtcp_sender.cc | 3 +- 7 files changed, 206 insertions(+), 89 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet.h b/webrtc/modules/rtp_rtcp/source/rtcp_packet.h index 965a667304..33064aac15 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet.h +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet.h @@ -38,11 +38,10 @@ class RawPacket; // // Fir fir; // fir.From(123); -// fir.To(234) -// fir.WithCommandSeqNum(123); +// fir.WithRequestTo(234, 56); // // size_t length = 0; // Builds an intra frame request -// uint8_t packet[kPacketSize]; // with sequence number 123. +// uint8_t packet[kPacketSize]; // with sequence number 56. // fir.Build(packet, &length, kPacketSize); // // RawPacket packet = fir.Build(); // Returns a RawPacket holding diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/compound_packet_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/compound_packet_unittest.cc index 0378566977..e52a65159b 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/compound_packet_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/compound_packet_unittest.cc @@ -30,9 +30,12 @@ using webrtc::test::RtcpPacketParser; namespace webrtc { const uint32_t kSenderSsrc = 0x12345678; +const uint32_t kRemoteSsrc = 0x23456789; +const uint8_t kSeqNo = 13; TEST(RtcpCompoundPacketTest, AppendPacket) { Fir fir; + fir.WithRequestTo(kRemoteSsrc, kSeqNo); ReportBlock rb; ReceiverReport rr; rr.From(kSenderSsrc); @@ -63,6 +66,7 @@ TEST(RtcpCompoundPacketTest, AppendPacketOnEmpty) { TEST(RtcpCompoundPacketTest, AppendPacketWithOwnAppendedPacket) { Fir fir; + fir.WithRequestTo(kRemoteSsrc, kSeqNo); Bye bye; ReportBlock rb; @@ -86,6 +90,7 @@ TEST(RtcpCompoundPacketTest, AppendPacketWithOwnAppendedPacket) { TEST(RtcpCompoundPacketTest, BuildWithInputBuffer) { Fir fir; + fir.WithRequestTo(kRemoteSsrc, kSeqNo); ReportBlock rb; ReceiverReport rr; rr.From(kSenderSsrc); @@ -117,6 +122,7 @@ TEST(RtcpCompoundPacketTest, BuildWithInputBuffer) { TEST(RtcpCompoundPacketTest, BuildWithTooSmallBuffer_FragmentedSend) { Fir fir; + fir.WithRequestTo(kRemoteSsrc, kSeqNo); ReportBlock rb; ReceiverReport rr; rr.From(kSenderSsrc); diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/fir.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/fir.cc index 674bbb7f75..9096af8192 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/fir.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/fir.cc @@ -14,65 +14,89 @@ #include "webrtc/base/logging.h" #include "webrtc/modules/rtp_rtcp/source/byte_io.h" -using webrtc::RTCPUtility::PT_PSFB; -using webrtc::RTCPUtility::RTCPPacketPSFBFIR; -using webrtc::RTCPUtility::RTCPPacketPSFBFIRItem; +using webrtc::RTCPUtility::RtcpCommonHeader; namespace webrtc { namespace rtcp { -namespace { -const uint32_t kUnusedMediaSourceSsrc0 = 0; - -void AssignUWord8(uint8_t* buffer, size_t* offset, uint8_t value) { - buffer[(*offset)++] = value; -} - -void AssignUWord24(uint8_t* buffer, size_t* offset, uint32_t value) { - ByteWriter::WriteBigEndian(buffer + *offset, value); - *offset += 3; -} - -void AssignUWord32(uint8_t* buffer, size_t* offset, uint32_t value) { - ByteWriter::WriteBigEndian(buffer + *offset, value); - *offset += 4; -} - +// RFC 4585: Feedback format. +// Common packet format: +// +// 0 1 2 3 +// 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +// |V=2|P| FMT | PT | length | +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +// | SSRC of packet sender | +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +// | SSRC of media source (unused) = 0 | +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +// : Feedback Control Information (FCI) : +// : : // Full intra request (FIR) (RFC 5104). -// +// The Feedback Control Information (FCI) for the Full Intra Request +// consists of one or more FCI entries. // FCI: -// -// 0 1 2 3 -// 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 -// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ -// | SSRC | -// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ -// | Seq nr. | Reserved | -// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ -void CreateFir(const RTCPPacketPSFBFIR& fir, - const RTCPPacketPSFBFIRItem& fir_item, - uint8_t* buffer, - size_t* pos) { - AssignUWord32(buffer, pos, fir.SenderSSRC); - AssignUWord32(buffer, pos, kUnusedMediaSourceSsrc0); - AssignUWord32(buffer, pos, fir_item.SSRC); - AssignUWord8(buffer, pos, fir_item.CommandSequenceNumber); - AssignUWord24(buffer, pos, 0); +// 0 1 2 3 +// 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +// | SSRC | +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +// | Seq nr. | Reserved = 0 | +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +bool Fir::Parse(const RtcpCommonHeader& header, const uint8_t* payload) { + RTC_CHECK(header.packet_type == kPacketType); + RTC_CHECK(header.count_or_format == kFeedbackMessageType); + + // The FCI field MUST contain one or more FIR entries. + if (header.payload_size_bytes < kCommonFeedbackLength + kFciLength) { + LOG(LS_WARNING) << "Packet is too small to be a valid FIR packet."; + return false; + } + + if ((header.payload_size_bytes - kCommonFeedbackLength) % kFciLength != 0) { + LOG(LS_WARNING) << "Invalid size for a valid FIR packet."; + return false; + } + + ParseCommonFeedback(payload); + + size_t number_of_fci_items = + (header.payload_size_bytes - kCommonFeedbackLength) / kFciLength; + const uint8_t* next_fci = payload + kCommonFeedbackLength; + items_.resize(number_of_fci_items); + for (Request& request : items_) { + request.ssrc = ByteReader::ReadBigEndian(next_fci); + request.seq_nr = ByteReader::ReadBigEndian(next_fci + 4); + next_fci += kFciLength; + } + return true; } -} // namespace bool Fir::Create(uint8_t* packet, size_t* index, size_t max_length, RtcpPacket::PacketReadyCallback* callback) const { + RTC_DCHECK(!items_.empty()); while (*index + BlockLength() > max_length) { if (!OnBufferFull(packet, index, callback)) return false; } - const uint8_t kFmt = 4; - CreateHeader(kFmt, PT_PSFB, HeaderLength(), packet, index); - CreateFir(fir_, fir_item_, packet, index); + size_t index_end = *index + BlockLength(); + CreateHeader(kFeedbackMessageType, kPacketType, HeaderLength(), packet, + index); + RTC_DCHECK_EQ(Psfb::media_ssrc(), 0u); + CreateCommonFeedback(packet + *index); + *index += kCommonFeedbackLength; + + const uint32_t kReserved = 0; + for (const Request& request : items_) { + ByteWriter::WriteBigEndian(packet + *index, request.ssrc); + ByteWriter::WriteBigEndian(packet + *index + 4, request.seq_nr); + ByteWriter::WriteBigEndian(packet + *index + 5, kReserved); + *index += kFciLength; + } + RTC_CHECK_EQ(*index, index_end); return true; } - } // namespace rtcp } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/fir.h b/webrtc/modules/rtp_rtcp/source/rtcp_packet/fir.h index c2b897e46e..992ce6da96 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/fir.h +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/fir.h @@ -6,38 +6,41 @@ * tree. An additional intellectual property rights grant can be found * in the file PATENTS. All contributing project authors may * be found in the AUTHORS file in the root of the source tree. - * */ #ifndef WEBRTC_MODULES_RTP_RTCP_SOURCE_RTCP_PACKET_FIR_H_ #define WEBRTC_MODULES_RTP_RTCP_SOURCE_RTCP_PACKET_FIR_H_ +#include + #include "webrtc/base/basictypes.h" -#include "webrtc/modules/rtp_rtcp/source/rtcp_packet.h" +#include "webrtc/modules/rtp_rtcp/source/rtcp_packet/psfb.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_utility.h" namespace webrtc { namespace rtcp { - // Full intra request (FIR) (RFC 5104). -class Fir : public RtcpPacket { +class Fir : public Psfb { public: - Fir() : RtcpPacket() { - memset(&fir_, 0, sizeof(fir_)); - memset(&fir_item_, 0, sizeof(fir_item_)); - } + static const uint8_t kFeedbackMessageType = 4; + struct Request { + Request() : ssrc(0), seq_nr(0) {} + Request(uint32_t ssrc, uint8_t seq_nr) : ssrc(ssrc), seq_nr(seq_nr) {} + uint32_t ssrc; + uint8_t seq_nr; + }; - virtual ~Fir() {} + Fir() {} + ~Fir() override {} - void From(uint32_t ssrc) { - fir_.SenderSSRC = ssrc; - } - void To(uint32_t ssrc) { - fir_item_.SSRC = ssrc; - } - void WithCommandSeqNum(uint8_t seq_num) { - fir_item_.CommandSequenceNumber = seq_num; + // 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. + + void WithRequestTo(uint32_t ssrc, uint8_t seq_num) { + items_.push_back(Request(ssrc, seq_num)); } + const std::vector& requests() const { return items_; } protected: bool Create(uint8_t* packet, @@ -46,15 +49,16 @@ class Fir : public RtcpPacket { RtcpPacket::PacketReadyCallback* callback) const override; private: - size_t BlockLength() const { - const size_t kFciLength = 8; - return kCommonFbFmtLength + kFciLength; + static const size_t kFciLength = 8; + size_t BlockLength() const override { + return kHeaderLength + kCommonFeedbackLength + kFciLength * items_.size(); } + // SSRC of media source is not used in FIR packet. Shadow base functions. + void To(uint32_t ssrc); + uint32_t media_ssrc() const; - RTCPUtility::RTCPPacketPSFBFIR fir_; - RTCPUtility::RTCPPacketPSFBFIRItem fir_item_; + std::vector items_; }; - } // namespace rtcp } // namespace webrtc #endif // WEBRTC_MODULES_RTP_RTCP_SOURCE_RTCP_PACKET_FIR_H_ diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/fir_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/fir_unittest.cc index b65a0c3f5a..089445eeab 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/fir_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/fir_unittest.cc @@ -12,31 +12,116 @@ #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" -#include "webrtc/test/rtcp_packet_parser.h" +using testing::AllOf; +using testing::ElementsAre; +using testing::ElementsAreArray; +using testing::Eq; +using testing::Field; +using testing::make_tuple; using webrtc::rtcp::Fir; using webrtc::rtcp::RawPacket; -using webrtc::test::RtcpPacketParser; +using webrtc::RTCPUtility::RtcpCommonHeader; +using webrtc::RTCPUtility::RtcpParseCommonHeader; namespace webrtc { +namespace { const uint32_t kSenderSsrc = 0x12345678; const uint32_t kRemoteSsrc = 0x23456789; +const uint8_t kSeqNr = 13; +// Manually created Fir packet matching constants above. +const uint8_t kPacket[] = {0x84, 206, 0x00, 0x04, + 0x12, 0x34, 0x56, 0x78, + 0x00, 0x00, 0x00, 0x00, + 0x23, 0x45, 0x67, 0x89, + 0x0d, 0x00, 0x00, 0x00}; -TEST(RtcpPacketFirTest, Fir) { - Fir fir; - fir.From(kSenderSsrc); - fir.To(kRemoteSsrc); - fir.WithCommandSeqNum(123); - - rtc::scoped_ptr packet(fir.Build()); - RtcpPacketParser parser; - parser.Parse(packet->Buffer(), packet->Length()); - EXPECT_EQ(1, parser.fir()->num_packets()); - EXPECT_EQ(kSenderSsrc, parser.fir()->Ssrc()); - EXPECT_EQ(1, parser.fir_item()->num_packets()); - EXPECT_EQ(kRemoteSsrc, parser.fir_item()->Ssrc()); - EXPECT_EQ(123U, parser.fir_item()->SeqNum()); +bool ParseFir(const uint8_t* buffer, size_t length, Fir* fir) { + RtcpCommonHeader header; + EXPECT_TRUE(RtcpParseCommonHeader(buffer, length, &header)); + EXPECT_THAT(header.BlockSize(), Eq(length)); + return fir->Parse(header, buffer + RtcpCommonHeader::kHeaderSizeBytes); } +TEST(RtcpPacketFirTest, Parse) { + Fir mutable_parsed; + EXPECT_TRUE(ParseFir(kPacket, sizeof(kPacket), &mutable_parsed)); + const Fir& parsed = mutable_parsed; // Read values from constant object. + + EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); + EXPECT_THAT(parsed.requests(), + ElementsAre(AllOf(Field(&Fir::Request::ssrc, Eq(kRemoteSsrc)), + Field(&Fir::Request::seq_nr, Eq(kSeqNr))))); +} + +TEST(RtcpPacketFirTest, Create) { + Fir fir; + fir.From(kSenderSsrc); + fir.WithRequestTo(kRemoteSsrc, kSeqNr); + + rtc::scoped_ptr packet = fir.Build(); + + EXPECT_THAT(make_tuple(packet->Buffer(), packet->Length()), + ElementsAreArray(kPacket)); +} + +TEST(RtcpPacketFirTest, TwoFciEntries) { + Fir fir; + fir.From(kSenderSsrc); + fir.WithRequestTo(kRemoteSsrc, kSeqNr); + fir.WithRequestTo(kRemoteSsrc + 1, kSeqNr + 1); + + rtc::scoped_ptr packet = fir.Build(); + Fir parsed; + EXPECT_TRUE(ParseFir(packet->Buffer(), packet->Length(), &parsed)); + + EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc()); + EXPECT_THAT(parsed.requests(), + ElementsAre(AllOf(Field(&Fir::Request::ssrc, Eq(kRemoteSsrc)), + Field(&Fir::Request::seq_nr, Eq(kSeqNr))), + AllOf(Field(&Fir::Request::ssrc, Eq(kRemoteSsrc + 1)), + Field(&Fir::Request::seq_nr, Eq(kSeqNr + 1))))); +} + +TEST(RtcpPacketFirTest, ParseFailsOnZeroFciEntries) { + Fir fir; + fir.From(kSenderSsrc); + fir.WithRequestTo(kRemoteSsrc, kSeqNr); + + rtc::scoped_ptr packet = fir.Build(); + + RtcpCommonHeader header; + RtcpParseCommonHeader(packet->Buffer(), packet->Length(), &header); + ASSERT_EQ(16u, header.payload_size_bytes); // Common: 8, 1xfci: 8. + header.payload_size_bytes = 8; // Common: 8, 0xfcis. + + Fir parsed; + EXPECT_FALSE(parsed.Parse( + header, packet->Buffer() + RtcpCommonHeader::kHeaderSizeBytes)); +} + +TEST(RtcpPacketFirTest, ParseFailsOnFractionalFciEntries) { + Fir fir; + fir.From(kSenderSsrc); + fir.WithRequestTo(kRemoteSsrc, kSeqNr); + fir.WithRequestTo(kRemoteSsrc + 1, kSeqNr + 1); + + rtc::scoped_ptr packet = fir.Build(); + + RtcpCommonHeader header; + RtcpParseCommonHeader(packet->Buffer(), packet->Length(), &header); + ASSERT_EQ(24u, header.payload_size_bytes); // Common: 8, 2xfcis: 16. + + const uint8_t* payload = + packet->Buffer() + RtcpCommonHeader::kHeaderSizeBytes; + Fir good; + EXPECT_TRUE(good.Parse(header, payload)); + for (size_t i = 1; i < 8; ++i) { + header.payload_size_bytes = 16 + i; + Fir bad; + EXPECT_FALSE(bad.Parse(header, payload)); + } +} +} // namespace } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc index d239d85d6c..7ec10bdb29 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc @@ -552,7 +552,7 @@ TEST_F(RtcpReceiverTest, InjectFirPacket) { rtcp_receiver_->SetSsrcs(kSourceSsrc, ssrcs); rtcp::Fir fir; - fir.To(kSourceSsrc); + fir.WithRequestTo(kSourceSsrc, 13); rtc::scoped_ptr packet(fir.Build()); EXPECT_EQ(0, InjectRtcpPacket(packet->Buffer(), packet->Length())); EXPECT_EQ(kRtcpFir, rtcp_packet_info_.rtcpPacketTypeFlags); @@ -565,7 +565,7 @@ TEST_F(RtcpReceiverTest, FirPacketNotToUsIgnored) { rtcp_receiver_->SetSsrcs(kSourceSsrc, ssrcs); rtcp::Fir fir; - fir.To(kSourceSsrc + 1); + fir.WithRequestTo(kSourceSsrc + 1, 13); rtc::scoped_ptr packet(fir.Build()); EXPECT_EQ(0, InjectRtcpPacket(packet->Buffer(), packet->Length())); EXPECT_EQ(0U, rtcp_packet_info_.rtcpPacketTypeFlags); diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc index 88ada8d957..37ea6922f7 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc @@ -553,8 +553,7 @@ rtc::scoped_ptr RTCPSender::BuildFIR(const RtcpContext& ctx) { rtcp::Fir* fir = new rtcp::Fir(); fir->From(ssrc_); - fir->To(remote_ssrc_); - fir->WithCommandSeqNum(sequence_number_fir_); + fir->WithRequestTo(remote_ssrc_, sequence_number_fir_); TRACE_EVENT_INSTANT0(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"), "RTCPSender::FIR");