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

Implement target connection bandwidth #1736

Merged
merged 4 commits into from
Jul 13, 2021

Conversation

lodoyun
Copy link
Contributor

@lodoyun lodoyun commented Jul 12, 2021

Description

This PR implements a way to set a target bandwidth for connections in Licode. This target can be set with as part of a strategy as connectionTargetBw or by calling room.setConnectionTargetBandwidth in the client.
The main use of target bitrate is to provide a goal or a target for the bandwidth estimation. Licode will generate padding to try to reach the target bitrate.

[] 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 have a question: I understand that we can enable either it by calling setConnectionTargetBw(), by setting connectionTargetBw in the strategy, or by passing it as an option when publishing or subscribing the first stream, am I right? I'm not sure I like the third option.

And I guess that the last API that is called updates the value, with no other preference or relationship between APIs.

I like having it

@@ -183,6 +186,7 @@ int64_t RtpPaddingManagerHandler::getTotalTargetBitrate() {
}
target_bitrate += media_stream->getTargetVideoBitrate();
});
target_bitrate = std::max(target_bitrate, static_cast<int64_t>(connection_->getConnectionTargetBw()));
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -23,6 +23,7 @@ class Client extends EventEmitter {
this.ioThreadPool = ioThreadPool;
this.singlePc = singlePc;
this.streamPriorityStrategy = Client._getStreamPriorityStrategy(streamPriorityStrategy);
this.connectionTargetBw = options.connectionTargetBw || 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'm not sure I like this third way of adding it. Shouldn't we obtain the value from the streamPriorityStrategy?

this.connectionTargetBw = this.streamPriorityStrategy.connectionTargetBw;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value may have been updated, erizoController sends the last version with every addPublisher or addSubscriber. Here we're accounting for the case where this client was not present in this erizoJS

@lodoyun lodoyun merged commit 802e42b into lynckia:master Jul 13, 2021
@lodoyun lodoyun deleted the add/TargetBitrateForConnection branch July 13, 2021 09:54
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