From 830a355bf61f3926b397df74f28de1fd19ba48aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Ma=C5=82ecki?= Date: Mon, 18 Sep 2023 16:28:25 +0200 Subject: [PATCH 1/4] Refax-only (with temporary differences) for 2677 --- srtcore/buffer_snd.cpp | 3 +- srtcore/buffer_snd.h | 2 +- srtcore/buffer_tools.cpp | 5 +-- srtcore/buffer_tools.h | 3 +- srtcore/core.cpp | 2 +- srtcore/core.h | 21 ++++++++---- srtcore/group_backup.h | 1 + srtcore/socketconfig.cpp | 73 ++++++++++++++++++++++++---------------- srtcore/socketconfig.h | 3 ++ 9 files changed, 71 insertions(+), 42 deletions(-) diff --git a/srtcore/buffer_snd.cpp b/srtcore/buffer_snd.cpp index 26f885dd6..9f224a678 100644 --- a/srtcore/buffer_snd.cpp +++ b/srtcore/buffer_snd.cpp @@ -64,7 +64,7 @@ using namespace std; using namespace srt_logging; using namespace sync; -CSndBuffer::CSndBuffer(int size, int maxpld, int authtag) +CSndBuffer::CSndBuffer(int ip_family, int size, int maxpld, int authtag) : m_BufLock() , m_pBlock(NULL) , m_pFirstBlock(NULL) @@ -77,6 +77,7 @@ CSndBuffer::CSndBuffer(int size, int maxpld, int authtag) , m_iAuthTagSize(authtag) , m_iCount(0) , m_iBytesCount(0) + , m_rateEstimator(ip_family) { // initial physical buffer of "size" m_pBuffer = new Buffer; diff --git a/srtcore/buffer_snd.h b/srtcore/buffer_snd.h index 4440b9bfd..5da8c9631 100644 --- a/srtcore/buffer_snd.h +++ b/srtcore/buffer_snd.h @@ -87,7 +87,7 @@ class CSndBuffer /// @param size initial number of blocks (each block to store one packet payload). /// @param maxpld maximum packet payload (including auth tag). /// @param authtag auth tag length in bytes (16 for GCM, 0 otherwise). - CSndBuffer(int size = 32, int maxpld = 1500, int authtag = 0); + CSndBuffer(int ip_family, int size, int maxpld, int authtag); ~CSndBuffer(); public: diff --git a/srtcore/buffer_tools.cpp b/srtcore/buffer_tools.cpp index 0dcf2547f..ac6dd3545 100644 --- a/srtcore/buffer_tools.cpp +++ b/srtcore/buffer_tools.cpp @@ -102,11 +102,12 @@ void AvgBufSize::update(const steady_clock::time_point& now, int pkts, int bytes m_dTimespanMAvg = avg_iir_w<1000, double>(m_dTimespanMAvg, timespan_ms, elapsed_ms); } -CRateEstimator::CRateEstimator() +CRateEstimator::CRateEstimator(int family) : m_iInRatePktsCount(0) , m_iInRateBytesCount(0) , m_InRatePeriod(INPUTRATE_FAST_START_US) // 0.5 sec (fast start) , m_iInRateBps(INPUTRATE_INITIAL_BYTESPS) + , m_iFullHeaderSize(CPacket::UDP_HDR_SIZE + CPacket::HDR_SIZE) {} void CRateEstimator::setInputRateSmpPeriod(int period) @@ -142,7 +143,7 @@ void CRateEstimator::updateInputRate(const time_point& time, int pkts, int bytes return; // Required Byte/sec rate (payload + headers) - m_iInRateBytesCount += (m_iInRatePktsCount * CPacket::SRT_DATA_HDR_SIZE); + m_iInRateBytesCount += (m_iInRatePktsCount * m_iFullHeaderSize); m_iInRateBps = (int)(((int64_t)m_iInRateBytesCount * 1000000) / period_us); HLOGC(bslog.Debug, log << "updateInputRate: pkts:" << m_iInRateBytesCount << " bytes:" << m_iInRatePktsCount diff --git a/srtcore/buffer_tools.h b/srtcore/buffer_tools.h index e6ce89d0d..aacbd8310 100644 --- a/srtcore/buffer_tools.h +++ b/srtcore/buffer_tools.h @@ -94,7 +94,7 @@ class CRateEstimator typedef sync::steady_clock::time_point time_point; typedef sync::steady_clock::duration duration; public: - CRateEstimator(); + CRateEstimator(int family); public: uint64_t getInRatePeriod() const { return m_InRatePeriod; } @@ -124,6 +124,7 @@ class CRateEstimator time_point m_tsInRateStartTime; uint64_t m_InRatePeriod; // usec int m_iInRateBps; // Input Rate in Bytes/sec + int m_iFullHeaderSize; }; diff --git a/srtcore/core.cpp b/srtcore/core.cpp index 084cad670..e42a01e85 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -5621,7 +5621,7 @@ bool srt::CUDT::prepareBuffers(CUDTException* eout) { // CryptoControl has to be initialized and in case of RESPONDER the KM REQ must be processed (interpretSrtHandshake(..)) for the crypto mode to be deduced. const int authtag = (m_pCryptoControl && m_pCryptoControl->getCryptoMode() == CSrtConfig::CIPHER_MODE_AES_GCM) ? HAICRYPT_AUTHTAG_MAX : 0; - m_pSndBuffer = new CSndBuffer(32, m_iMaxSRTPayloadSize, authtag); + m_pSndBuffer = new CSndBuffer(AF_INET, 32, m_iMaxSRTPayloadSize, authtag); SRT_ASSERT(m_iPeerISN != -1); m_pRcvBuffer = new srt::CRcvBuffer(m_iPeerISN, m_config.iRcvBufSize, m_pRcvQueue->m_pUnitQueue, m_config.bMessageAPI); // After introducing lite ACK, the sndlosslist may not be cleared in time, so it requires twice a space. diff --git a/srtcore/core.h b/srtcore/core.h index 71c955c33..ac88d58de 100644 --- a/srtcore/core.h +++ b/srtcore/core.h @@ -328,6 +328,20 @@ class CUDT int peerIdleTimeout_ms() const { return m_config.iPeerIdleTimeout_ms; } size_t maxPayloadSize() const { return m_iMaxSRTPayloadSize; } size_t OPT_PayloadSize() const { return m_config.zExpPayloadSize; } + size_t payloadSize() const + { + // If payloadsize is set, it should already be checked that + // it is less than the possible maximum payload size. So return it + // if it is set to nonzero value. In case when the connection isn't + // yet established, return also 0, if the value wasn't set. + if (m_config.zExpPayloadSize || !m_bConnected) + return m_config.zExpPayloadSize; + + // If SRTO_PAYLOADSIZE was remaining with 0 (default for FILE mode) + // then return the maximum payload size per packet. + return m_iMaxSRTPayloadSize; + } + int sndLossLength() { return m_pSndLossList->getLossLength(); } int32_t ISN() const { return m_iISN; } int32_t peerISN() const { return m_iPeerISN; } @@ -735,13 +749,6 @@ class CUDT static loss_seqs_t defaultPacketArrival(void* vself, CPacket& pkt); static loss_seqs_t groupPacketArrival(void* vself, CPacket& pkt); - CRateEstimator getRateEstimator() const - { - if (!m_pSndBuffer) - return CRateEstimator(); - return m_pSndBuffer->getRateEstimator(); - } - void setRateEstimator(const CRateEstimator& rate) { if (!m_pSndBuffer) diff --git a/srtcore/group_backup.h b/srtcore/group_backup.h index 790cf55ed..cecbc6d1b 100644 --- a/srtcore/group_backup.h +++ b/srtcore/group_backup.h @@ -79,6 +79,7 @@ namespace groups : m_stateCounter() // default init with zeros , m_activeMaxWeight() , m_standbyMaxWeight() + , m_rateEstimate(AF_INET6) // XXX Probably the whole solution is wrong { } diff --git a/srtcore/socketconfig.cpp b/srtcore/socketconfig.cpp index 5c5b8e090..e5b983b03 100644 --- a/srtcore/socketconfig.cpp +++ b/srtcore/socketconfig.cpp @@ -630,36 +630,10 @@ struct CSrtConfigSetter throw CUDTException(MJ_NOTSUP, MN_INVAL, 0); } - if (!co.sPacketFilterConfig.empty()) + std::string errorlog; + if (!co.payloadSizeFits(size_t(val), AF_INET, (errorlog))) { - // This means that the filter might have been installed before, - // and the fix to the maximum payload size was already applied. - // This needs to be checked now. - SrtFilterConfig fc; - if (!ParseFilterConfig(co.sPacketFilterConfig.str(), fc)) - { - // Break silently. This should not happen - LOGC(aclog.Error, log << "SRTO_PAYLOADSIZE: IPE: failing filter configuration installed"); - throw CUDTException(MJ_NOTSUP, MN_INVAL, 0); - } - - const size_t efc_max_payload_size = SRT_LIVE_MAX_PLSIZE - fc.extra_size; - if (size_t(val) > efc_max_payload_size) - { - LOGC(aclog.Error, - log << "SRTO_PAYLOADSIZE: value exceeds " << SRT_LIVE_MAX_PLSIZE << " bytes decreased by " << fc.extra_size - << " required for packet filter header"); - throw CUDTException(MJ_NOTSUP, MN_INVAL, 0); - } - } - - // Not checking AUTO to allow defaul 1456 bytes. - if ((co.iCryptoMode == CSrtConfig::CIPHER_MODE_AES_GCM) - && (val > (SRT_LIVE_MAX_PLSIZE - HAICRYPT_AUTHTAG_MAX))) - { - LOGC(aclog.Error, - log << "SRTO_PAYLOADSIZE: value exceeds " << SRT_LIVE_MAX_PLSIZE << " bytes decreased by " << HAICRYPT_AUTHTAG_MAX - << " required for AES-GCM."); + LOGP(aclog.Error, errorlog); throw CUDTException(MJ_NOTSUP, MN_INVAL, 0); } @@ -1029,6 +1003,47 @@ int CSrtConfig::set(SRT_SOCKOPT optName, const void* optval, int optlen) return dispatchSet(optName, *this, optval, optlen); } +bool CSrtConfig::payloadSizeFits(size_t val, int ip_family, std::string& w_errmsg) ATR_NOTHROW +{ + if (!this->sPacketFilterConfig.empty()) + { + // This means that the filter might have been installed before, + // and the fix to the maximum payload size was already applied. + // This needs to be checked now. + SrtFilterConfig fc; + if (!ParseFilterConfig(this->sPacketFilterConfig.str(), fc)) + { + // Break silently. This should not happen + w_errmsg = "SRTO_PAYLOADSIZE: IPE: failing filter configuration installed"; + return false; + } + + const size_t efc_max_payload_size = SRT_LIVE_MAX_PLSIZE - fc.extra_size; + if (size_t(val) > efc_max_payload_size) + { + std::ostringstream log; + log << "SRTO_PAYLOADSIZE: value exceeds " << SRT_LIVE_MAX_PLSIZE << " bytes decreased by " << fc.extra_size + << " required for packet filter header"; + w_errmsg = log.str(); + return false; + } + } + + // Not checking AUTO to allow defaul 1456 bytes. + if ((this->iCryptoMode == CSrtConfig::CIPHER_MODE_AES_GCM) + && (val > (SRT_LIVE_MAX_PLSIZE - HAICRYPT_AUTHTAG_MAX))) + { + std::ostringstream log; + log << "SRTO_PAYLOADSIZE: value exceeds " << SRT_LIVE_MAX_PLSIZE + << " bytes decreased by " << HAICRYPT_AUTHTAG_MAX + << " required for AES-GCM."; + w_errmsg = log.str(); + return false; + } + + return true; +} + #if ENABLE_BONDING bool SRT_SocketOptionObject::add(SRT_SOCKOPT optname, const void* optval, size_t optlen) { diff --git a/srtcore/socketconfig.h b/srtcore/socketconfig.h index 403616edf..ef31bfbcc 100644 --- a/srtcore/socketconfig.h +++ b/srtcore/socketconfig.h @@ -342,6 +342,9 @@ struct CSrtConfig: CSrtMuxerConfig } int set(SRT_SOCKOPT optName, const void* val, int size); + + bool payloadSizeFits(size_t val, int ip_family, std::string& w_errmsg) ATR_NOTHROW; + }; template From 7fd9da1b104dfe3f294f3b00423d937747e6f08c Mon Sep 17 00:00:00 2001 From: Sektor van Skijlen Date: Mon, 18 Sep 2023 17:16:39 +0200 Subject: [PATCH 2/4] Updated comment in rate estimator init for backup groups --- srtcore/group_backup.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/srtcore/group_backup.h b/srtcore/group_backup.h index cecbc6d1b..ed2fb29ff 100644 --- a/srtcore/group_backup.h +++ b/srtcore/group_backup.h @@ -79,7 +79,14 @@ namespace groups : m_stateCounter() // default init with zeros , m_activeMaxWeight() , m_standbyMaxWeight() - , m_rateEstimate(AF_INET6) // XXX Probably the whole solution is wrong + // XXX Setting AF_INET6 is a temporary solution for using rate estimator + // that counts a rate based on the current link's IP version. The results + // for links using IPv4 could be slightly falsified due to that (16 bytes + // more per a packet), but this makes the estimation results the same for + // the same data sent over the group, regardless of the IP version used + // for the currently active link (which in reality results in different + // load for the same stream, if links use different IP version). + , m_rateEstimate(AF_INET6) { } From cab1a18b6afae70578a24e22e8a9a41b2fd6ec5e Mon Sep 17 00:00:00 2001 From: Sektor van Skijlen Date: Tue, 19 Sep 2023 08:42:14 +0200 Subject: [PATCH 3/4] Update buffer_tools.cpp, hidden unused parameter --- srtcore/buffer_tools.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/srtcore/buffer_tools.cpp b/srtcore/buffer_tools.cpp index ac6dd3545..3f7a77be6 100644 --- a/srtcore/buffer_tools.cpp +++ b/srtcore/buffer_tools.cpp @@ -102,7 +102,7 @@ void AvgBufSize::update(const steady_clock::time_point& now, int pkts, int bytes m_dTimespanMAvg = avg_iir_w<1000, double>(m_dTimespanMAvg, timespan_ms, elapsed_ms); } -CRateEstimator::CRateEstimator(int family) +CRateEstimator::CRateEstimator(int /*family*/) : m_iInRatePktsCount(0) , m_iInRateBytesCount(0) , m_InRatePeriod(INPUTRATE_FAST_START_US) // 0.5 sec (fast start) From 8e68dfc0a47dbec5c3d2c57e82d63b7eb0c8bbfe Mon Sep 17 00:00:00 2001 From: Sektor van Skijlen Date: Tue, 19 Sep 2023 09:57:40 +0200 Subject: [PATCH 4/4] Update socketconfig.cpp: cleared temporary warning --- srtcore/socketconfig.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/srtcore/socketconfig.cpp b/srtcore/socketconfig.cpp index e5b983b03..7f4fcf45c 100644 --- a/srtcore/socketconfig.cpp +++ b/srtcore/socketconfig.cpp @@ -1003,7 +1003,7 @@ int CSrtConfig::set(SRT_SOCKOPT optName, const void* optval, int optlen) return dispatchSet(optName, *this, optval, optlen); } -bool CSrtConfig::payloadSizeFits(size_t val, int ip_family, std::string& w_errmsg) ATR_NOTHROW +bool CSrtConfig::payloadSizeFits(size_t val, int /*ip_family*/, std::string& w_errmsg) ATR_NOTHROW { if (!this->sPacketFilterConfig.empty()) {