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

Update dimensions when the remote track is detached #766

Merged

Conversation

burzomir
Copy link
Contributor

@burzomir burzomir commented Jul 5, 2023

@davidzhao @lukasIO

Use case for this change:
There are two video elements in the UI. The first is small (480x320) and placed in a participant grid. The second is big (1280x720) and only displays the active speaker. When no one is speaking, then the second video element is removed. In this case, I want to use only the low-resolution simulcast layer. Currently, simulcast layers change only on resize and visibility change events or when a remote track is attached.

@changeset-bot
Copy link

changeset-bot bot commented Jul 5, 2023

🦋 Changeset detected

Latest commit: 90ce2fa

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
Copy link
Contributor

lukasIO commented Jul 5, 2023

hi @burzomir ,
thanks for the PR, this sounds like a reasonable change.

Instead of calling it in detach could you add it to stopObservingElementInfo instead? We have updateVisibility there already.

@burzomir
Copy link
Contributor Author

burzomir commented Jul 5, 2023

@lukasIO I've updated the code. Please, take a look.

@lukasIO lukasIO requested a review from davidliu July 5, 2023 13:56
Copy link
Contributor

@lukasIO lukasIO left a comment

Choose a reason for hiding this comment

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

thanks, pulling in @davidliu for additional review, but looks good to me!

Copy link
Contributor

@davidliu davidliu left a comment

Choose a reason for hiding this comment

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

One small comment, but LGTM.

elementInfos are already filtered inside of stopObservingElementInfo method
@lukasIO
Copy link
Contributor

lukasIO commented Jul 6, 2023

this is looking great, thanks @burzomir !

@lukasIO lukasIO merged commit 5b1e48c into livekit:main Jul 6, 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.

3 participants