Google Transport Feedback: read Reference Time field as 24bits signed…

… as per spec (#1145) Fixes #1144
versatica · Aug 28, 2023 · 9b28af7 · 9b28af7
1 parent e8a18a1
commit 9b28af7
Show file tree

Hide file tree

Showing 9 changed files with 102 additions and 14 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,11 @@
 # Changelog
 
 
+### Next
+
+* Google Transport Feedback: Read Reference Time field as 24bits signed as per spec ([PR #1145](https://github.com/versatica/mediasoup/pull/1145)).
+
+
 ### 3.12.10
 
 * Node: Rename `WebRtcTransport.webRtcServerClosed()` to `listenServerClosed()` ([PR #1141](https://github.com/versatica/mediasoup/pull/1141) by @piranna).

diff --git a/worker/include/RTC/RTCP/FeedbackRtpTransport.hpp b/worker/include/RTC/RTCP/FeedbackRtpTransport.hpp
@@ -242,7 +242,8 @@ namespace RTC
 			{
 				return this->referenceTime;
 			}
-			void SetReferenceTime(uint64_t referenceTime) // We only use this for testing purpose.
+			// NOTE: We only use this for testing purpose.
+			void SetReferenceTime(int64_t referenceTime)
 			{
 				this->referenceTime = (referenceTime % TimeWrapPeriod) / BaseTimeTick;
 			}
@@ -315,7 +316,8 @@ namespace RTC
 
 		private:
 			uint16_t baseSequenceNumber{ 0u };
-			uint32_t referenceTime{ 0 };
+			// 24 bits signed integer.
+			int32_t referenceTime{ 0 };
 			// Just for locally generated packets.
 			uint16_t latestSequenceNumber{ 0u };
 			// Just for locally generated packets.

diff --git a/worker/include/Utils.hpp b/worker/include/Utils.hpp
@@ -119,6 +119,19 @@ namespace Utils
 			return uint32_t{ data[i + 2] } | uint32_t{ data[i + 1] } << 8 | uint32_t{ data[i] } << 16;
 		}
 
+		static int32_t Get3BytesSigned(const uint8_t* data, size_t i)
+		{
+			auto byte2 = data[i]; // The most significant byte.
+			auto byte1 = data[i + 1];
+			auto byte0 = data[i + 2]; // The less significant byte.
+
+			// Check bit 7 (sign).
+			uint8_t extension = byte2 & 0b10000000 ? 0b11111111 : 0b00000000;
+
+			return int32_t{ byte0 } | (int32_t{ byte1 } << 8) | (int32_t{ byte2 } << 16) |
+			       (int32_t{ extension } << 24);
+		}
+
 		static uint32_t Get4Bytes(const uint8_t* data, size_t i)
 		{
 			return uint32_t{ data[i + 3] } | uint32_t{ data[i + 2] } << 8 |
@@ -148,6 +161,13 @@ namespace Utils
 			data[i]     = static_cast<uint8_t>(value >> 16);
 		}
 
+		static void Set3BytesSigned(uint8_t* data, size_t i, int32_t value)
+		{
+			data[i + 2] = static_cast<int8_t>(value);
+			data[i + 1] = static_cast<uint8_t>(value >> 8);
+			data[i]     = static_cast<uint8_t>(value >> 16);
+		}
+
 		static void Set4Bytes(uint8_t* data, size_t i, uint32_t value)
 		{
 			data[i + 3] = static_cast<uint8_t>(value);

diff --git a/worker/meson.build b/worker/meson.build
@@ -321,6 +321,7 @@ test_sources = [
     'test/src/RTC/RTCP/TestPacket.cpp',
     'test/src/RTC/RTCP/TestXr.cpp',
     'test/src/Utils/TestBits.cpp',
+    'test/src/Utils/TestByte.cpp',
     'test/src/Utils/TestIP.cpp',
     'test/src/Utils/TestJson.cpp',
     'test/src/Utils/TestString.cpp',

diff --git a/worker/src/RTC/RTCP/FeedbackRtpTransport.cpp b/worker/src/RTC/RTCP/FeedbackRtpTransport.cpp
@@ -79,7 +79,7 @@ namespace RTC
 
 			this->baseSequenceNumber  = Utils::Byte::Get2Bytes(data, 0);
 			this->packetStatusCount   = Utils::Byte::Get2Bytes(data, 2);
-			this->referenceTime       = Utils::Byte::Get3Bytes(data, 4);
+			this->referenceTime       = Utils::Byte::Get3BytesSigned(data, 4);
 			this->feedbackPacketCount = Utils::Byte::Get1Byte(data, 7);
 			this->size                = len;
 
@@ -238,7 +238,7 @@ namespace RTC
 			offset += 2;
 
 			// Reference time.
-			Utils::Byte::Set3Bytes(buffer, offset, static_cast<uint32_t>(this->referenceTime));
+			Utils::Byte::Set3BytesSigned(buffer, offset, this->referenceTime);
 			offset += 3;
 
 			// Feedback packet count.

diff --git a/worker/test/src/RTC/RTCP/TestFeedbackRtpTransport.cpp b/worker/test/src/RTC/RTCP/TestFeedbackRtpTransport.cpp
@@ -513,11 +513,11 @@ SCENARIO("RTCP Feeback RTP transport", "[parser][rtcp][feedback-rtp][transport]"
 		REQUIRE(packet->GetSize() == sizeof(data));
 		REQUIRE(packet->GetBaseSequenceNumber() == 39);
 		REQUIRE(packet->GetPacketStatusCount() == 0);
-		REQUIRE(packet->GetReferenceTime() == 16777214); // 0xFFFFFE (unsigned 24 bits)
+		REQUIRE(packet->GetReferenceTime() == -2); // 0xFFFFFE = -2 (signed 24 bits)
 		REQUIRE(
 		  packet->GetReferenceTimestamp() ==
 		  FeedbackRtpTransportPacket::TimeWrapPeriod +
-		    static_cast<int64_t>(16777214) * FeedbackRtpTransportPacket::BaseTimeTick);
+		    static_cast<int64_t>(-2) * FeedbackRtpTransportPacket::BaseTimeTick);
 		REQUIRE(packet->GetFeedbackPacketCount() == 1);
 
 		SECTION("serialize packet")
@@ -552,11 +552,11 @@ SCENARIO("RTCP Feeback RTP transport", "[parser][rtcp][feedback-rtp][transport]"
 		REQUIRE(packet->GetSize() == sizeof(data));
 		REQUIRE(packet->GetBaseSequenceNumber() == 1);
 		REQUIRE(packet->GetPacketStatusCount() == 2);
-		REQUIRE(packet->GetReferenceTime() == 12408746);
+		REQUIRE(packet->GetReferenceTime() == -4368470);
 		REQUIRE(
 		  packet->GetReferenceTimestamp() ==
 		  FeedbackRtpTransportPacket::TimeWrapPeriod +
-		    static_cast<int64_t>(12408746) * FeedbackRtpTransportPacket::BaseTimeTick);
+		    static_cast<int64_t>(-4368470) * FeedbackRtpTransportPacket::BaseTimeTick);
 
 		REQUIRE(packet->GetFeedbackPacketCount() == 0);
 
@@ -576,8 +576,8 @@ SCENARIO("RTCP Feeback RTP transport", "[parser][rtcp][feedback-rtp][transport]"
 	{
 		using FeedbackPacketsMeta = struct
 		{
-			uint32_t baseTimeRaw;
-			uint64_t baseTimeMs;
+			int32_t baseTimeRaw;
+			int64_t baseTimeMs;
 			uint16_t baseSequence;
 			size_t packetStatusCount;
 			std::vector<int16_t> deltas;
@@ -624,8 +624,8 @@ SCENARIO("RTCP Feeback RTP transport", "[parser][rtcp][feedback-rtp][transport]"
 			                                  0x00, 0x00, 0x10, 0x00, 0x04, 0x00, 0x08, 0x00, 0x08,
 			                                  0x00, 0x08, 0x00, 0x08, 0x00, 0x04, 0x00, 0x10 } },
 
-			{ .baseTimeRaw       = 12408746,
-			  .baseTimeMs        = 1867901568,
+			{ .baseTimeRaw       = -4368470,
+			  .baseTimeMs        = 794159744,
 			  .baseSequence      = 1,
 			  .packetStatusCount = 2,
 			  .deltas            = std::vector<int16_t>{ 35, 17 },

diff --git a/worker/test/src/Utils/TestBits.cpp b/worker/test/src/Utils/TestBits.cpp
@@ -2,8 +2,6 @@
 #include "Utils.hpp"
 #include <catch2/catch.hpp>
 
-using namespace Utils;
-
 SCENARIO("Utils::Bits::CountSetBits()")
 {
 	uint16_t mask;

diff --git a/worker/test/src/Utils/TestByte.cpp b/worker/test/src/Utils/TestByte.cpp
@@ -0,0 +1,61 @@
+#include "common.hpp"
+#include "Utils.hpp"
+#include <catch2/catch.hpp>
+
+SCENARIO("Utils::Byte")
+{
+	// NOTE: The setup and teardown are implicit in how Catch2 works, meaning that
+	// this buffer is initialized before each SECTION below.
+	// Docs: https://github.com/catchorg/Catch2/blob/devel/docs/tutorial.md#test-cases-and-sections
+
+	// clang-format off
+	uint8_t buffer[] =
+	{
+		0b00000000, 0b00000001, 0b00000010, 0b00000011,
+		0b10000000, 0b01000000, 0b00100000, 0b00010000,
+		0b01111111, 0b11111111, 0b11111111, 0b00000000,
+		0b11111111, 0b11111111, 0b11111111, 0b00000000,
+		0b10000000, 0b00000000, 0b00000000, 0b00000000
+	};
+	// clang-format on
+
+	SECTION("Utils::Byte::Get3Bytes()")
+	{
+		// Bytes 4,5 and 6 in the array are number 8405024.
+		REQUIRE(Utils::Byte::Get3Bytes(buffer, 4) == 8405024);
+	}
+
+	SECTION("Utils::Byte::Set3Bytes()")
+	{
+		Utils::Byte::Set3Bytes(buffer, 4, 5666777);
+		REQUIRE(Utils::Byte::Get3Bytes(buffer, 4) == 5666777);
+	}
+
+	SECTION("Utils::Byte::Get3BytesSigned()")
+	{
+		// Bytes 8, 9 and 10 in the array are number 8388607 since first bit is 0 and
+		// all other bits are 1, so it must be maximum positive 24 bits signed integer,
+		// which is Math.pow(2, 23) - 1 = 8388607.
+		REQUIRE(Utils::Byte::Get3BytesSigned(buffer, 8) == 8388607);
+
+		// Bytes 12, 13 and 14 in the array are number -1.
+		REQUIRE(Utils::Byte::Get3BytesSigned(buffer, 12) == -1);
+
+		// Bytes 16, 17 and 18 in the array are number -8388608 since first bit is 1
+		// and all other bits are 0, so it must be minimum negative 24 bits signed
+		// integer, which is -1 * Math.pow(2, 23) = -8388608.
+		REQUIRE(Utils::Byte::Get3BytesSigned(buffer, 16) == -8388608);
+	}
+
+	SECTION("Utils::Byte::Set3BytesSigned()")
+	{
+		Utils::Byte::Set3BytesSigned(buffer, 0, 8388607);
+		REQUIRE(Utils::Byte::Get3BytesSigned(buffer, 0) == 8388607);
+
+		Utils::Byte::Set3BytesSigned(buffer, 0, -1);
+		REQUIRE(Utils::Byte::Get3BytesSigned(buffer, 0) == -1);
+
+		Utils::Byte::Set3BytesSigned(buffer, 0, -8388608);
+		REQUIRE(Utils::Byte::Get3BytesSigned(buffer, 0) == -8388608);
+	}
+}
diff --git a/worker/test/src/Utils/TestIP.cpp b/worker/test/src/Utils/TestIP.cpp
@@ -10,6 +10,7 @@
 #include <netinet/in.h> // sockaddr_in, sockaddr_in6
 #include <sys/socket.h> // struct sockaddr, struct sockaddr_storage, AF_INET, AF_INET6
 #endif
+
 using namespace Utils;
 
 SCENARIO("Utils::IP::GetFamily()")