From c75502c5636b16cc8fb3e2558cd34d0bda326257 Mon Sep 17 00:00:00 2001 From: Maria Sharabayko Date: Fri, 9 Apr 2021 14:00:13 +0200 Subject: [PATCH 1/3] Minor refactoring during the code studying --- srtcore/core.cpp | 24 ++++++++++++++---------- srtcore/core.h | 49 ++++++++++++++++++++++++------------------------ 2 files changed, 39 insertions(+), 34 deletions(-) diff --git a/srtcore/core.cpp b/srtcore/core.cpp index a3d6b217f..85a7d42dc 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -221,7 +221,9 @@ void CUDT::construct() m_pSndLossList = NULL; m_pRcvLossList = NULL; m_iReorderTolerance = 0; - m_iConsecEarlyDelivery = 0; // how many times so far the packet considered lost has been received before TTL expires + // How many times so far the packet considered lost has been received + // before TTL expires. + m_iConsecEarlyDelivery = 0; m_iConsecOrderedDelivery = 0; m_pSndQueue = NULL; @@ -229,7 +231,8 @@ void CUDT::construct() m_pSNode = NULL; m_pRNode = NULL; - m_iSndHsRetryCnt = SRT_MAX_HSRETRY + 1; // Will be reset to 0 for HSv5, this value is important for HSv4 + // Will be reset to 0 for HSv5, this value is important for HSv4. + m_iSndHsRetryCnt = SRT_MAX_HSRETRY + 1; // Initial status m_bOpened = false; @@ -239,12 +242,12 @@ void CUDT::construct() m_bClosing = false; m_bShutdown = false; m_bBroken = false; - // XXX m_iBrokenCounter should be still set to some default! + // TODO: m_iBrokenCounter should be still set to some default. m_bPeerHealth = true; m_RejectReason = SRT_REJ_UNKNOWN; m_tsLastReqTime = steady_clock::time_point(); m_SrtHsSide = HSD_DRAW; - m_uPeerSrtVersion = 0; // not defined until connected. + m_uPeerSrtVersion = 0; // Not defined until connected. m_iTsbPdDelay_ms = 0; m_iPeerTsbPdDelay_ms = 0; m_bPeerTsbPd = false; @@ -254,10 +257,10 @@ void CUDT::construct() m_bGroupTsbPd = false; m_bPeerTLPktDrop = false; - // Initilize mutex and condition variables + // Initilize mutex and condition variables. initSynch(); - // XXX: Unblock, when the callback is implemented + // TODO: Uncomment when the callback is implemented. // m_cbPacketArrival.set(this, &CUDT::defaultPacketArrival); } @@ -5433,17 +5436,18 @@ void CUDT::acceptAndRespond(const sockaddr_any& agent, const sockaddr_any& peer, // And of course, it is connected. m_bConnected = true; - // register this socket for receiving data packets + // Register this socket for receiving data packets. m_pRNode->m_bOnList = true; m_pRcvQueue->setNewEntry(this); // Save the handshake in m_ConnRes in case when needs repeating. m_ConnRes = w_hs; - // send the response to the peer, see listen() for more discussions about this - // XXX Here create CONCLUSION RESPONSE with: + // Send the response to the peer, see listen() for more discussions + // about this. + // TODO: Here create CONCLUSION RESPONSE with: // - just the UDT handshake, if HS_VERSION_UDT4, - // - if higher, the UDT handshake, the SRT HSRSP, the SRT KMRSP + // - if higher, the UDT handshake, the SRT HSRSP, the SRT KMRSP. size_t size = m_iMaxSRTPayloadSize; // Allocate the maximum possible memory for an SRT payload. // This is a maximum you can send once. diff --git a/srtcore/core.h b/srtcore/core.h index 982408c8b..709e8662a 100644 --- a/srtcore/core.h +++ b/srtcore/core.h @@ -87,7 +87,7 @@ inline T CountIIR(T base, T newval, double factor) return base+T(diff*factor); } -// XXX Probably a better rework for that can be done - this can be +// TODO: Probably a better rework for that can be done - this can be // turned into a serializable structure, just like it's for CHandShake. enum AckDataItem { @@ -97,22 +97,23 @@ enum AckDataItem ACKD_BUFFERLEFT = 3, ACKD_TOTAL_SIZE_SMALL = 4, - // Extra fields existing in UDT (not always sent) - - ACKD_RCVSPEED = 4, // length would be 16 + // Extra fields existing in UDT (not always sent). + ACKD_RCVSPEED = 4, // length = 16 ACKD_BANDWIDTH = 5, - ACKD_TOTAL_SIZE_UDTBASE = 6, // length = 24 - // Extra stats for SRT - + ACKD_TOTAL_SIZE_UDTBASE = 6, // length = 24 + + // Extra stats for SRT. ACKD_RCVRATE = 6, - ACKD_TOTAL_SIZE_VER101 = 7, // length = 28 - ACKD_XMRATE = 7, // XXX This is a weird compat stuff. Version 1.1.3 defines it as ACKD_BANDWIDTH*m_iMaxSRTPayloadSize when set. Never got. - // XXX NOTE: field number 7 may be used for something in future, need to confirm destruction of all !compat 1.0.2 version - - ACKD_TOTAL_SIZE_VER102 = 8, // 32 -// FEATURE BLOCKED. Probably not to be restored. -// ACKD_ACKBITMAP = 8, - ACKD_TOTAL_SIZE = ACKD_TOTAL_SIZE_VER102 // length = 32 (or more) + ACKD_TOTAL_SIZE_VER101 = 7, // length = 28 + // XXX This is a weird compat stuff. Version 1.1.3 defines it as + // ACKD_BANDWIDTH*m_iMaxSRTPayloadSize when set. Never got. + // XXX NOTE: field number 7 may be used for something in future, + // need to confirm destruction of all !compat 1.0.2 version. + ACKD_XMRATE = 7, + + ACKD_TOTAL_SIZE_VER102 = 8, // length = 32 + // ACKD_ACKBITMAP = 8, // Feature blocked, probably not to be restored. + ACKD_TOTAL_SIZE = ACKD_TOTAL_SIZE_VER102 // length = 32 (or more) }; const size_t ACKD_FIELD_SIZE = sizeof(int32_t); @@ -267,14 +268,15 @@ class CUDT // Parameters // - // Note: use notation with X*1000*1000*... instead of million zeros in a row - static const int COMM_RESPONSE_MAX_EXP = 16; - static const int SRT_TLPKTDROP_MINTHRESHOLD_MS = 1000; - static const uint64_t COMM_KEEPALIVE_PERIOD_US = 1*1000*1000; - static const int32_t COMM_SYN_INTERVAL_US = 10*1000; - static const int COMM_CLOSE_BROKEN_LISTENER_TIMEOUT_MS = 3000; - static const uint16_t MAX_WEIGHT = 32767; - static const size_t ACK_WND_SIZE = 1024; + // NOTE: Use notation with X*1000*1000*... instead of + // million zeros in a row. + static const int COMM_RESPONSE_MAX_EXP = 16; + static const int SRT_TLPKTDROP_MINTHRESHOLD_MS = 1000; + static const uint64_t COMM_KEEPALIVE_PERIOD_US = 1*1000*1000; + static const int32_t COMM_SYN_INTERVAL_US = 10*1000; + static const int COMM_CLOSE_BROKEN_LISTENER_TIMEOUT_MS = 3000; + static const uint16_t MAX_WEIGHT = 32767; + static const size_t ACK_WND_SIZE = 1024; int handshakeVersion() { @@ -745,7 +747,6 @@ class CUDT int m_iDeliveryRate; // Packet arrival rate at the receiver side int m_iByteDeliveryRate; // Byte arrival rate at the receiver side - CHandShake m_ConnReq; // Connection request CHandShake m_ConnRes; // Connection response CHandShake::RendezvousState m_RdvState; // HSv5 rendezvous state From 5e78f9d44e984cbbf269e1cc1e747fe4df2f93de Mon Sep 17 00:00:00 2001 From: Maria Sharabayko Date: Mon, 12 Apr 2021 11:26:54 +0200 Subject: [PATCH 2/3] Updated PR after review --- srtcore/core.cpp | 16 ++++++++-------- srtcore/core.h | 47 ++++++++++++++++++++++------------------------- 2 files changed, 30 insertions(+), 33 deletions(-) diff --git a/srtcore/core.cpp b/srtcore/core.cpp index 85a7d42dc..f21759834 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -7715,8 +7715,8 @@ int CUDT::sendCtrlAck(CPacket& ctrlpkt, int size) if (m_uPeerSrtVersion == SrtVersion(1, 0, 2)) { data[ACKD_RCVRATE] = rcvRate; // bytes/sec - data[ACKD_XMRATE] = data[ACKD_BANDWIDTH] * m_iMaxSRTPayloadSize; // bytes/sec - ctrlsz = ACKD_FIELD_SIZE * ACKD_TOTAL_SIZE_VER102; + data[ACKD_XMRATE_VER102_ONLY] = data[ACKD_BANDWIDTH] * m_iMaxSRTPayloadSize; // bytes/sec + ctrlsz = ACKD_FIELD_SIZE * ACKD_TOTAL_SIZE_VER102_ONLY; } else if (m_uPeerSrtVersion >= SrtVersion(1, 0, 3)) { @@ -8005,10 +8005,10 @@ void CUDT::processCtrlAck(const CPacket &ctrlpkt, const steady_clock::time_point * Additional UDT fields, not always attached: * ACKD_RCVSPEED * ACKD_BANDWIDTH - * SRT extension version 1.0.2 (bstats): + * SRT extension since v1.0.1: * ACKD_RCVRATE - * SRT extension version 1.0.4: - * ACKD_XMRATE + * SRT extension since v1.0.2: + * ACKD_XMRATE_VER102_ONLY */ if (acksize > ACKD_TOTAL_SIZE_SMALL) @@ -8018,7 +8018,7 @@ void CUDT::processCtrlAck(const CPacket &ctrlpkt, const steady_clock::time_point int bandwidth = ackdata[ACKD_BANDWIDTH]; int bytesps; - /* SRT v1.0.2 Bytes-based stats: bandwidth (pcData[ACKD_XMRATE]) and delivery rate (pcData[ACKD_RCVRATE]) in + /* SRT v1.0.2 Bytes-based stats: bandwidth (pcData[ACKD_XMRATE_VER102_ONLY]) and delivery rate (pcData[ACKD_RCVRATE]) in * bytes/sec instead of pkts/sec */ /* SRT v1.0.3 Bytes-based stats: only delivery rate (pcData[ACKD_RCVRATE]) in bytes/sec instead of pkts/sec */ if (acksize > ACKD_TOTAL_SIZE_UDTBASE) @@ -8029,8 +8029,8 @@ void CUDT::processCtrlAck(const CPacket &ctrlpkt, const steady_clock::time_point m_iBandwidth = avg_iir<8>(m_iBandwidth, bandwidth); m_iDeliveryRate = avg_iir<8>(m_iDeliveryRate, pktps); m_iByteDeliveryRate = avg_iir<8>(m_iByteDeliveryRate, bytesps); - // XXX not sure if ACKD_XMRATE is of any use. This is simply - // calculated as ACKD_BANDWIDTH * m_iMaxSRTPayloadSize. + // TODO: Not sure if ACKD_XMRATE_VER102_ONLY is of any use. This is + // simply calculated as ACKD_BANDWIDTH * m_iMaxSRTPayloadSize. // Update Estimated Bandwidth and packet delivery rate // m_iRcvRate = m_iDeliveryRate; diff --git a/srtcore/core.h b/srtcore/core.h index 709e8662a..70dbb2f7d 100644 --- a/srtcore/core.h +++ b/srtcore/core.h @@ -76,7 +76,7 @@ modified by #include -// XXX Utility function - to be moved to utilities.h? +// TODO: Utility function - to be moved to utilities.h? template inline T CountIIR(T base, T newval, double factor) { @@ -88,32 +88,29 @@ inline T CountIIR(T base, T newval, double factor) } // TODO: Probably a better rework for that can be done - this can be -// turned into a serializable structure, just like it's for CHandShake. +// turned into a serializable structure, just like it's done for CHandShake. enum AckDataItem { - ACKD_RCVLASTACK = 0, - ACKD_RTT = 1, - ACKD_RTTVAR = 2, - ACKD_BUFFERLEFT = 3, - ACKD_TOTAL_SIZE_SMALL = 4, - - // Extra fields existing in UDT (not always sent). - ACKD_RCVSPEED = 4, // length = 16 - ACKD_BANDWIDTH = 5, - ACKD_TOTAL_SIZE_UDTBASE = 6, // length = 24 - - // Extra stats for SRT. - ACKD_RCVRATE = 6, - ACKD_TOTAL_SIZE_VER101 = 7, // length = 28 - // XXX This is a weird compat stuff. Version 1.1.3 defines it as - // ACKD_BANDWIDTH*m_iMaxSRTPayloadSize when set. Never got. - // XXX NOTE: field number 7 may be used for something in future, - // need to confirm destruction of all !compat 1.0.2 version. - ACKD_XMRATE = 7, - - ACKD_TOTAL_SIZE_VER102 = 8, // length = 32 - // ACKD_ACKBITMAP = 8, // Feature blocked, probably not to be restored. - ACKD_TOTAL_SIZE = ACKD_TOTAL_SIZE_VER102 // length = 32 (or more) + ACKD_RCVLASTACK = 0, + ACKD_RTT = 1, + ACKD_RTTVAR = 2, + ACKD_BUFFERLEFT = 3, + ACKD_TOTAL_SIZE_SMALL = 4, // Size of the Small ACK, packet length = 16. + + // Extra fields for Full ACK. + ACKD_RCVSPEED = 4, + ACKD_BANDWIDTH = 5, + ACKD_TOTAL_SIZE_UDTBASE = 6, // Packet length = 24. + + // Extra stats since SRT v1.0.1. + ACKD_RCVRATE = 6, + ACKD_TOTAL_SIZE_VER101 = 7, // Packet length = 28. + + // Only in SRT v1.0.2. + ACKD_XMRATE_VER102_ONLY = 7, + ACKD_TOTAL_SIZE_VER102_ONLY = 8, // Packet length = 32. + + ACKD_TOTAL_SIZE = ACKD_TOTAL_SIZE_VER102_ONLY // The maximum known ACK length is 32 bytes. }; const size_t ACKD_FIELD_SIZE = sizeof(int32_t); From 1521db5ebd7da1ab6cdffcf48bfeff1a4bd5c4ae Mon Sep 17 00:00:00 2001 From: Maria Sharabayko Date: Mon, 12 Apr 2021 14:15:21 +0200 Subject: [PATCH 3/3] Final corrections after review --- srtcore/core.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/srtcore/core.cpp b/srtcore/core.cpp index f21759834..3149c08b2 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -8007,7 +8007,7 @@ void CUDT::processCtrlAck(const CPacket &ctrlpkt, const steady_clock::time_point * ACKD_BANDWIDTH * SRT extension since v1.0.1: * ACKD_RCVRATE - * SRT extension since v1.0.2: + * SRT extension in v1.0.2 only: * ACKD_XMRATE_VER102_ONLY */ @@ -8029,8 +8029,6 @@ void CUDT::processCtrlAck(const CPacket &ctrlpkt, const steady_clock::time_point m_iBandwidth = avg_iir<8>(m_iBandwidth, bandwidth); m_iDeliveryRate = avg_iir<8>(m_iDeliveryRate, pktps); m_iByteDeliveryRate = avg_iir<8>(m_iByteDeliveryRate, bytesps); - // TODO: Not sure if ACKD_XMRATE_VER102_ONLY is of any use. This is - // simply calculated as ACKD_BANDWIDTH * m_iMaxSRTPayloadSize. // Update Estimated Bandwidth and packet delivery rate // m_iRcvRate = m_iDeliveryRate;