Unify and move receive timestamp config

Summary: We shouldn't have config in the codec types. Instead solve the plumbing problem more explicitly, and only define the config in one place. Reviewed By: jbeshay Differential Revision: D45881730 fbshipit-source-id: fab6c967a38172f16e57a8978b10460fd196902e
facebook · May 17, 2023 · b257b1f · b257b1f
1 parent c603fbd
commit b257b1f
Show file tree

Hide file tree

Showing 11 changed files with 121 additions and 100 deletions.
diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp
@@ -574,8 +574,7 @@ folly::Optional<PacketNum> AckScheduler::writeNextAcks(
       ackDelay, /* ackDelay */
       static_cast<uint8_t>(ackDelayExponentToUse), /* ackDelayExponent */
       conn_.connectionTime, /* connect timestamp */
-      folly::none, /* recvTimestampsConfig */
-      folly::none /* maxAckReceiveTimestampsToSend */};
+  };
 
   folly::Optional<WriteAckFrameResult> ackWriteResult;
 
@@ -594,12 +593,12 @@ folly::Optional<PacketNum> AckScheduler::writeNextAcks(
   if (!isAckReceiveTimestampsSupported || !peerRequestedTimestampsCount) {
     ackWriteResult = writeAckFrame(meta, builder, FrameType::ACK);
   } else {
-    meta.recvTimestampsConfig =
-        conn_.transportSettings.maybeAckReceiveTimestampsConfigSentToPeer
-            .value();
-    meta.maxAckReceiveTimestampsToSend = peerRequestedTimestampsCount;
     ackWriteResult = writeAckFrameWithReceivedTimestamps(
-        meta, builder, FrameType::ACK_RECEIVE_TIMESTAMPS);
+        meta,
+        builder,
+        conn_.transportSettings.maybeAckReceiveTimestampsConfigSentToPeer
+            .value(),
+        peerRequestedTimestampsCount);
   }
   if (!ackWriteResult) {
     return folly::none;

diff --git a/quic/client/QuicClientTransport.cpp b/quic/client/QuicClientTransport.cpp
@@ -1689,14 +1689,14 @@ void QuicClientTransport::setSupportedExtensionTransportParameters() {
         static_cast<uint64_t>(
             TransportParameterId::max_receive_timestamps_per_ack),
         ts.maybeAckReceiveTimestampsConfigSentToPeer.value()
-            .max_receive_timestamps_per_ack);
+            .maxReceiveTimestampsPerAck);
     customTransportParameters_.push_back(maxReceiveTimestampsPerAck.encode());
 
     CustomIntegralTransportParameter receiveTimestampsExponent(
         static_cast<uint64_t>(
             TransportParameterId::receive_timestamps_exponent),
         ts.maybeAckReceiveTimestampsConfigSentToPeer.value()
-            .receive_timestamps_exponent);
+            .receiveTimestampsExponent);
     customTransportParameters_.push_back(receiveTimestampsExponent.encode());
   }
 

diff --git a/quic/codec/Decode.cpp b/quic/codec/Decode.cpp
@@ -307,7 +307,7 @@ ReadAckFrame decodeAckFrameWithReceivedTimestamps(
     uint8_t receiveTimestampsExponentToUse =
         (params.maybeAckReceiveTimestampsConfig)
         ? params.maybeAckReceiveTimestampsConfig.value()
-              .receive_timestamps_exponent
+              .receiveTimestampsExponent
         : kDefaultReceiveTimestampsExponent;
     for (uint64_t i = 0; i < receiveTimeStampsLen->first; i++) {
       auto delta = decodeQuicInteger(cursor);

diff --git a/quic/codec/QuicPacketRebuilder.cpp b/quic/codec/QuicPacketRebuilder.cpp
@@ -235,8 +235,7 @@ folly::Optional<PacketEvent> PacketRebuilder::rebuildFromPacket(
         ackDelay, /* ackDelay */
         static_cast<uint8_t>(ackDelayExponent), /* ackDelayExponent */
         conn_.connectionTime, /* connect timestamp */
-        folly::none, /* recvTimestampsConfig */
-        folly::none /* maxAckReceiveTimestampsToSend */};
+    };
 
     folly::Optional<WriteAckFrameResult> ackWriteResult;
 
@@ -254,12 +253,12 @@ folly::Optional<PacketEvent> PacketRebuilder::rebuildFromPacket(
     if (!isAckReceiveTimestampsSupported || !peerRequestedTimestampsCount) {
       writeAckFrame(meta, builder_, FrameType::ACK);
     } else {
-      meta.recvTimestampsConfig =
-          conn_.transportSettings.maybeAckReceiveTimestampsConfigSentToPeer
-              .value();
-      meta.maxAckReceiveTimestampsToSend = peerRequestedTimestampsCount;
       writeAckFrameWithReceivedTimestamps(
-          meta, builder_, FrameType::ACK_RECEIVE_TIMESTAMPS);
+          meta,
+          builder_,
+          conn_.transportSettings.maybeAckReceiveTimestampsConfigSentToPeer
+              .value(),
+          peerRequestedTimestampsCount);
     }
   }
   // We shouldn't clone if:

diff --git a/quic/codec/QuicWriteCodec.cpp b/quic/codec/QuicWriteCodec.cpp
@@ -286,17 +286,15 @@ static size_t fillPacketReceiveTimestamps(
     WriteAckFrame& ackFrame,
     uint64_t largestAckedPacketNum,
     uint64_t spaceLeft,
-    uint64_t receiveTimestampsExponent) {
+    uint64_t receiveTimestampsExponent,
+    uint64_t maxRecvTimestampsToSend) {
   if (ackFrameMetaData.ackState.recvdPacketInfos.size() == 0) {
     return 0;
   }
   auto recvdPacketInfos = ackFrameMetaData.ackState.recvdPacketInfos;
   // Insert all received packet timestamps into an interval set, to identify
   // continguous ranges
 
-  DCHECK(ackFrameMetaData.maxAckReceiveTimestampsToSend.has_value());
-  auto maxRecvTimestampsToSend =
-      ackFrameMetaData.maxAckReceiveTimestampsToSend.value();
   uint64_t pktsAdded = 0;
   IntervalSet<PacketNum> receivedPktNumsIntervalSet;
   for (auto& recvdPkt : recvdPacketInfos) {
@@ -491,10 +489,11 @@ folly::Optional<WriteAckFrameResult> writeAckFrame(
 folly::Optional<WriteAckFrameResult> writeAckFrameWithReceivedTimestamps(
     const quic::WriteAckFrameMetaData& ackFrameMetaData,
     PacketBuilderInterface& builder,
-    FrameType frameType) {
+    const AckReceiveTimestampsConfig& recvTimestampsConfig,
+    uint64_t maxRecvTimestampsToSend) {
   auto beginningSpace = builder.remainingSpaceInPkt();
-  auto maybeAckFrame =
-      writeAckFrameToPacketBuilder(ackFrameMetaData, builder, frameType);
+  auto maybeAckFrame = writeAckFrameToPacketBuilder(
+      ackFrameMetaData, builder, FrameType::ACK_RECEIVE_TIMESTAMPS);
   if (!maybeAckFrame.has_value()) {
     return folly::none;
   }
@@ -527,16 +526,14 @@ folly::Optional<WriteAckFrameResult> writeAckFrameWithReceivedTimestamps(
   if (spaceLeft > 0) {
     auto largestAckedPacket = ackState.acks.back().end;
     uint8_t receiveTimestampsExponentToUse =
-        ackFrameMetaData.recvTimestampsConfig.has_value()
-        ? ackFrameMetaData.recvTimestampsConfig.value()
-              .receive_timestamps_exponent
-        : kDefaultReceiveTimestampsExponent;
+        recvTimestampsConfig.receiveTimestampsExponent;
     countTimestampRanges = fillPacketReceiveTimestamps(
         ackFrameMetaData,
         ackFrame,
         largestAckedPacket,
         spaceLeft,
-        receiveTimestampsExponentToUse);
+        receiveTimestampsExponentToUse,
+        maxRecvTimestampsToSend);
     if (countTimestampRanges > 0) {
       QuicInteger timeStampRangeCountInt(
           ackFrame.recvdPacketsTimestampRanges.size());

diff --git a/quic/codec/QuicWriteCodec.h b/quic/codec/QuicWriteCodec.h
@@ -126,7 +126,8 @@ size_t computeSizeUsedByRecvdTimestamps(quic::WriteAckFrame& writeAckFrame);
 folly::Optional<WriteAckFrameResult> writeAckFrameWithReceivedTimestamps(
     const WriteAckFrameMetaData& ackFrameMetaData,
     PacketBuilderInterface& builder,
-    FrameType frameType = FrameType::ACK_RECEIVE_TIMESTAMPS);
+    const AckReceiveTimestampsConfig& recvTimestampsConfig,
+    uint64_t maxRecvTimestampsToSend);
 
 folly::Optional<quic::WriteAckFrame> writeAckFrameToPacketBuilder(
     const WriteAckFrameMetaData& ackFrameMetaData,

diff --git a/quic/codec/Types.h b/quic/codec/Types.h
@@ -21,7 +21,6 @@
 #include <quic/common/IntervalSet.h>
 #include <quic/common/SmallCollections.h>
 #include <quic/common/Variant.h>
-#include <quic/state/TransportSettings.h>
 
 /**
  * This details the types of objects that can be serialized or deserialized
@@ -263,11 +262,6 @@ struct WriteAckFrameMetaData {
 
   // Receive timestamps basis
   TimePoint connTime;
-
-  folly::Optional<AckReceiveTimestampsConfig> recvTimestampsConfig =
-      folly::none;
-
-  folly::Optional<uint64_t> maxAckReceiveTimestampsToSend = folly::none;
 };
 
 struct WriteAckFrameResult {