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.

Sign up for GitHub

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

Fix wrong PictureID rolling over in VP9 and VP8 #984

Merged
merged 6 commits into from
Jan 19, 2023
Merged

Fix wrong PictureID rolling over in VP9 and VP8 #984

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ee5d68d
Fix wrong PictureID rollover in VP9 and VP8
jcague Jan 17, 2023
f5786c4
Fix wrong delta calculation inside SeqManager
jcague Jan 18, 2023
43ad91a
Fix lint issues
jcague Jan 18, 2023
7bb6198
Apply comments in the PR
jcague Jan 19, 2023
b84ae54
Apply comments in the PR
jcague Jan 19, 2023
291406f
Add blank line before return
ibc Jan 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion worker/include/RTC/SeqManager.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@

namespace RTC
{
template<typename T, unsigned int N = 0>
// T is the base type (uint16_t, uint32_t, ...).
// N is the max number of bits used in T.
template<typename T, uint8_t N = 0>
class SeqManager
{
public:
Expand All @@ -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);
Expand Down
37 changes: 22 additions & 15 deletions worker/src/RTC/SeqManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,39 +7,46 @@

namespace RTC
{
template<typename T, unsigned int N>
template<typename T, uint8_t N>
bool SeqManager<T, N>::SeqLowerThan::operator()(const T lhs, const T rhs) const
{
return ((rhs > lhs) && (rhs - lhs <= MaxValue / 2)) ||
((lhs > rhs) && (lhs - rhs > MaxValue / 2));
}

template<typename T, unsigned int N>
template<typename T, uint8_t N>
bool SeqManager<T, N>::SeqHigherThan::operator()(const T lhs, const T rhs) const
{
return ((lhs > rhs) && (lhs - rhs <= MaxValue / 2)) ||
((rhs > lhs) && (rhs - lhs > MaxValue / 2));
}

template<typename T, unsigned int N>
template<typename T, uint8_t N>
const typename SeqManager<T, N>::SeqLowerThan SeqManager<T, N>::isSeqLowerThan{};

template<typename T, unsigned int N>
template<typename T, uint8_t N>
const typename SeqManager<T, N>::SeqHigherThan SeqManager<T, N>::isSeqHigherThan{};

template<typename T, unsigned int N>
template<typename T, uint8_t N>
bool SeqManager<T, N>::IsSeqLowerThan(const T lhs, const T rhs)
{
return isSeqLowerThan(lhs, rhs);
}

template<typename T, unsigned int N>
template<typename T, uint8_t N>
bool SeqManager<T, N>::IsSeqHigherThan(const T lhs, const T rhs)
{
return isSeqHigherThan(lhs, rhs);
}

template<typename T, unsigned int N>
template<typename T, uint8_t N>
T SeqManager<T, N>::Delta(const T lhs, const T rhs)
{
T value = (lhs > rhs) ? (lhs - rhs) : (MaxValue - rhs + lhs);
return value & MaxValue;
ibc marked this conversation as resolved.
Show resolved Hide resolved
}

template<typename T, uint8_t N>
void SeqManager<T, N>::Sync(T input)
{
// Update base.
Expand All @@ -52,7 +59,7 @@ namespace RTC
this->dropped.clear();
}

template<typename T, unsigned int N>
template<typename T, uint8_t N>
void SeqManager<T, N>::Drop(T input)
{
// Mark as dropped if 'input' is higher than anyone already processed.
Expand All @@ -62,13 +69,13 @@ namespace RTC
}
}

template<typename T, unsigned int N>
template<typename T, uint8_t N>
void SeqManager<T, N>::Offset(T offset)
{
this->base = (this->base + offset) & MaxValue;
}

template<typename T, unsigned int N>
template<typename T, uint8_t N>
bool SeqManager<T, N>::Input(const T input, T& output)
{
auto base = this->base;
Expand Down Expand Up @@ -105,8 +112,8 @@ namespace RTC

output = (input + base) & MaxValue;

T idelta = input - this->maxInput;
T odelta = output - this->maxOutput;
T idelta = SeqManager<T, N>::Delta(input, this->maxInput);
T odelta = SeqManager<T, N>::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().
Expand All @@ -121,13 +128,13 @@ namespace RTC
return true;
}

template<typename T, unsigned int N>
template<typename T, uint8_t N>
T SeqManager<T, N>::GetMaxInput() const
{
return this->maxInput;
}

template<typename T, unsigned int N>
template<typename T, uint8_t N>
T SeqManager<T, N>::GetMaxOutput() const
{
return this->maxOutput;
Expand All @@ -136,7 +143,7 @@ namespace RTC
// Explicit instantiation to have all SeqManager definitions in this file.
template class SeqManager<uint8_t>;
template class SeqManager<uint16_t>;
template class SeqManager<uint16_t, 15>; // For PictureID (15bits).
template class SeqManager<uint16_t, 15>; // For PictureID (15 bits).
template class SeqManager<uint32_t>;

} // namespace RTC
4 changes: 3 additions & 1 deletion worker/test/src/RTC/Codecs/TestVP8.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

using namespace RTC;

constexpr uint16_t kMaxPictureId = (1 << 15) - 1;
ibc marked this conversation as resolved.
Show resolved Hide resolved

SCENARIO("parse VP8 payload descriptor", "[codecs][vp8]")
{
SECTION("parse payload descriptor")
Expand Down Expand Up @@ -305,7 +307,7 @@ SCENARIO("process VP8 payload descriptor", "[codecs][vp8]")
context.SetTargetTemporalLayer(0);

// Frame 1
ibc marked this conversation as resolved.
Show resolved Hide resolved
auto forwarded = ProcessPacket(context, 32767, 0, 0);
auto forwarded = ProcessPacket(context, kMaxPictureId, 0, 0);
REQUIRE(forwarded);
REQUIRE(forwarded->pictureId == 1);
REQUIRE(forwarded->tl0PictureIndex == 1);
Expand Down
6 changes: 4 additions & 2 deletions worker/test/src/RTC/Codecs/TestVP9.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -60,9 +62,9 @@ SCENARIO("process VP9 payload descriptor", "[codecs][vp9]")
context.SetTargetSpatialLayer(0);

// Frame 1
ibc marked this conversation as resolved.
Show resolved Hide resolved
auto forwarded = ProcessVP9Packet(context, 32767, 0);
auto forwarded = ProcessVP9Packet(context, kMaxPictureId, 0);
REQUIRE(forwarded);
REQUIRE(forwarded->pictureId == 32767);
REQUIRE(forwarded->pictureId == kMaxPictureId);

// Frame 2
ibc marked this conversation as resolved.
Show resolved Hide resolved
forwarded = ProcessVP9Packet(context, 0, 0);
Expand Down
70 changes: 57 additions & 13 deletions worker/test/src/RTC/TestSeqManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@

using namespace RTC;

constexpr uint16_t kMaxNumberFor15Bits = (1 << 15) - 1;

template<typename T>
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)
{
}

Expand All @@ -19,9 +21,10 @@ struct TestSeqManagerInput
bool sync{ false };
bool drop{ false };
T offset{ 0 };
int64_t maxInput{ -1 };
};

template<typename T, unsigned int N>
template<typename T, uint8_t N>
void validate(SeqManager<T, N>& seqManager, std::vector<TestSeqManagerInput<T>>& inputs)
{
for (auto& element : inputs)
Expand All @@ -44,6 +47,9 @@ void validate(SeqManager<T, N>& seqManager, std::vector<TestSeqManagerInput<T>>&

// 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()));
}
}
}
}
Expand Down Expand Up @@ -545,20 +551,58 @@ SCENARIO("SeqManager", "[rtc]")
// clang-format off
std::vector<TestSeqManagerInput<uint16_t>> 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<uint16_t, 15> seqManager;
validate(seqManager, inputs);
}

SECTION("should update all values during multiple roll overs")
{
// clang-format off
std::vector<TestSeqManagerInput<uint16_t>> inputs =
{
{ 0, 1, true, false, 0, 0 },
};
for (uint16_t j = 0; j < 100; ++j) {
for (uint16_t i = 1; i < std::numeric_limits<uint16_t>::max(); ++i) {
uint16_t output = i + 1;
inputs.push_back({ i, output, false, false, 0, i });
}
}
// clang-format on

SeqManager<uint16_t> seqManager;
validate(seqManager, inputs);
}

SECTION("should update all values during multiple roll overs (15 bits range)")
{
// clang-format off
std::vector<TestSeqManagerInput<uint16_t>> 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<uint16_t, 15> seqManager;
validate(seqManager, inputs);
}
}