Reverse the kbits logic according to RFC
The updated Flexfec RFC states that a kbit of "0" means this is the last block of the mask, whereas in the 03 draft, "0" meant there's another block.
Reversing the logic in the updated RFC parser to fix.
Bug: webrtc:15002
Change-Id: I40e4c950b09ddf2db9da6c01908737282161bf1c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/327580
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41174}
diff --git a/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc b/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc
index cfca7cb..3e6d04d 100644
--- a/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc
+++ b/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc
@@ -138,9 +138,9 @@
mask_part0 <<= 1;
ByteWriter<uint16_t>::WriteBigEndian(&data[byte_index], mask_part0);
byte_index += kFlexfecPacketMaskSizes[0];
- if (k_bit0) {
- // The first K-bit is set, and the packet mask is thus only 2 bytes long.
- // We have finished reading the properties for current ssrc.
+ if (!k_bit0) {
+ // The first K-bit is clear, and the packet mask is thus only 2 bytes
+ // long. We have finished reading the properties for current ssrc.
fec_packet->protected_streams[i].packet_mask_size =
kFlexfecPacketMaskSizes[0];
} else {
@@ -162,8 +162,8 @@
mask_part1 <<= 2;
ByteWriter<uint32_t>::WriteBigEndian(&data[byte_index], mask_part1);
byte_index += kFlexfecPacketMaskSizes[1] - kFlexfecPacketMaskSizes[0];
- if (k_bit1) {
- // The first K-bit is clear, but the second K-bit is set. The packet
+ if (!k_bit1) {
+ // The first K-bit is set, but the second K-bit is clear. The packet
// mask is thus 6 bytes long. We have finished reading the properties
// for current ssrc.
fec_packet->protected_streams[i].packet_mask_size =
@@ -273,8 +273,9 @@
tmp_mask_part0 >>= 1; // Shift, thus clearing K-bit 0.
ByteWriter<uint16_t>::WriteBigEndian(write_at, tmp_mask_part0);
+ *write_at |= 0x80; // Set K-bit 0.
write_at += kFlexfecPacketMaskSizes[0];
- tmp_mask_part1 >>= 2; // Shift, thus clearing K-bit 1 and bit 15.
+ tmp_mask_part1 >>= 2; // Shift twice, thus clearing K-bit 1 and bit 15.
ByteWriter<uint32_t>::WriteBigEndian(write_at, tmp_mask_part1);
bool bit15 = (protected_stream.packet_mask[1] & 0x01) != 0;
@@ -284,9 +285,9 @@
bool bit46 = (protected_stream.packet_mask[5] & 0x02) != 0;
bool bit47 = (protected_stream.packet_mask[5] & 0x01) != 0;
if (!bit46 && !bit47) {
- *write_at |= 0x80; // Set K-bit 1.
write_at += kFlexfecPacketMaskSizes[1] - kFlexfecPacketMaskSizes[0];
} else {
+ *write_at |= 0x80; // Set K-bit 1.
write_at += kFlexfecPacketMaskSizes[1] - kFlexfecPacketMaskSizes[0];
// Clear all trailing bits.
memset(write_at, 0,
@@ -307,14 +308,13 @@
ByteWriter<uint16_t>::WriteBigEndian(write_at, tmp_mask_part0);
bool bit15 = (protected_stream.packet_mask[1] & 0x01) != 0;
if (!bit15) {
- *write_at |= 0x80; // Set K-bit 0.
write_at += kFlexfecPacketMaskSizes[0];
} else {
+ *write_at |= 0x80; // Set K-bit 0.
write_at += kFlexfecPacketMaskSizes[0];
// Clear all trailing bits.
memset(write_at, 0U,
kFlexfecPacketMaskSizes[1] - kFlexfecPacketMaskSizes[0]);
- *write_at |= 0x80; // Set K-bit 1.
*write_at |= 0x40; // Set bit 15.
write_at += kFlexfecPacketMaskSizes[1] - kFlexfecPacketMaskSizes[0];
}
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 6995ba3..f25e0d8d 100644
--- a/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
+++ b/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
@@ -36,11 +36,12 @@
using ::testing::Each;
using ::testing::ElementsAreArray;
-constexpr uint8_t kMask0[] = {0xAB, 0xCD}; // First K bit is set.
-constexpr uint8_t kMask1[] = {0x12, 0x34, // First K bit cleared.
- 0xF6, 0x78, 0x9A, 0xBC}; // Second K bit set.
-constexpr uint8_t kMask2[] = {0x12, 0x34, // First K bit cleared.
- 0x56, 0x78, 0x9A, 0xBC, // Second K bit cleared.
+constexpr uint8_t kKBit = 1 << 7;
+constexpr uint8_t kMask0[] = {0x2B, 0xCD}; // First K bit is cleared.
+constexpr uint8_t kMask1[] = {0x92, 0x34, // First K bit set.
+ 0x76, 0x78, 0x9A, 0xBC}; // Second K bit cleared.
+constexpr uint8_t kMask2[] = {0x92, 0x34, // First K bit set.
+ 0xD6, 0x78, 0x9A, 0xBC, // Second K bit set.
0xDE, 0xF0, 0x12, 0x34, 0x56, 0x78, 0x9A, 0xBC};
constexpr size_t kMediaPacketLength = 1234;
@@ -186,11 +187,10 @@
} // namespace
-TEST(FlexfecHeaderReaderTest, ReadsHeaderWithKBit0SetSingleStream) {
- constexpr uint8_t kKBit0 = 1 << 7;
+TEST(FlexfecHeaderReaderTest, ReadsHeaderWithKBit0ClearSingleStream) {
constexpr size_t kExpectedFecHeaderSize = 12;
constexpr uint16_t kSnBase = 0x0102;
- constexpr uint8_t kFlexfecPktMask[] = {kKBit0 | 0x08, 0x81};
+ constexpr uint8_t kFlexfecPktMask[] = {0x08, 0x81};
constexpr uint8_t kUlpfecPacketMask[] = {0x11, 0x02};
constexpr uint8_t kPacketData[] = {
kFlexible, kPtRecovery, kLengthRecovery[0], kLengthRecovery[1],
@@ -215,13 +215,11 @@
VerifyReadHeaders(kExpectedFecHeaderSize, read_packet, expected);
}
-TEST(FlexfecHeaderReaderTest, ReadsHeaderWithKBit1SetSingleStream) {
- constexpr uint8_t kKBit0 = 0 << 7;
- constexpr uint8_t kKBit1 = 1 << 7;
+TEST(FlexfecHeaderReaderTest, ReadsHeaderWithKBit1ClearSingleStream) {
constexpr size_t kExpectedFecHeaderSize = 16;
constexpr uint16_t kSnBase = 0x0102;
- constexpr uint8_t kFlexfecPktMask[] = {kKBit0 | 0x48, 0x81, //
- kKBit1 | 0x02, 0x11, 0x00, 0x21};
+ constexpr uint8_t kFlexfecPktMask[] = {kKBit | 0x48, 0x81, //
+ 0x02, 0x11, 0x00, 0x21};
constexpr uint8_t kUlpfecPacketMask[] = {0x91, 0x02, //
0x08, 0x44, 0x00, 0x84};
constexpr uint8_t kPacketData[] = {
@@ -250,15 +248,13 @@
VerifyReadHeaders(kExpectedFecHeaderSize, read_packet, expected);
}
-TEST(FlexfecHeaderReaderTest, ReadsHeaderWithNoKBitsSetSingleStream) {
- constexpr uint8_t kKBit0 = 0 << 7;
- constexpr uint8_t kKBit1 = 0 << 7;
+TEST(FlexfecHeaderReaderTest, ReadsHeaderWithBothKBitsSetSingleStream) {
constexpr size_t kExpectedFecHeaderSize = 24;
constexpr uint16_t kSnBase = 0x0102;
- constexpr uint8_t kFlexfecPacketMask[] = {kKBit0 | 0x48, 0x81, //
- kKBit1 | 0x02, 0x11, 0x00, 0x21, //
- 0x01, 0x11, 0x11, 0x11,
- 0x11, 0x11, 0x11, 0x11};
+ constexpr uint8_t kFlexfecPacketMask[] = {kKBit | 0x48, 0x81, //
+ kKBit | 0x02, 0x11, 0x00, 0x21, //
+ 0x01, 0x11, 0x11, 0x11,
+ 0x11, 0x11, 0x11, 0x11};
constexpr uint8_t kUlpfecPacketMask[] = {0x91, 0x02, //
0x08, 0x44, 0x00, 0x84, //
0x04, 0x44, 0x44, 0x44,
@@ -309,14 +305,13 @@
VerifyReadHeaders(kExpectedFecHeaderSize, read_packet, expected);
}
-TEST(FlexfecHeaderReaderTest, ReadsHeaderWithKBit0Set2Streams) {
- constexpr uint8_t kKBit0 = 1 << 7;
+TEST(FlexfecHeaderReaderTest, ReadsHeaderWithKBit0Clear2Streams) {
constexpr size_t kExpectedFecHeaderSize = 16;
constexpr uint16_t kSnBase0 = 0x0102;
constexpr uint16_t kSnBase1 = 0x0304;
- constexpr uint8_t kFlexfecPktMask1[] = {kKBit0 | 0x08, 0x81};
+ constexpr uint8_t kFlexfecPktMask1[] = {0x08, 0x81};
constexpr uint8_t kUlpfecPacketMask1[] = {0x11, 0x02};
- constexpr uint8_t kFlexfecPktMask2[] = {kKBit0 | 0x04, 0x41};
+ constexpr uint8_t kFlexfecPktMask2[] = {0x04, 0x41};
constexpr uint8_t kUlpfecPacketMask2[] = {0x08, 0x82};
constexpr uint8_t kPacketData[] = {
@@ -349,18 +344,16 @@
VerifyReadHeaders(kExpectedFecHeaderSize, read_packet, expected);
}
-TEST(FlexfecHeaderReaderTest, ReadsHeaderWithKBit1Set2Streams) {
- constexpr uint8_t kKBit0 = 0 << 7;
- constexpr uint8_t kKBit1 = 1 << 7;
+TEST(FlexfecHeaderReaderTest, ReadsHeaderWithKBit1Clear2Streams) {
constexpr size_t kExpectedFecHeaderSize = 24;
constexpr uint16_t kSnBase0 = 0x0102;
constexpr uint16_t kSnBase1 = 0x0304;
- constexpr uint8_t kFlexfecPktMask1[] = {kKBit0 | 0x48, 0x81, //
- kKBit1 | 0x02, 0x11, 0x00, 0x21};
+ constexpr uint8_t kFlexfecPktMask1[] = {kKBit | 0x48, 0x81, //
+ 0x02, 0x11, 0x00, 0x21};
constexpr uint8_t kUlpfecPacketMask1[] = {0x91, 0x02, //
0x08, 0x44, 0x00, 0x84};
- constexpr uint8_t kFlexfecPktMask2[] = {kKBit0 | 0x57, 0x82, //
- kKBit1 | 0x04, 0x33, 0x00, 0x51};
+ constexpr uint8_t kFlexfecPktMask2[] = {kKBit | 0x57, 0x82, //
+ 0x04, 0x33, 0x00, 0x51};
constexpr uint8_t kUlpfecPacketMask2[] = {0xAF, 0x04, //
0x10, 0xCC, 0x01, 0x44};
constexpr uint8_t kPacketData[] = {
@@ -398,24 +391,22 @@
VerifyReadHeaders(kExpectedFecHeaderSize, read_packet, expected);
}
-TEST(FlexfecHeaderReaderTest, ReadsHeaderWithNoKBitsSet2Streams) {
- constexpr uint8_t kKBit0 = 0 << 7;
- constexpr uint8_t kKBit1 = 0 << 7;
+TEST(FlexfecHeaderReaderTest, ReadsHeaderWithBothKBitsSet2Streams) {
constexpr size_t kExpectedFecHeaderSize = 40;
constexpr uint16_t kSnBase0 = 0x0102;
constexpr uint16_t kSnBase1 = 0x0304;
- constexpr uint8_t kFlexfecPktMask1[] = {kKBit0 | 0x48, 0x81, //
- kKBit1 | 0x02, 0x11, 0x00, 0x21, //
- 0x01, 0x11, 0x11, 0x11,
- 0x11, 0x11, 0x11, 0x11};
+ constexpr uint8_t kFlexfecPktMask1[] = {kKBit | 0x48, 0x81, //
+ kKBit | 0x02, 0x11, 0x00, 0x21, //
+ 0x01, 0x11, 0x11, 0x11,
+ 0x11, 0x11, 0x11, 0x11};
constexpr uint8_t kUlpfecPacketMask1[] = {0x91, 0x02, //
0x08, 0x44, 0x00, 0x84, //
0x04, 0x44, 0x44, 0x44,
0x44, 0x44, 0x44, 0x44};
- constexpr uint8_t kFlexfecPktMask2[] = {kKBit0 | 0x32, 0x84, //
- kKBit1 | 0x05, 0x23, 0x00, 0x55, //
- 0xA3, 0x22, 0x22, 0x22,
- 0x22, 0x22, 0x22, 0x35};
+ constexpr uint8_t kFlexfecPktMask2[] = {kKBit | 0x32, 0x84, //
+ kKBit | 0x05, 0x23, 0x00, 0x55, //
+ 0xA3, 0x22, 0x22, 0x22,
+ 0x22, 0x22, 0x22, 0x35};
constexpr uint8_t kUlpfecPacketMask2[] = {0x65, 0x08, //
0x14, 0x8C, 0x01, 0x56, //
0x8C, 0x88, 0x88, 0x88,
@@ -490,29 +481,27 @@
}
TEST(FlexfecHeaderReaderTest, ReadsHeaderWithMultipleStreamsMultipleMasks) {
- constexpr uint8_t kBit0 = 0 << 7;
- constexpr uint8_t kBit1 = 1 << 7;
constexpr size_t kExpectedFecHeaderSize = 44;
constexpr uint16_t kSnBase0 = 0x0102;
constexpr uint16_t kSnBase1 = 0x0304;
constexpr uint16_t kSnBase2 = 0x0506;
constexpr uint16_t kSnBase3 = 0x0708;
- constexpr uint8_t kFlexfecPacketMask1[] = {kBit1 | 0x29, 0x91};
+ constexpr uint8_t kFlexfecPacketMask1[] = {0x29, 0x91};
constexpr uint8_t kUlpfecPacketMask1[] = {0x53, 0x22};
- constexpr uint8_t kFlexfecPacketMask2[] = {kBit0 | 0x32, 0xA1, //
- kBit1 | 0x02, 0x11, 0x00, 0x21};
+ constexpr uint8_t kFlexfecPacketMask2[] = {kKBit | 0x32, 0xA1, //
+ 0x02, 0x11, 0x00, 0x21};
constexpr uint8_t kUlpfecPacketMask2[] = {0x65, 0x42, //
0x08, 0x44, 0x00, 0x84};
- constexpr uint8_t kFlexfecPacketMask3[] = {kBit0 | 0x48, 0x81, //
- kBit0 | 0x02, 0x11, 0x00, 0x21, //
+ constexpr uint8_t kFlexfecPacketMask3[] = {kKBit | 0x48, 0x81, //
+ kKBit | 0x02, 0x11, 0x00, 0x21, //
0x01, 0x11, 0x11, 0x11,
0x11, 0x11, 0x11, 0x11};
constexpr uint8_t kUlpfecPacketMask3[] = {0x91, 0x02, //
0x08, 0x44, 0x00, 0x84, //
0x04, 0x44, 0x44, 0x44,
0x44, 0x44, 0x44, 0x44};
- constexpr uint8_t kFlexfecPacketMask4[] = {kBit0 | 0x32, 0x84, //
- kBit1 | 0x05, 0x23, 0x00, 0x55};
+ constexpr uint8_t kFlexfecPacketMask4[] = {kKBit | 0x32, 0x84, //
+ 0x05, 0x23, 0x00, 0x55};
constexpr uint8_t kUlpfecPacketMask4[] = {0x65, 0x08, //
0x14, 0x8C, 0x01, 0x54};
constexpr uint8_t kPacketData[] = {kFlexible,
@@ -642,7 +631,7 @@
EXPECT_FALSE(reader.ReadFecHeader(&read_packet));
}
-TEST(FlexfecHeaderReaderTest, ReadShortPacketWithKBit1SetShouldFail) {
+TEST(FlexfecHeaderReaderTest, ReadShortPacketWithKBit1ClearShouldFail) {
// Simulate short received packet.
constexpr uint8_t kPacketData[] = {
kFlexible, kPtRecovery, kLengthRecovery[0], kLengthRecovery[1],
@@ -659,7 +648,7 @@
EXPECT_FALSE(reader.ReadFecHeader(&read_packet));
}
-TEST(FlexfecHeaderReaderTest, ReadShortPacketWithKBit1ClearedShouldFail) {
+TEST(FlexfecHeaderReaderTest, ReadShortPacketWithKBit1SetShouldFail) {
// Simulate short received packet.
constexpr uint8_t kPacketData[] = {
kFlexible, kPtRecovery, kLengthRecovery[0], kLengthRecovery[1],
@@ -698,8 +687,8 @@
EXPECT_FALSE(reader.ReadFecHeader(&read_packet));
}
-TEST(FlexfecHeaderWriterTest, FinalizesHeaderWithKBit0SetSingleStream) {
- constexpr uint8_t kFlexfecPacketMask[] = {0x88, 0x81};
+TEST(FlexfecHeaderWriterTest, FinalizesHeaderWithKBit0ClearSingleStream) {
+ constexpr uint8_t kFlexfecPacketMask[] = {0x08, 0x81};
constexpr uint8_t kUlpfecPacketMask[] = {0x11, 0x02};
constexpr uint16_t kMediaStartSeqNum = 1234;
Packet written_packet = WritePacket({{.ssrc = 0x01,
@@ -714,8 +703,8 @@
VerifyFinalizedHeaders(written_packet, expected);
}
-TEST(FlexfecHeaderWriterTest, FinalizesHeaderWithKBit1SetSingleStream) {
- constexpr uint8_t kFlexfecPacketMask[] = {0x48, 0x81, 0x82, 0x11, 0x00, 0x21};
+TEST(FlexfecHeaderWriterTest, FinalizesHeaderWithKBit1ClearSingleStream) {
+ constexpr uint8_t kFlexfecPacketMask[] = {0xC8, 0x81, 0x02, 0x11, 0x00, 0x21};
constexpr uint8_t kUlpfecPacketMask[] = {0x91, 0x02, 0x08, 0x44, 0x00, 0x84};
constexpr uint16_t kMediaStartSeqNum = 1234;
Packet written_packet = WritePacket({{.ssrc = 0x01,
@@ -730,10 +719,10 @@
VerifyFinalizedHeaders(written_packet, expected);
}
-TEST(FlexfecHeaderWriterTest, FinalizesHeaderWithNoKBitsSetSingleStream) {
+TEST(FlexfecHeaderWriterTest, FinalizesHeaderWithBothKBitsSetSingleStream) {
constexpr uint8_t kFlexfecPacketMask[] = {
- 0x11, 0x11, // K-bit 0 clear.
- 0x11, 0x11, 0x11, 0x10, // K-bit 1 clear.
+ 0x91, 0x11, // K-bit 0 set.
+ 0x91, 0x11, 0x11, 0x10, // K-bit 1 set.
0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 //
};
constexpr uint8_t kUlpfecPacketMask[] = {0x22, 0x22, 0x44, 0x44, 0x44, 0x41};
@@ -752,22 +741,22 @@
TEST(FlexfecHeaderWriterTest, FinalizesHeaderMultipleStreamsMultipleMasks) {
constexpr uint8_t kFlexfecPacketMask1[] = {
- 0x11, 0x11, // K-bit 0 clear.
- 0x11, 0x11, 0x11, 0x10, // K-bit 1 clear.
+ 0x91, 0x11, // K-bit 0 set.
+ 0x91, 0x11, 0x11, 0x10, // K-bit 1 set.
0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 //
};
constexpr uint8_t kUlpfecPacketMask1[] = {0x22, 0x22, 0x44, 0x44, 0x44, 0x41};
constexpr uint16_t kMediaStartSeqNum1 = 1234;
- constexpr uint8_t kFlexfecPacketMask2[] = {0x88, 0x81};
+ constexpr uint8_t kFlexfecPacketMask2[] = {0x08, 0x81};
constexpr uint8_t kUlpfecPacketMask2[] = {0x11, 0x02};
constexpr uint16_t kMediaStartSeqNum2 = 2345;
- constexpr uint8_t kFlexfecPacketMask3[] = {0x48, 0x81, 0x82,
+ constexpr uint8_t kFlexfecPacketMask3[] = {0xC8, 0x81, 0x02,
0x11, 0x00, 0x21};
constexpr uint8_t kUlpfecPacketMask3[] = {0x91, 0x02, 0x08, 0x44, 0x00, 0x84};
constexpr uint16_t kMediaStartSeqNum3 = 3456;
constexpr uint8_t kFlexfecPacketMask4[] = {
- 0x55, 0xAA, // K-bit 0 clear.
- 0x22, 0xAB, 0xCD, 0xEF, // K-bit 1 clear.
+ 0xD5, 0xAA, // K-bit 0 set.
+ 0xA2, 0xAB, 0xCD, 0xEF, // K-bit 1 set.
0xC0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 //
};
constexpr uint8_t kUlpfecPacketMask4[] = {0xAB, 0x54, 0x8A, 0xAF, 0x37, 0xBF};
