diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e81b85c00..45b24b01b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +### Next + +- Fix regression (crash) in transport-cc feedback generation ([PR #1339](https://github.com/versatica/mediasoup/pull/1339)). + ### 3.13.19 - Node: Fix `router.createWebRtcTransport()` with `listenIps` ([PR #1330](https://github.com/versatica/mediasoup/pull/1330)). diff --git a/worker/include/RTC/RTCP/FeedbackRtpTransport.hpp b/worker/include/RTC/RTCP/FeedbackRtpTransport.hpp index 901df0c46c..fff4d3fbe0 100644 --- a/worker/include/RTC/RTCP/FeedbackRtpTransport.hpp +++ b/worker/include/RTC/RTCP/FeedbackRtpTransport.hpp @@ -212,6 +212,10 @@ namespace RTC ~FeedbackRtpTransportPacket() override; public: + bool IsBaseSet() const + { + return this->baseSet; + } void SetBase(uint16_t sequenceNumber, uint64_t timestamp); AddPacketResult AddPacket(uint16_t sequenceNumber, uint64_t timestamp, size_t maxRtcpPacketLen); // Just for locally generated packets. diff --git a/worker/include/RTC/TransportCongestionControlServer.hpp b/worker/include/RTC/TransportCongestionControlServer.hpp index 5cfd676796..af5e6d7482 100644 --- a/worker/include/RTC/TransportCongestionControlServer.hpp +++ b/worker/include/RTC/TransportCongestionControlServer.hpp @@ -58,10 +58,12 @@ namespace RTC void FillAndSendTransportCcFeedback(); private: - void SendTransportCcFeedback(); + // Returns true if a feedback packet was sent. + bool SendTransportCcFeedback(); void MayDropOldPacketArrivalTimes(uint16_t seqNum, uint64_t nowMs); void MaySendLimitationRembFeedback(uint64_t nowMs); void UpdatePacketLoss(double packetLoss); + void ResetTransportCcFeedback(uint8_t feedbackPacketCount); /* Pure virtual methods inherited from webrtc::RemoteBitrateEstimator::Listener. */ public: @@ -95,6 +97,7 @@ namespace RTC // Whether any packet with transport wide sequence number was received. bool transportWideSeqNumberReceived{ false }; uint16_t transportCcFeedbackWideSeqNumStart{ 0u }; + // Map of arrival timestamp (ms) indexed by wide seq number. std::map::SeqLowerThan> mapPacketArrivalTimes; }; } // namespace RTC diff --git a/worker/src/RTC/RTCP/FeedbackRtpTransport.cpp b/worker/src/RTC/RTCP/FeedbackRtpTransport.cpp index efd9d591bc..9d497677b4 100644 --- a/worker/src/RTC/RTCP/FeedbackRtpTransport.cpp +++ b/worker/src/RTC/RTCP/FeedbackRtpTransport.cpp @@ -282,6 +282,8 @@ namespace RTC { MS_TRACE(); + MS_ASSERT(!this->baseSet, "base already set"); + this->baseSet = true; this->baseSequenceNumber = sequenceNumber; this->referenceTime = static_cast((timestamp & 0x1FFFFFC0) / 64); @@ -297,8 +299,8 @@ namespace RTC MS_ASSERT(this->baseSet, "base not set"); MS_ASSERT(!IsFull(), "packet is full"); - // If the wide sequence number of the new packet is lower than the latest seen, - // ignore it. + // If the wide sequence number of the new packet is lower than the latest + // seen, ignore it. // NOTE: Not very spec compliant but libwebrtc does it. // Also ignore if the sequence number matches the latest seen. if (!RTC::SeqManager::IsSeqHigherThan(sequenceNumber, this->latestSequenceNumber)) @@ -340,7 +342,8 @@ namespace RTC // Delta in 16 bits signed. auto delta = static_cast(delta64); - // Check whether another chunks and corresponding delta infos could be added. + // Check whether another chunks and corresponding delta infos could be + // added. { // Fixed packet size. size_t size = FeedbackRtpPacket::GetSize(); diff --git a/worker/src/RTC/TransportCongestionControlServer.cpp b/worker/src/RTC/TransportCongestionControlServer.cpp index 6fe639e175..001bf6b9e0 100644 --- a/worker/src/RTC/TransportCongestionControlServer.cpp +++ b/worker/src/RTC/TransportCongestionControlServer.cpp @@ -33,10 +33,7 @@ namespace RTC case RTC::BweType::TRANSPORT_CC: { // Create a feedback packet. - this->transportCcFeedbackPacket.reset(new RTC::RTCP::FeedbackRtpTransportPacket(0u, 0u)); - - // Set initial packet count. - this->transportCcFeedbackPacket->SetFeedbackPacketCount(this->transportCcFeedbackPacketCount); + ResetTransportCcFeedback(0u); // Create the feedback send periodic timer. this->transportCcFeedbackSendPeriodicTimer = new TimerHandle(this); @@ -94,7 +91,7 @@ namespace RTC this->transportCcFeedbackSendPeriodicTimer->Stop(); // Create a new feedback packet. - this->transportCcFeedbackPacket.reset(new RTC::RTCP::FeedbackRtpTransportPacket(0u, 0u)); + ResetTransportCcFeedback(this->transportCcFeedbackPacketCount); break; } @@ -131,9 +128,10 @@ namespace RTC break; } - // We may receive packets with sequence number lower than the one in previous - // tcc feedback, these packets may have been reported as lost previously, - // therefore we need to reset the start sequence num for the next tcc feedback. + // We may receive packets with sequence number lower than the one in + // previous tcc feedback, these packets may have been reported as lost + // previously, therefore we need to reset the start sequence num for the + // next tcc feedback. if ( !this->transportWideSeqNumberReceived || RTC::SeqManager::IsSeqLowerThan( @@ -192,13 +190,23 @@ namespace RTC return; } - // Set base sequence num and reference time. - this->transportCcFeedbackPacket->SetBase(this->transportCcFeedbackWideSeqNumStart, it->second); - for (; it != this->mapPacketArrivalTimes.end(); ++it) { - auto result = - this->transportCcFeedbackPacket->AddPacket(it->first, it->second, this->maxRtcpPacketLen); + auto sequenceNumber = it->first; + auto timestamp = it->second; + + // If the base is not set in this packet let's set it. + // NOTE: This maybe needed many times during this loop since the current + // feedback packet maybe a fresh new one if the previous one was full (so + // already sent) or failed to be built. + if (!this->transportCcFeedbackPacket->IsBaseSet()) + { + // Set base sequence num and reference time. + this->transportCcFeedbackPacket->SetBase(this->transportCcFeedbackWideSeqNumStart, timestamp); + } + + auto result = this->transportCcFeedbackPacket->AddPacket( + sequenceNumber, timestamp, this->maxRtcpPacketLen); switch (result) { @@ -209,7 +217,15 @@ namespace RTC { MS_DEBUG_DEV("transport-cc feedback packet is full, sending feedback now"); - SendTransportCcFeedback(); + auto sent = SendTransportCcFeedback(); + + if (sent) + { + ++this->transportCcFeedbackPacketCount; + } + + // Create a new feedback packet. + ResetTransportCcFeedback(this->transportCcFeedbackPacketCount); } break; @@ -218,31 +234,39 @@ namespace RTC case RTC::RTCP::FeedbackRtpTransportPacket::AddPacketResult::MAX_SIZE_EXCEEDED: { // This should not happen. - MS_WARN_DEV("transport-cc feedback packet is exceeded"); + MS_WARN_TAG(rtcp, "transport-cc feedback packet is exceeded"); // Create a new feedback packet. - this->transportCcFeedbackPacket.reset(new RTC::RTCP::FeedbackRtpTransportPacket( - this->transportCcFeedbackSenderSsrc, this->transportCcFeedbackMediaSsrc)); + // NOTE: Do not increment packet count it since the previous ongoing + // feedback packet was not sent. + ResetTransportCcFeedback(this->transportCcFeedbackPacketCount); + + break; } case RTC::RTCP::FeedbackRtpTransportPacket::AddPacketResult::FATAL: { // Create a new feedback packet. - this->transportCcFeedbackPacket.reset(new RTC::RTCP::FeedbackRtpTransportPacket( - this->transportCcFeedbackSenderSsrc, this->transportCcFeedbackMediaSsrc)); - - // Use current packet count. - // NOTE: Do not increment it since the previous ongoing feedback - // packet was not sent. - this->transportCcFeedbackPacket->SetFeedbackPacketCount( - this->transportCcFeedbackPacketCount); + // NOTE: Do not increment packet count it since the previous ongoing + // feedback packet was not sent. + ResetTransportCcFeedback(this->transportCcFeedbackPacketCount); break; } } } - SendTransportCcFeedback(); + // It may happen that the packet is empty (no deltas) but in that case + // SendTransportCcFeedback() won't send it so we are safe. + auto sent = SendTransportCcFeedback(); + + if (sent) + { + ++this->transportCcFeedbackPacketCount; + } + + // Create a new feedback packet. + ResetTransportCcFeedback(this->transportCcFeedbackPacketCount); } void TransportCongestionControlServer::SetMaxIncomingBitrate(uint32_t bitrate) @@ -264,7 +288,7 @@ namespace RTC } } - inline void TransportCongestionControlServer::SendTransportCcFeedback() + bool TransportCongestionControlServer::SendTransportCcFeedback() { MS_TRACE(); @@ -272,7 +296,9 @@ namespace RTC if (!this->transportCcFeedbackPacket->IsSerializable()) { - return; + MS_WARN_TAG(rtcp, "couldn't send feedback-cc packet because it is not serializable"); + + return false; } auto latestWideSeqNumber = this->transportCcFeedbackPacket->GetLatestSequenceNumber(); @@ -298,17 +324,12 @@ namespace RTC this->UpdatePacketLoss(static_cast(lostPackets) / expectedPackets); } - // Create a new feedback packet. - this->transportCcFeedbackPacket.reset(new RTC::RTCP::FeedbackRtpTransportPacket( - this->transportCcFeedbackSenderSsrc, this->transportCcFeedbackMediaSsrc)); - - // Increment packet count. - this->transportCcFeedbackPacket->SetFeedbackPacketCount(++this->transportCcFeedbackPacketCount); this->transportCcFeedbackWideSeqNumStart = latestWideSeqNumber + 1; + + return true; } - inline void TransportCongestionControlServer::MayDropOldPacketArrivalTimes( - uint16_t seqNum, uint64_t nowMs) + void TransportCongestionControlServer::MayDropOldPacketArrivalTimes(uint16_t seqNum, uint64_t nowMs) { MS_TRACE(); @@ -330,7 +351,7 @@ namespace RTC } } - inline void TransportCongestionControlServer::MaySendLimitationRembFeedback(uint64_t nowMs) + void TransportCongestionControlServer::MaySendLimitationRembFeedback(uint64_t nowMs) { MS_TRACE(); @@ -402,7 +423,17 @@ namespace RTC this->packetLoss = totalPacketLoss / samples; } - inline void TransportCongestionControlServer::OnRembServerAvailableBitrate( + void TransportCongestionControlServer::ResetTransportCcFeedback(uint8_t feedbackPacketCount) + { + MS_TRACE(); + + this->transportCcFeedbackPacket.reset(new RTC::RTCP::FeedbackRtpTransportPacket( + this->transportCcFeedbackSenderSsrc, this->transportCcFeedbackMediaSsrc)); + + this->transportCcFeedbackPacket->SetFeedbackPacketCount(feedbackPacketCount); + } + + void TransportCongestionControlServer::OnRembServerAvailableBitrate( const webrtc::RemoteBitrateEstimator* /*rembServer*/, const std::vector& ssrcs, uint32_t availableBitrate) @@ -440,7 +471,7 @@ namespace RTC this->listener->OnTransportCongestionControlServerSendRtcpPacket(this, &packet); } - inline void TransportCongestionControlServer::OnTimer(TimerHandle* timer) + void TransportCongestionControlServer::OnTimer(TimerHandle* timer) { MS_TRACE();