[core] Improved SND dropping mechanism #2791
Conversation
There was a problem hiding this comment.
PR #2230 introduced (SRT v1.5.0) the CSndBuffer::getBufferingDelay(const time_point& tnow) function. The function returns the delay (oldness) of the oldest (the very first) packet in the SRT buffer towards the current time.
The CSndBuffer::getFirstPacketTS() function introduced in this PR does the same, just returns the time instead of the delay.
The logical changes in this PR are:
- threshold accuracy changed from ms to microseconds.
- the threshold is increased from
m_iPeerTsbPdDelay_ms + 20mstoiPeerTsbPdDelay_us + (duration(m_iSRTT/2)) m_config.iSndDropDelay(SRTO_SNDDROPDELAY) is now ignored (❗)
The effective change is only
index 0e3cce0..6d24c08 100644
--- a/srtcore/core.cpp
+++ b/srtcore/core.cpp
@@ -6375,7 +6378,7 @@ int srt::CUDT::sndDropTooLate()
// XXX Make SRT_TLPKTDROP_MINTHRESHOLD_MS option-configurable
const int threshold_ms = (m_config.iSndDropDelay >= 0)
? std::max(m_iPeerTsbPdDelay_ms + m_config.iSndDropDelay, +SRT_TLPKTDROP_MINTHRESHOLD_MS)
- + (2 * COMM_SYN_INTERVAL_US / 1000)
+ + (m_iSRTT / 2 / 1000)
: 0;
if (threshold_ms == 0 || buffdelay_ms <= threshold_ms)Probably SRT_TLPKTDROP_MINTHRESHOLD_MS can be removed then (it seems to be a workaround for smaller latencies) if testing confirms no side effects at small SRT latencies and RTT values < 1 ms.
srtcore/core.cpp
Outdated
| have no chance to make it through the network | ||
| RTT/2 is used instead of OWD, further developments are needed to handle networks that are not symmetrical*/ | ||
|
|
||
| duration iPeerTsbPdDelay_us(m_iPeerTsbPdDelay_ms*1000); |
There was a problem hiding this comment.
How do you know that duration defaults to microseconds?
srtcore/core.cpp
Outdated
|
|
||
| time_point time_to_drop(first_pkt_ts); | ||
| time_point threshold_us = first_pkt_ts + iPeerTsbPdDelay_us - (duration(m_iSRTT/2)); |
There was a problem hiding this comment.
This removes the possibility to override this value by the setting of the SRTO_SNDDROPDELAY option available here as m_config.iSndDropDelay field. Maybe the rules for setting this option and whether and how it overrides this threshold value can be changed, but it should still be used.
srtcore/buffer_snd.cpp
Outdated
| ScopedLock lck(m_BufLock); | ||
| SRT_ASSERT(m_pFirstBlock); | ||
| if (m_iCount == 0) | ||
| return time_point(0); |
There was a problem hiding this comment.
time_point(0) -> time_point().
Then you'll need to check if (first_pkt_ts != time_point()).
But might be better to keep using the CSndBuffer::getBufferingDelay(..) function.
maxsharabayko
added
Type: Bug
As stated in #1910, dropping mechanism was using buffer timespan to decide when to drop packets. This new mechanism is based on SRT latency and RTT, and should only drop packets that have no chance to make it through the network