worker: Fix NACK timer and avoid negative RTT (#1082)

- Stop NACK timer when NACK list is empty as explained in #1076 (comment).
- Avoid RTT becoming negative in `RtpStreamSend` and `RtpStreamRecv`. If so, assign it with 0.0f.
- And if 0.0f, use `DefaultRtt` (100 ms) in `NackGenerator`.
- Remove useless `RtpStream::hasRtt` and check `RtpStream::rtt > 0.0f` instead.
- Add some brackets in condition blocks.

CHANGELOG.md

@@ -1,6 +1,11 @@
 # Changelog
 
 
+### NEXT
+
+* `Worker`: Fix NACK timer and avoid negative RTT ([PR #1082](https://github.com/versatica/mediasoup/pull/1082), thanks to o-u-p for his work in ([PR #1076](https://github.com/versatica/mediasoup/pull/1076)).
+
+
 ### 3.11.25
 
 * `Worker`: Require C++17, Meson >= 1.1.0 and update subprojects ([PR #1081](https://github.com/versatica/mediasoup/pull/1081)).

worker/include/RTC/RtpStream.hpp

@@ -214,8 +214,7 @@ namespace RTC
 		uint64_t lastSenderReportNtpMs{ 0u };
 		// RTP timestamp in last Sender Report.
 		uint32_t lastSenderReportTs{ 0u };
-		float rtt{ 0 };
-		bool hasRtt{ false };
+		float rtt{ 0.0f };
 		// Instance of RtxStream.
 		RTC::RtxStream* rtxStream{ nullptr };
 

worker/src/RTC/NackGenerator.cpp

@@ -51,14 +51,18 @@ namespace RTC
 			this->lastSeq = seq;
 
 			if (isKeyFrame)
+			{
 				this->keyFrameList.insert(seq);
+			}
 
 			return false;
 		}
 
 		// Obviously never nacked, so ignore.
 		if (seq == this->lastSeq)
+		{
 			return false;
+		}
 
 		// May be an out of order packet, or already handled retransmitted packet,
 		// or a retransmitted packet.
@@ -80,9 +84,13 @@ namespace RTC
 				this->nackList.erase(it);
 
 				if (retries != 0)
+				{
 					return true;
+				}
 				else
+				{
 					return false;
+				}
 			}
 
 			// Out of order packet or already handled NACKed packet.
@@ -101,14 +109,18 @@ namespace RTC
 		// newer than the latest seq seen.
 
 		if (isKeyFrame)
+		{
 			this->keyFrameList.insert(seq);
+		}
 
 		// Remove old keyframes.
 		{
 			auto it = this->keyFrameList.lower_bound(seq - MaxPacketAge);
 
 			if (it != this->keyFrameList.begin())
+			{
 				this->keyFrameList.erase(this->keyFrameList.begin(), it);
+			}
 		}
 
 		if (isRecovered)
@@ -119,7 +131,9 @@ namespace RTC
 			auto it = this->recoveredList.lower_bound(seq - MaxPacketAge);
 
 			if (it != this->recoveredList.begin())
+			{
 				this->recoveredList.erase(this->recoveredList.begin(), it);
+			}
 
 			// Do not let a packet pass if it's newer than last seen seq and came via
 			// RTX.
@@ -134,12 +148,16 @@ namespace RTC
 		std::vector<uint16_t> nackBatch = GetNackBatch(NackFilter::SEQ);
 
 		if (!nackBatch.empty())
+		{
 			this->listener->OnNackGeneratorNackRequired(nackBatch);
+		}
 
 		// This is important. Otherwise the running timer (filter:TIME) would be
 		// interrupted and NACKs would never been sent more than once for each seq.
 		if (!this->timer->IsActive())
+		{
 			MayRunTimer();
+		}
 
 		return false;
 	}
@@ -187,7 +205,9 @@ namespace RTC
 
 			// Do not send NACK for packets that are already recovered by RTX.
 			if (this->recoveredList.find(seq) != this->recoveredList.end())
+			{
 				continue;
+			}
 
 			this->nackList.emplace(std::make_pair(
 			  seq,
@@ -277,7 +297,10 @@ namespace RTC
 				continue;
 			}
 
-			if (filter == NackFilter::TIME && (nackInfo.sentAtMs == 0 || nowMs - nackInfo.sentAtMs >= this->rtt))
+			if (
+			  filter == NackFilter::TIME &&
+			  (nackInfo.sentAtMs == 0 ||
+			   nowMs - nackInfo.sentAtMs >= (this->rtt > 0u ? this->rtt : DefaultRtt)))
 			{
 				nackBatch.emplace_back(seq);
 				nackInfo.retries++;
@@ -313,9 +336,13 @@ namespace RTC
 			seqsStream << nackBatch.back();
 
 			if (filter == NackFilter::SEQ)
+			{
 				MS_DEBUG_DEV("[filter:SEQ, asking seqs:%s]", seqsStream.str().c_str());
+			}
 			else
+			{
 				MS_DEBUG_DEV("[filter:TIME, asking seqs:%s]", seqsStream.str().c_str());
+			}
 		}
 #endif
 
@@ -329,15 +356,20 @@ namespace RTC
 		this->nackList.clear();
 		this->keyFrameList.clear();
 		this->recoveredList.clear();
-
 		this->started = false;
 		this->lastSeq = 0u;
 	}
 
 	inline void NackGenerator::MayRunTimer() const
 	{
-		if (!this->nackList.empty())
+		if (this->nackList.empty())
+		{
+			this->timer->Stop();
+		}
+		else
+		{
 			this->timer->Start(TimerInterval);
+		}
 	}
 
 	inline void NackGenerator::OnTimer(Timer* /*timer*/)
@@ -347,7 +379,9 @@ namespace RTC
 		std::vector<uint16_t> nackBatch = GetNackBatch(NackFilter::TIME);
 
 		if (!nackBatch.empty())
+		{
 			this->listener->OnNackGeneratorNackRequired(nackBatch);
+		}
 
 		MayRunTimer();
 	}

worker/src/RTC/RtpStream.cpp

@@ -76,7 +76,7 @@ namespace RTC
 		if (this->rtxStream)
 			jsonObject["rtxPacketsDiscarded"] = this->rtxStream->GetPacketsDiscarded();
 
-		if (this->hasRtt)
+		if (this->rtt > 0.0f)
 			jsonObject["roundTripTime"] = this->rtt;
 	}
 

worker/src/RTC/RtpStreamRecv.cpp

@@ -608,9 +608,10 @@ namespace RTC
 		this->rtt = static_cast<float>(rtt >> 16) * 1000;
 		this->rtt += (static_cast<float>(rtt & 0x0000FFFF) / 65536) * 1000;
 
-		if (this->rtt > 0.0f)
+		// Avoid negative RTT value since it doesn't make sense.
+		if (this->rtt <= 0.0f)
 		{
-			this->hasRtt = true;
+			this->rtt = 0.0f;
 		}
 
 		// Tell it to the NackGenerator.

worker/src/RTC/RtpStreamSend.cpp

@@ -223,9 +223,10 @@ namespace RTC
 		this->rtt = static_cast<float>(rtt >> 16) * 1000;
 		this->rtt += (static_cast<float>(rtt & 0x0000FFFF) / 65536) * 1000;
 
-		if (this->rtt > 0.0f)
+		// Avoid negative RTT value since it doesn't make sense.
+		if (this->rtt <= 0.0f)
 		{
-			this->hasRtt = true;
+			this->rtt = 0.0f;
 		}
 
 		this->packetsLost  = report->GetTotalLost();
@@ -399,7 +400,7 @@ namespace RTC
 
 		// Look for each requested packet.
 		const uint64_t nowMs = DepLibUV::GetTimeMs();
-		const uint16_t rtt   = (this->rtt != 0u ? this->rtt : DefaultRtt);
+		const uint16_t rtt   = (this->rtt > 0.0f ? this->rtt : DefaultRtt);
 		uint16_t currentSeq  = seq;
 		bool requested{ true };
 		size_t containerIdx{ 0 };