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

Automatic subscription management based on video track visibility #588

Merged
merged 9 commits into from
Aug 17, 2023

Conversation

lukasIO
Copy link
Contributor

@lukasIO lukasIO commented Aug 14, 2023

This adds automatic subscription management based on visibility of the VideoTrack component.

It allows for meetings with a lot of users, where subscription limits might become a constraint.

  • setSubscribed(true) is immediate once the element comes into view
  • setSubscribed(false) is debounced with 3s right now (happy to change that around and/or make it configurable?)
  • once this is enabled adaptiveStream doesn't provide any benefit any more (afaict)

Open questions:

  • What should be the default value for manageSubscription - true or false?
  • Do we want to merge this already even if audio muxing isn't available yet? (I guess we'll run into the same constraints with audio subscriptions, as we cannot use the same approach for that)

@changeset-bot
Copy link

changeset-bot bot commented Aug 14, 2023

⚠️ No Changeset found

Latest commit: e28b128

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 14, 2023

size-limit report 📦

Path Size
LiveKitRoom only 1.9 KB (+1.2% 🔺)
LiveKitRoom with VideoConference 29.32 KB (+1.17% 🔺)
All exports 40.16 KB (+0.84% 🔺)

@Ocupe
Copy link
Contributor

Ocupe commented Aug 15, 2023

Looking good 👍

  • What should be the default value for manageSubscription - true or false?
    Do you mean the useTracks option onlySubscribed? I was wondering about that too. Not sure if we need to change the default, but we should definitely highlight the option in the docs with some examples of how and when to use it.
  • Do we want to merge this already even if audio muxing isn't available yet? (I guess we'll run into the same constraints with audio subscriptions, as we cannot use the same approach for that)

As far as I can see, there is no change to the public API. So we could merge it and get some real world feedback if it works as expected for the "normal" (not so many participants) use cases. If something comes up, we can iterate/roll back without breaking the API.

@@ -1,4 +1,4 @@
import type { Participant } from 'livekit-client';
import { type Participant } from 'livekit-client';
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, does this offer any advantages (bundle size, tree shaking) over the other type of import?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it's just what VSCode does when auto importing 🤷

Comment on lines 1 to 5
import { isLocal } from '@livekit/components-core';
import { Track } from 'livekit-client';
import { RemoteTrackPublication, Track } from 'livekit-client';
import * as React from 'react';
import { useTracks } from '../hooks';
import { AudioTrack } from './participant/AudioTrack';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { isLocal } from '@livekit/components-core';
import { Track } from 'livekit-client';
import { RemoteTrackPublication, Track } from 'livekit-client';
import * as React from 'react';
import { useTracks } from '../hooks';
import { AudioTrack } from './participant/AudioTrack';
import { isLocal } from '@livekit/components-core';
import type { RemoteTrackPublication } from 'livekit-client';
import { Track } from 'livekit-client';
import * as React from 'react';
import { useTracks } from '../hooks';
import { AudioTrack } from './participant/AudioTrack';

examples/nextjs/pages/minimal.tsx Outdated Show resolved Hide resolved
@davidzhao
Copy link
Member

What should be the default value for manageSubscription - true or false?

I think we can default to false, but enable in our sample apps for now. then once it's gotten some real-world testing, we can enable?

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.

lgtm

@@ -19,7 +20,13 @@ import { AudioTrack } from './participant/AudioTrack';
export function RoomAudioRenderer() {
const tracks = useTracks([Track.Source.Microphone, Track.Source.ScreenShareAudio], {
Copy link
Member

Choose a reason for hiding this comment

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

can we also make a change here to subscribe to all audio tracks, even ones tagged as unknown?

we've seen users getting confused when they couldn't playback tracks that are not tagged 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.

currently there's no way to filter track references based on kind.
an unknown source could also be a video track. We would probably have to add kind to the track reference definition to make that happen.

React.useEffect(() => {
tracks.forEach((track) => (track.publication as RemoteTrackPublication).setSubscribed(true));
}, [tracks]);

return (
<div style={{ display: 'none' }}>
{tracks.map((trackRef) => (
Copy link
Member

Choose a reason for hiding this comment

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

I guess this handles unsubscribed tracks automatically? that's cool

@lukasIO lukasIO merged commit 243e6bf into main Aug 17, 2023
2 checks passed
@lukasIO lukasIO deleted the lukas/selective-pagination branch August 17, 2023 08:01
@lukasIO lukasIO restored the lukas/selective-pagination branch August 17, 2023 08:04
lukasIO added a commit that referenced this pull request Aug 17, 2023
Co-authored-by: Jonas Schell <[email protected]>
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.

3 participants