diff --git a/modules/rtp_rtcp/source/rtcp_packet.cc b/modules/rtp_rtcp/source/rtcp_packet.cc index 194b992ef8..bac03e73d2 100644 --- a/modules/rtp_rtcp/source/rtcp_packet.cc +++ b/modules/rtp_rtcp/source/rtcp_packet.cc @@ -52,7 +52,8 @@ bool RtcpPacket::OnBufferFull(uint8_t* packet, size_t RtcpPacket::HeaderLength() const { size_t length_in_bytes = BlockLength(); RTC_DCHECK_GT(length_in_bytes, 0); - RTC_DCHECK_EQ(length_in_bytes % 4, 0) << "Padding not supported"; + RTC_DCHECK_EQ(length_in_bytes % 4, 0) + << "Padding must be handled by each subclass."; // Length in 32-bit words without common header. return (length_in_bytes - kHeaderLength) / 4; } @@ -71,12 +72,23 @@ void RtcpPacket::CreateHeader( size_t length, uint8_t* buffer, size_t* pos) { + CreateHeader(count_or_format, packet_type, length, /*padding=*/false, buffer, + pos); +} + +void RtcpPacket::CreateHeader( + size_t count_or_format, // Depends on packet type. + uint8_t packet_type, + size_t length, + bool padding, + uint8_t* buffer, + size_t* pos) { RTC_DCHECK_LE(length, 0xffffU); RTC_DCHECK_LE(count_or_format, 0x1f); constexpr uint8_t kVersionBits = 2 << 6; - constexpr uint8_t kNoPaddingBit = 0 << 5; + uint8_t padding_bit = padding ? 1 << 5 : 0; buffer[*pos + 0] = - kVersionBits | kNoPaddingBit | static_cast(count_or_format); + kVersionBits | padding_bit | static_cast(count_or_format); buffer[*pos + 1] = packet_type; buffer[*pos + 2] = (length >> 8) & 0xff; buffer[*pos + 3] = length & 0xff; diff --git a/modules/rtp_rtcp/source/rtcp_packet.h b/modules/rtp_rtcp/source/rtcp_packet.h index 40e51e87ad..94bf9f0823 100644 --- a/modules/rtp_rtcp/source/rtcp_packet.h +++ b/modules/rtp_rtcp/source/rtcp_packet.h @@ -87,6 +87,13 @@ class RtcpPacket { uint8_t* buffer, size_t* pos); + static void CreateHeader(size_t count_or_format, + uint8_t packet_type, + size_t block_length, // Payload size in 32bit words. + bool padding, // True if there are padding bytes. + uint8_t* buffer, + size_t* pos); + bool OnBufferFull(uint8_t* packet, size_t* index, PacketReadyCallback callback) const; diff --git a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc index 28165594be..81eda9e5ce 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc +++ b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc @@ -521,6 +521,10 @@ size_t TransportFeedback::BlockLength() const { return (size_bytes_ + 3) & (~static_cast(3)); } +size_t TransportFeedback::PaddingLength() const { + return BlockLength() - size_bytes_; +} + // Serialize packet. bool TransportFeedback::Create(uint8_t* packet, size_t* position, @@ -534,9 +538,10 @@ bool TransportFeedback::Create(uint8_t* packet, return false; } const size_t position_end = *position + BlockLength(); - - CreateHeader(kFeedbackMessageType, kPacketType, HeaderLength(), packet, - position); + const size_t padding_length = PaddingLength(); + bool has_padding = padding_length > 0; + CreateHeader(kFeedbackMessageType, kPacketType, HeaderLength(), has_padding, + packet, position); CreateCommonFeedback(packet + *position); *position += kCommonFeedbackLength; @@ -571,9 +576,12 @@ bool TransportFeedback::Create(uint8_t* packet, } } - while ((*position % 4) != 0) - packet[(*position)++] = 0; - + if (padding_length > 0) { + for (size_t i = 0; i < padding_length - 1; ++i) { + packet[(*position)++] = 0; + } + packet[(*position)++] = padding_length; + } RTC_DCHECK_EQ(*position, position_end); return true; } diff --git a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h index fbdc38e926..b7bed9d21a 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h +++ b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h @@ -73,6 +73,7 @@ class TransportFeedback : public Rtpfb { bool IsConsistent() const; size_t BlockLength() const override; + size_t PaddingLength() const; bool Create(uint8_t* packet, size_t* position, diff --git a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc index 04965256bd..43eeeb9b36 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc @@ -82,7 +82,7 @@ class FeedbackTester { feedback_ = TransportFeedback::ParseFrom(serialized_.data(), serialized_.size()); ASSERT_TRUE(feedback_->IsConsistent()); - ASSERT_NE(nullptr, feedback_.get()); + ASSERT_NE(nullptr, feedback_); VerifyInternal(); } @@ -130,7 +130,7 @@ class FeedbackTester { rtc::Buffer serialized_; }; -TEST(RtcpPacketTest, TransportFeedback_OneBitVector) { +TEST(RtcpPacketTest, TransportFeedbackOneBitVector) { const uint16_t kReceived[] = {1, 2, 7, 8, 9, 10, 13}; const size_t kLength = sizeof(kReceived) / sizeof(uint16_t); const size_t kExpectedSizeBytes = @@ -142,7 +142,7 @@ TEST(RtcpPacketTest, TransportFeedback_OneBitVector) { test.VerifyPacket(); } -TEST(RtcpPacketTest, TransportFeedback_FullOneBitVector) { +TEST(RtcpPacketTest, TransportFeedbackFullOneBitVector) { const uint16_t kReceived[] = {1, 2, 7, 8, 9, 10, 13, 14}; const size_t kLength = sizeof(kReceived) / sizeof(uint16_t); const size_t kExpectedSizeBytes = @@ -154,7 +154,7 @@ TEST(RtcpPacketTest, TransportFeedback_FullOneBitVector) { test.VerifyPacket(); } -TEST(RtcpPacketTest, TransportFeedback_OneBitVector_WrapReceived) { +TEST(RtcpPacketTest, TransportFeedbackOneBitVectorWrapReceived) { const uint16_t kMax = 0xFFFF; const uint16_t kReceived[] = {kMax - 2, kMax - 1, kMax, 0, 1, 2}; const size_t kLength = sizeof(kReceived) / sizeof(uint16_t); @@ -167,7 +167,7 @@ TEST(RtcpPacketTest, TransportFeedback_OneBitVector_WrapReceived) { test.VerifyPacket(); } -TEST(RtcpPacketTest, TransportFeedback_OneBitVector_WrapMissing) { +TEST(RtcpPacketTest, TransportFeedbackOneBitVectorWrapMissing) { const uint16_t kMax = 0xFFFF; const uint16_t kReceived[] = {kMax - 2, kMax - 1, 1, 2}; const size_t kLength = sizeof(kReceived) / sizeof(uint16_t); @@ -180,7 +180,7 @@ TEST(RtcpPacketTest, TransportFeedback_OneBitVector_WrapMissing) { test.VerifyPacket(); } -TEST(RtcpPacketTest, TransportFeedback_TwoBitVector) { +TEST(RtcpPacketTest, TransportFeedbackTwoBitVector) { const uint16_t kReceived[] = {1, 2, 6, 7}; const size_t kLength = sizeof(kReceived) / sizeof(uint16_t); const size_t kExpectedSizeBytes = @@ -193,7 +193,7 @@ TEST(RtcpPacketTest, TransportFeedback_TwoBitVector) { test.VerifyPacket(); } -TEST(RtcpPacketTest, TransportFeedback_TwoBitVectorFull) { +TEST(RtcpPacketTest, TransportFeedbackTwoBitVectorFull) { const uint16_t kReceived[] = {1, 2, 6, 7, 8}; const size_t kLength = sizeof(kReceived) / sizeof(uint16_t); const size_t kExpectedSizeBytes = @@ -206,7 +206,7 @@ TEST(RtcpPacketTest, TransportFeedback_TwoBitVectorFull) { test.VerifyPacket(); } -TEST(RtcpPacketTest, TransportFeedback_LargeAndNegativeDeltas) { +TEST(RtcpPacketTest, TransportFeedbackLargeAndNegativeDeltas) { const uint16_t kReceived[] = {1, 2, 6, 7, 8}; const int64_t kReceiveTimes[] = { 2000, 1000, 4000, 3000, @@ -221,7 +221,7 @@ TEST(RtcpPacketTest, TransportFeedback_LargeAndNegativeDeltas) { test.VerifyPacket(); } -TEST(RtcpPacketTest, TransportFeedback_MaxRle) { +TEST(RtcpPacketTest, TransportFeedbackMaxRle) { // Expected chunks created: // * 1-bit vector chunk (1xreceived + 13xdropped) // * RLE chunk of max length for dropped symbol @@ -240,7 +240,7 @@ TEST(RtcpPacketTest, TransportFeedback_MaxRle) { test.VerifyPacket(); } -TEST(RtcpPacketTest, TransportFeedback_MinRle) { +TEST(RtcpPacketTest, TransportFeedbackMinRle) { // Expected chunks created: // * 1-bit vector chunk (1xreceived + 13xdropped) // * RLE chunk of length 15 for dropped symbol @@ -258,7 +258,7 @@ TEST(RtcpPacketTest, TransportFeedback_MinRle) { test.VerifyPacket(); } -TEST(RtcpPacketTest, TransportFeedback_OneToTwoBitVector) { +TEST(RtcpPacketTest, TransportFeedbackOneToTwoBitVector) { const size_t kTwoBitVectorCapacity = 7; const uint16_t kReceived[] = {0, kTwoBitVectorCapacity - 1}; const int64_t kReceiveTimes[] = { @@ -273,7 +273,7 @@ TEST(RtcpPacketTest, TransportFeedback_OneToTwoBitVector) { test.VerifyPacket(); } -TEST(RtcpPacketTest, TransportFeedback_OneToTwoBitVectorSimpleSplit) { +TEST(RtcpPacketTest, TransportFeedbackOneToTwoBitVectorSimpleSplit) { const size_t kTwoBitVectorCapacity = 7; const uint16_t kReceived[] = {0, kTwoBitVectorCapacity}; const int64_t kReceiveTimes[] = { @@ -288,7 +288,7 @@ TEST(RtcpPacketTest, TransportFeedback_OneToTwoBitVectorSimpleSplit) { test.VerifyPacket(); } -TEST(RtcpPacketTest, TransportFeedback_OneToTwoBitVectorSplit) { +TEST(RtcpPacketTest, TransportFeedbackOneToTwoBitVectorSplit) { // With received small delta = S, received large delta = L, use input // SSSSSSSSLSSSSSSSSSSSS. This will cause a 1:2 split at the L. // After split there will be two symbols in symbol_vec: SL. @@ -316,7 +316,7 @@ TEST(RtcpPacketTest, TransportFeedback_OneToTwoBitVectorSplit) { test.VerifyPacket(); } -TEST(RtcpPacketTest, TransportFeedback_Aliasing) { +TEST(RtcpPacketTest, TransportFeedbackAliasing) { TransportFeedback feedback; feedback.SetBase(0, 0); @@ -340,7 +340,7 @@ TEST(RtcpPacketTest, TransportFeedback_Aliasing) { } } -TEST(RtcpPacketTest, TransportFeedback_Limits) { +TEST(RtcpPacketTest, TransportFeedbackLimits) { // Sequence number wrap above 0x8000. std::unique_ptr packet(new TransportFeedback()); packet->SetBase(0, 0); @@ -404,10 +404,12 @@ TEST(RtcpPacketTest, TransportFeedback_Limits) { // add back test for max size in bytes. } -TEST(RtcpPacketTest, TransportFeedback_Padding) { +TEST(RtcpPacketTest, TransportFeedbackPadding) { const size_t kExpectedSizeBytes = kHeaderSize + kStatusChunkSize + kSmallDeltaSize; const size_t kExpectedSizeWords = (kExpectedSizeBytes + 3) / 4; + const size_t kExpectedPaddingSizeBytes = + 4 * kExpectedSizeWords - kExpectedSizeBytes; TransportFeedback feedback; feedback.SetBase(0, 0); @@ -416,8 +418,10 @@ TEST(RtcpPacketTest, TransportFeedback_Padding) { rtc::Buffer packet = feedback.Build(); EXPECT_EQ(kExpectedSizeWords * 4, packet.size()); ASSERT_GT(kExpectedSizeWords * 4, kExpectedSizeBytes); - for (size_t i = kExpectedSizeBytes; i < kExpectedSizeWords * 4; ++i) - EXPECT_EQ(0u, packet.data()[i]); + for (size_t i = kExpectedSizeBytes; i < (kExpectedSizeWords * 4 - 1); ++i) + EXPECT_EQ(0u, packet[i]); + + EXPECT_EQ(kExpectedPaddingSizeBytes, packet[kExpectedSizeWords * 4 - 1]); // Modify packet by adding 4 bytes of padding at the end. Not currently used // when we're sending, but need to be able to handle it when receiving. @@ -428,7 +432,8 @@ TEST(RtcpPacketTest, TransportFeedback_Padding) { uint8_t mod_buffer[kExpectedSizeWithPadding]; memcpy(mod_buffer, packet.data(), kExpectedSizeWords * 4); memset(&mod_buffer[kExpectedSizeWords * 4], 0, kPaddingBytes - 1); - mod_buffer[kExpectedSizeWithPadding - 1] = kPaddingBytes; + mod_buffer[kExpectedSizeWithPadding - 1] = + kPaddingBytes + kExpectedPaddingSizeBytes; const uint8_t padding_flag = 1 << 5; mod_buffer[0] |= padding_flag; ByteWriter::WriteBigEndian( @@ -437,11 +442,46 @@ TEST(RtcpPacketTest, TransportFeedback_Padding) { std::unique_ptr parsed_packet( TransportFeedback::ParseFrom(mod_buffer, kExpectedSizeWithPadding)); - ASSERT_TRUE(parsed_packet.get() != nullptr); + ASSERT_TRUE(parsed_packet != nullptr); EXPECT_EQ(kExpectedSizeWords * 4, packet.size()); // Padding not included. } -TEST(RtcpPacketTest, TransportFeedback_CorrectlySplitsVectorChunks) { +TEST(RtcpPacketTest, TransportFeedbackPaddingBackwardsCompatibility) { + const size_t kExpectedSizeBytes = + kHeaderSize + kStatusChunkSize + kSmallDeltaSize; + const size_t kExpectedSizeWords = (kExpectedSizeBytes + 3) / 4; + const size_t kExpectedPaddingSizeBytes = + 4 * kExpectedSizeWords - kExpectedSizeBytes; + + TransportFeedback feedback; + feedback.SetBase(0, 0); + EXPECT_TRUE(feedback.AddReceivedPacket(0, 0)); + + rtc::Buffer packet = feedback.Build(); + EXPECT_EQ(kExpectedSizeWords * 4, packet.size()); + ASSERT_GT(kExpectedSizeWords * 4, kExpectedSizeBytes); + for (size_t i = kExpectedSizeBytes; i < (kExpectedSizeWords * 4 - 1); ++i) + EXPECT_EQ(0u, packet[i]); + + EXPECT_GT(kExpectedPaddingSizeBytes, 0u); + EXPECT_EQ(kExpectedPaddingSizeBytes, packet[kExpectedSizeWords * 4 - 1]); + + // Modify packet by removing padding bit and writing zero at the last padding + // byte to verify that we can parse packets from old clients, where zero + // padding of up to three bytes was used without the padding bit being set. + uint8_t mod_buffer[kExpectedSizeWords * 4]; + memcpy(mod_buffer, packet.data(), kExpectedSizeWords * 4); + mod_buffer[kExpectedSizeWords * 4 - 1] = 0; + const uint8_t padding_flag = 1 << 5; + mod_buffer[0] &= ~padding_flag; // Unset padding flag. + + std::unique_ptr parsed_packet( + TransportFeedback::ParseFrom(mod_buffer, kExpectedSizeWords * 4)); + ASSERT_TRUE(parsed_packet != nullptr); + EXPECT_EQ(kExpectedSizeWords * 4, packet.size()); +} + +TEST(RtcpPacketTest, TransportFeedbackCorrectlySplitsVectorChunks) { const int kOneBitVectorCapacity = 14; const int64_t kLargeTimeDelta = TransportFeedback::kDeltaScaleFactor * (1 << 8); @@ -460,11 +500,11 @@ TEST(RtcpPacketTest, TransportFeedback_CorrectlySplitsVectorChunks) { std::unique_ptr deserialized_packet = TransportFeedback::ParseFrom(serialized_packet.data(), serialized_packet.size()); - EXPECT_TRUE(deserialized_packet.get() != nullptr); + EXPECT_TRUE(deserialized_packet != nullptr); } } -TEST(RtcpPacketTest, TransportFeedback_MoveConstructor) { +TEST(RtcpPacketTest, TransportFeedbackMoveConstructor) { const int kSamples = 100; const int64_t kDelta = TransportFeedback::kDeltaScaleFactor; const uint16_t kBaseSeqNo = 7531;