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

[core] Shared moving average calc for RCV and SND buffers #1348

Merged
merged 3 commits into from
Jun 17, 2020

Conversation

maxsharabayko
Copy link
Collaborator

Both Receiver and Sender buffers share the same logic of the moving average calculation of the buffer state (packets, bytes, timespan).
This PR moves this logic into a separate class, which is used as aggregation by both buffers.

Didn't use inheritance to avoid virtual tables.

Fixes #1272

@maxsharabayko maxsharabayko added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels Jun 15, 2020
@maxsharabayko maxsharabayko added this to the v1.5.0 - Sprint 17 milestone Jun 15, 2020
@maxsharabayko maxsharabayko requested a review from ethouris June 15, 2020 10:04
@maxsharabayko maxsharabayko marked this pull request as draft June 15, 2020 13:22
@maxsharabayko maxsharabayko force-pushed the hotfix/buffer-stats branch 2 times, most recently from 5585e53 to 4e49708 Compare June 15, 2020 16:46
@maxsharabayko
Copy link
Collaborator Author

Integer-based calculation of the moving average does not work well.
The image below shows different averaging methods on the receiver buffer timespan. 1 Mbps streaming rate was used to collect samples.
Due to the loss of precision the current SRT averaging method is underestimating the actual value.

Legend

  • Instant Values - Instant values of the receiver buffer timespan
  • MAvg elapsed/1s (int) - the current SRT moving average calculation
  • MAvg elapsed/1s (float) - same as above, but using floats instead of ints
  • MAvg 1/8 (float) - moving average with 1/8 for new value and 7/8 for old value.

image


if (elapsed_ms > 1000)
{
// No sampling in last 1 sec, initialize average
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is assumption, that now > 1000 will always be true, safe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is unlikely to be true. Most of the time elapsed_ms is below 1s.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean, a situation that now would ship a value less than 1000. Then when you subtract 0 from it you'll get the same value, and already less than 1000. If not, then the "too long time ago" situation would be handled together with "didn't initialize before".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

now would ship a value less than 1000

now is the current time. How can it ship a value of less than 1000?
Or do you mean now - m_tsLastSamplingTime?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, I meant now exactly - so that the result of the value compared against 1000 would be less than 1000. I personally think that even if you have a steady clock that starts from 0 at the moment when the system starts, no system in the world is capable of running the application in less than 1s after bootup.

Copy link
Collaborator

@ethouris ethouris left a comment

Choose a reason for hiding this comment

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

Consider ^

@maxsharabayko maxsharabayko marked this pull request as ready for review June 16, 2020 15:35
@maxsharabayko maxsharabayko merged commit a78a999 into Haivision:master Jun 17, 2020
@maxsharabayko maxsharabayko deleted the hotfix/buffer-stats branch June 17, 2020 09:25
@maxsharabayko
Copy link
Collaborator Author

Saving scripts and data used for analysis.
python-analyse.zip

@mbakholdina mbakholdina modified the milestones: v1.5.0 - Sprint 17, v1.4.2 Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Statistics always return instantaneous value for send and recv buffers
3 participants