Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SimulcastConsumer cannot switch layers if initial tsReferenceSpatialLayer disappears #492

Closed
LewisWolfgang opened this issue Dec 21, 2020 · 6 comments · Fixed by #1459 · May be fixed by #500
Closed

SimulcastConsumer cannot switch layers if initial tsReferenceSpatialLayer disappears #492

LewisWolfgang opened this issue Dec 21, 2020 · 6 comments · Fixed by #1459 · May be fixed by #500
Assignees
Labels
Milestone

Comments

@LewisWolfgang
Copy link

Bug Report

SimulcastConsumer cannot switch layers if initial tsReferenceSpatialLayer disappears

Your environment

  • Operating system: Linux (multiple)
  • Node version: N/A
  • npm version: N/A
  • gcc/clang version: N/A
  • mediasoup version: 3.6.24, 3.6.29 (Probably since 3.6.17, but only tested those)
  • mediasoup-client version: 3.6.11

Issue description

The problem happens if the higher spatial layer is never resumed. For example:

  • Start with 3 spatial layers
  • SimulcastConsumer will select layer 2 for tsReferenceSpatialLayer and currentSpatialLayer. (I believe this is only since the change in 3.6.17 (SimulcastConsumer: Prefer the highest spatial layer initially (PR SimulcastConsumer: prefer the highest spatial layer initially #450).)
  • Client sets max spatial layer to 0 before any Sender Reports are sent. (And leaves it at this forever).
  • SimulcastConsumer will eventually notice layer 2 as inactive and set currentSpatialLayer to -1, but the logic in CanSwitchToSpatialLayer will not allow any other layer to be selected since GetProducerTsReferenceRtpStream()->GetSenderReportNtpMs() will remain false.
@ibc ibc assigned ibc and jmillan Dec 21, 2020
@ibc ibc added this to the v3 updates milestone Dec 21, 2020
@ibc
Copy link
Member

ibc commented Dec 21, 2020

This is not really related to PR #450. The same issue would happen if client starts sending stream 0, then immediately stops it without sending RTCP Sender Report, then in mediasoup we try to switch to stream 0. Since we don't have RTP Timestamp for stream 1 (the previous one in the SimulcastConsumer) we cannot switch to stream 0.

Will fix it in next days.

@LewisWolfgang
Copy link
Author

Sure. The fix we have tested looks like this:

diff --git a/worker/include/RTC/SimulcastConsumer.hpp b/worker/include/RTC/SimulcastConsumer.hpp
index fad2728c..3c55a66f 100644
--- a/worker/include/RTC/SimulcastConsumer.hpp
+++ b/worker/include/RTC/SimulcastConsumer.hpp
@@ -111,7 +111,8 @@ namespace RTC
 		int16_t currentSpatialLayer{ -1 };
 		int16_t tsReferenceSpatialLayer{ -1 }; // Used for RTP TS sync.
 		std::unique_ptr<RTC::Codecs::EncodingContext> encodingContext;
-		uint32_t tsOffset{ 0u }; // RTP Timestamp offset.
+		uint32_t tsOffset{ 0u };          // RTP Timestamp offset.
+		uint32_t tsReferenceOffset{ 0u }; // RTP Timestamp offset from the tsReferenceSpatialLayer.
 		bool keyFrameForTsOffsetRequested{ false };
 		uint64_t lastBweDowngradeAtMs{ 0u }; // Last time we moved to lower spatial layer due to BWE.
 	};
diff --git a/worker/src/RTC/SimulcastConsumer.cpp b/worker/src/RTC/SimulcastConsumer.cpp
index eefc7c63..60a0941b 100644
--- a/worker/src/RTC/SimulcastConsumer.cpp
+++ b/worker/src/RTC/SimulcastConsumer.cpp
@@ -700,9 +700,27 @@ namespace RTC
 			uint32_t tsOffset{ 0u };
 
 			// Sync our RTP stream's RTP timestamp.
-			if (spatialLayer == this->tsReferenceSpatialLayer)
+			if (-1 == this->tsReferenceSpatialLayer)
+			{
+				if (shouldSwitchCurrentSpatialLayer)
+				{
+					// If we're actually switching streams, push the RTP timestamp up (we don't have a way to
+					// offset based on abs time yet) otherwise, if it's the first packet, we can just keep the
+					// timestamps
+					if (this->rtpStream->GetMaxPacketTs())
+						tsOffset = packet->GetTimestamp() - this->rtpStream->GetMaxPacketTs() -
+						           33 * this->rtpStream->GetClockRate() / 1000;
+					else
+						tsOffset = 0u;
+				}
+				else
+				{
+					tsOffset = this->tsOffset; // status quo
+				}
+			}
+			else if (spatialLayer == this->tsReferenceSpatialLayer)
 			{
-				tsOffset = 0u;
+				tsOffset = this->tsReferenceOffset;
 			}
 			// If this is not the RTP stream we use as TS reference, do NTP based RTP TS synchronization.
 			else
@@ -730,7 +748,7 @@ namespace RTC
 
 				// Apply offset. This is the difference that later must be removed from the
 				// sending RTP packet.
-				tsOffset = newTs2 - ts1;
+				tsOffset = this->tsReferenceOffset + newTs2 - ts1;
 			}
 
 			// When switching to a new stream it may happen that the timestamp of this
@@ -807,6 +825,10 @@ namespace RTC
 
 			this->tsOffset = tsOffset;
 
+			// We need this update here and below, it picks up and carries forward any adjustments we made above
+			if (spatialLayer == this->tsReferenceSpatialLayer)
+				this->tsReferenceOffset = this->tsOffset;
+
 			// Sync our RTP stream's sequence number.
 			this->rtpSeqManager.Sync(packet->GetSequenceNumber() - 1);
 
@@ -821,6 +843,9 @@ namespace RTC
 			// Update current spatial layer.
 			this->currentSpatialLayer = this->targetSpatialLayer;
 
+			if (this->currentSpatialLayer == this->tsReferenceSpatialLayer)
+				this->tsReferenceOffset = this->tsOffset;
+
 			// Update target and current temporal layer.
 			this->encodingContext->SetTargetTemporalLayer(this->targetTemporalLayer);
 			this->encodingContext->SetCurrentTemporalLayer(packet->GetTemporalLayer());
@@ -853,6 +878,16 @@ namespace RTC
 				EmitLayersChange();
 		}
 
+		if (-1 == this->tsReferenceSpatialLayer)
+		{
+			auto* producerCurrentRtpStream = GetProducerCurrentRtpStream();
+			if (producerCurrentRtpStream && producerCurrentRtpStream->GetSenderReportNtpMs())
+			{
+				this->tsReferenceSpatialLayer = this->currentSpatialLayer;
+				this->tsReferenceOffset       = this->tsOffset;
+			}
+		}
+
 		// Update RTP seq number and timestamp based on NTP offset.
 		uint16_t seq;
 		uint32_t timestamp = packet->GetTimestamp() - this->tsOffset;
@@ -1323,14 +1358,18 @@ namespace RTC
 	{
 		MS_TRACE();
 
 		// If we don't have yet a RTP timestamp reference, set it now.
-		if (newTargetSpatialLayer != -1 && this->tsReferenceSpatialLayer == -1)
-		{
-			MS_DEBUG_TAG(
-			  simulcast, "using spatial layer %" PRIi16 " as RTP timestamp reference", newTargetSpatialLayer);
-
-			this->tsReferenceSpatialLayer = newTargetSpatialLayer;
-		}
+		// if (newTargetSpatialLayer != -1 && this->tsReferenceSpatialLayer == -1)
+		//{
+		//	MS_DEBUG_TAG(
+		//	  simulcast, "using spatial layer %" PRIi16 " as RTP timestamp reference",
+		// newTargetSpatialLayer);
+		//
+		//	this->tsReferenceSpatialLayer = newTargetSpatialLayer;
+		//}
 
 		if (newTargetSpatialLayer == -1)
 		{

This seems to work well in our tests.
Note: in addition to delaying the setting of tsReferenceSpatialLayer, we are also keeping track of the small adjustments to the timestamps added to keep the sequence monotonic, and carrying those forward (in the new tsReferenceOffset). This change is a bit subtle, and maybe debatable, but it seemed like a good idea to me.
Of course, there are other ways to fix the original issue...

@ibc
Copy link
Member

ibc commented Jan 4, 2021

Thanks, we need some time to review this (we have some other pending work yet).

@ibc
Copy link
Member

ibc commented Jan 5, 2021

Can you please create a PR?

@LewisWolfgang
Copy link
Author

Yes, happy to do so. It might be in a couple of days due to other commitments.

@ibc
Copy link
Member

ibc commented Jan 6, 2021

👍

LewisWolfgang added a commit to LewisWolfgang/mediasoup that referenced this issue Jan 6, 2021
…tialLayer disappears" (versatica#492)

* Set tsReferenceSpatialLayer later, only after receiving an RTCP Sender Report.
* Track and carry forward offset from tsReferenceSpatialLayer.
sarumjanuch pushed a commit to QuarkWorks/mediasoup that referenced this issue Apr 12, 2021
 tsReferenceSpatialLayer disappears" (versatica#492) * Set tsReferenceSpatialLayer
 later, only after receiving an RTCP Sender Report. * Track and carry forward
 offset from tsReferenceSpatialLayer.
Lynnworld added a commit to Lynnworld/mediasoup that referenced this issue Oct 18, 2024
@ibc ibc closed this as completed in #1459 Oct 18, 2024
@ibc ibc closed this as completed in 22a5f5f Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment