-
Notifications
You must be signed in to change notification settings - Fork 21
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
fix(screensharing): picker performance and UI improvements #1003
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ShGKme
force-pushed
the
fix/screensharing-performance
branch
from
January 19, 2025 22:57
7239513
to
3b4a827
Compare
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
ShGKme
force-pushed
the
fix/screensharing-performance
branch
from
January 31, 2025 12:30
3b4a827
to
fd588af
Compare
ShGKme
force-pushed
the
fix/screensharing-performance
branch
from
January 31, 2025 12:54
fd588af
to
8281c85
Compare
ShGKme
force-pushed
the
fix/screensharing-performance
branch
from
January 31, 2025 13:21
8281c85
to
d75c5a2
Compare
Antreesy
approved these changes
Jan 31, 2025
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 on Windows and WSL/Wayland
ShGKme
force-pushed
the
fix/screensharing-performance
branch
from
January 31, 2025 15:37
d75c5a2
to
4566538
Compare
ShGKme
changed the title
fix(screensharing): performance and ux improvements
fix(screensharing): picker performance and UI improvements
Jan 31, 2025
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
ShGKme
force-pushed
the
fix/screensharing-performance
branch
from
January 31, 2025 19:53
4566538
to
9659757
Compare
Applied suggestions from review. Replaces |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
☑️ Resolves
Performance issue 1: huge memory leak on quick cancel
If a user cancels screensharing before live previews are initialized, they won't be released in
onBeforUnmount
. The stream is not ready yet inonBeforUnmount
, so we cannot release it yet.Now checking that when stream is got, the video element to bind it to still exists and release otherwise.
How to test
main
branchPerformance issue 2: sources list update is resource consuming
The list of sources is updated every second. It makes a short freeze every second. Then more sources are available - then more is the freeze and resource usage.
We may increase the interval to, e.g., 5 seconds.
But I'd consider an alternative - do it on visibility change. If a user goes from Talk Desktop to another window to open/close is and then goes back - it's refreshed.
The only case that is not covered here, if some window is closed on it's own. But the user just won't be able to share it in the end.
Performance issue 3: live preview is resource consuming
Live previews for many screens and windows may have a huge performance impact.
Making live preview optional.
Other changes
🖼️ Screenshots
🏚️ Before
🏡 After
Single source available
Only screens (no windows)
Wayland emulation
Video
screensharing.mp4