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 "max" option for strategy #1713

Merged
merged 8 commits into from
May 24, 2021
Merged

Conversation

lodoyun
Copy link
Contributor

@lodoyun lodoyun commented May 21, 2021

Description
This PR adds a new option in the priority configuration -> max. This option means that the distributor will always try to assign the defined maxVideoBW for that stream. In order to have that work correctly, this PR also adds new functionality: subscribers will, by default, be assigned the maxVideoBW of the publisher. If a different maxVideoBW is assigned directly to the subscription, the minimum between the new maxVideoBW and the publishers' one will be assigned.

  • It needs and includes Unit Tests

Changes in Client or Server public APIs

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

Copy link
Contributor

@jcague jcague left a comment

Choose a reason for hiding this comment

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

LGTM I just left some minor comments and questions.

@@ -719,6 +724,14 @@ void MediaStream::setSlideShowMode(bool state) {
notifyUpdateToHandlers();
}

void MediaStream::setTargetIsMaxVideoBW(bool state) {
ELOG_DEBUG("%s targetIsMaxVideoBw: %u", toLog(), state);
if (target_is_max_video_bw_ == state) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we are going to be calling this every time the bw estimation algorithm runs. It's true that, in this particular case, it's an atomic and we're not doing anything else like updating stats. As a result, I'm not sure if this is actually an optimization vs updating the value every time. I somewhat like the pattern but I can be convinced to remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt that this optimization worths making the function not threadsafe.

@@ -719,6 +724,14 @@ void MediaStream::setSlideShowMode(bool state) {
notifyUpdateToHandlers();
}

void MediaStream::setTargetIsMaxVideoBW(bool state) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a TODO here to remove this new state once we have a connection maximum bandwidth limit.

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 don't think we would necessarily have to remove this, I think it adds a different option that is helpful when you have connection maximum bandwidth limit

@@ -732,6 +745,11 @@ uint32_t MediaStream::getTargetVideoBitrate() {
uint32_t bitrate_from_max_quality_layer = getBitrateFromMaxQualityLayer();

uint32_t target_bitrate = max_bitrate;

if (target_is_max_video_bw_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could max_bitrate be 0 because it has not been initialized? Maybe we should return kInitialBitrate in that case, as we do below.

@lodoyun lodoyun merged commit 311f046 into lynckia:master May 24, 2021
@lodoyun lodoyun deleted the add/maxOptionForStrategy branch May 24, 2021 15:41
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