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

feat: screen sharing #595

Merged
merged 11 commits into from
Apr 10, 2024
Merged

feat: screen sharing #595

merged 11 commits into from
Apr 10, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Apr 3, 2024

☑️ Resolves

🖼️ Screenshots

image

  • With live previews (except on Wayland)
  • Supports "All screens with audio". All of the other options don't support audio streaming.
  • When there is only one screen, the first option is renamed to "Entire screen with audio"
  • On Mac - opens settings to allow screensharing if it is not allowed

Windows / Mac / Linux (xorg)

Windows.mp4

Linux (Wayland)

Wayland with PipeWire has a different workflow:

  • Requesting all available sources (screens, windows) - triggers native system dialog to choose what to share
  • Showing the stream for the selected source also triggers the system dialog
  • So live preview for all options is not possible, each preview triggers the dialog
  • Instead, only one static image preview is shown for the selected source
  • Starting screensharing also triggers the dialog, and the second selected source is actually streamed
  • Unfortunately, this is a known issue with double dialog. There is the same issue in Chromium
Linux-Wayland.mp4

🚧 Tasks

  • Add app:getDesktopCaptureSources method to get the list of available sources on backend
  • Add DesktopMediaSourceDialog with the list of available sources to share (screens, windows)
  • Add OCA.Talk.Desktop.getDesktopMediaSource to invoke the dialog and get the selected source
  • On Talk side, use OCA.Talk.Desktop.getDesktopMediaSource instead of unavailable navigator.mediaDevices.getDisplayMedia (desktop: add screen sharing support spreed#12003)
  • Polishing

@ShGKme ShGKme added enhancement New feature or request 2. developing labels Apr 3, 2024
@ShGKme ShGKme self-assigned this Apr 3, 2024
@ShGKme ShGKme force-pushed the feat/screen-sharing branch 3 times, most recently from e4d7cab to cf5df33 Compare April 4, 2024 16:05
@ShGKme ShGKme marked this pull request as ready for review April 4, 2024 16:07
@ShGKme ShGKme force-pushed the feat/screen-sharing branch 2 times, most recently from 9035afd to e8e81ce Compare April 5, 2024 07:02
@ShGKme
Copy link
Contributor Author

ShGKme commented Apr 5, 2024

Rebased onto main to resolve README conflict

@nickvergessen
Copy link
Member

nickvergessen commented Apr 5, 2024

I receive:

Error occurred in handler for 'app:getDesktopCapturerSources': TypeError: systemPreferences.getMediaAccessStatus is not a function
    at /home/nickv/Nextcloud/TalkDesktop/talk-desktop/.webpack/main/index.js:48014:24
    at WebContents.<anonymous> (node:electron/js2c/browser_init:2:78183)
    at WebContents.emit (node:events:514:28)

On linux.

https://www.electronjs.org/docs/latest/api/system-preferences#systempreferencesgetmediaaccessstatusmediatype-windows-macos
Seems expected

@nickvergessen
Copy link
Member

Works now, but there is some "stacking issue"

I have to "pick" a screen 3 times. After

  1. The "full screen with audio" frame appears
  2. The next screen option appears
  3. The video in 1. appears
Before 1 Before 2 Before 3 After 3
Bildschirmfoto vom 2024-04-05 09-34-07 Bildschirmfoto vom 2024-04-05 09-34-16 Bildschirmfoto vom 2024-04-05 09-34-33 Bildschirmfoto vom 2024-04-05 09-34-41

🗣️ At this moment (After 3) I have a Audio Feedback Loop (is it avoidable to play the shared audio?)

After picking either option I have to accept the screenshare again (but that is also the case with normal Chromium)
Bildschirmfoto vom 2024-04-05 09-36-02

After selecting that the screenshare menu is "open" (should be closed)
Bildschirmfoto vom 2024-04-05 09-36-17

@ShGKme ShGKme marked this pull request as draft April 5, 2024 09:04
@ShGKme
Copy link
Contributor Author

ShGKme commented Apr 5, 2024

The solution should be different for Wayland 🥲

@ShGKme ShGKme force-pushed the feat/screen-sharing branch from 4e86c9c to 3f97ee0 Compare April 5, 2024 11:30
src/main.js Outdated Show resolved Hide resolved
@ShGKme ShGKme force-pushed the feat/screen-sharing branch 4 times, most recently from 898b5b2 to c8aca91 Compare April 5, 2024 20:33
@ShGKme ShGKme force-pushed the feat/screen-sharing branch from c8aca91 to 819148b Compare April 5, 2024 20:40
@ShGKme ShGKme force-pushed the feat/screen-sharing branch from 9780aef to 6ac7cb6 Compare April 8, 2024 07:12
@nickvergessen
Copy link
Member

After selecting that the screenshare menu is "open" (should be closed)

This is still the case, otherwise fine by me

@ShGKme
Copy link
Contributor Author

ShGKme commented Apr 8, 2024

After selecting that the screenshare menu is "open" (should be closed)

This is still the case, otherwise fine by me

This part is outside Talk Desktop...

@nickvergessen
Copy link
Member

I never noticed it in the browser. But seems you are right

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Tested with Linux package, don't have any comments or suggestions. Nice work!

@DorraJaouad
Copy link

On Windows, I am not sure it is related to the PR or setup 🙈 :

  • Icons are not resized
  • Call button is not there and after sharing, the sharing button also disappears

image

and I think there should be a watcher over streams so when a the modal is open and you close a window, the list gets updated. For now, it turns into a black stream but remains there.

@ShGKme
Copy link
Contributor Author

ShGKme commented Apr 8, 2024

  • Icons are not resized

This is NcDialog feature... Talk Desktop, and even Talk should have no effect on it. Can you check that you have fresh npm ci installed on both talk-desktop and spreed? And if it is, what styles are applied on the button.

Which version of spreed you tested with?

  • Call button is not there and after sharing, the sharing button also disappears

Also should not be affected by the Talk Desktop...

and I think there should be a watcher over streams so when a the modal is open and you close a window, the list gets updated. For now, it turns into a black stream but remains there.

Nice catch, checking...

@ShGKme
Copy link
Contributor Author

ShGKme commented Apr 8, 2024

I never noticed it in the browser. But seems you are right

I can reproduce in Web:

  1. Start a call
  2. Start screen sharing
  3. Stop the screen sharing
  4. Start screen sharing again
  5. The menu is open

I have no idea why it happens

@ShGKme
Copy link
Contributor Author

ShGKme commented Apr 8, 2024

  • Icons are not resized

I cannot reproduce, but after some debugging with @DorraJaouad it seems to be Vue 2.7 + weird v-bind CSS issue. It correctly compiles styles, but it doesn't set CSS variable for "v-bind in css" on the root element of NcIconSvgWrapper.

@ShGKme ShGKme force-pushed the feat/screen-sharing branch from 113588d to faf0dad Compare April 9, 2024 16:35
@ShGKme
Copy link
Contributor Author

ShGKme commented Apr 9, 2024

and I think there should be a watcher over streams so when a the modal is open and you close a window, the list gets updated. For now, it turns into a black stream but remains there.

Remove preview when the source is not available anymore and request sources each second to check for new sources.

cc @DorraJaouad

Copy link

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

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

Tested.
All good 💯 thanks !

@ShGKme
Copy link
Contributor Author

ShGKme commented Apr 9, 2024

All good 💯

Even icons? o_O

@DorraJaouad
Copy link

Even icons? o_O

No :( , but it's not related to this PR.

ShGKme added 11 commits April 9, 2024 23:13
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>
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 ShGKme force-pushed the feat/screen-sharing branch from faf0dad to 0c1bc9a Compare April 10, 2024 10:12
@ShGKme ShGKme enabled auto-merge April 10, 2024 10:24
@ShGKme ShGKme merged commit 14bf3d4 into main Apr 10, 2024
6 checks passed
@delete-merged-branch delete-merged-branch bot deleted the feat/screen-sharing branch April 10, 2024 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Screensharing
4 participants