From 49d31b18dbdd8f4b84c29597ea4488bf4e64d54e Mon Sep 17 00:00:00 2001 From: Fletcher Dunn Date: Thu, 29 Aug 2024 16:56:57 -0700 Subject: [PATCH 1/4] Fix bug not properly detecting an invalid stop_waiting value. P4:9130151 --- .../clientlib/steamnetworkingsockets_snp.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp index 2709eb5..305ff47 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp @@ -989,12 +989,18 @@ bool CSteamNetworkConnectionBase::ProcessPlainTextDataChunk( int usecTimeSinceLa case 2: READ_24BITU( nOffset, szStopWaitingOffset ); break; case 3: READ_64BITU( nOffset, szStopWaitingOffset ); break; } - if ( nOffset >= nPktNum ) + ++nOffset; + int64 nMinPktNumToSendAcks = nPktNum-nOffset; + + // Make sure the resulting stop value makes sense. + // Force the use of an unsigned comparison, since a negative stop value + // is also illegal. That would be rejected below, but rejecting it here + // is clearer. + if ( (uint64)nMinPktNumToSendAcks >= (uint64)nPktNum ) { DECODE_ERROR( "stop_waiting pktNum %llu offset %llu", nPktNum, nOffset ); } - ++nOffset; - int64 nMinPktNumToSendAcks = nPktNum-nOffset; + if ( nMinPktNumToSendAcks == m_receiverState.m_nMinPktNumToSendAcks ) continue; if ( nMinPktNumToSendAcks < m_receiverState.m_nMinPktNumToSendAcks ) From b2b104f25c0e917a0a434c75a2904751f4dc8eea Mon Sep 17 00:00:00 2001 From: Fletcher Dunn Date: Thu, 29 Aug 2024 16:58:32 -0700 Subject: [PATCH 2/4] Factor out processing of a jitter sample. Make a method that can be "overridden" and customized by specific stats trackers. P4:9130218 --- .../steamnetworking_statsutils.h | 30 +++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/steamnetworkingsockets/steamnetworking_statsutils.h b/src/steamnetworkingsockets/steamnetworking_statsutils.h index 9e13308..0cb8f13 100644 --- a/src/steamnetworkingsockets/steamnetworking_statsutils.h +++ b/src/steamnetworkingsockets/steamnetworking_statsutils.h @@ -1000,6 +1000,18 @@ protected: m_seqPktCounters.OnDropped( nDropped ); } + inline void InternalProcessJitterSample( int usecJitter ) + { + // This code only cares about absolute value + usecJitter = abs( usecJitter ); + + // Update max jitter for current interval + m_seqPktCounters.m_usecMaxJitter = std::max( m_seqPktCounters.m_usecMaxJitter, usecJitter ); + + // Add to histogram + m_jitterHistogram.AddSample( usecJitter ); + } + /// Called when we receive stats message from remote host template inline static void InternalProcessMessage( TLinkStatsTracker *pThis, const CMsgSteamDatagramConnectionQuality &msg, SteamNetworkingMicroseconds usecNow ) @@ -1340,20 +1352,14 @@ struct LinkStatsTracker final : public TLinkStatsTracker ++TLinkStatsTracker::m_nDebugPktsRecvInOrder; // We've received two packets, in order. Did the sender supply the time between packets on his side? - if ( usecSenderTimeSincePrev > 0 ) + if ( usecSenderTimeSincePrev > 0 && usecSenderTimeSincePrev < k_usecTimeSinceLastPacketMaxReasonable ) { - int usecJitter = ( usecNow - TLinkStatsTracker::m_usecTimeLastRecvSeq ) - usecSenderTimeSincePrev; - usecJitter = abs( usecJitter ); - if ( usecJitter < k_usecTimeSinceLastPacketMaxReasonable ) + SteamNetworkingMicroseconds usecRecvTimeSincePrev = ( usecNow - TLinkStatsTracker::m_usecTimeLastRecvSeq ); + Assert( usecRecvTimeSincePrev >= 0 ); + if ( (uint64)usecRecvTimeSincePrev < (uint64)k_usecTimeSinceLastPacketMaxReasonable ) { - - // Update max jitter for current interval - TLinkStatsTracker::m_seqPktCounters.m_usecMaxJitter = std::max( TLinkStatsTracker::m_seqPktCounters.m_usecMaxJitter, usecJitter ); - TLinkStatsTracker::m_jitterHistogram.AddSample( usecJitter ); - } - else - { - // Something is really, really off. Discard measurement + int usecJitter = usecRecvTimeSincePrev - usecSenderTimeSincePrev; + TLinkStatsTracker::InternalProcessJitterSample( usecJitter ); } } From 879a52e335fdddc9d12011d9850029a027707cea Mon Sep 17 00:00:00 2001 From: Fletcher Dunn Date: Thu, 29 Aug 2024 17:00:00 -0700 Subject: [PATCH 3/4] Don't discard "time since previous" values of 0 or 1. I don't know why I did this, it seems silly. Delete k_usecTimeSinceLastPacketMinReasonable. Any value is reasonable, because it is common to send two packets in immediate succession! P4:9134854 --- src/steamnetworkingsockets/steamnetworking_statsutils.h | 2 +- src/steamnetworkingsockets/steamnetworkingsockets_internal.h | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/src/steamnetworkingsockets/steamnetworking_statsutils.h b/src/steamnetworkingsockets/steamnetworking_statsutils.h index 0cb8f13..fad7187 100644 --- a/src/steamnetworkingsockets/steamnetworking_statsutils.h +++ b/src/steamnetworkingsockets/steamnetworking_statsutils.h @@ -1352,7 +1352,7 @@ struct LinkStatsTracker final : public TLinkStatsTracker ++TLinkStatsTracker::m_nDebugPktsRecvInOrder; // We've received two packets, in order. Did the sender supply the time between packets on his side? - if ( usecSenderTimeSincePrev > 0 && usecSenderTimeSincePrev < k_usecTimeSinceLastPacketMaxReasonable ) + if ( usecSenderTimeSincePrev >= 0 && usecSenderTimeSincePrev < k_usecTimeSinceLastPacketMaxReasonable ) { SteamNetworkingMicroseconds usecRecvTimeSincePrev = ( usecNow - TLinkStatsTracker::m_usecTimeLastRecvSeq ); Assert( usecRecvTimeSincePrev >= 0 ); diff --git a/src/steamnetworkingsockets/steamnetworkingsockets_internal.h b/src/steamnetworkingsockets/steamnetworkingsockets_internal.h index 5a00904..27b6caa 100644 --- a/src/steamnetworkingsockets/steamnetworkingsockets_internal.h +++ b/src/steamnetworkingsockets/steamnetworkingsockets_internal.h @@ -358,10 +358,6 @@ const unsigned k_usecTimeSinceLastPacketSerializedPrecisionShift = 4; const SteamNetworkingMicroseconds k_usecTimeSinceLastPacketMaxReasonable = k_nMillion/4; COMPILE_TIME_ASSERT( ( k_usecTimeSinceLastPacketMaxReasonable >> k_usecTimeSinceLastPacketSerializedPrecisionShift ) < 0x8000 ); // make sure all "reasonable" values can get serialized into 16-bits -/// Don't send spacing values when packets are sent extremely close together. The spacing -/// should be a bit higher that our serialization precision. -const SteamNetworkingMicroseconds k_usecTimeSinceLastPacketMinReasonable = 2 << k_usecTimeSinceLastPacketSerializedPrecisionShift; - /// A really terrible ping score, but one that we can do some math with without overflowing constexpr int k_nRouteScoreHuge = INT_MAX/8; From 9ebccd7646b4445287020d775c8e3aac90bd2200 Mon Sep 17 00:00:00 2001 From: Fletcher Dunn Date: Thu, 29 Aug 2024 17:02:59 -0700 Subject: [PATCH 4/4] Make plain UDP connections know how to decode "time since previous." If it is present. This is the value used to calculate jitter. (The sender is not sending this yet, but I'll add that next.) P4:9134859 --- .../clientlib/steamnetworkingsockets_udp.cpp | 15 ++++++++++++++- .../clientlib/steamnetworkingsockets_udp.h | 2 ++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_udp.cpp b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_udp.cpp index d39c486..6f474eb 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_udp.cpp +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_udp.cpp @@ -753,6 +753,20 @@ void CConnectionTransportUDPBase::Received_Data( const uint8 *pPkt, int cbPkt, S pIn += cbStatsMsgIn; } + // Time between the last sequenced packet? + int usecTimeSinceLast = -1; + if ( hdr->m_unMsgFlags & hdr->kFlag_TimeSincePrev ) + { + if ( pIn + sizeof(unsigned short) > pPktEnd ) + { + ReportBadUDPPacketFromConnectionPeer( "DataPacket", "Flags indicate presence of TimeSincePrev, but no room for it. Stats message size %d, packet size %d", cbStatsMsgIn, cbPkt ); + return; + } + + usecTimeSinceLast = (int)LittleWord( *(unsigned short*)pIn ) << k_usecTimeSinceLastPacketSerializedPrecisionShift; + pIn += sizeof(unsigned short); + } + const void *pChunk = pIn; int cbChunk = pPktEnd - pIn; @@ -768,7 +782,6 @@ void CConnectionTransportUDPBase::Received_Data( const uint8 *pPkt, int cbPkt, S RecvValidUDPDataPacket( ctx ); // Process plaintext - int usecTimeSinceLast = 0; // FIXME - should we plumb this through so we can measure jitter? if ( !m_connection.ProcessPlainTextDataChunk( usecTimeSinceLast, ctx ) ) return; diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_udp.h b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_udp.h index f01eb95..13140ab 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_udp.h +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_udp.h @@ -27,6 +27,7 @@ struct UDPDataMsgHdr enum { kFlag_ProtobufBlob = 0x01, // Protobuf-encoded message is inline (CMsgSteamSockets_UDP_Stats) + kFlag_TimeSincePrev = 0x02, // If set, measurement(s) of the time since the last sequenced packet is present, for packet delay variation estimation }; uint8 m_unMsgFlags; @@ -34,6 +35,7 @@ struct UDPDataMsgHdr uint16 m_unSeqNum; // [optional, if flags&kFlag_ProtobufBlob] varint-encoded protobuf blob size, followed by blob + // [optional, if flags&kFlag_TimeSincePrev] uint16 time between this packet and the previous one, on client. See k_usecTimeSinceLastPacketSerializedPrecisionShift // Data frame(s) // End of packet };