VP8: Fix and improve temporal layer selection

Fixes versatica#989 * An old payload cannot upgrade the current temporal layer. * Update current temporal layer to the target one.
jmillan · Jan 31, 2023 · 13d850b · 13d850b
1 parent c942fd9
commit 13d850b
Show file tree

Hide file tree

Showing 4 changed files with 125 additions and 19 deletions.
diff --git a/worker/include/RTC/SeqManager.hpp b/worker/include/RTC/SeqManager.hpp
@@ -43,9 +43,14 @@ namespace RTC
 		void Drop(T input);
 		void Offset(T offset);
 		bool Input(const T input, T& output);
+		bool IsSeqLowerThanMaxInput(const T input) const;
+		bool IsSeqHigherThanMaxInput(const T input) const;
 		T GetMaxInput() const;
 		T GetMaxOutput() const;
 
+	private:
+		bool Started() const;
+
 	private:
 		T base{ 0 };
 		T maxOutput{ 0 };

diff --git a/worker/src/RTC/Codecs/VP8.cpp b/worker/src/RTC/Codecs/VP8.cpp
@@ -237,9 +237,13 @@ namespace RTC
 				MS_WARN_DEV("stream is supposed to have >1 temporal layers but does not have TlIndex field");
 			}
 
+			auto oldPictureId =
+			  context->pictureIdManager.IsSeqLowerThanMaxInput(this->payloadDescriptor->pictureId);
+
 			// Check whether pictureId and tl0PictureIndex sync is required.
 			// clang-format off
 			if (
+				!oldPictureId &&
 				context->syncRequired &&
 				this->payloadDescriptor->hasPictureId &&
 				this->payloadDescriptor->hasTl0PictureIndex
@@ -255,12 +259,10 @@ namespace RTC
 			// Incremental pictureId. Check the temporal layer.
 			// clang-format off
 			if (
+				!oldPictureId &&
 				this->payloadDescriptor->hasPictureId &&
 				this->payloadDescriptor->hasTlIndex &&
-				this->payloadDescriptor->hasTl0PictureIndex &&
-				!RTC::SeqManager<uint16_t, 15>::IsSeqLowerThan(
-					this->payloadDescriptor->pictureId,
-					context->pictureIdManager.GetMaxInput())
+				this->payloadDescriptor->hasTl0PictureIndex
 			)
 			// clang-format on
 			{
@@ -321,23 +323,26 @@ namespace RTC
 				return false;
 			}
 
-			// Update/fix current temporal layer.
-			// clang-format off
-			if (
-				this->payloadDescriptor->hasTlIndex &&
-				this->payloadDescriptor->tlIndex > context->GetCurrentTemporalLayer()
-			)
-			// clang-format on
-			{
-				context->SetCurrentTemporalLayer(this->payloadDescriptor->tlIndex);
-			}
-			else if (!this->payloadDescriptor->hasTlIndex)
+			if (!oldPictureId)
 			{
-				context->SetCurrentTemporalLayer(0);
-			}
+				// Update/fix current temporal layer.
+				// clang-format off
+				if (
+					this->payloadDescriptor->hasTlIndex &&
+					this->payloadDescriptor->tlIndex == context->GetTargetTemporalLayer()
+				)
+				// clang-format on
+				{
+					context->SetCurrentTemporalLayer(this->payloadDescriptor->tlIndex);
+				}
+				else if (!this->payloadDescriptor->hasTlIndex)
+				{
+					context->SetCurrentTemporalLayer(0);
+				}
 
-			if (context->GetCurrentTemporalLayer() > context->GetTargetTemporalLayer())
-				context->SetCurrentTemporalLayer(context->GetTargetTemporalLayer());
+				if (context->GetCurrentTemporalLayer() > context->GetTargetTemporalLayer())
+					context->SetCurrentTemporalLayer(context->GetTargetTemporalLayer());
+			}
 
 			// clang-format off
 			if (

diff --git a/worker/src/RTC/SeqManager.cpp b/worker/src/RTC/SeqManager.cpp
@@ -141,6 +141,36 @@ namespace RTC
 		return this->maxOutput;
 	}
 
+	template<typename T, uint8_t N>
+	bool SeqManager<T, N>::IsSeqLowerThanMaxInput(const T input) const
+	{
+		// Not started. Input cannot be lower than nothing.
+		if (!this->Started())
+		{
+			return false;
+		}
+
+		return RTC::SeqManager<T, N>::IsSeqLowerThan(input, this->maxInput);
+	}
+
+	template<typename T, uint8_t N>
+	bool SeqManager<T, N>::IsSeqHigherThanMaxInput(const T input) const
+	{
+		// Not started. Input is higher than nothing.
+		if (!this->Started())
+		{
+			return true;
+		}
+
+		return RTC::SeqManager<T, N>::IsSeqHigherThan(input, this->maxInput);
+	}
+
+	template<typename T, uint8_t N>
+	bool SeqManager<T, N>::Started() const
+	{
+		return (this->base != 0 || this->maxOutput != 0 || this->maxInput != 0);
+	}
+
 	// Explicit instantiation to have all SeqManager definitions in this file.
 	template class SeqManager<uint8_t>;
 	template class SeqManager<uint16_t>;

diff --git a/worker/test/src/RTC/Codecs/TestVP8.cpp b/worker/test/src/RTC/Codecs/TestVP8.cpp
@@ -322,4 +322,70 @@ SCENARIO("process VP8 payload descriptor", "[codecs][vp8]")
 		forwarded = ProcessPacket(context, 1, 0, 1);
 		REQUIRE_FALSE(forwarded);
 	}
+
+	SECTION("old packet with higher temporal layer than current does not change current temporal layer")
+	{
+		RTC::Codecs::EncodingContext::Params params;
+		params.spatialLayers  = 0;
+		params.temporalLayers = 2;
+		Codecs::VP8::EncodingContext context(params);
+		context.SyncRequired();
+
+		context.SetCurrentTemporalLayer(0);
+		context.SetTargetTemporalLayer(0);
+
+		// Frame 1.
+		auto forwarded = ProcessPacket(context, 1, 0, 0);
+		REQUIRE(forwarded);
+		REQUIRE(forwarded->pictureId == 1);
+		REQUIRE(forwarded->tlIndex == 0);
+		REQUIRE(forwarded->tl0PictureIndex == 1);
+
+		// Frame 2.
+		forwarded = ProcessPacket(context, 2, 0, 0);
+		REQUIRE(forwarded);
+		REQUIRE(forwarded->pictureId == 2);
+		REQUIRE(forwarded->tlIndex == 0);
+		REQUIRE(forwarded->tl0PictureIndex == 1);
+
+		// Frame 3. Old packet with higher temporal layer.
+		forwarded = ProcessPacket(context, 0, 0, 1);
+		REQUIRE(forwarded);
+		REQUIRE(forwarded->pictureId == 0);
+		REQUIRE(forwarded->tlIndex == 1);
+		REQUIRE(context.GetCurrentTemporalLayer() == 0);
+	}
+
+	SECTION("packet with higher temporal layer than target does not change current temporal layer")
+	{
+		RTC::Codecs::EncodingContext::Params params;
+		params.spatialLayers  = 0;
+		params.temporalLayers = 2;
+		Codecs::VP8::EncodingContext context(params);
+		context.SyncRequired();
+
+		context.SetCurrentTemporalLayer(0);
+		context.SetTargetTemporalLayer(0);
+
+		// Frame 1.
+		auto forwarded = ProcessPacket(context, 1, 0, 0);
+		REQUIRE(forwarded);
+		REQUIRE(forwarded->pictureId == 1);
+		REQUIRE(forwarded->tlIndex == 0);
+		REQUIRE(forwarded->tl0PictureIndex == 1);
+
+		// Frame 2.
+		forwarded = ProcessPacket(context, 2, 0, 0);
+		REQUIRE(forwarded);
+		REQUIRE(forwarded->pictureId == 2);
+		REQUIRE(forwarded->tlIndex == 0);
+		REQUIRE(forwarded->tl0PictureIndex == 1);
+
+		context.SetTargetTemporalLayer(2);
+
+		// Frame 3. Packet with higher temporal layer than target.
+		forwarded = ProcessPacket(context, 3, 0, 3, true /*layerSync*/);
+		REQUIRE_FALSE(forwarded);
+		REQUIRE(context.GetCurrentTemporalLayer() == 0);
+	}
 }