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 support for local priority in presets #677

Merged
merged 11 commits into from
May 3, 2023
Merged

Conversation

lukasIO
Copy link
Contributor

@lukasIO lukasIO commented Apr 28, 2023

In order to satisfy TS I had to update to v5.0.0 which makes this PR a bit unwieldy.
TS 5.0 changes are only to make type imports explicit (we had this setting before already, but 5.0 caught a lot more cases)

This PR adds support to construct video and audio presets with an additional priority option which (on supported browsers) tells the browser about how to distribute available resources (cpu + network) between different tracks.

  • deprecates audioBitrate setting on TrackPublishDefaults in favor of audioPreset that now includes priority
  • by default audio tracks will get the priority high, screen share video tracks medium and camera video tracks low

@changeset-bot
Copy link

changeset-bot bot commented Apr 28, 2023

🦋 Changeset detected

Latest commit: 221f819

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
livekit-client Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@lukasIO lukasIO requested a review from davidzhao April 28, 2023 08:16
Copy link
Member

@davidzhao davidzhao left a comment

Choose a reason for hiding this comment

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

lg, how well does the priorities work in practice? does it show a visible improvement when using NLC?

super();
this.pc = new RTCPeerConnection(config);
this.pc = isChromiumBased()
Copy link
Member

Choose a reason for hiding this comment

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

should this be

isChromiumBased() ? new RTCPeerConnection(config, mediaConstraints) : new RTCPeerConnection(config)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, of course, thanks!

encodings = [
{
maxBitrate: opts.audioBitrate,
maxBitrate: opts.audioPreset?.maxBitrate ?? opts.audioBitrate,
Copy link
Member

Choose a reason for hiding this comment

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

should we avoid setting this field if left as undefined? I know we had issues with FF before with leaving FPS as undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opts gets merged with trackPublishDefaults in publishTrack so it should always be defined!

@lukasIO
Copy link
Contributor Author

lukasIO commented May 3, 2023

how well does the priorities work in practice? does it show a visible improvement when using NLC?

just did some tests on chrome:
the target bitrate of lower priority layers changes more significantly when others with higher priority are published as well while, in comparison, layers with higher priority tend to have less fluctuations in the target bitrate.

@lukasIO lukasIO mentioned this pull request May 3, 2023
1 task
Copy link
Member

@davidzhao davidzhao left a comment

Choose a reason for hiding this comment

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

lg!

@lukasIO lukasIO merged commit c7b3fc0 into main May 3, 2023
1 check passed
@lukasIO lukasIO deleted the lukas/track-priority branch May 3, 2023 18:45
@github-actions github-actions bot mentioned this pull request May 3, 2023
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.

None yet

2 participants