From ee5d68dcc29a71e9c7539e53992172c769c6bf92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Cervi=C3=B1o?= Date: Tue, 17 Jan 2023 18:11:49 +0100 Subject: [PATCH 1/6] Fix wrong PictureID rollover in VP9 and VP8 --- worker/include/RTC/Codecs/H264_SVC.hpp | 2 +- worker/include/RTC/Codecs/VP8.hpp | 2 +- worker/include/RTC/Codecs/VP9.hpp | 2 +- worker/include/RTC/SeqManager.hpp | 4 +- worker/meson.build | 1 + worker/src/RTC/Codecs/VP8.cpp | 2 +- worker/src/RTC/Codecs/VP9.cpp | 2 +- worker/src/RTC/SeqManager.cpp | 65 ++++++------ worker/src/RTC/SvcConsumer.cpp | 1 + worker/test/src/RTC/Codecs/TestVP8.cpp | 28 +++++ worker/test/src/RTC/Codecs/TestVP9.cpp | 76 +++++++++++++ worker/test/src/RTC/TestSeqManager.cpp | 141 ++++++++++++++++++++++++- 12 files changed, 285 insertions(+), 41 deletions(-) create mode 100644 worker/test/src/RTC/Codecs/TestVP9.cpp diff --git a/worker/include/RTC/Codecs/H264_SVC.hpp b/worker/include/RTC/Codecs/H264_SVC.hpp index 01be7e78f9..ed3df48a81 100644 --- a/worker/include/RTC/Codecs/H264_SVC.hpp +++ b/worker/include/RTC/Codecs/H264_SVC.hpp @@ -71,7 +71,7 @@ namespace RTC } public: - RTC::SeqManager pictureIdManager; + RTC::SeqManager pictureIdManager; bool syncRequired{ false }; }; diff --git a/worker/include/RTC/Codecs/VP8.hpp b/worker/include/RTC/Codecs/VP8.hpp index 7b760787f5..f8fe037e1a 100644 --- a/worker/include/RTC/Codecs/VP8.hpp +++ b/worker/include/RTC/Codecs/VP8.hpp @@ -96,7 +96,7 @@ namespace RTC } public: - RTC::SeqManager pictureIdManager; + RTC::SeqManager pictureIdManager; RTC::SeqManager tl0PictureIndexManager; bool syncRequired{ false }; }; diff --git a/worker/include/RTC/Codecs/VP9.hpp b/worker/include/RTC/Codecs/VP9.hpp index 38cb3e6b38..b69688d638 100644 --- a/worker/include/RTC/Codecs/VP9.hpp +++ b/worker/include/RTC/Codecs/VP9.hpp @@ -113,7 +113,7 @@ namespace RTC } public: - RTC::SeqManager pictureIdManager; + RTC::SeqManager pictureIdManager; bool syncRequired{ false }; }; diff --git a/worker/include/RTC/SeqManager.hpp b/worker/include/RTC/SeqManager.hpp index ec20b96e88..8d310ee9d9 100644 --- a/worker/include/RTC/SeqManager.hpp +++ b/worker/include/RTC/SeqManager.hpp @@ -7,11 +7,11 @@ namespace RTC { - template + template class SeqManager { public: - static constexpr T MaxValue = std::numeric_limits::max(); + static constexpr T MaxValue = (N == 0) ? std::numeric_limits::max() : ((1 << N) - 1); public: struct SeqLowerThan diff --git a/worker/meson.build b/worker/meson.build index b93ca83098..ef6e6a9597 100644 --- a/worker/meson.build +++ b/worker/meson.build @@ -295,6 +295,7 @@ mediasoup_worker_test = executable( 'test/src/RTC/TestTrendCalculator.cpp', 'test/src/RTC/TestRtpEncodingParameters.cpp', 'test/src/RTC/Codecs/TestVP8.cpp', + 'test/src/RTC/Codecs/TestVP9.cpp', 'test/src/RTC/Codecs/TestH264.cpp', 'test/src/RTC/Codecs/TestH264_SVC.cpp', 'test/src/RTC/RTCP/TestFeedbackPsAfb.cpp', diff --git a/worker/src/RTC/Codecs/VP8.cpp b/worker/src/RTC/Codecs/VP8.cpp index 043ac93cd2..3d5a2139ca 100644 --- a/worker/src/RTC/Codecs/VP8.cpp +++ b/worker/src/RTC/Codecs/VP8.cpp @@ -258,7 +258,7 @@ namespace RTC this->payloadDescriptor->hasPictureId && this->payloadDescriptor->hasTlIndex && this->payloadDescriptor->hasTl0PictureIndex && - !RTC::SeqManager::IsSeqLowerThan( + !RTC::SeqManager::IsSeqLowerThan( this->payloadDescriptor->pictureId, context->pictureIdManager.GetMaxInput()) ) diff --git a/worker/src/RTC/Codecs/VP9.cpp b/worker/src/RTC/Codecs/VP9.cpp index d63df49a49..539308f1af 100644 --- a/worker/src/RTC/Codecs/VP9.cpp +++ b/worker/src/RTC/Codecs/VP9.cpp @@ -213,7 +213,7 @@ namespace RTC // clang-format off bool isOldPacket = ( this->payloadDescriptor->hasPictureId && - RTC::SeqManager::IsSeqLowerThan( + RTC::SeqManager::IsSeqLowerThan( this->payloadDescriptor->pictureId, context->pictureIdManager.GetMaxInput()) ); diff --git a/worker/src/RTC/SeqManager.cpp b/worker/src/RTC/SeqManager.cpp index 61b5aa729b..02d6872164 100644 --- a/worker/src/RTC/SeqManager.cpp +++ b/worker/src/RTC/SeqManager.cpp @@ -7,43 +7,43 @@ namespace RTC { - template - bool SeqManager::SeqLowerThan::operator()(const T lhs, const T rhs) const + template + bool SeqManager::SeqLowerThan::operator()(const T lhs, const T rhs) const { return ((rhs > lhs) && (rhs - lhs <= MaxValue / 2)) || ((lhs > rhs) && (lhs - rhs > MaxValue / 2)); } - template - bool SeqManager::SeqHigherThan::operator()(const T lhs, const T rhs) const + template + bool SeqManager::SeqHigherThan::operator()(const T lhs, const T rhs) const { return ((lhs > rhs) && (lhs - rhs <= MaxValue / 2)) || ((rhs > lhs) && (rhs - lhs > MaxValue / 2)); } - template - const typename SeqManager::SeqLowerThan SeqManager::isSeqLowerThan{}; + template + const typename SeqManager::SeqLowerThan SeqManager::isSeqLowerThan{}; - template - const typename SeqManager::SeqHigherThan SeqManager::isSeqHigherThan{}; + template + const typename SeqManager::SeqHigherThan SeqManager::isSeqHigherThan{}; - template - bool SeqManager::IsSeqLowerThan(const T lhs, const T rhs) + template + bool SeqManager::IsSeqLowerThan(const T lhs, const T rhs) { return isSeqLowerThan(lhs, rhs); } - template - bool SeqManager::IsSeqHigherThan(const T lhs, const T rhs) + template + bool SeqManager::IsSeqHigherThan(const T lhs, const T rhs) { return isSeqHigherThan(lhs, rhs); } - template - void SeqManager::Sync(T input) + template + void SeqManager::Sync(T input) { // Update base. - this->base = this->maxOutput - input; + this->base = (this->maxOutput - input) & MaxValue; // Update maxInput. this->maxInput = input; @@ -52,24 +52,24 @@ namespace RTC this->dropped.clear(); } - template - void SeqManager::Drop(T input) + template + void SeqManager::Drop(T input) { // Mark as dropped if 'input' is higher than anyone already processed. - if (SeqManager::IsSeqHigherThan(input, this->maxInput)) + if (SeqManager::IsSeqHigherThan(input, this->maxInput)) { this->dropped.insert(input); } } - template - void SeqManager::Offset(T offset) + template + void SeqManager::Offset(T offset) { - this->base += offset; + this->base = (this->base + offset) & MaxValue; } - template - bool SeqManager::Input(const T input, T& output) + template + bool SeqManager::Input(const T input, T& output) { auto base = this->base; @@ -78,10 +78,10 @@ namespace RTC { // Delete dropped inputs older than input - MaxValue/2. size_t droppedCount = this->dropped.size(); - auto it = this->dropped.lower_bound(input - MaxValue / 2); - + size_t threshold = (input - MaxValue / 2) & MaxValue; + auto it = this->dropped.lower_bound(threshold); this->dropped.erase(this->dropped.begin(), it); - this->base -= (droppedCount - this->dropped.size()); + this->base = (this->base - (droppedCount - this->dropped.size())) & MaxValue; // Count dropped entries before 'input' in order to adapt the base. droppedCount = this->dropped.size(); @@ -100,10 +100,10 @@ namespace RTC droppedCount -= std::distance(it, this->dropped.end()); } - base = this->base - droppedCount; + base = (this->base - droppedCount) & MaxValue; } - output = input + base; + output = (input + base) & MaxValue; T idelta = input - this->maxInput; T odelta = output - this->maxOutput; @@ -121,14 +121,14 @@ namespace RTC return true; } - template - T SeqManager::GetMaxInput() const + template + T SeqManager::GetMaxInput() const { return this->maxInput; } - template - T SeqManager::GetMaxOutput() const + template + T SeqManager::GetMaxOutput() const { return this->maxOutput; } @@ -136,6 +136,7 @@ namespace RTC // Explicit instantiation to have all SeqManager definitions in this file. template class SeqManager; template class SeqManager; + template class SeqManager; // For PictureID (15bits). template class SeqManager; } // namespace RTC diff --git a/worker/src/RTC/SvcConsumer.cpp b/worker/src/RTC/SvcConsumer.cpp index 5f5d1a7156..c13b809177 100644 --- a/worker/src/RTC/SvcConsumer.cpp +++ b/worker/src/RTC/SvcConsumer.cpp @@ -586,6 +586,7 @@ namespace RTC MS_DEBUG_TAG(rtp, "sync key frame received"); this->rtpSeqManager.Sync(packet->GetSequenceNumber() - 1); + this->encodingContext->SyncRequired(); this->syncRequired = false; } diff --git a/worker/test/src/RTC/Codecs/TestVP8.cpp b/worker/test/src/RTC/Codecs/TestVP8.cpp index 58abf0b755..1d5f2974be 100644 --- a/worker/test/src/RTC/Codecs/TestVP8.cpp +++ b/worker/test/src/RTC/Codecs/TestVP8.cpp @@ -292,4 +292,32 @@ SCENARIO("process VP8 payload descriptor", "[codecs][vp8]") REQUIRE(forwarded->pictureId == 1); REQUIRE(forwarded->tl0PictureIndex == 1); } + + SECTION("drop packets that belong to other temporal layers after rolling over pictureID") + { + 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, 32767, 0, 0); + REQUIRE(forwarded); + REQUIRE(forwarded->pictureId == 1); + REQUIRE(forwarded->tl0PictureIndex == 1); + + // Frame 2 + forwarded = ProcessPacket(context, 0, 0, 0); + REQUIRE(forwarded); + REQUIRE(forwarded->pictureId == 2); + REQUIRE(forwarded->tl0PictureIndex == 1); + + // Frame 3 + forwarded = ProcessPacket(context, 1, 0, 1); + REQUIRE_FALSE(forwarded); + } } diff --git a/worker/test/src/RTC/Codecs/TestVP9.cpp b/worker/test/src/RTC/Codecs/TestVP9.cpp new file mode 100644 index 0000000000..264c836f2d --- /dev/null +++ b/worker/test/src/RTC/Codecs/TestVP9.cpp @@ -0,0 +1,76 @@ +#include "common.hpp" +#include "RTC/Codecs/VP9.hpp" +#include +#include // std::memcmp() + +using namespace RTC; + +Codecs::VP9::PayloadDescriptor* CreateVP9Packet( + uint8_t* buffer, size_t bufferLen, uint16_t pictureId, uint8_t tlIndex) +{ + buffer[0] = 0xAD; // I and L bits + uint16_t netPictureId = htons(pictureId); + std::memcpy(buffer + 1, &netPictureId, 2); + buffer[1] |= 0x80; + buffer[3] = tlIndex << 6; + + auto* payloadDescriptor = Codecs::VP9::Parse(buffer, bufferLen); + + REQUIRE(payloadDescriptor); + + return payloadDescriptor; +} + +std::unique_ptr ProcessVP9Packet( + Codecs::VP9::EncodingContext& context, uint16_t pictureId, uint8_t tlIndex) +{ + // clang-format off + uint8_t buffer[] = + { + 0xAD, 0x80, 0x00, 0x00, 0x00, 0x00 + }; + // clang-format on + bool marker; + auto* payloadDescriptor = CreateVP9Packet(buffer, sizeof(buffer), pictureId, tlIndex); + std::unique_ptr payloadDescriptorHandler( + new Codecs::VP9::PayloadDescriptorHandler(payloadDescriptor)); + + if (payloadDescriptorHandler->Process(&context, buffer, marker)) + { + return std::unique_ptr(Codecs::VP9::Parse(buffer, sizeof(buffer))); + } + + return nullptr; +} + +SCENARIO("process VP9 payload descriptor", "[codecs][vp9]") +{ + SECTION("drop packets that belong to other temporal layers after rolling over pictureID") + { + RTC::Codecs::EncodingContext::Params params; + params.spatialLayers = 1; + params.temporalLayers = 3; + + Codecs::VP9::EncodingContext context(params); + context.SyncRequired(); + context.SetCurrentTemporalLayer(0); + context.SetTargetTemporalLayer(0); + + context.SetCurrentSpatialLayer(0); + context.SetTargetSpatialLayer(0); + + // Frame 1 + auto forwarded = ProcessVP9Packet(context, 32767, 0); + REQUIRE(forwarded); + REQUIRE(forwarded->pictureId == 32767); + + // Frame 2 + forwarded = ProcessVP9Packet(context, 0, 0); + REQUIRE(forwarded); + REQUIRE(forwarded->pictureId == 0); + + // Frame 3 + forwarded = ProcessVP9Packet(context, 1, 1); + REQUIRE_FALSE(forwarded); + } +} diff --git a/worker/test/src/RTC/TestSeqManager.cpp b/worker/test/src/RTC/TestSeqManager.cpp index d959f2e392..7f4db1f115 100644 --- a/worker/test/src/RTC/TestSeqManager.cpp +++ b/worker/test/src/RTC/TestSeqManager.cpp @@ -21,8 +21,8 @@ struct TestSeqManagerInput T offset{ 0 }; }; -template -void validate(SeqManager& seqManager, std::vector>& inputs) +template +void validate(SeqManager& seqManager, std::vector>& inputs) { for (auto& element : inputs) { @@ -55,6 +55,11 @@ SCENARIO("SeqManager", "[rtc]") REQUIRE(SeqManager::IsSeqHigherThan(0, 65000) == true); } + SECTION("0 is greater than 32500 in range 15") + { + REQUIRE(SeqManager::IsSeqHigherThan(0, 32500) == true); + } + SECTION("receive ordered numbers, no sync, no drop") { // clang-format off @@ -76,7 +81,9 @@ SCENARIO("SeqManager", "[rtc]") // clang-format on SeqManager seqManager; + SeqManager seqManager2; validate(seqManager, inputs); + validate(seqManager2, inputs); } SECTION("receive ordered numbers, sync, no drop") @@ -96,7 +103,9 @@ SCENARIO("SeqManager", "[rtc]") // clang-format on SeqManager seqManager; + SeqManager seqManager2; validate(seqManager, inputs); + validate(seqManager2, inputs); } SECTION("receive ordered numbers, sync, drop") @@ -121,7 +130,9 @@ SCENARIO("SeqManager", "[rtc]") // clang-format on SeqManager seqManager; + SeqManager seqManager2; validate(seqManager, inputs); + validate(seqManager2, inputs); } SECTION("receive ordered wrapped numbers") @@ -154,7 +165,9 @@ SCENARIO("SeqManager", "[rtc]") // clang-format on SeqManager seqManager; + SeqManager seqManager2; validate(seqManager, inputs); + validate(seqManager2, inputs); } SECTION("receive mixed numbers with a big jump, drop before jump") @@ -172,7 +185,9 @@ SCENARIO("SeqManager", "[rtc]") // clang-format on SeqManager seqManager; + SeqManager seqManager2; validate(seqManager, inputs); + validate(seqManager2, inputs); } SECTION("receive mixed numbers with a big jump, drop after jump") @@ -189,7 +204,9 @@ SCENARIO("SeqManager", "[rtc]") // clang-format on SeqManager seqManager; + SeqManager seqManager2; validate(seqManager, inputs); + validate(seqManager2, inputs); } SECTION("drop, receive numbers newer and older than the one dropped") @@ -206,7 +223,9 @@ SCENARIO("SeqManager", "[rtc]") // clang-format on SeqManager seqManager; + SeqManager seqManager2; validate(seqManager, inputs); + validate(seqManager2, inputs); } SECTION("receive mixed numbers, sync, drop") @@ -284,7 +303,9 @@ SCENARIO("SeqManager", "[rtc]") // clang-format on SeqManager seqManager; + SeqManager seqManager2; validate(seqManager, inputs); + validate(seqManager2, inputs); } SECTION("drop many inputs at the beginning (using uint16_t)") @@ -325,7 +346,9 @@ SCENARIO("SeqManager", "[rtc]") // clang-format on SeqManager seqManager; + SeqManager seqManager2; validate(seqManager, inputs); + validate(seqManager2, inputs); } SECTION("drop many inputs at the beginning (using uint8_t)") @@ -369,6 +392,64 @@ SCENARIO("SeqManager", "[rtc]") validate(seqManager, inputs); } + SECTION("receive mixed numbers, sync, drop in range 15") + { + // clang-format off + std::vector> inputs = + { + { 0, 0, false, false }, + { 1, 1, false, false }, + { 2, 2, false, false }, + { 3, 3, false, false }, + { 7, 7, false, false }, + { 6, 0, false, true }, // drop. + { 8, 8, false, false }, + { 10, 10, false, false }, + { 9, 9, false, false }, + { 11, 11, false, false }, + { 0, 12, true, false }, // sync. + { 2, 14, false, false }, + { 3, 15, false, false }, + { 4, 16, false, false }, + { 5, 17, false, false }, + { 6, 18, false, false }, + { 7, 19, false, false }, + { 8, 20, false, false }, + { 9, 21, false, false }, + { 10, 22, false, false }, + { 9, 0, false, true }, // drop. + { 61, 23, true, false }, // sync. + { 62, 24, false, false }, + { 63, 25, false, false }, + { 64, 26, false, false }, + { 65, 27, false, false }, + { 11, 28, true, false }, // sync. + { 12, 29, false, false }, + { 13, 30, false, false }, + { 14, 31, false, false }, + { 15, 32, false, false }, + { 1, 33, true, false }, // sync. + { 2, 34, false, false }, + { 3, 35, false, false }, + { 4, 36, false, false }, + { 5, 37, false, false }, + { 32767, 38, true, false }, // sync. + { 32768, 39, false, false }, + { 32769, 40, false, false }, + { 0, 41, true, false }, // sync. + { 1, 42, false, false }, + { 3, 0, false, true }, // drop. + { 4, 44, false, false }, + { 5, 45, false, false }, + { 6, 46, false, false }, + { 7, 47, false, false } + }; + // clang-format on + + SeqManager seqManager; + validate(seqManager, inputs); + } + SECTION("drop many inputs at the beginning (using uint16_t with high values)") { // clang-format off @@ -424,4 +505,60 @@ SCENARIO("SeqManager", "[rtc]") SeqManager seqManager; validate(seqManager, inputs); } + + SECTION("drop many inputs at the beginning (using uint16_t range 15 with high values)") + { + // clang-format off + std::vector> inputs = + { + { 1, 1, false, false }, + { 2, 0, false, true }, // drop. + { 3, 0, false, true }, // drop. + { 4, 0, false, true }, // drop. + { 5, 0, false, true }, // drop. + { 6, 0, false, true }, // drop. + { 7, 0, false, true }, // drop. + { 8, 0, false, true }, // drop. + { 9, 0, false, true }, // drop. + { 16384, 16376, false, false }, + { 16385, 16377, false, false }, + { 16386, 16378, false, false }, + { 16387, 16379, false, false }, + { 16388, 16380, false, false }, + { 16389, 16381, false, false }, + { 16390, 16382, false, false }, + { 16391, 16383, false, false }, + { 16392, 16384, false, false }, + { 16393, 16385, false, false }, + { 16394, 16386, false, false }, + { 16395, 16387, false, false }, + { 16396, 16388, false, false } + }; + // clang-format on + + SeqManager seqManager; + validate(seqManager, inputs); + } + + SECTION("sync and drop some input near max-value in a 15bit range") + { + // clang-format off + std::vector> inputs = + { + { 32762, 1, true, false }, + { 32763, 2, false, false }, + { 32764, 3, false, false }, + { 32765, 0, false, true }, + { 32766, 0, false, true }, + { 32767, 4, false, false }, + { 0, 5, false, false }, + { 1, 6, false, false }, + { 2, 7, false, false }, + { 3, 8, false, false } + }; + // clang-format on + + SeqManager seqManager; + validate(seqManager, inputs); + } } From f5786c4f103fb00058d976a09e93fa5a2f0a50b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Cervi=C3=B1o?= Date: Wed, 18 Jan 2023 13:55:04 +0100 Subject: [PATCH 2/6] Fix wrong delta calculation inside SeqManager --- worker/include/RTC/SeqManager.hpp | 5 +- worker/src/RTC/SeqManager.cpp | 37 ++++++++------ worker/test/src/RTC/Codecs/TestVP8.cpp | 4 +- worker/test/src/RTC/Codecs/TestVP9.cpp | 6 ++- worker/test/src/RTC/TestSeqManager.cpp | 70 +++++++++++++++++++++----- 5 files changed, 90 insertions(+), 32 deletions(-) diff --git a/worker/include/RTC/SeqManager.hpp b/worker/include/RTC/SeqManager.hpp index 8d310ee9d9..3bf0e74dc8 100644 --- a/worker/include/RTC/SeqManager.hpp +++ b/worker/include/RTC/SeqManager.hpp @@ -7,7 +7,9 @@ namespace RTC { - template + // T is the base type (uint16_t, uint32_t, ...). + // N is the max number of bits used in T. + template class SeqManager { public: @@ -27,6 +29,7 @@ namespace RTC private: static const SeqLowerThan isSeqLowerThan; static const SeqHigherThan isSeqHigherThan; + static T Delta(const T lhs, const T rhs); public: static bool IsSeqLowerThan(const T lhs, const T rhs); diff --git a/worker/src/RTC/SeqManager.cpp b/worker/src/RTC/SeqManager.cpp index 02d6872164..20dbaf9f70 100644 --- a/worker/src/RTC/SeqManager.cpp +++ b/worker/src/RTC/SeqManager.cpp @@ -7,39 +7,46 @@ namespace RTC { - template + template bool SeqManager::SeqLowerThan::operator()(const T lhs, const T rhs) const { return ((rhs > lhs) && (rhs - lhs <= MaxValue / 2)) || ((lhs > rhs) && (lhs - rhs > MaxValue / 2)); } - template + template bool SeqManager::SeqHigherThan::operator()(const T lhs, const T rhs) const { return ((lhs > rhs) && (lhs - rhs <= MaxValue / 2)) || ((rhs > lhs) && (rhs - lhs > MaxValue / 2)); } - template + template const typename SeqManager::SeqLowerThan SeqManager::isSeqLowerThan{}; - template + template const typename SeqManager::SeqHigherThan SeqManager::isSeqHigherThan{}; - template + template bool SeqManager::IsSeqLowerThan(const T lhs, const T rhs) { return isSeqLowerThan(lhs, rhs); } - template + template bool SeqManager::IsSeqHigherThan(const T lhs, const T rhs) { return isSeqHigherThan(lhs, rhs); } - template + template + T SeqManager::Delta(const T lhs, const T rhs) + { + T value = (lhs > rhs) ? (lhs - rhs) : (MaxValue - rhs + lhs); + return value & MaxValue; + } + + template void SeqManager::Sync(T input) { // Update base. @@ -52,7 +59,7 @@ namespace RTC this->dropped.clear(); } - template + template void SeqManager::Drop(T input) { // Mark as dropped if 'input' is higher than anyone already processed. @@ -62,13 +69,13 @@ namespace RTC } } - template + template void SeqManager::Offset(T offset) { this->base = (this->base + offset) & MaxValue; } - template + template bool SeqManager::Input(const T input, T& output) { auto base = this->base; @@ -105,8 +112,8 @@ namespace RTC output = (input + base) & MaxValue; - T idelta = input - this->maxInput; - T odelta = output - this->maxOutput; + T idelta = SeqManager::Delta(input, this->maxInput); + T odelta = SeqManager::Delta(output, this->maxOutput); // New input is higher than the maximum seen. But less than acceptable units higher. // Keep it as the maximum seen. See Drop(). @@ -121,13 +128,13 @@ namespace RTC return true; } - template + template T SeqManager::GetMaxInput() const { return this->maxInput; } - template + template T SeqManager::GetMaxOutput() const { return this->maxOutput; @@ -136,7 +143,7 @@ namespace RTC // Explicit instantiation to have all SeqManager definitions in this file. template class SeqManager; template class SeqManager; - template class SeqManager; // For PictureID (15bits). + template class SeqManager; // For PictureID (15 bits). template class SeqManager; } // namespace RTC diff --git a/worker/test/src/RTC/Codecs/TestVP8.cpp b/worker/test/src/RTC/Codecs/TestVP8.cpp index 1d5f2974be..4e7f3a48a2 100644 --- a/worker/test/src/RTC/Codecs/TestVP8.cpp +++ b/worker/test/src/RTC/Codecs/TestVP8.cpp @@ -5,6 +5,8 @@ using namespace RTC; +constexpr uint16_t kMaxPictureId = (1 << 15) - 1; + SCENARIO("parse VP8 payload descriptor", "[codecs][vp8]") { SECTION("parse payload descriptor") @@ -305,7 +307,7 @@ SCENARIO("process VP8 payload descriptor", "[codecs][vp8]") context.SetTargetTemporalLayer(0); // Frame 1 - auto forwarded = ProcessPacket(context, 32767, 0, 0); + auto forwarded = ProcessPacket(context, kMaxPictureId, 0, 0); REQUIRE(forwarded); REQUIRE(forwarded->pictureId == 1); REQUIRE(forwarded->tl0PictureIndex == 1); diff --git a/worker/test/src/RTC/Codecs/TestVP9.cpp b/worker/test/src/RTC/Codecs/TestVP9.cpp index 264c836f2d..8c3347d298 100644 --- a/worker/test/src/RTC/Codecs/TestVP9.cpp +++ b/worker/test/src/RTC/Codecs/TestVP9.cpp @@ -5,6 +5,8 @@ using namespace RTC; +constexpr uint16_t kMaxPictureId = (1 << 15) - 1; + Codecs::VP9::PayloadDescriptor* CreateVP9Packet( uint8_t* buffer, size_t bufferLen, uint16_t pictureId, uint8_t tlIndex) { @@ -60,9 +62,9 @@ SCENARIO("process VP9 payload descriptor", "[codecs][vp9]") context.SetTargetSpatialLayer(0); // Frame 1 - auto forwarded = ProcessVP9Packet(context, 32767, 0); + auto forwarded = ProcessVP9Packet(context, kMaxPictureId, 0); REQUIRE(forwarded); - REQUIRE(forwarded->pictureId == 32767); + REQUIRE(forwarded->pictureId == kMaxPictureId); // Frame 2 forwarded = ProcessVP9Packet(context, 0, 0); diff --git a/worker/test/src/RTC/TestSeqManager.cpp b/worker/test/src/RTC/TestSeqManager.cpp index 7f4db1f115..3276de97bc 100644 --- a/worker/test/src/RTC/TestSeqManager.cpp +++ b/worker/test/src/RTC/TestSeqManager.cpp @@ -6,11 +6,13 @@ using namespace RTC; +constexpr uint16_t kMaxNumberFor15Bits = (1 << 15) - 1; + template struct TestSeqManagerInput { - TestSeqManagerInput(T input, T output, bool sync = false, bool drop = false, T offset = 0) - : input(input), output(output), sync(sync), drop(drop), offset(offset) + TestSeqManagerInput(T input, T output, bool sync = false, bool drop = false, T offset = 0, int64_t maxInput = -1) + : input(input), output(output), sync(sync), drop(drop), offset(offset), maxInput(maxInput) { } @@ -19,9 +21,10 @@ struct TestSeqManagerInput bool sync{ false }; bool drop{ false }; T offset{ 0 }; + int64_t maxInput{ -1 }; }; -template +template void validate(SeqManager& seqManager, std::vector>& inputs) { for (auto& element : inputs) @@ -44,6 +47,9 @@ void validate(SeqManager& seqManager, std::vector>& // Covert to string because otherwise Catch will print uint8_t as char. REQUIRE(std::to_string(output) == std::to_string(element.output)); + if (element.maxInput != -1) { + REQUIRE(std::to_string(element.maxInput) == std::to_string(seqManager.GetMaxInput())); + } } } } @@ -545,20 +551,58 @@ SCENARIO("SeqManager", "[rtc]") // clang-format off std::vector> inputs = { - { 32762, 1, true, false }, - { 32763, 2, false, false }, - { 32764, 3, false, false }, - { 32765, 0, false, true }, - { 32766, 0, false, true }, - { 32767, 4, false, false }, - { 0, 5, false, false }, - { 1, 6, false, false }, - { 2, 7, false, false }, - { 3, 8, false, false } + { 32762, 1, true, false, 0, 32762 }, + { 32763, 2, false, false, 0, 32763 }, + { 32764, 3, false, false, 0, 32764 }, + { 32765, 0, false, true, 0, 32765 }, + { 32766, 0, false, true, 0, 32766 }, + { 32767, 4, false, false, 0, 32767 }, + { 0, 5, false, false, 0, 0 }, + { 1, 6, false, false, 0, 1 }, + { 2, 7, false, false, 0, 2 }, + { 3, 8, false, false, 0, 3 } }; // clang-format on SeqManager seqManager; validate(seqManager, inputs); } + + SECTION("should update all values during multiple roll overs") + { + // clang-format off + std::vector> inputs = + { + { 0, 1, true, false, 0, 0 }, + }; + for (uint16_t j = 0; j < 100; ++j) { + for (uint16_t i = 1; i < std::numeric_limits::max(); ++i) { + uint16_t output = i + 1; + inputs.push_back({ i, output, false, false, 0, i }); + } + } + // clang-format on + + SeqManager seqManager; + validate(seqManager, inputs); + } + + SECTION("should update all values during multiple roll overs (15 bits range)") + { + // clang-format off + std::vector> inputs = + { + { 0, 1, true, false, 0, 0 }, + }; + for (uint16_t j = 0; j < 100; ++j) { + for (uint16_t i = 1; i < kMaxNumberFor15Bits; ++i) { + uint16_t output = i + 1; + inputs.push_back({ i, output, false, false, 0, i }); + } + } + // clang-format on + + SeqManager seqManager; + validate(seqManager, inputs); + } } From 43ad91a26ac81b20654aec5d235a82318b817c71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Cervi=C3=B1o?= Date: Wed, 18 Jan 2023 14:15:47 +0100 Subject: [PATCH 3/6] Fix lint issues --- worker/test/src/RTC/TestSeqManager.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/worker/test/src/RTC/TestSeqManager.cpp b/worker/test/src/RTC/TestSeqManager.cpp index 3276de97bc..5c4624fe6d 100644 --- a/worker/test/src/RTC/TestSeqManager.cpp +++ b/worker/test/src/RTC/TestSeqManager.cpp @@ -11,7 +11,8 @@ constexpr uint16_t kMaxNumberFor15Bits = (1 << 15) - 1; template struct TestSeqManagerInput { - TestSeqManagerInput(T input, T output, bool sync = false, bool drop = false, T offset = 0, int64_t maxInput = -1) + TestSeqManagerInput( + T input, T output, bool sync = false, bool drop = false, T offset = 0, int64_t maxInput = -1) : input(input), output(output), sync(sync), drop(drop), offset(offset), maxInput(maxInput) { } @@ -47,7 +48,8 @@ void validate(SeqManager& seqManager, std::vector>& // Covert to string because otherwise Catch will print uint8_t as char. REQUIRE(std::to_string(output) == std::to_string(element.output)); - if (element.maxInput != -1) { + if (element.maxInput != -1) + { REQUIRE(std::to_string(element.maxInput) == std::to_string(seqManager.GetMaxInput())); } } From 7bb6198c2c8b2de3176e7c22d7859647efc43906 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Cervi=C3=B1o?= Date: Thu, 19 Jan 2023 10:25:43 +0100 Subject: [PATCH 4/6] Apply comments in the PR --- worker/test/src/RTC/Codecs/TestVP8.cpp | 14 +++++++------- worker/test/src/RTC/Codecs/TestVP9.cpp | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/worker/test/src/RTC/Codecs/TestVP8.cpp b/worker/test/src/RTC/Codecs/TestVP8.cpp index 4e7f3a48a2..9ba5c42864 100644 --- a/worker/test/src/RTC/Codecs/TestVP8.cpp +++ b/worker/test/src/RTC/Codecs/TestVP8.cpp @@ -276,19 +276,19 @@ SCENARIO("process VP8 payload descriptor", "[codecs][vp8]") context.SetCurrentTemporalLayer(0); context.SetTargetTemporalLayer(0); - // Frame 1 + // Frame 1. auto forwarded = ProcessPacket(context, 0, 0, 0); REQUIRE(forwarded); REQUIRE(forwarded->pictureId == 0); REQUIRE(forwarded->tl0PictureIndex == 0); - // Frame 2 gets lost + // Frame 2 gets lost. - // Frame 3 + // Frame 3. forwarded = ProcessPacket(context, 2, 1, 1); REQUIRE_FALSE(forwarded); - // Frame 2 retransmitted + // Frame 2 retransmitted. forwarded = ProcessPacket(context, 1, 1, 0); REQUIRE(forwarded); REQUIRE(forwarded->pictureId == 1); @@ -306,19 +306,19 @@ SCENARIO("process VP8 payload descriptor", "[codecs][vp8]") context.SetCurrentTemporalLayer(0); context.SetTargetTemporalLayer(0); - // Frame 1 + // Frame 1. auto forwarded = ProcessPacket(context, kMaxPictureId, 0, 0); REQUIRE(forwarded); REQUIRE(forwarded->pictureId == 1); REQUIRE(forwarded->tl0PictureIndex == 1); - // Frame 2 + // Frame 2. forwarded = ProcessPacket(context, 0, 0, 0); REQUIRE(forwarded); REQUIRE(forwarded->pictureId == 2); REQUIRE(forwarded->tl0PictureIndex == 1); - // Frame 3 + // Frame 3. forwarded = ProcessPacket(context, 1, 0, 1); REQUIRE_FALSE(forwarded); } diff --git a/worker/test/src/RTC/Codecs/TestVP9.cpp b/worker/test/src/RTC/Codecs/TestVP9.cpp index 8c3347d298..7a43dcd73b 100644 --- a/worker/test/src/RTC/Codecs/TestVP9.cpp +++ b/worker/test/src/RTC/Codecs/TestVP9.cpp @@ -61,17 +61,17 @@ SCENARIO("process VP9 payload descriptor", "[codecs][vp9]") context.SetCurrentSpatialLayer(0); context.SetTargetSpatialLayer(0); - // Frame 1 + // Frame 1. auto forwarded = ProcessVP9Packet(context, kMaxPictureId, 0); REQUIRE(forwarded); REQUIRE(forwarded->pictureId == kMaxPictureId); - // Frame 2 + // Frame 2. forwarded = ProcessVP9Packet(context, 0, 0); REQUIRE(forwarded); REQUIRE(forwarded->pictureId == 0); - // Frame 3 + // Frame 3. forwarded = ProcessVP9Packet(context, 1, 1); REQUIRE_FALSE(forwarded); } From b84ae54bf21395dc068dcb68f4a61911ebf595f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Cervi=C3=B1o?= Date: Thu, 19 Jan 2023 10:27:27 +0100 Subject: [PATCH 5/6] Apply comments in the PR --- worker/test/src/RTC/Codecs/TestVP8.cpp | 4 ++-- worker/test/src/RTC/Codecs/TestVP9.cpp | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/worker/test/src/RTC/Codecs/TestVP8.cpp b/worker/test/src/RTC/Codecs/TestVP8.cpp index 9ba5c42864..9675fb2e51 100644 --- a/worker/test/src/RTC/Codecs/TestVP8.cpp +++ b/worker/test/src/RTC/Codecs/TestVP8.cpp @@ -5,7 +5,7 @@ using namespace RTC; -constexpr uint16_t kMaxPictureId = (1 << 15) - 1; +constexpr uint16_t MaxPictureId = (1 << 15) - 1; SCENARIO("parse VP8 payload descriptor", "[codecs][vp8]") { @@ -307,7 +307,7 @@ SCENARIO("process VP8 payload descriptor", "[codecs][vp8]") context.SetTargetTemporalLayer(0); // Frame 1. - auto forwarded = ProcessPacket(context, kMaxPictureId, 0, 0); + auto forwarded = ProcessPacket(context, MaxPictureId, 0, 0); REQUIRE(forwarded); REQUIRE(forwarded->pictureId == 1); REQUIRE(forwarded->tl0PictureIndex == 1); diff --git a/worker/test/src/RTC/Codecs/TestVP9.cpp b/worker/test/src/RTC/Codecs/TestVP9.cpp index 7a43dcd73b..0a94ac1988 100644 --- a/worker/test/src/RTC/Codecs/TestVP9.cpp +++ b/worker/test/src/RTC/Codecs/TestVP9.cpp @@ -5,7 +5,7 @@ using namespace RTC; -constexpr uint16_t kMaxPictureId = (1 << 15) - 1; +constexpr uint16_t MaxPictureId = (1 << 15) - 1; Codecs::VP9::PayloadDescriptor* CreateVP9Packet( uint8_t* buffer, size_t bufferLen, uint16_t pictureId, uint8_t tlIndex) @@ -62,9 +62,9 @@ SCENARIO("process VP9 payload descriptor", "[codecs][vp9]") context.SetTargetSpatialLayer(0); // Frame 1. - auto forwarded = ProcessVP9Packet(context, kMaxPictureId, 0); + auto forwarded = ProcessVP9Packet(context, MaxPictureId, 0); REQUIRE(forwarded); - REQUIRE(forwarded->pictureId == kMaxPictureId); + REQUIRE(forwarded->pictureId == MaxPictureId); // Frame 2. forwarded = ProcessVP9Packet(context, 0, 0); From 291406f92c7a36328eaf071a32321656cbcc6616 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Thu, 19 Jan 2023 10:45:41 +0100 Subject: [PATCH 6/6] Add blank line before return --- worker/src/RTC/SeqManager.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/worker/src/RTC/SeqManager.cpp b/worker/src/RTC/SeqManager.cpp index 20dbaf9f70..47f1731f69 100644 --- a/worker/src/RTC/SeqManager.cpp +++ b/worker/src/RTC/SeqManager.cpp @@ -43,6 +43,7 @@ namespace RTC T SeqManager::Delta(const T lhs, const T rhs) { T value = (lhs > rhs) ? (lhs - rhs) : (MaxValue - rhs + lhs); + return value & MaxValue; }