From f3de65aebe80b2c6f3fa6b6de1e710086debdfec Mon Sep 17 00:00:00 2001 From: Yosef Twaik Date: Thu, 4 May 2023 18:19:54 +0300 Subject: [PATCH] Change ReceivedFecPacket to have list of ssrcs, seq nums and masks. This change replaces ReceivedFecPacket FEC header fields with vectors (for protected ssrcs, sequence numbers and masks), which is needed to support protection of multiple ssrcs in the same FEC packet (as part of the flexfec RFC - https://datatracker.ietf.org/doc/html/rfc8627). Bug: webrtc:15002 Change-Id: I82c54203fcfec10c760f34f805cc6308562e3df1 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/303200 Reviewed-by: Danil Chapovalov Commit-Queue: Rasmus Brandt Reviewed-by: Rasmus Brandt Cr-Commit-Position: refs/heads/main@{#40075} --- .../source/flexfec_header_reader_writer.cc | 9 ++- .../flexfec_header_reader_writer_unittest.cc | 57 ++++++++++--------- .../source/forward_error_correction.cc | 22 ++++--- .../source/forward_error_correction.h | 19 +++++-- .../source/ulpfec_header_reader_writer.cc | 8 +-- .../ulpfec_header_reader_writer_unittest.cc | 29 ++++++---- 6 files changed, 84 insertions(+), 60 deletions(-) diff --git a/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc b/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc index 2fbb3caae8..7a7bff70e3 100644 --- a/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc +++ b/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc @@ -195,11 +195,10 @@ bool FlexfecHeaderReader::ReadFecHeader( // Store "ULPFECized" packet mask info. fec_packet->fec_header_size = FlexfecHeaderSize(packet_mask_size); - fec_packet->protected_ssrc = protected_ssrc; - fec_packet->seq_num_base = seq_num_base; - fec_packet->packet_mask_offset = kPacketMaskOffset; - fec_packet->packet_mask_size = packet_mask_size; - + fec_packet->protected_streams = {{.ssrc = protected_ssrc, + .seq_num_base = seq_num_base, + .packet_mask_offset = kPacketMaskOffset, + .packet_mask_size = packet_mask_size}}; // In FlexFEC, all media packets are protected in their entirety. fec_packet->protection_length = fec_packet->pkt->data.size() - fec_packet->fec_header_size; diff --git a/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc b/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc index 4a24e90ec3..54875105c0 100644 --- a/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc +++ b/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc @@ -30,6 +30,9 @@ namespace { using Packet = ForwardErrorCorrection::Packet; using ReceivedFecPacket = ForwardErrorCorrection::ReceivedFecPacket; +using ::testing::ElementsAreArray; +using ::testing::make_tuple; +using ::testing::SizeIs; // General. Assume single-stream protection. constexpr uint32_t kMediaSsrc = 1254983; @@ -103,20 +106,20 @@ void VerifyReadHeaders(size_t expected_fec_header_size, const ReceivedFecPacket& read_packet) { EXPECT_EQ(expected_fec_header_size, read_packet.fec_header_size); EXPECT_EQ(ByteReader::ReadBigEndian(kProtSsrc), - read_packet.protected_ssrc); + read_packet.protected_streams[0].ssrc); EXPECT_EQ(ByteReader::ReadBigEndian(kSnBase), - read_packet.seq_num_base); - const size_t packet_mask_offset = read_packet.packet_mask_offset; + read_packet.protected_streams[0].seq_num_base); + auto packet_mask_offset = read_packet.protected_streams[0].packet_mask_offset; EXPECT_EQ(kFlexfecPacketMaskOffset, packet_mask_offset); - EXPECT_EQ(expected_packet_mask_size, read_packet.packet_mask_size); + EXPECT_EQ(expected_packet_mask_size, + read_packet.protected_streams[0].packet_mask_size); EXPECT_EQ(read_packet.pkt->data.size() - expected_fec_header_size, read_packet.protection_length); // Ensure that the K-bits are removed and the packet mask has been packed. EXPECT_THAT( - ::testing::make_tuple(read_packet.pkt->data.cdata() + packet_mask_offset, - read_packet.packet_mask_size), - ::testing::ElementsAreArray(expected_packet_mask, - expected_packet_mask_size)); + make_tuple(read_packet.pkt->data.cdata() + packet_mask_offset, + read_packet.protected_streams[0].packet_mask_size), + ElementsAreArray(expected_packet_mask, expected_packet_mask_size)); } void VerifyFinalizedHeaders(const uint8_t* expected_packet_mask, @@ -129,10 +132,9 @@ void VerifyFinalizedHeaders(const uint8_t* expected_packet_mask, EXPECT_EQ(kMediaSsrc, ByteReader::ReadBigEndian(packet + 12)); EXPECT_EQ(kMediaStartSeqNum, ByteReader::ReadBigEndian(packet + 16)); - EXPECT_THAT(::testing::make_tuple(packet + kFlexfecPacketMaskOffset, - expected_packet_mask_size), - ::testing::ElementsAreArray(expected_packet_mask, - expected_packet_mask_size)); + EXPECT_THAT( + make_tuple(packet + kFlexfecPacketMaskOffset, expected_packet_mask_size), + ElementsAreArray(expected_packet_mask, expected_packet_mask_size)); } void VerifyWrittenAndReadHeaders(size_t expected_fec_header_size, @@ -142,25 +144,26 @@ void VerifyWrittenAndReadHeaders(size_t expected_fec_header_size, const ReceivedFecPacket& read_packet) { EXPECT_EQ(kFlexfecSsrc, read_packet.ssrc); EXPECT_EQ(expected_fec_header_size, read_packet.fec_header_size); - EXPECT_EQ(kMediaSsrc, read_packet.protected_ssrc); - EXPECT_EQ(kMediaStartSeqNum, read_packet.seq_num_base); - EXPECT_EQ(kFlexfecPacketMaskOffset, read_packet.packet_mask_offset); - ASSERT_EQ(expected_packet_mask_size, read_packet.packet_mask_size); + ASSERT_THAT(read_packet.protected_streams, SizeIs(1)); + EXPECT_EQ(read_packet.protected_streams[0].ssrc, kMediaSsrc); + EXPECT_EQ(read_packet.protected_streams[0].seq_num_base, kMediaStartSeqNum); + EXPECT_EQ(read_packet.protected_streams[0].packet_mask_offset, + kFlexfecPacketMaskOffset); + ASSERT_EQ(read_packet.protected_streams[0].packet_mask_size, + expected_packet_mask_size); EXPECT_EQ(written_packet.data.size() - expected_fec_header_size, read_packet.protection_length); // Verify that the call to ReadFecHeader did normalize the packet masks. - EXPECT_THAT(::testing::make_tuple( - read_packet.pkt->data.cdata() + kFlexfecPacketMaskOffset, - read_packet.packet_mask_size), - ::testing::ElementsAreArray(expected_packet_mask, - expected_packet_mask_size)); + EXPECT_THAT( + make_tuple(read_packet.pkt->data.cdata() + kFlexfecPacketMaskOffset, + read_packet.protected_streams[0].packet_mask_size), + ElementsAreArray(expected_packet_mask, expected_packet_mask_size)); // Verify that the call to ReadFecHeader did not tamper with the payload. - EXPECT_THAT(::testing::make_tuple( - read_packet.pkt->data.cdata() + read_packet.fec_header_size, - read_packet.pkt->data.size() - read_packet.fec_header_size), - ::testing::ElementsAreArray( - written_packet.data.cdata() + expected_fec_header_size, - written_packet.data.size() - expected_fec_header_size)); + EXPECT_THAT( + make_tuple(read_packet.pkt->data.cdata() + read_packet.fec_header_size, + read_packet.pkt->data.size() - read_packet.fec_header_size), + ElementsAreArray(written_packet.data.cdata() + expected_fec_header_size, + written_packet.data.size() - expected_fec_header_size)); } } // namespace diff --git a/modules/rtp_rtcp/source/forward_error_correction.cc b/modules/rtp_rtcp/source/forward_error_correction.cc index 1462c2f481..ada9742b2d 100644 --- a/modules/rtp_rtcp/source/forward_error_correction.cc +++ b/modules/rtp_rtcp/source/forward_error_correction.cc @@ -407,24 +407,29 @@ void ForwardErrorCorrection::InsertFecPacket( return; } - // TODO(brandtr): Update here when we support multistream protection. - if (fec_packet->protected_ssrc != protected_media_ssrc_) { + RTC_CHECK_EQ(fec_packet->protected_streams.size(), 1); + + if (fec_packet->protected_streams[0].ssrc != protected_media_ssrc_) { RTC_LOG(LS_INFO) << "Received FEC packet is protecting an unknown media SSRC; dropping."; return; } - if (fec_packet->packet_mask_offset + fec_packet->packet_mask_size > + if (fec_packet->protected_streams[0].packet_mask_offset + + fec_packet->protected_streams[0].packet_mask_size > fec_packet->pkt->data.size()) { RTC_LOG(LS_INFO) << "Received corrupted FEC packet; dropping."; return; } // Parse packet mask from header and represent as protected packets. - for (uint16_t byte_idx = 0; byte_idx < fec_packet->packet_mask_size; + for (uint16_t byte_idx = 0; + byte_idx < fec_packet->protected_streams[0].packet_mask_size; ++byte_idx) { uint8_t packet_mask = - fec_packet->pkt->data[fec_packet->packet_mask_offset + byte_idx]; + fec_packet->pkt + ->data[fec_packet->protected_streams[0].packet_mask_offset + + byte_idx]; for (uint16_t bit_idx = 0; bit_idx < 8; ++bit_idx) { if (packet_mask & (1 << (7 - bit_idx))) { std::unique_ptr protected_packet( @@ -432,7 +437,8 @@ void ForwardErrorCorrection::InsertFecPacket( // This wraps naturally with the sequence number. protected_packet->ssrc = protected_media_ssrc_; protected_packet->seq_num = static_cast( - fec_packet->seq_num_base + (byte_idx << 3) + bit_idx); + fec_packet->protected_streams[0].seq_num_base + (byte_idx << 3) + + bit_idx); protected_packet->pkt = nullptr; fec_packet->protected_packets.push_back(std::move(protected_packet)); } @@ -583,8 +589,7 @@ bool ForwardErrorCorrection::FinishPacketRecovery( // Set the SN field. ByteWriter::WriteBigEndian(&data[2], recovered_packet->seq_num); // Set the SSRC field. - ByteWriter::WriteBigEndian(&data[8], fec_packet.protected_ssrc); - recovered_packet->ssrc = fec_packet.protected_ssrc; + ByteWriter::WriteBigEndian(&data[8], recovered_packet->ssrc); return true; } @@ -640,6 +645,7 @@ bool ForwardErrorCorrection::RecoverPacket(const ReceivedFecPacket& fec_packet, if (protected_packet->pkt == nullptr) { // This is the packet we're recovering. recovered_packet->seq_num = protected_packet->seq_num; + recovered_packet->ssrc = protected_packet->ssrc; } else { XorHeaders(*protected_packet->pkt, recovered_packet->pkt.get()); XorPayloads(*protected_packet->pkt, diff --git a/modules/rtp_rtcp/source/forward_error_correction.h b/modules/rtp_rtcp/source/forward_error_correction.h index ac8f5361e4..3da3e6b71a 100644 --- a/modules/rtp_rtcp/source/forward_error_correction.h +++ b/modules/rtp_rtcp/source/forward_error_correction.h @@ -18,6 +18,7 @@ #include #include +#include "absl/container/inlined_vector.h" #include "api/scoped_refptr.h" #include "api/units/timestamp.h" #include "modules/include/module_fec_types.h" @@ -118,11 +119,23 @@ class ForwardErrorCorrection { using ProtectedPacketList = std::list>; + struct ProtectedStream { + uint32_t ssrc = 0; + uint16_t seq_num_base = 0; + size_t packet_mask_offset = 0; // Relative start of FEC header. + size_t packet_mask_size = 0; + }; + // Used for internal storage of received FEC packets in a list. // // TODO(holmer): Refactor into a proper class. class ReceivedFecPacket : public SortablePacket { public: + // SSRC count is limited by 4 bits of CSRC count in RTP header (max 15). + // Since most of the time number of SSRCs will be low (probably 1 most of + // the time) setting this value to 4 for optimization. + static constexpr size_t kInlinedSsrcsVectorSize = 4; + ReceivedFecPacket(); ~ReceivedFecPacket(); @@ -132,10 +145,8 @@ class ForwardErrorCorrection { uint32_t ssrc; // FEC header fields. size_t fec_header_size; - uint32_t protected_ssrc; - uint16_t seq_num_base; - size_t packet_mask_offset; // Relative start of FEC header. - size_t packet_mask_size; + absl::InlinedVector + protected_streams; size_t protection_length; // Raw data. rtc::scoped_refptr pkt; diff --git a/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc b/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc index 8378a8f19f..e93d7f0e31 100644 --- a/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc +++ b/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc @@ -71,10 +71,10 @@ bool UlpfecHeaderReader::ReadFecHeader( l_bit ? kUlpfecPacketMaskSizeLBitSet : kUlpfecPacketMaskSizeLBitClear; fec_packet->fec_header_size = UlpfecHeaderSize(packet_mask_size); uint16_t seq_num_base = ByteReader::ReadBigEndian(&data[2]); - fec_packet->protected_ssrc = fec_packet->ssrc; // Due to RED. - fec_packet->seq_num_base = seq_num_base; - fec_packet->packet_mask_offset = kPacketMaskOffset; - fec_packet->packet_mask_size = packet_mask_size; + fec_packet->protected_streams = {{.ssrc = fec_packet->ssrc, // Due to RED. + .seq_num_base = seq_num_base, + .packet_mask_offset = kPacketMaskOffset, + .packet_mask_size = packet_mask_size}}; fec_packet->protection_length = ByteReader::ReadBigEndian(&data[10]); diff --git a/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc b/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc index a190a548e4..89581ee960 100644 --- a/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc +++ b/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc @@ -21,6 +21,7 @@ #include "modules/rtp_rtcp/source/forward_error_correction_internal.h" #include "rtc_base/checks.h" #include "rtc_base/random.h" +#include "test/gmock.h" #include "test/gtest.h" namespace webrtc { @@ -29,6 +30,7 @@ namespace { using Packet = ForwardErrorCorrection::Packet; using ReceivedFecPacket = ForwardErrorCorrection::ReceivedFecPacket; +using ::testing::SizeIs; constexpr uint32_t kMediaSsrc = 1254983; constexpr uint16_t kMediaStartSeqNum = 825; @@ -79,16 +81,19 @@ void VerifyHeaders(size_t expected_fec_header_size, const ReceivedFecPacket& read_packet) { EXPECT_EQ(kMediaSsrc, read_packet.ssrc); EXPECT_EQ(expected_fec_header_size, read_packet.fec_header_size); - EXPECT_EQ(kMediaSsrc, read_packet.protected_ssrc); - EXPECT_EQ(kMediaStartSeqNum, read_packet.seq_num_base); - EXPECT_EQ(kUlpfecPacketMaskOffset, read_packet.packet_mask_offset); - ASSERT_EQ(expected_packet_mask_size, read_packet.packet_mask_size); + ASSERT_THAT(read_packet.protected_streams, SizeIs(1)); + EXPECT_EQ(read_packet.protected_streams[0].ssrc, kMediaSsrc); + EXPECT_EQ(read_packet.protected_streams[0].seq_num_base, kMediaStartSeqNum); + EXPECT_EQ(read_packet.protected_streams[0].packet_mask_offset, + kUlpfecPacketMaskOffset); + ASSERT_EQ(read_packet.protected_streams[0].packet_mask_size, + expected_packet_mask_size); EXPECT_EQ(written_packet.data.size() - expected_fec_header_size, read_packet.protection_length); EXPECT_EQ(0, memcmp(expected_packet_mask, read_packet.pkt->data.MutableData() + - read_packet.packet_mask_offset, - read_packet.packet_mask_size)); + read_packet.protected_streams[0].packet_mask_offset, + read_packet.protected_streams[0].packet_mask_size)); // Verify that the call to ReadFecHeader did not tamper with the payload. EXPECT_EQ(0, memcmp(written_packet.data.data() + expected_fec_header_size, read_packet.pkt->data.cdata() + expected_fec_header_size, @@ -114,9 +119,9 @@ TEST(UlpfecHeaderReaderTest, ReadsSmallHeader) { EXPECT_TRUE(reader.ReadFecHeader(&read_packet)); EXPECT_EQ(14U, read_packet.fec_header_size); - EXPECT_EQ(0xabcdU, read_packet.seq_num_base); - EXPECT_EQ(12U, read_packet.packet_mask_offset); - EXPECT_EQ(2U, read_packet.packet_mask_size); + EXPECT_EQ(0xabcdU, read_packet.protected_streams[0].seq_num_base); + EXPECT_EQ(12U, read_packet.protected_streams[0].packet_mask_offset); + EXPECT_EQ(2U, read_packet.protected_streams[0].packet_mask_size); EXPECT_EQ(0x1122U, read_packet.protection_length); } @@ -138,9 +143,9 @@ TEST(UlpfecHeaderReaderTest, ReadsLargeHeader) { EXPECT_TRUE(reader.ReadFecHeader(&read_packet)); EXPECT_EQ(18U, read_packet.fec_header_size); - EXPECT_EQ(0xabcdU, read_packet.seq_num_base); - EXPECT_EQ(12U, read_packet.packet_mask_offset); - EXPECT_EQ(6U, read_packet.packet_mask_size); + EXPECT_EQ(0xabcdU, read_packet.protected_streams[0].seq_num_base); + EXPECT_EQ(12U, read_packet.protected_streams[0].packet_mask_offset); + EXPECT_EQ(6U, read_packet.protected_streams[0].packet_mask_size); EXPECT_EQ(0x1122U, read_packet.protection_length); }