Fix regression (crash) in transport-cc feedback generation (#1339)

Fixes #1336

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,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<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

@@ -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<uint16_t>::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,15 +288,17 @@ namespace RTC
 		}
 	}
 
-	inline void TransportCongestionControlServer::SendTransportCcFeedback()
+	bool TransportCongestionControlServer::SendTransportCcFeedback()
 	{
 		MS_TRACE();
 
 		this->transportCcFeedbackPacket->Finish();
 
 		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<double>(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<uint32_t>& 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();