From fabd52ff9b0af350176b3f0c06658705399af0ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Ma=C5=82ecki?= Date: Thu, 13 Feb 2020 10:19:15 +0100 Subject: [PATCH 1/3] Improved log --- srtcore/congctl.cpp | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/srtcore/congctl.cpp b/srtcore/congctl.cpp index db934b1a7..04989c650 100644 --- a/srtcore/congctl.cpp +++ b/srtcore/congctl.cpp @@ -74,10 +74,8 @@ class LiveCC: public SrtCongestionControlBase : SrtCongestionControlBase(parent) { m_llSndMaxBW = BW_INFINITE; // 1 Gbbps in Bytes/sec BW_INFINITE - m_zMaxPayloadSize = parent->OPT_PayloadSize(); - if ( m_zMaxPayloadSize == 0 ) - m_zMaxPayloadSize = parent->maxPayloadSize(); - m_zSndAvgPayloadSize = m_zMaxPayloadSize; + m_zMaxPayloadSize = parent->maxPayloadSize(); + m_zSndAvgPayloadSize = 0; // NOT 0 and not MAX m_iMinNakInterval_us = 20000; //Minimum NAK Report Period (usec) m_iNakReportAccel = 2; //Default NAK Report Period (RTT) accelerator @@ -148,8 +146,13 @@ class LiveCC: public SrtCongestionControlBase // Worst case scenario, the procedure running in CRcvQueue::worker // thread will pick up a "slightly outdated" average value from this // field - this is insignificant. - m_zSndAvgPayloadSize = avg_iir<128, size_t>(m_zSndAvgPayloadSize, packet.getLength()); - HLOGC(cclog.Debug, log << "LiveCC: avg payload size updated: " << m_zSndAvgPayloadSize); + size_t plen = packet.getLength(); + if (m_zSndAvgPayloadSize == 0) + m_zSndAvgPayloadSize = plen; + else + m_zSndAvgPayloadSize = avg_iir<128>(m_zSndAvgPayloadSize, plen); + + HLOGC(cclog.Debug, log << "LiveCC: avg payload size updated: " << plen << " -> " << m_zSndAvgPayloadSize); } void updatePktSndPeriod_onTimer(ETransmissionEvent , EventVariant var) @@ -165,6 +168,9 @@ class LiveCC: public SrtCongestionControlBase void updatePktSndPeriod() { + if (m_zSndAvgPayloadSize == 0) + return; // do not update - nothing was sent so far + // packet = payload + header const double pktsize = (double) m_zSndAvgPayloadSize + CPacket::SRT_DATA_HDR_SIZE; m_dPktSndPeriod = 1000 * 1000.0 * (pktsize / m_llSndMaxBW); From 164cccf559bd6b1d38f67c4ed9b1d9e571f294dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Ma=C5=82ecki?= Date: Thu, 13 Feb 2020 12:27:25 +0100 Subject: [PATCH 2/3] Fixed FEC-corrected payload size. Restored payload size as single sending unit --- srtcore/congctl.cpp | 6 ++- srtcore/core.cpp | 82 +++++++++++++++------------------------- srtcore/core.h | 8 ++-- srtcore/packetfilter.cpp | 3 +- 4 files changed, 39 insertions(+), 60 deletions(-) diff --git a/srtcore/congctl.cpp b/srtcore/congctl.cpp index 04989c650..f2cb51704 100644 --- a/srtcore/congctl.cpp +++ b/srtcore/congctl.cpp @@ -74,8 +74,10 @@ class LiveCC: public SrtCongestionControlBase : SrtCongestionControlBase(parent) { m_llSndMaxBW = BW_INFINITE; // 1 Gbbps in Bytes/sec BW_INFINITE - m_zMaxPayloadSize = parent->maxPayloadSize(); - m_zSndAvgPayloadSize = 0; // NOT 0 and not MAX + m_zMaxPayloadSize = parent->userPayloadSize(); + if ( m_zMaxPayloadSize == 0 ) + m_zMaxPayloadSize = parent->maxPayloadSize(); + m_zSndAvgPayloadSize = 0; m_iMinNakInterval_us = 20000; //Minimum NAK Report Period (usec) m_iNakReportAccel = 2; //Default NAK Report Period (RTT) accelerator diff --git a/srtcore/core.cpp b/srtcore/core.cpp index 68573b549..f934213ee 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -260,7 +260,7 @@ CUDT::CUDT(CUDTSocket* parent): m_parent(parent) m_bOPT_GroupConnect = false; m_bTLPktDrop = true; // Too-late Packet Drop m_bMessageAPI = true; - m_zOPT_ExpPayloadSize = SRT_LIVE_DEF_PLSIZE; + m_zUserPayloadSize = SRT_LIVE_MAX_PLSIZE; m_iIpV6Only = -1; // Runtime m_bRcvNakReport = true; // Receiver's Periodic NAK Reports @@ -321,7 +321,7 @@ CUDT::CUDT(CUDTSocket* parent, const CUDT& ancestor): m_parent(parent) m_bOPT_StrictEncryption = ancestor.m_bOPT_StrictEncryption; m_iOPT_PeerIdleTimeout = ancestor.m_iOPT_PeerIdleTimeout; m_bOPT_GroupConnect = ancestor.m_bOPT_GroupConnect; - m_zOPT_ExpPayloadSize = ancestor.m_zOPT_ExpPayloadSize; + m_zUserPayloadSize = ancestor.m_zUserPayloadSize; m_bTLPktDrop = ancestor.m_bTLPktDrop; m_bMessageAPI = ancestor.m_bMessageAPI; m_iIpV6Only = ancestor.m_iIpV6Only; @@ -786,30 +786,7 @@ void CUDT::setOpt(SRT_SOCKOPT optName, const void* optval, int optlen) throw CUDTException(MJ_NOTSUP, MN_INVAL, 0); } - if (m_OPT_PktFilterConfigString != "") - { - // 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(m_OPT_PktFilterConfigString, fc)) - { - // Break silently. This should not happen - LOGC(mglog.Error, log << "SRTO_PAYLOADSIZE: IPE: failing filter configuration installed"); - throw CUDTException(MJ_NOTSUP, MN_INVAL, 0); - } - - size_t efc_max_payload_size = SRT_LIVE_MAX_PLSIZE - fc.extra_size; - if (m_zOPT_ExpPayloadSize > efc_max_payload_size) - { - LOGC(mglog.Error, - log << "SRTO_PAYLOADSIZE: value exceeds SRT_LIVE_MAX_PLSIZE decreased by " << fc.extra_size - << " required for packet filter header"); - throw CUDTException(MJ_NOTSUP, MN_INVAL, 0); - } - } - - m_zOPT_ExpPayloadSize = *(int *)optval; + LOGC(mglog.Warn, log << "SRTO_PAYLOADSIZE: this option is deprecated and ignored; you can safely remove it."); break; case SRTO_TRANSTYPE: @@ -835,7 +812,7 @@ void CUDT::setOpt(SRT_SOCKOPT optName, const void* optval, int optlen) m_iOPT_SndDropDelay = 0; m_bMessageAPI = true; m_bRcvNakReport = true; - m_zOPT_ExpPayloadSize = SRT_LIVE_DEF_PLSIZE; + m_zUserPayloadSize = calcMaxPayloadSize(); m_Linger.l_onoff = 0; m_Linger.l_linger = 0; m_CongCtl.select("live"); @@ -855,7 +832,7 @@ void CUDT::setOpt(SRT_SOCKOPT optName, const void* optval, int optlen) m_iOPT_SndDropDelay = -1; m_bMessageAPI = false; m_bRcvNakReport = false; - m_zOPT_ExpPayloadSize = 0; // use maximum + m_zUserPayloadSize = 0; // no limit check m_Linger.l_onoff = 1; m_Linger.l_linger = DEF_LINGER_S; m_CongCtl.select("file"); @@ -945,13 +922,11 @@ void CUDT::setOpt(SRT_SOCKOPT optName, const void* optval, int optlen) throw CUDTException(MJ_NOTSUP, MN_INVAL, 0); } - size_t efc_max_payload_size = SRT_LIVE_MAX_PLSIZE - fc.extra_size; - if (m_zOPT_ExpPayloadSize > efc_max_payload_size) + size_t efc_max_payload_size = calcMaxPayloadSize() - fc.extra_size; + if (m_zUserPayloadSize > efc_max_payload_size) { - LOGC(mglog.Warn, - log << "Due to filter-required extra " << fc.extra_size << " bytes, SRTO_PAYLOADSIZE fixed to " - << efc_max_payload_size << " bytes"); - m_zOPT_ExpPayloadSize = efc_max_payload_size; + m_zUserPayloadSize = efc_max_payload_size; + HLOGC(mglog.Debug, log << "SRTO_PACKETFILTER: fixed user max payload size: " << m_zUserPayloadSize); } m_OPT_PktFilterConfigString = arg; @@ -1229,7 +1204,8 @@ void CUDT::getOpt(SRT_SOCKOPT optName, void *optval, int &optlen) case SRTO_PAYLOADSIZE: optlen = sizeof(int); - *(int *)optval = m_zOPT_ExpPayloadSize; + LOGC(mglog.Warn, log << "SRTO_PAYLOADSIZE: option deprecated and ignored."); + *(int *)optval = 0; break; case SRTO_ENFORCEDENCRYPTION: @@ -1290,9 +1266,9 @@ std::string CUDT::getstreamid(SRTSOCKET u) void CUDT::clearData() { // Initial sequence number, loss, acknowledgement, etc. - int udpsize = m_iMSS - CPacket::UDP_HDR_SIZE; - - m_iMaxSRTPayloadSize = udpsize - CPacket::HDR_SIZE; + m_iMaxSRTPayloadSize = calcMaxPayloadSize(); + if (m_zUserPayloadSize != 0 && int(m_zUserPayloadSize) > m_iMaxSRTPayloadSize) + m_zUserPayloadSize = m_iMaxSRTPayloadSize; HLOGC(mglog.Debug, log << "clearData: PAYLOAD SIZE: " << m_iMaxSRTPayloadSize); @@ -3229,13 +3205,11 @@ bool CUDT::checkApplyFilterConfig(const std::string &confstr) m_OPT_PktFilterConfigString = confstr; } - size_t efc_max_payload_size = SRT_LIVE_MAX_PLSIZE - cfg.extra_size; - if (m_zOPT_ExpPayloadSize > efc_max_payload_size) + size_t efc_max_payload_size = calcMaxPayloadSize() - cfg.extra_size; + if (m_zUserPayloadSize > efc_max_payload_size) { - LOGC(mglog.Warn, - log << "Due to filter-required extra " << cfg.extra_size << " bytes, SRTO_PAYLOADSIZE fixed to " - << efc_max_payload_size << " bytes"); - m_zOPT_ExpPayloadSize = efc_max_payload_size; + m_zUserPayloadSize = efc_max_payload_size; + HLOGC(mglog.Debug, log << "checkApplyFilterConfig: fixed user max payload size: " << m_zUserPayloadSize); } return true; @@ -4802,13 +4776,18 @@ EConnectStatus CUDT::processConnectResponse(const CPacket& response, CUDTExcepti return postConnect(response, false, eout, synchro); } +size_t CUDT::calcMaxPayloadSize() const +{ + const int udpsize = m_iMSS - CPacket::UDP_HDR_SIZE; + return udpsize - CPacket::HDR_SIZE; +} + void CUDT::applyResponseSettings() { // Re-configure according to the negotiated values. m_iMSS = m_ConnRes.m_iMSS; m_iFlowWindowSize = m_ConnRes.m_iFlightFlagSize; - int udpsize = m_iMSS - CPacket::UDP_HDR_SIZE; - m_iMaxSRTPayloadSize = udpsize - CPacket::HDR_SIZE; + m_iMaxSRTPayloadSize = calcMaxPayloadSize(); m_iPeerISN = m_ConnRes.m_iISN; setInitialRcvSeq(m_iPeerISN); @@ -5676,9 +5655,9 @@ void CUDT::acceptAndRespond(const sockaddr_any& peer, const CPacket& hspkt, CHan memcpy((m_piSelfIP), w_hs.m_piPeerIP, sizeof m_piSelfIP); CIPAddress::ntop(peer, (w_hs.m_piPeerIP)); - int udpsize = m_iMSS - CPacket::UDP_HDR_SIZE; - m_iMaxSRTPayloadSize = udpsize - CPacket::HDR_SIZE; - HLOGC(mglog.Debug, log << "acceptAndRespond: PAYLOAD SIZE: " << m_iMaxSRTPayloadSize); + m_iMaxSRTPayloadSize = calcMaxPayloadSize(); + HLOGC(mglog.Debug, log << "acceptAndRespond: PAYLOAD SIZE user: " << m_zUserPayloadSize + << " max:" << m_iMaxSRTPayloadSize); // Prepare all structures if (!prepareConnectionObjects(w_hs, HSD_DRAW, 0)) @@ -5868,7 +5847,7 @@ SRT_REJECT_REASON CUDT::setupCC() // At this point we state everything is checked and the appropriate // corrector type is already selected, so now create it. - HLOGC(mglog.Debug, log << "filter: Configuring Corrector: " << m_OPT_PktFilterConfigString); + HLOGC(mglog.Debug, log << "filter: Configuring: " << m_OPT_PktFilterConfigString); if (!m_PacketFilter.configure(this, m_pRcvBuffer->getUnitQueue(), m_OPT_PktFilterConfigString)) { return SRT_REJ_FILTER; @@ -10874,7 +10853,7 @@ CUDTGroup::gli_t CUDTGroup::add(SocketData data) gli_t end = m_Group.end(); if (m_iMaxPayloadSize == -1) { - int plsize = data.ps->m_pUDT->OPT_PayloadSize(); + int plsize = data.ps->m_pUDT->m_zUserPayloadSize; HLOGC(mglog.Debug, log << "CUDTGroup::add: taking MAX payload size from socket @" << data.ps->m_SocketID << ": " << plsize << " " << (plsize ? "(explicit)" : "(unspecified = fallback to 1456)")); if (plsize == 0) @@ -11106,7 +11085,6 @@ void CUDTGroup::deriveSettings(CUDT* u) IM(SRTO_RCVLATENCY, m_iOPT_TsbPdDelay); IM(SRTO_PEERLATENCY, m_iOPT_PeerTsbPdDelay); IM(SRTO_SNDDROPDELAY, m_iOPT_SndDropDelay); - IM(SRTO_PAYLOADSIZE, m_zOPT_ExpPayloadSize); IM(SRTO_TLPKTDROP, m_bTLPktDrop); IM(SRTO_MESSAGEAPI, m_bMessageAPI); IM(SRTO_NAKREPORT, m_bRcvNakReport); diff --git a/srtcore/core.h b/srtcore/core.h index 013ba8c38..74a11aa7b 100644 --- a/srtcore/core.h +++ b/srtcore/core.h @@ -696,7 +696,8 @@ class CUDT uint32_t latency_us() const {return m_iTsbPdDelay_ms*1000; } size_t maxPayloadSize() const { return m_iMaxSRTPayloadSize; } - size_t OPT_PayloadSize() const { return m_zOPT_ExpPayloadSize; } + size_t calcMaxPayloadSize() const; + size_t userPayloadSize() const { return m_zUserPayloadSize; } int sndLossLength() { return m_pSndLossList->getLossLength(); } int32_t ISN() const { return m_iISN; } int32_t peerISN() const { return m_iPeerISN; } @@ -1002,9 +1003,8 @@ class CUDT CUDTSocket* const m_parent; // temporary, until the CUDTSocket class is merged with CUDT SRTSOCKET m_SocketID; // UDT socket number SRTSOCKET m_PeerID; // peer id, for multiplexer - - int m_iMaxSRTPayloadSize; // Maximum/regular payload size, in bytes - size_t m_zOPT_ExpPayloadSize; // Expected average payload size (user option) + int m_iMaxSRTPayloadSize; // Maximum/regular payload size, in bytes (dependent on MSS) + size_t m_zUserPayloadSize; // Expected average payload size (user option) // Options int m_iMSS; // Maximum Segment Size, in bytes diff --git a/srtcore/packetfilter.cpp b/srtcore/packetfilter.cpp index 207dba39f..1d0489ff8 100644 --- a/srtcore/packetfilter.cpp +++ b/srtcore/packetfilter.cpp @@ -254,8 +254,7 @@ bool PacketFilter::configure(CUDT* parent, CUnitQueue* uq, const std::string& co init.socket_id = parent->socketID(); init.snd_isn = parent->sndSeqNo(); init.rcv_isn = parent->rcvSeqNo(); - init.payload_size = parent->OPT_PayloadSize(); - + init.payload_size = parent->userPayloadSize(); // Found a filter, so call the creation function m_filter = selector->second->Create(init, m_provided, confstr); From b4e906b1b2b788824dd7a55483e39768f3d1cead Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Ma=C5=82ecki?= Date: Thu, 13 Feb 2020 13:03:38 +0100 Subject: [PATCH 3/3] Fixed unit test --- srtcore/core.cpp | 2 +- test/test_epoll.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/srtcore/core.cpp b/srtcore/core.cpp index f934213ee..5babd2661 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -260,7 +260,7 @@ CUDT::CUDT(CUDTSocket* parent): m_parent(parent) m_bOPT_GroupConnect = false; m_bTLPktDrop = true; // Too-late Packet Drop m_bMessageAPI = true; - m_zUserPayloadSize = SRT_LIVE_MAX_PLSIZE; + m_zUserPayloadSize = calcMaxPayloadSize(); m_iIpV6Only = -1; // Runtime m_bRcvNakReport = true; // Receiver's Periodic NAK Reports diff --git a/test/test_epoll.cpp b/test/test_epoll.cpp index 249e17404..d98472a7d 100644 --- a/test/test_epoll.cpp +++ b/test/test_epoll.cpp @@ -717,7 +717,7 @@ class TestEPoll: public testing::Test ASSERT_EQ(read[0], acpsock); // read event is for bind socket } - char buffer[1316]; + char buffer[SRT_LIVE_MAX_PLSIZE]; ASSERT_EQ(srt_recvmsg(acpsock, buffer, sizeof buffer), 1316); char pattern[4] = {1, 2, 3, 4};