Refactor ACK writing for the different ACK types

Summary:
This replaces three separate functions for writing ACK, ACK_ECN, and ACK_RECEIVE_TIMESTAMPS with one function that can write all three.

Previously, we have two paths:
1. Default path which writes the base ACK frame and optionally adds the ECN counts if the frame type is ACK_ECN.
2. If ACK_RECEIVE_TIMESTAMPS are supported, another path would take the ART config and write that frame with the ART fields.
Path #2 does not support ECN marks.

This change consolidates ACK writing into a single path which takes ART config and frame type. It decides which fields to include based upon the type and config passed. This change does not add any new frame types but it prepares for one that can carry both ECN counts and ART fields.

Differential Revision: D68931147

fbshipit-source-id: 47b425b30f00b6c76574bc768d0ec249c60a0aa7

quic/api/QuicPacketScheduler.cpp

@@ -621,9 +621,10 @@ Optional<PacketNum> AckScheduler::writeNextAcks(
       isAckReceiveTimestampsSupported && (peerRequestedTimestampsCount > 0)) {
     // Use ACK_RECEIVE_TIMESTAMPS if its enabled on both endpoints AND the peer
     // requests at least 1 timestamp
-    ackWriteResult = writeAckFrameWithReceivedTimestamps(
+    ackWriteResult = writeAckFrame(
         meta,
         builder,
+        FrameType::ACK_RECEIVE_TIMESTAMPS,
         conn_.transportSettings.maybeAckReceiveTimestampsConfigSentToPeer
             .value(),
         peerRequestedTimestampsCount);

quic/codec/QuicPacketRebuilder.cpp

@@ -256,9 +256,10 @@ Optional<ClonedPacketIdentifier> PacketRebuilder::rebuildFromPacket(
     if (!isAckReceiveTimestampsSupported || !peerRequestedTimestampsCount) {
       writeAckFrame(meta, builder_, FrameType::ACK);
     } else {
-      writeAckFrameWithReceivedTimestamps(
+      writeAckFrame(
           meta,
           builder_,
+          FrameType::ACK_RECEIVE_TIMESTAMPS,
           conn_.transportSettings.maybeAckReceiveTimestampsConfigSentToPeer
               .value(),
           peerRequestedTimestampsCount);

quic/codec/QuicWriteCodec.cpp

@@ -274,7 +274,7 @@ size_t computeSizeUsedByRecvdTimestamps(WriteAckFrame& ackFrame) {
   return usedSize;
 }
 
-static size_t fillPacketReceiveTimestamps(
+static size_t fillFrameWithPacketReceiveTimestamps(
     const quic::WriteAckFrameMetaData& ackFrameMetaData,
     WriteAckFrame& ackFrame,
     uint64_t largestAckedPacketNum,
@@ -369,39 +369,18 @@ static size_t fillPacketReceiveTimestamps(
   return ackFrame.recvdPacketsTimestampRanges.size();
 }
 
-Optional<WriteAckFrame> writeAckFrameToPacketBuilder(
+static Optional<WriteAckFrame> maybeWriteAckBaseFields(
     const quic::WriteAckFrameMetaData& ackFrameMetaData,
     PacketBuilderInterface& builder,
-    FrameType frameType) {
+    FrameType frameType,
-  if (ackFrameMetaData.ackState.acks.empty()) {
+    uint64_t maxSpaceToUse) {
-    return none;
+  auto spaceLeft = maxSpaceToUse;
-  }
   const WriteAckFrameState& ackState = ackFrameMetaData.ackState;
   // The last block must be the largest block.
   auto largestAckedPacket = ackState.acks.back().end;
   // ackBlocks are already an interval set so each value is naturally
   // non-overlapping.
   auto firstAckBlockLength = largestAckedPacket - ackState.acks.back().start;
-
-  WriteAckFrame ackFrame;
-  ackFrame.frameType = frameType;
-  uint64_t spaceLeft = builder.remainingSpaceInPkt();
-
-  // Reserve space for ECN counts if enabled
-  QuicInteger ecnECT0Count(ackFrameMetaData.ackState.ecnECT0CountReceived);
-  QuicInteger ecnECT1Count(ackFrameMetaData.ackState.ecnECT1CountReceived);
-  QuicInteger ecnCECount(ackFrameMetaData.ackState.ecnCECountReceived);
-  if (frameType == FrameType::ACK_ECN) {
-    ackFrame.ecnECT0Count = ackFrameMetaData.ackState.ecnECT0CountReceived;
-    ackFrame.ecnECT1Count = ackFrameMetaData.ackState.ecnECT1CountReceived;
-    ackFrame.ecnCECount = ackFrameMetaData.ackState.ecnCECountReceived;
-    spaceLeft -=
-        (ecnECT0Count.getSize() + ecnECT1Count.getSize() +
-         ecnCECount.getSize());
-  }
-
-  ackFrame.ackBlocks.reserve(spaceLeft / 4);
-
   // We could technically split the range if the size of the representation of
   // the integer is too large, but that gets super tricky and is of dubious
   // value.
@@ -417,43 +396,25 @@ Optional<WriteAckFrame> writeAckFrameToPacketBuilder(
   // Required fields are Type, LargestAcked, AckDelay, AckBlockCount,
   // firstAckBlockLength
   QuicInteger encodedintFrameType(static_cast<uint8_t>(frameType));
-  auto headerSize = encodedintFrameType.getSize() +
+  uint64_t headerSize = encodedintFrameType.getSize() +
       largestAckedPacketInt.getSize() + ackDelayInt.getSize() +
       minAdditionalAckBlockCount.getSize() + firstAckBlockLengthInt.getSize();
 
-  size_t minAdditionalAckReceiveTimestampsFieldsSize = 0;
+  if (spaceLeft < headerSize) {
-  if (frameType == FrameType::ACK_RECEIVE_TIMESTAMPS) {
-    // Compute minimum size requirements for 3 fields that must be sent
-    // in every ACK_RECEIVE_TIMESTAMPS frame
-    uint64_t countTimestampRanges = 0;
-    uint64_t maybeLastPktNum = 0;
-    std::chrono::microseconds maybeLastPktTsDelta = 0us;
-    if (ackState.lastRecvdPacketInfo)
-      maybeLastPktNum = ackState.lastRecvdPacketInfo.value().pktNum;
-
-    maybeLastPktTsDelta =
-        (ackState.lastRecvdPacketInfo.value().timings.receiveTimePoint >
-                 ackFrameMetaData.connTime
-             ? std::chrono::duration_cast<std::chrono::microseconds>(
-                   ackState.lastRecvdPacketInfo.value()
-                       .timings.receiveTimePoint -
-                   ackFrameMetaData.connTime)
-             : 0us);
-
-    minAdditionalAckReceiveTimestampsFieldsSize =
-        getQuicIntegerSize(countTimestampRanges).value_or(0) +
-        getQuicIntegerSize(maybeLastPktNum).value_or(0) +
-        getQuicIntegerSize(maybeLastPktTsDelta.count()).value_or(0);
-  }
-  if (spaceLeft < (headerSize + minAdditionalAckReceiveTimestampsFieldsSize)) {
     return none;
   }
-  spaceLeft -= (headerSize + minAdditionalAckReceiveTimestampsFieldsSize);
+  WriteAckFrame ackFrame;
+  ackFrame.frameType = frameType;
+
+  // Reserve the number of ack blocks we could fit in the remaining space.
+  ackFrame.ackBlocks.reserve(spaceLeft / 4);
+
+  // Account for the header size
+  spaceLeft -= headerSize;
 
   ackFrame.ackBlocks.push_back(ackState.acks.back());
   auto numAdditionalAckBlocks =
       fillFrameWithAckBlocks(ackState.acks, ackFrame, spaceLeft);
-
   QuicInteger numAdditionalAckBlocksInt(numAdditionalAckBlocks);
   builder.write(encodedintFrameType);
   builder.write(largestAckedPacketInt);
@@ -475,45 +436,72 @@ Optional<WriteAckFrame> writeAckFrameToPacketBuilder(
     currentSeqNum = it->start;
   }
   ackFrame.ackDelay = ackFrameMetaData.ackDelay;
-  if (frameType == FrameType::ACK_ECN) {
+
-    builder.write(ecnECT0Count);
-    builder.write(ecnECT1Count);
-    builder.write(ecnCECount);
-  }
   return ackFrame;
 }
 
-Optional<WriteAckFrameResult> writeAckFrame(
+static uint64_t computeEcnRequiredSpace(
-    const quic::WriteAckFrameMetaData& ackFrameMetaData,
+    const quic::WriteAckFrameMetaData& ackFrameMetaData) {
-    PacketBuilderInterface& builder,
+  QuicInteger ecnECT0Count(ackFrameMetaData.ackState.ecnECT0CountReceived);
-    FrameType frameType) {
+  QuicInteger ecnECT1Count(ackFrameMetaData.ackState.ecnECT1CountReceived);
-  uint64_t beginningSpace = builder.remainingSpaceInPkt();
+  QuicInteger ecnCECount(ackFrameMetaData.ackState.ecnCECountReceived);
-  auto maybeWriteAckFrame =
+  return ecnECT0Count.getSize() + ecnECT1Count.getSize() + ecnCECount.getSize();
-      writeAckFrameToPacketBuilder(ackFrameMetaData, builder, frameType);
-
-  if (maybeWriteAckFrame.has_value()) {
-    builder.appendFrame(std::move(maybeWriteAckFrame.value()));
-    return WriteAckFrameResult(
-        beginningSpace - builder.remainingSpaceInPkt(),
-        maybeWriteAckFrame.value(),
-        maybeWriteAckFrame.value().ackBlocks.size());
-  } else {
-    return none;
-  }
 }
 
-Optional<WriteAckFrameResult> writeAckFrameWithReceivedTimestamps(
+static uint64_t computeReceiveTimestampsMinimumSpace(
+    const quic::WriteAckFrameMetaData& ackFrameMetaData) {
+  // Compute minimum size requirements for 3 fields that must be sent
+  // in every ACK_RECEIVE_TIMESTAMPS frame
+  const WriteAckFrameState& ackState = ackFrameMetaData.ackState;
+  uint64_t countTimestampRanges = 0;
+  uint64_t maybeLastPktNum = 0;
+  std::chrono::microseconds maybeLastPktTsDelta = 0us;
+  if (ackState.lastRecvdPacketInfo) {
+    maybeLastPktNum = ackState.lastRecvdPacketInfo.value().pktNum;
+
+    maybeLastPktTsDelta =
+        (ackState.lastRecvdPacketInfo.value().timings.receiveTimePoint >
+                 ackFrameMetaData.connTime
+             ? std::chrono::duration_cast<std::chrono::microseconds>(
+                   ackState.lastRecvdPacketInfo.value()
+                       .timings.receiveTimePoint -
+                   ackFrameMetaData.connTime)
+             : 0us);
+  }
+
+  return getQuicIntegerSize(countTimestampRanges).value_or(0) +
+      getQuicIntegerSize(maybeLastPktNum).value_or(0) +
+      getQuicIntegerSize(maybeLastPktTsDelta.count()).value_or(0);
+}
+
+static void writeECNFieldsToAck(
     const quic::WriteAckFrameMetaData& ackFrameMetaData,
+    WriteAckFrame& ackFrame,
+    PacketBuilderInterface& builder) {
+  ackFrame.ecnECT0Count = ackFrameMetaData.ackState.ecnECT0CountReceived;
+  ackFrame.ecnECT1Count = ackFrameMetaData.ackState.ecnECT1CountReceived;
+  ackFrame.ecnCECount = ackFrameMetaData.ackState.ecnCECountReceived;
+  QuicInteger ecnECT0Count(ackFrameMetaData.ackState.ecnECT0CountReceived);
+  QuicInteger ecnECT1Count(ackFrameMetaData.ackState.ecnECT1CountReceived);
+  QuicInteger ecnCECount(ackFrameMetaData.ackState.ecnCECountReceived);
+  builder.write(ecnECT0Count);
+  builder.write(ecnECT1Count);
+  builder.write(ecnCECount);
+}
+
+namespace {
+struct AckReceiveTimesStampsWritten {
+  size_t TimestampRangesWritten{0};
+  size_t TimestampWritten{0};
+};
+} // namespace
+
+AckReceiveTimesStampsWritten writeReceiveTimestampFieldsToAck(
+    const quic::WriteAckFrameMetaData& ackFrameMetaData,
+    WriteAckFrame& ackFrame,
     PacketBuilderInterface& builder,
     const AckReceiveTimestampsConfig& recvTimestampsConfig,
     uint64_t maxRecvTimestampsToSend) {
-  auto beginningSpace = builder.remainingSpaceInPkt();
-  auto maybeAckFrame = writeAckFrameToPacketBuilder(
-      ackFrameMetaData, builder, FrameType::ACK_RECEIVE_TIMESTAMPS);
-  if (!maybeAckFrame.has_value()) {
-    return none;
-  }
-  auto ackFrame = maybeAckFrame.value();
   const WriteAckFrameState& ackState = ackFrameMetaData.ackState;
   uint64_t spaceLeft = builder.remainingSpaceInPkt();
   uint64_t lastPktNum = 0;
@@ -544,7 +532,7 @@ Optional<WriteAckFrameResult> writeAckFrameWithReceivedTimestamps(
     auto largestAckedPacket = ackState.acks.back().end;
     uint64_t receiveTimestampsExponentToUse =
         recvTimestampsConfig.receiveTimestampsExponent;
-    countTimestampRanges = fillPacketReceiveTimestamps(
+    countTimestampRanges = fillFrameWithPacketReceiveTimestamps(
         ackFrameMetaData,
         ackFrame,
         largestAckedPacket,
@@ -573,12 +561,72 @@ Optional<WriteAckFrameResult> writeAckFrameWithReceivedTimestamps(
       builder.write(timeStampRangeCountInt);
     }
   }
+  return AckReceiveTimesStampsWritten{countTimestampRanges, countTimestamps};
+}
+
+Optional<WriteAckFrameResult> writeAckFrame(
+    const quic::WriteAckFrameMetaData& ackFrameMetaData,
+    PacketBuilderInterface& builder,
+    FrameType frameType,
+    const AckReceiveTimestampsConfig& recvTimestampsConfig,
+    uint64_t maxRecvTimestampsToSend) {
+  if (ackFrameMetaData.ackState.acks.empty()) {
+    return none;
+  }
+  uint64_t beginningSpace = builder.remainingSpaceInPkt();
+  uint64_t spaceLeft = beginningSpace;
+
+  // Reserve space for ECN counts if enabled
+  if (frameType == FrameType::ACK_ECN) {
+    auto ecnRequiredSpace = computeEcnRequiredSpace(ackFrameMetaData);
+    if (spaceLeft < ecnRequiredSpace) {
+      return none;
+    }
+    spaceLeft -= ecnRequiredSpace;
+  }
+
+  // Reserve space for receive timestamps if enabled
+  if (frameType == FrameType::ACK_RECEIVE_TIMESTAMPS) {
+    auto receiveTimestampsMinimumSpace =
+        computeReceiveTimestampsMinimumSpace(ackFrameMetaData);
+    if (spaceLeft < receiveTimestampsMinimumSpace) {
+      return none;
+    }
+    spaceLeft -= receiveTimestampsMinimumSpace;
+  }
+
+  // Start writing fields to the builder
+
+  // 1. Write the base ack fields (ACK packet type)
+  auto maybeAckFrame =
+      maybeWriteAckBaseFields(ackFrameMetaData, builder, frameType, spaceLeft);
+  if (!maybeAckFrame.has_value()) {
+    return none;
+  }
+  auto& ackFrame = maybeAckFrame.value();
+
+  // 2. Write ECN fields if enabled
+  if (frameType == FrameType::ACK_ECN) {
+    writeECNFieldsToAck(ackFrameMetaData, ackFrame, builder);
+  }
+
+  // 3. Write receive timestamp fields if enabled
+  AckReceiveTimesStampsWritten receiveTimestampsWritten;
+  if (frameType == FrameType::ACK_RECEIVE_TIMESTAMPS) {
+    receiveTimestampsWritten = writeReceiveTimestampFieldsToAck(
+        ackFrameMetaData,
+        ackFrame,
+        builder,
+        recvTimestampsConfig,
+        maxRecvTimestampsToSend);
+  }
+  // Everything written
   auto ackFrameWriteResult = WriteAckFrameResult(
       beginningSpace - builder.remainingSpaceInPkt(),
       ackFrame,
       ackFrame.ackBlocks.size(),
-      countTimestampRanges,
+      receiveTimestampsWritten.TimestampRangesWritten,
-      countTimestamps);
+      receiveTimestampsWritten.TimestampWritten);
 
   builder.appendFrame(std::move(ackFrame));
   return ackFrameWriteResult;

quic/codec/QuicWriteCodec.h

@@ -100,24 +100,16 @@ Optional<WriteCryptoFrame> writeCryptoFrame(
 Optional<WriteAckFrameResult> writeAckFrame(
     const WriteAckFrameMetaData& ackFrameMetaData,
     PacketBuilderInterface& builder,
-    FrameType frameType = FrameType::ACK);
+    FrameType frameType = FrameType::ACK,
+    const AckReceiveTimestampsConfig& recvTimestampsConfig =
+        AckReceiveTimestampsConfig(),
+    uint64_t maxRecvTimestampsToSend = 0);
 
 /**
  * Helper functions to write the fields for ACK_RECEIVE_TIMESTAMPS frame
  */
 size_t computeSizeUsedByRecvdTimestamps(quic::WriteAckFrame& writeAckFrame);
 
-Optional<WriteAckFrameResult> writeAckFrameWithReceivedTimestamps(
-    const WriteAckFrameMetaData& ackFrameMetaData,
-    PacketBuilderInterface& builder,
-    const AckReceiveTimestampsConfig& recvTimestampsConfig,
-    uint64_t maxRecvTimestampsToSend);
-
-Optional<quic::WriteAckFrame> writeAckFrameToPacketBuilder(
-    const WriteAckFrameMetaData& ackFrameMetaData,
-    quic::PacketBuilderInterface& builder,
-    quic::FrameType frameType);
-
 } // namespace quic
 // namespace quic
 // namespace quic

quic/codec/test/QuicWriteCodecTest.cpp

@@ -912,13 +912,12 @@ TEST_P(QuicWriteCodecTest, AckFrameGapExceedsRepresentation) {
   };
 
   EXPECT_THROW(
-      ((frameType == FrameType::ACK || frameType == FrameType::ACK_ECN)
+      writeAckFrame(
-           ? writeAckFrame(ackFrameMetaData, pktBuilder, frameType)
-           : writeAckFrameWithReceivedTimestamps(
           ackFrameMetaData,
           pktBuilder,
+          frameType,
           defaultAckReceiveTimestmpsConfig,
-                 0)),
+          0),
       QuicTransportException);
 }
 
@@ -973,12 +972,10 @@ TEST_P(QuicWriteCodecTest, AckFrameVeryLargeAckRange) {
       .ackDelayExponent = static_cast<uint8_t>(kDefaultAckDelayExponent),
       .connTime = connTime,
   };
-  auto ackFrameWriteResult =
+  auto ackFrameWriteResult = *writeAckFrame(
-      (frameType == FrameType::ACK || frameType == FrameType::ACK_ECN)
-      ? *writeAckFrame(ackFrameMetaData, pktBuilder, frameType)
-      : *writeAckFrameWithReceivedTimestamps(
       ackFrameMetaData,
       pktBuilder,
+      frameType,
       defaultAckReceiveTimestmpsConfig,
       kMaxReceivedPktsTimestampsStored);
   auto addlBytesConsumed = computeBytesForOptionalAckFields(
@@ -1040,13 +1037,12 @@ TEST_P(QuicWriteCodecTest, AckFrameNotEnoughForAnything) {
       .connTime = connTime,
   };
 
-  auto result = (frameType == FrameType::ACK || frameType == FrameType::ACK_ECN)
+  auto result = writeAckFrame(
-      ? writeAckFrame(ackFrameMetaData, pktBuilder, frameType)
-      : writeAckFrameWithReceivedTimestamps(
       ackFrameMetaData,
       pktBuilder,
+      frameType,
       defaultAckReceiveTimestmpsConfig,
-            kDefaultAckDelayExponent);
+      kMaxReceivedPktsTimestampsStored);
   EXPECT_FALSE(result.has_value());
   EXPECT_EQ(pktBuilder.remainingSpaceInPkt(), 4);
 }
@@ -1072,12 +1068,10 @@ TEST_P(QuicWriteCodecTest, WriteSimpleAckFrame) {
   // There is 1 gap => each represented by 2 bytes => 2 bytes
   // 2 byte for first ack block length, then 2 bytes for the next len => 4 bytes
   // total 11 bytes
-  auto ackFrameWriteResult =
+  auto ackFrameWriteResult = *writeAckFrame(
-      (frameType == FrameType::ACK || frameType == FrameType::ACK_ECN)
-      ? *writeAckFrame(ackFrameMetaData, pktBuilder, frameType)
-      : *writeAckFrameWithReceivedTimestamps(
       ackFrameMetaData,
       pktBuilder,
+      frameType,
       defaultAckReceiveTimestmpsConfig,
       kMaxReceivedPktsTimestampsStored);
   auto addlBytesConsumed = computeBytesForOptionalAckFields(
@@ -1143,12 +1137,10 @@ TEST_P(QuicWriteCodecTest, WriteAckFrameWillSaveAckDelay) {
       .connTime = connTime,
   };
 
-  auto ackFrameWriteResult =
+  auto ackFrameWriteResult = *writeAckFrame(
-      (frameType == FrameType::ACK || frameType == FrameType::ACK_ECN)
-      ? *writeAckFrame(ackFrameMetaData, pktBuilder, frameType)
-      : *writeAckFrameWithReceivedTimestamps(
       ackFrameMetaData,
       pktBuilder,
+      frameType,
       defaultAckReceiveTimestmpsConfig,
       kMaxReceivedPktsTimestampsStored);
   auto builtOut = std::move(pktBuilder).buildTestPacket();
@@ -1198,12 +1190,10 @@ TEST_P(QuicWriteCodecTest, VerifyNumAckBlocksSizeAccounted) {
   // There is 1 gap => each represented by 2 bytes => 2 bytes
   // 2 byte for first ack block length, then 2 bytes for the next len => 4 bytes
   // total 11 bytes
-  auto ackFrameWriteResult =
+  auto ackFrameWriteResult = *writeAckFrame(
-      (frameType == FrameType::ACK || frameType == FrameType::ACK_ECN)
-      ? *writeAckFrame(ackFrameMetaData, pktBuilder, frameType)
-      : *writeAckFrameWithReceivedTimestamps(
       ackFrameMetaData,
       pktBuilder,
+      frameType,
       defaultAckReceiveTimestmpsConfig,
       kMaxReceivedPktsTimestampsStored);
   auto addlBytesConsumed = computeBytesForOptionalAckFields(
@@ -1276,11 +1266,10 @@ TEST_P(QuicWriteCodecTest, WriteWithDifferentAckDelayExponent) {
       .connTime = connTime,
   };
 
-  (frameType == FrameType::ACK || frameType == FrameType::ACK_ECN)
+  writeAckFrame(
-      ? writeAckFrame(ackFrameMetaData, pktBuilder, frameType)
-      : *writeAckFrameWithReceivedTimestamps(
       ackFrameMetaData,
       pktBuilder,
+      frameType,
       defaultAckReceiveTimestmpsConfig,
       kMaxReceivedPktsTimestampsStored);
   auto builtOut = std::move(pktBuilder).buildTestPacket();
@@ -1314,11 +1303,10 @@ TEST_P(QuicWriteCodecTest, WriteExponentInLongHeaderPacket) {
       .connTime = connTime,
   };
 
-  (frameType == FrameType::ACK || frameType == FrameType::ACK_ECN)
+  *writeAckFrame(
-      ? writeAckFrame(ackFrameMetaData, pktBuilder, frameType)
-      : writeAckFrameWithReceivedTimestamps(
       ackFrameMetaData,
       pktBuilder,
+      frameType,
       defaultAckReceiveTimestmpsConfig,
       kMaxReceivedPktsTimestampsStored);
   auto builtOut = std::move(pktBuilder).buildLongHeaderPacket();
@@ -1361,12 +1349,10 @@ TEST_P(QuicWriteCodecTest, OnlyAckLargestPacket) {
       .connTime = connTime,
   };
 
-  auto ackFrameWriteResult =
+  auto ackFrameWriteResult = *writeAckFrame(
-      (frameType == FrameType::ACK || frameType == FrameType::ACK_ECN)
-      ? *writeAckFrame(ackFrameMetaData, pktBuilder, frameType)
-      : *writeAckFrameWithReceivedTimestamps(
       ackFrameMetaData,
       pktBuilder,
+      frameType,
       defaultAckReceiveTimestmpsConfig,
       kMaxReceivedPktsTimestampsStored);
   auto addlBytesConsumed = computeBytesForOptionalAckFields(
@@ -1454,12 +1440,10 @@ TEST_P(QuicWriteCodecTest, WriteSomeAckBlocks) {
       .connTime = connTime,
   };
 
-  auto ackFrameWriteResult =
+  auto ackFrameWriteResult = *writeAckFrame(
-      (frameType == FrameType::ACK || frameType == FrameType::ACK_ECN)
-      ? *writeAckFrame(ackFrameMetaData, pktBuilder, frameType)
-      : *writeAckFrameWithReceivedTimestamps(
       ackFrameMetaData,
       pktBuilder,
+      frameType,
       defaultAckReceiveTimestmpsConfig,
       kMaxReceivedPktsTimestampsStored);
   auto addlBytesConsumed = computeBytesForOptionalAckFields(
@@ -1523,12 +1507,10 @@ TEST_P(QuicWriteCodecTest, NoSpaceForAckBlockSection) {
       .connTime = connTime,
   };
 
-  auto ackFrameWriteResult =
+  auto ackFrameWriteResult = writeAckFrame(
-      (frameType == FrameType::ACK || frameType == FrameType::ACK_ECN)
-      ? writeAckFrame(ackFrameMetaData, pktBuilder, frameType)
-      : writeAckFrameWithReceivedTimestamps(
       ackFrameMetaData,
       pktBuilder,
+      frameType,
       defaultAckReceiveTimestmpsConfig,
       kMaxReceivedPktsTimestampsStored);
   EXPECT_FALSE(ackFrameWriteResult.has_value());
@@ -1563,12 +1545,10 @@ TEST_P(QuicWriteCodecTest, OnlyHasSpaceForFirstAckBlock) {
       .connTime = connTime,
   };
 
-  auto ackFrameWriteResult =
+  auto ackFrameWriteResult = *writeAckFrame(
-      (frameType == FrameType::ACK || frameType == FrameType::ACK_ECN)
-      ? *writeAckFrame(ackFrameMetaData, pktBuilder, frameType)
-      : *writeAckFrameWithReceivedTimestamps(
       ackFrameMetaData,
       pktBuilder,
+      frameType,
       defaultAckReceiveTimestmpsConfig,
       kMaxReceivedPktsTimestampsStored);
   auto addlBytesConsumed = computeBytesForOptionalAckFields(
@@ -1633,11 +1613,12 @@ TEST_P(QuicWriteCodecTest, WriteAckFrameWithMultipleTimestampRanges) {
       .connTime = connTime,
   };
 
-  auto ackFrameWriteResult =
+  auto ackFrameWriteResult = *writeAckFrame(
-      (frameType == FrameType::ACK || frameType == FrameType::ACK_ECN)
+      ackFrameMetaData,
-      ? *writeAckFrame(ackFrameMetaData, pktBuilder, frameType)
+      pktBuilder,
-      : *writeAckFrameWithReceivedTimestamps(
+      frameType,
-            ackFrameMetaData, pktBuilder, defaultAckReceiveTimestmpsConfig, 50);
+      defaultAckReceiveTimestmpsConfig,
+      50);
   auto addlBytesConsumed = computeBytesForOptionalAckFields(
       ackFrameMetaData, ackFrameWriteResult, frameType);
 
@@ -1714,12 +1695,10 @@ TEST_P(
   if (frameType == FrameType::ACK_RECEIVE_TIMESTAMPS) {
     pktBuilder.remaining_ = 80;
   }
-  auto ackFrameWriteResult =
+  auto ackFrameWriteResult = *writeAckFrame(
-      (frameType == FrameType::ACK || frameType == FrameType::ACK_ECN)
-      ? *writeAckFrame(ackFrameMetaData, pktBuilder, frameType)
-      : *writeAckFrameWithReceivedTimestamps(
       ackFrameMetaData,
       pktBuilder,
+      frameType,
       defaultAckReceiveTimestmpsConfig,
       100);
   auto addlBytesConsumed = computeBytesForOptionalAckFields(
@@ -2476,6 +2455,4 @@ INSTANTIATE_TEST_SUITE_P(
         FrameType::ACK,
         FrameType::ACK_ECN,
         FrameType::ACK_RECEIVE_TIMESTAMPS));
-// Values(FrameType::ACK, FrameType::ACK_ECN,
-// FrameType::ACK_RECEIVE_TIMESTAMPS));
 } // namespace quic::test