From 75c11eee93f007f1149af0057d91082d93eadb4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Thu, 18 May 2023 12:34:19 +0200 Subject: [PATCH] worker: Fix NACK timer and avoid negative RTT (#1082) - Stop NACK timer when NACK list is empty as explained in https://github.com/versatica/mediasoup/pull/1076#issuecomment-1537148706. - 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 | 5 ++++ worker/include/RTC/RtpStream.hpp | 3 +-- worker/src/RTC/NackGenerator.cpp | 40 +++++++++++++++++++++++++++++--- worker/src/RTC/RtpStream.cpp | 2 +- worker/src/RTC/RtpStreamRecv.cpp | 5 ++-- worker/src/RTC/RtpStreamSend.cpp | 7 +++--- 6 files changed, 51 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e55034911f..3239d06a55 100644 --- a/CHANGELOG.md +++ b/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)). diff --git a/worker/include/RTC/RtpStream.hpp b/worker/include/RTC/RtpStream.hpp index 9a0ca8daa6..e404013a56 100644 --- a/worker/include/RTC/RtpStream.hpp +++ b/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 }; diff --git a/worker/src/RTC/NackGenerator.cpp b/worker/src/RTC/NackGenerator.cpp index 48fe2e8512..289d6dd3df 100644 --- a/worker/src/RTC/NackGenerator.cpp +++ b/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 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 nackBatch = GetNackBatch(NackFilter::TIME); if (!nackBatch.empty()) + { this->listener->OnNackGeneratorNackRequired(nackBatch); + } MayRunTimer(); } diff --git a/worker/src/RTC/RtpStream.cpp b/worker/src/RTC/RtpStream.cpp index 85b6850b81..8503025f31 100644 --- a/worker/src/RTC/RtpStream.cpp +++ b/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; } diff --git a/worker/src/RTC/RtpStreamRecv.cpp b/worker/src/RTC/RtpStreamRecv.cpp index abee7f293b..0fe1de02cb 100644 --- a/worker/src/RTC/RtpStreamRecv.cpp +++ b/worker/src/RTC/RtpStreamRecv.cpp @@ -608,9 +608,10 @@ namespace RTC this->rtt = static_cast(rtt >> 16) * 1000; this->rtt += (static_cast(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. diff --git a/worker/src/RTC/RtpStreamSend.cpp b/worker/src/RTC/RtpStreamSend.cpp index b8949a37ff..ebc8780d58 100644 --- a/worker/src/RTC/RtpStreamSend.cpp +++ b/worker/src/RTC/RtpStreamSend.cpp @@ -223,9 +223,10 @@ namespace RTC this->rtt = static_cast(rtt >> 16) * 1000; this->rtt += (static_cast(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 };