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

Add a new algorithm to allocate better bandwidth in Single PC #1296

Merged
merged 6 commits into from
Sep 18, 2018

Conversation

jcague
Copy link
Contributor

@jcague jcague commented Sep 13, 2018

Description

The way we were distributing available bandwidth among streams in the same single PC was suboptimal for some cases. This PR aims to improve the algorithm when we use Simulcast and Slideshow.

  • It needs and includes Unit Tests

Changes in Client or Server public APIs

Not needed.

[] It includes documentation for these changes in /doc.

Copy link
Contributor

@lodoyun lodoyun left a comment

Choose a reason for hiding this comment

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

Looks good to me! Just a couple of comments and a possible bug that should be inoffensive anyway.

remaining_bitrate -= bitrate;
remaining_streams--;
});
distributor_->distribute(chead->getREMBBitRate(), chead->getSSRC(), streams, transport);
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to have this elsewhere

@@ -0,0 +1,35 @@
/*
* MaxVideoBWDistributor.cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have this just to keep the code of the previous algorithm? Do you foresee using this algorithm in any case. I'm ok with keeping it but almost every time we've done this in the past we ended up with code that is only updated when the tests fail but is never used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I completely agree, I will remove it once we're 100% sure the new code works for all cases.

uint32_t remaining_avg_bitrate = remaining_bitrate / remaining_streams;
uint32_t bitrate = std::min(target_bitrate, remaining_avg_bitrate);
uint32_t remb = std::min(max_bitrate, remaining_avg_bitrate);
uint32_t bitrate_to_transfer = std::max(bitrate - target_bitrate * 1.2, 0.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need bitrate_to_transfer it will always be 0 unless we have a bug somewhere else. As far as I can tell bitrate is enough to update remaining_bitrate and distribute the bitrate correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

@jcague jcague merged commit 460f384 into lynckia:master Sep 18, 2018
@jcague jcague deleted the update/bw_distribution_single_pc branch September 18, 2018 09:39
Arri98 pushed a commit to Arri98/licode that referenced this pull request Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants