Skip to content

Conversation

@jmillan
Copy link
Member

@jmillan jmillan commented Apr 5, 2023

Fixes #1037.

Keep dropped entries clean by removing those higher than this->maxInput.

This cleans both the Insert() and ClearDropped() methods. We remove those dropped entries which are from a previous sequence iteration.

STR with mediasoup-demo with single host:

Tab 1: roomId=loss&forceVP9=true&consume=false
Tab 2: roomId=loss&forceVP9=true&produce=false

In tab 2, select the preferred layers (0,0). Many packets are discarded by the codec

It takes few minutes until the seq number wraps ups. Wait for 1 or two wraps up. Vide in Tab 2 should be OK.

Fixes #1037.

Keep dropped entries clean by removing those higher than this->maxInput.
@jmillan jmillan requested a review from ibc April 5, 2023 13:31
@jmillan
Copy link
Member Author

jmillan commented Apr 5, 2023

@vpalmisano I've tested this with the STR that I provided and which is failing in V3. Can you give it a try?

@ibc
Copy link
Member

ibc commented Apr 5, 2023

I must confess that I don't understand whether the PR description describes an issue or the working solution.

@vpalmisano
Copy link
Contributor

Can you give it a try?

After 1h test I got no big loss events, so probably we could say that this was fixed :)

@jmillan
Copy link
Member Author

jmillan commented Apr 5, 2023

Test failing.. I was filtering by [SeqManager] in local..., checking..

jmillan added 2 commits April 5, 2023 17:46
A dropped input is part of the same sequence, hence set it to 'maxInput'
if the dropped value is higher than the current 'maxInput'.

Without this consideration we would be wrongly erasing dropped inputs
that are legitimally just the maximum input of the sequence stream.
@jmillan
Copy link
Member Author

jmillan commented Apr 5, 2023

There are some changes in the tests generated out of the Fuzzer output. I've reviewed them and they make sense.

jmillan added 3 commits April 6, 2023 14:38
Which has a linear complexity. Use lower_bound which has logarithmic
complexity.
@jmillan jmillan requested a review from ibc April 11, 2023 09:49
@jmillan
Copy link
Member Author

jmillan commented Apr 11, 2023

@ibc, no hurries. Reassigning for review since I've done some performance improvements. IMHO Input() method is now even more clear and understandable:

1: No need to consider dropped inputs if this->dropped is empty.
2: If 'input' is higher than this->maxInput then reassign this->maxInput and clean dropped inputs accordingly.
3: No need to consider dropped inputs if this->dropped is empty after cleanup.
4: Nothing to do if 'input' is within this->dropped.
5: Calculate new base if there are dropped inputs lower than 'input'.

Copy link
Member

@ibc ibc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Is it fuzzer safe?

@jmillan
Copy link
Member Author

jmillan commented Apr 12, 2023

LGTM. Is it fuzzer safe?

I didn't try honestly. let me try it tomorrow morning.

@jmillan
Copy link
Member Author

jmillan commented Apr 13, 2023

LGTM. Is it fuzzer safe?

I didn't try honestly. let me try it tomorrow morning.

We are safe.

@jmillan jmillan merged commit 937bb6c into v3 Apr 13, 2023
@jmillan jmillan deleted the SeqManagerFix branch April 13, 2023 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

High packet loss after upgrading to 3.11.16 version

4 participants