Fix regression (crash) in transport-cc feedback generation

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)).

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.

worker/include/RTC/TransportCongestionControlServer.hpp

@@ -58,7 +58,8 @@ 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);
@@ -95,6 +96,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<uint16_t, uint64_t, RTC::SeqManager<uint16_t>::SeqLowerThan> mapPacketArrivalTimes;
 	};
 } // namespace RTC

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<int32_t>((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<uint16_t>::IsSeqHigherThan(sequenceNumber, this->latestSequenceNumber))
@@ -340,7 +342,8 @@ namespace RTC
 			// Delta in 16 bits signed.
 			auto delta = static_cast<int16_t>(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();

worker/src/RTC/TransportCongestionControlServer.cpp

@@ -35,9 +35,6 @@ namespace RTC
 				// Create a feedback packet.
 				this->transportCcFeedbackPacket.reset(new RTC::RTCP::FeedbackRtpTransportPacket(0u, 0u));
 
-				// Set initial packet count.
-				this->transportCcFeedbackPacket->SetFeedbackPacketCount(this->transportCcFeedbackPacketCount);
-
 				// Create the feedback send periodic timer.
 				this->transportCcFeedbackSendPeriodicTimer = new TimerHandle(this);
 
@@ -96,6 +93,9 @@ namespace RTC
 				// Create a new feedback packet.
 				this->transportCcFeedbackPacket.reset(new RTC::RTCP::FeedbackRtpTransportPacket(0u, 0u));
 
+				// Set current packet count.
+				this->transportCcFeedbackPacket->SetFeedbackPacketCount(this->transportCcFeedbackPacketCount);
+
 				break;
 			}
 
@@ -131,9 +131,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<uint16_t>::IsSeqLowerThan(
@@ -192,13 +193,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 +220,19 @@ namespace RTC
 					{
 						MS_DEBUG_DEV("transport-cc feedback packet is full, sending feedback now");
 
-						SendTransportCcFeedback();
+						if (!SendTransportCcFeedback())
+						{
+							// If no feedback packet was sent then the feedback packet was
+							// not reset and hence it remains full so we cannot add more
+							// packets on it, so we have to reset it.
+							this->transportCcFeedbackPacket.reset(new RTC::RTCP::FeedbackRtpTransportPacket(
+							  this->transportCcFeedbackSenderSsrc, this->transportCcFeedbackMediaSsrc));
+
+							// Set current packet count.
+							// NOTE: Do not increment it since no feedback packet was sent.
+							this->transportCcFeedbackPacket->SetFeedbackPacketCount(
+							  this->transportCcFeedbackPacketCount);
+						}
 					}
 
 					break;
@@ -218,11 +241,19 @@ 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));
+
+					// Set current packet count.
+					// NOTE: Do not increment it since the previous ongoing feedback
+					// packet was not sent.
+					this->transportCcFeedbackPacket->SetFeedbackPacketCount(
+					  this->transportCcFeedbackPacketCount);
+
+					break;
 				}
 
 				case RTC::RTCP::FeedbackRtpTransportPacket::AddPacketResult::FATAL:
@@ -231,7 +262,7 @@ namespace RTC
 					this->transportCcFeedbackPacket.reset(new RTC::RTCP::FeedbackRtpTransportPacket(
 					  this->transportCcFeedbackSenderSsrc, this->transportCcFeedbackMediaSsrc));
 
-					// Use current packet count.
+					// Set current packet count.
 					// NOTE: Do not increment it since the previous ongoing feedback
 					// packet was not sent.
 					this->transportCcFeedbackPacket->SetFeedbackPacketCount(
@@ -242,6 +273,8 @@ namespace RTC
 			}
 		}
 
+		// It may happen that the packet is empty (no deltas) but in that case
+		// SendTransportCcFeedback() won't send it so we are safe.
 		SendTransportCcFeedback();
 	}
 
@@ -264,15 +297,17 @@ namespace RTC
 		}
 	}
 
-	inline void TransportCongestionControlServer::SendTransportCcFeedback()
+	bool TransportCongestionControlServer::SendTransportCcFeedback()
 	{
 		MS_TRACE();
 
 		this->transportCcFeedbackPacket->Finish();
 
 		if (!this->transportCcFeedbackPacket->IsSerializable())
 		{
-			return;
+			MS_WARN_DEV("couldn't send feedback packet (not serializable)");
+
+			return false;
 		}
 
 		auto latestWideSeqNumber = this->transportCcFeedbackPacket->GetLatestSequenceNumber();
@@ -305,10 +340,11 @@ namespace RTC
 		// 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 +366,7 @@ namespace RTC
 		}
 	}
 
-	inline void TransportCongestionControlServer::MaySendLimitationRembFeedback(uint64_t nowMs)
+	void TransportCongestionControlServer::MaySendLimitationRembFeedback(uint64_t nowMs)
 	{
 		MS_TRACE();
 
@@ -402,7 +438,7 @@ namespace RTC
 		this->packetLoss = totalPacketLoss / samples;
 	}
 
-	inline void TransportCongestionControlServer::OnRembServerAvailableBitrate(
+	void TransportCongestionControlServer::OnRembServerAvailableBitrate(
 	  const webrtc::RemoteBitrateEstimator* /*rembServer*/,
 	  const std::vector<uint32_t>& ssrcs,
 	  uint32_t availableBitrate)
@@ -440,7 +476,7 @@ namespace RTC
 		this->listener->OnTransportCongestionControlServerSendRtcpPacket(this, &packet);
 	}
 
-	inline void TransportCongestionControlServer::OnTimer(TimerHandle* timer)
+	void TransportCongestionControlServer::OnTimer(TimerHandle* timer)
 	{
 		MS_TRACE();