Enable padding bit in TransportFeedback packets
Set padding bit if the TransportFeedback packet contains zero padding.
Also write number of padding elements at the last position of the packet.
Bug: webrtc:10263
Change-Id: I8d17bc0e889f658ac383dec64ddcb95d438c9702
Reviewed-on: https://webrtc-review.googlesource.com/c/122240
Reviewed-by: Erik Språng <sprang@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Johannes Kron <kron@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26646}diff --git a/modules/rtp_rtcp/source/rtcp_packet.cc b/modules/rtp_rtcp/source/rtcp_packet.cc
index 194b992..bac03e7 100644
--- a/modules/rtp_rtcp/source/rtcp_packet.cc
+++ b/modules/rtp_rtcp/source/rtcp_packet.cc
@@ -52,7 +52,8 @@
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 @@
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<uint8_t>(count_or_format);
+ kVersionBits | padding_bit | static_cast<uint8_t>(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 40e51e8..94bf9f0 100644
--- a/modules/rtp_rtcp/source/rtcp_packet.h
+++ b/modules/rtp_rtcp/source/rtcp_packet.h
@@ -87,6 +87,13 @@
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 2816559..81eda9e 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 @@
return (size_bytes_ + 3) & (~static_cast<size_t>(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 @@
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 @@
}
}
- 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 fbdc38e..b7bed9d 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 @@
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 0496525..43eeeb9 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 @@
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 @@
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.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.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.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.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.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.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.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.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.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.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.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.VerifyPacket();
}
-TEST(RtcpPacketTest, TransportFeedback_Aliasing) {
+TEST(RtcpPacketTest, TransportFeedbackAliasing) {
TransportFeedback feedback;
feedback.SetBase(0, 0);
@@ -340,7 +340,7 @@
}
}
-TEST(RtcpPacketTest, TransportFeedback_Limits) {
+TEST(RtcpPacketTest, TransportFeedbackLimits) {
// Sequence number wrap above 0x8000.
std::unique_ptr<TransportFeedback> packet(new TransportFeedback());
packet->SetBase(0, 0);
@@ -404,10 +404,12 @@
// 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 @@
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 @@
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<uint16_t>::WriteBigEndian(
@@ -437,11 +442,46 @@
std::unique_ptr<TransportFeedback> 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<TransportFeedback> 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 @@
std::unique_ptr<TransportFeedback> 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;
