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.

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 old buffer items cleanup in RTC::RateCalculator #830

Merged
merged 4 commits into from
Jun 9, 2022
Merged

Fix old buffer items cleanup in RTC::RateCalculator #830

merged 4 commits into from
Jun 9, 2022

Conversation

dsdolzhenko
Copy link
Contributor

This is the fix for the issue described in #818 and the infinite loop found in #819 (the first attempt to fix #818)

`RemoveOldData` is supposed to remove old buffer items
It fixes the case when `nowMs` equals `oldestItemStartTime` + `windowSizeMs`.
@dsdolzhenko
Copy link
Contributor Author

@rtctt Do you have a chance to verify the fix in the setup you've described in #819?

@ibc
Copy link
Member

ibc commented Jun 7, 2022

Thanks. Could you please tell the differences between this PR and the previous one? Specifically to see how this solves the infinite loop.

@dsdolzhenko
Copy link
Contributor Author

The commit adds a unit test and fixes the case @rtctt described in the previous PR.

Basically, when we receive a packet with timestamp greater or equal than current newestItemStartTime + windowSizeMs we reset the buffer.

I believe the case isn't properly handled in the current version either; even though it doesn't gets into an infinite loop, it calculates bitrate wrong.

@gnauhtt
Copy link
Contributor

gnauhtt commented Jun 8, 2022

@dsdolzhenko I have checked this PR and there is no problem. I understand what you mean, reset the buffer instead of removing the last item to avoid getting stuck in a loop. 👍

@jmillan
Copy link
Member

jmillan commented Jun 8, 2022

I believe the case isn't properly handled in the current version either; even though it doesn't gets into an infinite loop, it calculates bitrate wrong.

What's wrong?

@dsdolzhenko
Copy link
Contributor Author

What's wrong?

I suppose, in the edge case, when the next packet timestamp is exactly windowSizeMs away from newestItemStartTime already accumulated stats must be discarded. So, for example, if we receive 5 bits every 1000ms, the bitrate must be 40 rather than 80.

Copy link
Member

@jmillan jmillan left a comment

Choose a reason for hiding this comment

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

👍

@jmillan
Copy link
Member

jmillan commented Jun 9, 2022

Thanks @dsdolzhenko

@ibc ibc merged commit 908a9ab into versatica:v3 Jun 9, 2022
@ibc
Copy link
Member

ibc commented Jun 9, 2022

Thanks a lot, merged and released in 3.9.17.

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.

RTC::RateCalculator warnings about overflowed calculation buffer
4 participants