-
Notifications
You must be signed in to change notification settings - Fork 508
fix(media): release devices and workers after the call #15989
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
Conversation
danxuliu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works. But...
Most of this are work arounds for the real problem, which is that useDevices is being used by the local media buttons.
The devices mixin, which the useDevices composable comes from, was meant to be used only in the settings and media devices dialogs. It handled devices in a completly separate way from the devices used in the call (except for the fact that, due to historical reasons for browser compatibility, only a single device of each kind could be active at the same time, so changing the device in the dialog also changed it indirectly in the call through the MediaDevicesManager). Due to that, it requests its own audio and video streams and has its own virtual background handler to be able to show the previews.
When the composable started to be used in the local media buttons it started to request those streams when the buttons were mounted, and then it never freed them.
Although the streams are now freed they should not be requested in first place, because they are never used (and neither the virtual background) and it is just duplicating the streams used in the call.
Having said all that, given the tight timing I guess we can merge it for now (although it would be good to address the comment) and do a proper fix later...
Regarding freeing the virtual background worker that is indeed a legit fix (I would have sworn that it was loaded and then not freed for performance reasons during the initialization, although after testing it loading the model is quite fast 🤷 Not sure if I misremember, it was wrongly tested in the past, or it was indeed an issue when it was introduced back in the day but the loading time improved since then), and I think it would be good to have it too in the stable branches.
danxuliu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicks, but fine even with them.
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
330724a to
a5eefcf
Compare
|
/backport to stable32 |
☑️ Resolves
fix(media): [wip] move init logic from lifecycle hooks:
fix(media): release media devices when not used by app:
fix(media): reattach video stream, when devices already initialized
fix(media): terminate background workers, when not used
JitsiStreamBackgroundEffectworker was never terminated (can be checked on stable branches at DevTools -> Memory -> VM instances - we always have 1-2 of them before/after the call🖌️ UI Checklist
🖼️ Screenshots / Screencasts
No visual changes, see tab memory consumption
🚧 Tasks
🏁 Checklist