Skip to content

Commit 42c09be

Browse files
RichLoganRichLogan
committed
Mutable & immutable fixup
1 parent de7a74c commit 42c09be

2 files changed

Lines changed: 116 additions & 69 deletions

File tree

‎moxygen/MoQFramer.cpp‎

Lines changed: 81 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -3239,6 +3239,8 @@ folly::Expected<folly::Unit, ErrorCode> MoQFrameParser::parseExtension(
32393239
<< " extLen=" << extLen->first;
32403240
return folly::makeUnexpected(parseInnerResult.error());
32413241
}
3242+
// Reset delta encoding state post-immutable.
3243+
previousExtensionType_ = kImmutableExtensionType;
32423244
// Advance the outer cursor past the immutable container payload and
32433245
// consume the bytes from the local length tracker
32443246
cursor.skip(extLen->first);
@@ -3728,75 +3730,106 @@ void MoQFrameWriter::writeExtensions(
37283730
size_t& size,
37293731
bool& error,
37303732
bool withLengthPrefix) const noexcept {
3731-
// Calculate total extension length (mutable + immutable blob if present)
3732-
auto mutableExtLen =
3733-
calculateExtensionVectorSize(extensions.getMutableExtensions(), error);
3734-
if (error) {
3735-
return;
3736-
}
3737-
3738-
size_t immutableBlobLen = 0;
3739-
size_t immutableExtensionsSize = 0; // Store calculated size for reuse
3740-
if (getDraftMajorVersion(*version_) >= 14 &&
3741-
!extensions.getImmutableExtensions().empty()) {
3742-
// Calculate size of immutable extensions blob:
3743-
// - Type (kImmutableExtensionType)
3744-
// - Length
3745-
// - Key-value pairs data
3746-
immutableExtensionsSize = calculateExtensionVectorSize(
3733+
// Get immutable length, if any.
3734+
const bool hasImmutableBlob = getDraftMajorVersion(*version_) >= 14 &&
3735+
!extensions.getImmutableExtensions().empty();
3736+
size_t immutableBodySize = 0;
3737+
if (hasImmutableBlob) {
3738+
immutableBodySize = calculateExtensionVectorSize(
37473739
extensions.getImmutableExtensions(), error);
37483740
if (error) {
37493741
return;
37503742
}
3743+
}
37513744

3752-
auto maybeTypeSize = quic::getQuicIntegerSize(kImmutableExtensionType);
3753-
if (maybeTypeSize.hasError()) {
3754-
error = true;
3755-
return;
3756-
}
3745+
folly::IOBufQueue blockBuf{folly::IOBufQueue::cacheChainLength()};
3746+
size_t blockSize = 0;
37573747

3758-
auto maybeLengthSize = quic::getQuicIntegerSize(immutableExtensionsSize);
3759-
if (maybeLengthSize.hasError()) {
3760-
error = true;
3748+
auto writeImmutableContainer = [&](uint64_t typeToWrite) {
3749+
writeVarint(blockBuf, typeToWrite, blockSize, error);
3750+
if (error) {
37613751
return;
37623752
}
3763-
3764-
immutableBlobLen =
3765-
*maybeTypeSize + *maybeLengthSize + immutableExtensionsSize;
3766-
}
3767-
3768-
auto totalExtLen = mutableExtLen + immutableBlobLen;
3769-
if (withLengthPrefix) {
3770-
writeVarint(writeBuf, totalExtLen, size, error);
3753+
writeVarint(blockBuf, immutableBodySize, blockSize, error);
37713754
if (error) {
37723755
return;
37733756
}
3774-
}
3757+
writeKeyValuePairs(
3758+
blockBuf, extensions.getImmutableExtensions(), blockSize, error);
3759+
};
37753760

3776-
// Write mutable extensions first
3777-
writeKeyValuePairs(writeBuf, extensions.getMutableExtensions(), size, error);
3778-
if (error) {
3779-
return;
3780-
}
3761+
if (getDraftMajorVersion(*version_) >= 16) {
3762+
auto sortedMutable =
3763+
sortExtensionsByType(extensions.getMutableExtensions());
3764+
uint64_t previousType = 0;
3765+
bool containerPlaced = false;
3766+
for (const auto& ext : sortedMutable) {
3767+
// Do we need to prepend immutable before going further?
3768+
if (hasImmutableBlob && !containerPlaced &&
3769+
ext.type > kImmutableExtensionType) {
3770+
writeImmutableContainer(kImmutableExtensionType - previousType);
3771+
if (error) {
3772+
return;
3773+
}
3774+
previousType = kImmutableExtensionType;
3775+
containerPlaced = true;
3776+
}
3777+
3778+
// Write the current mutable extension.
3779+
writeVarint(blockBuf, ext.type - previousType, blockSize, error);
3780+
if (error) {
3781+
return;
3782+
}
3783+
previousType = ext.type;
3784+
if (ext.isOddType()) {
3785+
auto dataLen =
3786+
ext.arrayValue ? ext.arrayValue->computeChainDataLength() : 0;
3787+
writeVarint(blockBuf, dataLen, blockSize, error);
3788+
if (error) {
3789+
return;
3790+
}
3791+
if (ext.arrayValue) {
3792+
blockBuf.append(ext.arrayValue->clone());
3793+
blockSize += dataLen;
3794+
}
3795+
} else {
3796+
writeVarint(blockBuf, ext.intValue, blockSize, error);
3797+
if (error) {
3798+
return;
3799+
}
3800+
}
3801+
}
37813802

3782-
// Write immutable extensions blob if present
3783-
if (getDraftMajorVersion(*version_) >= 14 &&
3784-
!extensions.getImmutableExtensions().empty()) {
3785-
writeVarint(writeBuf, kImmutableExtensionType, size, error);
3803+
// Otherwise, immutable is at the end.
3804+
if (hasImmutableBlob && !containerPlaced) {
3805+
writeImmutableContainer(kImmutableExtensionType - previousType);
3806+
if (error) {
3807+
return;
3808+
}
3809+
}
3810+
} else {
3811+
// v<16: no delta encoding, mutable extensions then immutable.
3812+
writeKeyValuePairs(
3813+
blockBuf, extensions.getMutableExtensions(), blockSize, error);
37863814
if (error) {
37873815
return;
37883816
}
3817+
if (hasImmutableBlob) {
3818+
writeImmutableContainer(kImmutableExtensionType);
3819+
if (error) {
3820+
return;
3821+
}
3822+
}
3823+
}
37893824

3790-
// Use the previously calculated size (no need to recalculate)
3791-
writeVarint(writeBuf, immutableExtensionsSize, size, error);
3825+
if (withLengthPrefix) {
3826+
writeVarint(writeBuf, blockSize, size, error);
37923827
if (error) {
37933828
return;
37943829
}
3795-
3796-
// Write the immutable extensions as key-value pairs
3797-
writeKeyValuePairs(
3798-
writeBuf, extensions.getImmutableExtensions(), size, error);
37993830
}
3831+
writeBuf.append(blockBuf.move());
3832+
size += blockSize;
38003833
}
38013834

38023835
size_t MoQFrameWriter::calculateExtensionVectorSize(

‎moxygen/test/MoQFramerTest.cpp‎

Lines changed: 35 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1843,6 +1843,10 @@ class MoQImmutableExtensionsTest : public ::testing::TestWithParam<uint64_t> {
18431843
MoQFrameParser parser_;
18441844
MoQFrameWriter writer_;
18451845

1846+
bool deltaEncoding() const {
1847+
return getDraftMajorVersion(GetParam()) >= 16;
1848+
}
1849+
18461850
// Creates the following extensions (encoded back-to-back):
18471851
// {type = 20, value = 100, immutable}
18481852
// {type = 21, value = binary(0xAB,0xCD,0xEF), immutable}
@@ -1856,7 +1860,7 @@ class MoQImmutableExtensionsTest : public ::testing::TestWithParam<uint64_t> {
18561860

18571861
// Extension type 21 (odd => length + bytes), binary value
18581862
static uint8_t testData[] = {0xAB, 0xCD, 0xEF};
1859-
writeVarintTo(immutableBuf, 21); // type
1863+
writeVarintTo(immutableBuf, deltaEncoding() ? 21 - 20 : 21); // type
18601864
writeVarintTo(immutableBuf, sizeof(testData)); // length
18611865
immutableBuf.append(testData, sizeof(testData)); // data
18621866

@@ -1868,25 +1872,33 @@ class MoQImmutableExtensionsTest : public ::testing::TestWithParam<uint64_t> {
18681872
// type = kImmutableExtensionType (odd) and an arbitrary byte value. This is
18691873
// invalid when parsed as nested immutable extensions (parseImmutable=false).
18701874
std::unique_ptr<folly::IOBuf> createImmutableExtensionsBufMalformed() {
1871-
// Start with the valid immutable extensions blob
1872-
auto base = createImmutableExtensionsBuf();
1873-
18741875
// Append an additional immutable-extensions container entry with arbitrary
18751876
// 1-byte payload. This makes the nested immutable blob malformed for our
18761877
// parser (immutable container found while already parsing immutable).
18771878
folly::IOBufQueue q{folly::IOBufQueue::cacheChainLength()};
18781879

1879-
if (base) {
1880-
q.append(std::move(base));
1881-
}
1880+
if (deltaEncoding()) {
1881+
// Different path to support delta encoding.
1882+
writeVarintTo(q, 2); // type
1883+
writeVarintTo(q, 1); // value
1884+
writeVarintTo(q, kImmutableExtensionType - 2); // nested
1885+
writeVarintTo(q, 1); // length
1886+
static const uint8_t kArbitrary = 0xAA;
1887+
q.append(&kArbitrary, 1);
1888+
} else {
1889+
auto base = createImmutableExtensionsBuf();
1890+
if (base) {
1891+
q.append(std::move(base));
1892+
}
18821893

1883-
// Write type = kImmutableExtensionType (odd)
1884-
writeVarintTo(q, kImmutableExtensionType);
1894+
// Write type = kImmutableExtensionType (odd)
1895+
writeVarintTo(q, kImmutableExtensionType);
18851896

1886-
// Write length = 1, then one arbitrary byte value 0xAA
1887-
writeVarintTo(q, 1); // length
1888-
static const uint8_t kArbitrary = 0xAA;
1889-
q.append(&kArbitrary, 1);
1897+
// Write length = 1, then one arbitrary byte value 0xAA
1898+
writeVarintTo(q, 1); // length
1899+
static const uint8_t kArbitrary = 0xAA;
1900+
q.append(&kArbitrary, 1);
1901+
}
18901902

18911903
return q.move();
18921904
}
@@ -1906,14 +1918,14 @@ class MoQImmutableExtensionsTest : public ::testing::TestWithParam<uint64_t> {
19061918
// 2) type = kImmutableExtensionType (odd => length + bytes), value =
19071919
// output of createImmutableExtensionsBuf()
19081920
auto imm = createImmutableExtensionsBuf();
1909-
writeVarintTo(buf, kImmutableExtensionType); // type
1921+
writeVarintTo(buf, deltaEncoding() ? kImmutableExtensionType - 10 : kImmutableExtensionType); // type
19101922
writeVarintTo(buf, imm ? imm->computeChainDataLength() : 0); // length
19111923
if (imm) {
19121924
buf.append(std::move(imm)); // payload
19131925
}
19141926

19151927
// 3) type = 30 (even => integer value follows), value = 999
1916-
writeVarintTo(buf, 30); // type
1928+
writeVarintTo(buf, deltaEncoding() ? 30 - kImmutableExtensionType : 30); // type
19171929
writeVarintTo(buf, 999); // value
19181930

19191931
return buf.move();
@@ -1932,14 +1944,14 @@ class MoQImmutableExtensionsTest : public ::testing::TestWithParam<uint64_t> {
19321944
// 2) type = kImmutableExtensionType (odd => length + bytes), value =
19331945
// output of createImmutableExtensionsBufMalformed()
19341946
auto imm = createImmutableExtensionsBufMalformed();
1935-
writeVarintTo(buf, kImmutableExtensionType); // type
1947+
writeVarintTo(buf, deltaEncoding() ? kImmutableExtensionType - 10 : kImmutableExtensionType); // type
19361948
writeVarintTo(buf, imm ? imm->computeChainDataLength() : 0); // length
19371949
if (imm) {
19381950
buf.append(std::move(imm)); // payload
19391951
}
19401952

19411953
// 3) type = 30 (even => integer value follows), value = 999
1942-
writeVarintTo(buf, 30); // type
1954+
writeVarintTo(buf, deltaEncoding() ? 30 - kImmutableExtensionType : 30); // type
19431955
writeVarintTo(buf, 999); // value
19441956

19451957
return buf.move();
@@ -2025,9 +2037,11 @@ TEST_P(MoQImmutableExtensionsTest, WriteImmutableExtensionsDraft) {
20252037

20262038
static const uint8_t kTestBinary[] = {0xAB, 0xCD, 0xEF};
20272039
std::vector<Extension> immutableExts = {
2040+
Extension{2, 2},
20282041
Extension{20, 100},
20292042
Extension{
2030-
21, folly::IOBuf::copyBuffer(kTestBinary, sizeof(kTestBinary))}};
2043+
21, folly::IOBuf::copyBuffer(kTestBinary, sizeof(kTestBinary))},
2044+
Extension{32, 32}};
20312045

20322046
Extensions extensions(mutableExts, immutableExts);
20332047

@@ -2056,8 +2070,8 @@ TEST_P(MoQImmutableExtensionsTest, WriteImmutableExtensionsDraft) {
20562070
EXPECT_TRUE(parseResult.hasValue()) << "Parsing should succeed";
20572071

20582072
// Verify both mutable and immutable extensions are present
2059-
EXPECT_EQ(obj.extensions.size(), 4)
2060-
<< "Should have 4 total extensions (2 mutable + 2 immutable)";
2073+
EXPECT_EQ(obj.extensions.size(), 6)
2074+
<< "Should have 6 total extensions (2 mutable + 4 immutable)";
20612075
EXPECT_THAT(
20622076
obj.extensions.getMutableExtensions(), testing::ContainerEq(mutableExts));
20632077
EXPECT_THAT(
@@ -2114,7 +2128,7 @@ TEST_P(MoQImmutableExtensionsTest, WriteOnlyImmutableExtensionsDraft) {
21142128
INSTANTIATE_TEST_SUITE_P(
21152129
MoQImmutableExtensionsTest,
21162130
MoQImmutableExtensionsTest,
2117-
::testing::Values(kVersionDraft14, kVersionDraft15));
2131+
::testing::Values(kVersionDraft14, kVersionDraft15, kVersionDraft16));
21182132

21192133
TEST_P(MoQFramerTest, DatagramWithExtensionsAndNonNormalStatus) {
21202134
folly::IOBufQueue writeBuf{folly::IOBufQueue::cacheChainLength()};

0 commit comments

Comments
 (0)