element-hq / element-desktop

A glossy Matrix collaboration client for desktop.
https://element.io
GNU Affero General Public License v3.0
1.13k stars 258 forks source link

Jitsi screensharing doesn't work on desktop #683

Open t3chguy opened 7 years ago

t3chguy commented 7 years ago

given option to share everything or an application window, the latter sees no application windows and hitting share on either does nothing, video stream remains from webcam

t3chguy commented 7 years ago

maybe needs something like https://github.com/jitsi/jitsi-meet-electron/

lampholder commented 7 years ago

Not strictly speaking voip but it's the tag that's most likely to see this issue picked up by the right people.

t3chguy commented 6 years ago

Seems to still not be working even though PR https://github.com/matrix-org/matrix-react-sdk/pull/1355 has landed :( image

t3chguy commented 6 years ago

image

xmeng1 commented 6 years ago

+1

t3chguy commented 6 years ago

Now that I have an NVL hat and can see both sides of the issue I'll take a look at it

t3chguy commented 6 years ago

This seems to have even further regressed as this never fires: https://github.com/matrix-org/matrix-react-sdk/commit/53574541c3a480ee3fda37b642d2e8d1e82c5183#diff-0e2cb0e79e0d376b964d6d11e52c4c45R150

MurzNN commented 6 years ago

Seems that this is Chrome engine issue, because Jitsi Screen Sharing works well in Riot-web on Firefox, but in Chrome browser - needs separate extension.

t3chguy commented 6 years ago

That's not correct. It's a nested iframe issue as I have pointed out above. For electron the extension would not work as electron exposes screen sharing devices via a native api and not gUM, and you can run screensharing in chrome without extensions if you start it with a command line argument

BloodyIron commented 6 years ago

I'm trying to do desktop sharing, via jitsi channel widget, with the desktop riot client, and it never ends up presenting options to share. When I try to share whole screen, or per app, just spins circles. :(

MurzNN commented 6 years ago

Can this https://chrome.google.com/webstore/detail/riotim-screen-sharing/ehgcnaneidbjcmblghjepmamomchgahd extension been integrated in Electron app for solve this issue?

t3chguy commented 6 years ago

Nope. Electron has special screen sharing apis which that extension does not support.

MurzNN commented 6 years ago

Developers of Jitsi recommend use https://github.com/jitsi/jitsi-meet-electron-utils for solve this permission problem, did you try this way?

t3chguy commented 6 years ago

@MurzNN Yup that is already fitted but it doesn't work due to it being an iframe inside an iframe and it not getting passed through because of this.

BloodyIron commented 6 years ago

I really wish we could get this solved. Using riot for screen sharing would be seriously useful! :D

MurzNN commented 5 years ago

Seems permission problem can be fixed using new getDisplayMedia() API in Chromium v70 https://groups.google.com/forum/#!msg/discuss-webrtc/Uf0SrR4uxzk/uO8sLrWuEAAJ

MurzNN commented 5 years ago

Jisti Meet implement new Chrome API via this commit https://github.com/jitsi/jitsi-meet/issues/2233#issuecomment-441087489 so we can update Jitsi lib + Chromium in Riot electron build, and recheck permission problem.

dbkr commented 5 years ago

Just been looking at this after encountering the feature and how it's buggy as above. Agreed the way to go here is to let it use getDisplayMedia() once Electron is using a new enough chromium.

ara4n commented 5 years ago

@dbkr: do we magically get this for free after all the recent electron upgrades then?

dbkr commented 5 years ago

Chrome 70 is the magic milestone with getDisplayMedia. Electron 4.0 is chromium 69 - almost!

MurzNN commented 5 years ago

Seems update to Chrome 70 will not be done quickly - only in Electron 5.0 https://github.com/electron/electron/issues/15059 :( Also maybe we need Chrome 72, as described in https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/getDisplayMedia:

Chrome 70 — 72 Notes: Available as a member of Navigator instead of MediaDevices in Chrome 70 and 71.

MurzNN commented 5 years ago

Together with screensharing, maybe we also can got Remote control feature to Riot, available in Jitsi Meet, too? https://github.com/jitsi/jitsi-meet/issues/1437

BloodyIron commented 5 years ago

Remote control as in during a conference? That could be super handy!

akontsevich commented 5 years ago

Yes, screensharing works in Chromium 70+. Whole screen sharing does not work in Firefox - only separate window (Jitsi has same problem). Why?

For Riot waiting for next Electron version with Chromium 70+?

pinpox commented 5 years ago

Just rebuild riot-web using the electron beta. No change, the screen-sharing window still loads for ever. If you want to try you'll need this patch:

diff --git a/electron_app/src/electron-main.js b/electron_app/src/electron-main.js
index 99ddfbd1..c3aa4f5d 100644
--- a/electron_app/src/electron-main.js
+++ b/electron_app/src/electron-main.js
@@ -206,7 +206,10 @@ const launcher = new AutoLaunch({
 // work.
 // Also mark it as secure (ie. accessing resources from this
 // protocol and HTTPS won't trigger mixed content warnings).
-protocol.registerStandardSchemes(['vector'], {secure: true});
+//protocol.registerStandardSchemes(['vector'], {secure: true});
+protocol.registerSchemesAsPrivileged([{
+    scheme: 'vector', privileges: {standard: true, secure: true, supportFetchAPI: true},
+}]);

 app.on('ready', () => {
     if (argv['devtools']) {
diff --git a/electron_app/src/preload.js b/electron_app/src/preload.js
index 3a4f7c9a..f63e19d4 100644
--- a/electron_app/src/preload.js
+++ b/electron_app/src/preload.js
@@ -23,7 +23,7 @@ window.ipcRenderer = ipcRenderer;
 // protocol: this is necessary to load olm.wasm.
 // (Also mark it a secure although we've already
 // done this in the main process).
-webFrame.registerURLSchemeAsPrivileged('vector', {
+/*webFrame.registerURLSchemeAsPrivileged('vector', {
     secure: true,
     supportFetchAPI: true,
-});
+});*/
t3chguy commented 5 years ago

Jitsi would have to update to get rid of their electron special case most likely

BloodyIron commented 5 years ago

Since @akontsevich mentions it works in Chromium 70+ but not FIrefox, are we also testing for this as a desktop application? (Win/Mac/Lin). Namely because the web interface being able to do this is awesome, but I think it's also worth keeping in-mind the desktop app facet of it too.

I really hope we can get this sorted because this is something I and others around me would use very regularly!

I can't recall, are we just talking about this for rooms, or for 1on1's too?

t3chguy commented 5 years ago

Orr 1:1 the interface is different and you can't select what to share (unless the browser let's you like Firefox does) or less you fire up a jitsi widget but it does work. (shift click the video call button in a 1:1)

pinpox commented 5 years ago

To clarify: I was testing this with the desktop app on Linux (Arch).It works fine in Chromium (not in Firefox though)

pinpox commented 5 years ago

Are there any (working) alternatives for the jitsi video conference widget that support screen sharing? I would consider using something else in matrix rooms as a workaround until this is fixed.

MurzNN commented 5 years ago

Electron 5.0 is released with Chromium 73 version, that have full support of getDisplayMedia! Can anybody try Riot with Electron 5.0 and check this issue?

bytepoets-bkr commented 5 years ago

i have tried Riot with Electron 5.0, but screen sharing still doesn't work. Has anybody else tried it? Error log:

/BuildRoot/Library/Caches/com.apple.xbs/Sources/AppleFSCompression/AppleFSCompression-96.200.3/Libraries/CompressData/CompressData.c:35: getxattr(/System/Library/CoreServices/CoreTypes.bundle/Contents/Resources/Exceptions.plist): Operation not permitted

Bildschirmfoto 2019-04-29 um 09 39 32

Bendodroid commented 5 years ago

I am using the desktop riot app on Manjaro (Awesome wm) and screenshare does not work at all.

ara4n commented 5 years ago

@saghul do you know of anything which would prevent screensharing from working in Electron 5 on the jitsi side?

saghul commented 5 years ago

Last I checked, Electron 5 doesn't ship with a desktop picker UI. It does ship the getDisplayMedia API, but not a native desktop picker, that must be implemnted by the app for now, using Electron APIs, so from the usablity prespective, nothing has changed.

Thus, you should use jitsi-meet-electron-utils for enabling screen-sharing in the Jitsi iframe: https://github.com/jitsi/jitsi-meet-electron-utils#screen-sharing

ChristianChiarulli commented 5 years ago

I am using the desktop riot app on ubuntu and screensharing doesn't work for me.

Mitch9378 commented 5 years ago

Last I checked, Electron 5 doesn't ship with a desktop picker UI. It does ship the getDisplayMedia API, but not a native desktop picker, that must be implemnted by the app for now, using Electron APIs, so from the usablity prespective, nothing has changed.

Thus, you should use jitsi-meet-electron-utils for enabling screen-sharing in the Jitsi iframe: https://github.com/jitsi/jitsi-meet-electron-utils#screen-sharing

@saghul see @t3chguy comment:

@MurzNN Yup that is already fitted but it doesn't work due to it being an iframe inside an iframe and it not getting passed through because of this.

Was getDisplayMedia supposed to help us work around this nested iframe issue?

I wan't to spend some time looking into this but the conversation seems a little fragmented and I am hoping to get us back on a path forward.

Thanks

MurzNN commented 5 years ago

As I understand we need to add something like this to Riot source:


const { RemoteControl } = require("jitsi-meet-electron-utils"); // iframe - the Jitsi Meet iframe
const remoteControl = new RemoteControl(iframe);
t3chguy commented 5 years ago

Remote control is entirely unrelated.

BloodyIron commented 5 years ago

Screensharing without remote input is half the functionality of screensharing by any other tool that does it. I disagree that remote control is unrelated.

t3chguy commented 5 years ago

What other instant messaging tool with screensharing offers remote control? I don't see it on slack, discord, Facebook, Skype, etc

t3chguy commented 5 years ago

This issue is not about remote control. I made the issue, I never mentioned remote control, don't hijack it for such.

BloodyIron commented 5 years ago

Skype does have remote control, as you can share control to other parties. I can't speak about Slack, Discord or Facebook, however I have used other screensharing tools that do allow "other user control" equivalent functionality.

I'm not hijacking it, I'm trying to point out that enabling others in a screen sharing situation to interactively control the system is actually commonplace and a relevant feature. Like many other GitHub Issue posts, not all contributions come from the original poster.

t3chguy commented 5 years ago

Of course, but riot is not primarily a screen sharing tool. This issue is about matching the functionality of the web client in the desktop client. Not adding new functionality

Mitch9378 commented 5 years ago

Jitsi would have to update to get rid of their electron special case most likely

@t3chguy good catch

https://github.com/jitsi/lib-jitsi-meet/blob/9f123f34264bea52feb6efe25fa2f6a5876f1116/modules/RTC/ScreenObtainer.js#L163-L207

In Riots case it will fall into the electron code before every trying to do getDisplayMedia. They will be reluctant to remove this as many electron based based applications will not be on electron 5 for a while.

It seems though that if they just move the electron case before the getDisplayMedia case this would solve the issue and not break anyone.

Ie. If the browser supports getDisplayMedia use it regardless of if its electron or otherwise and then fall into the electron case if it doesn't support getDisplayMedia.

Mitch9378 commented 5 years ago

Alright so so far I was able to build and run Riot on electron 5 using @binaryplease 's patch above and verified it reports that it supports getDisplayMedia.

I also modified and built jitsi-meet/lib-jits-meet and moved the electron case to a different spot. Were I am struggling a bit now is to get my Riot client to point to the local jitsi server but I am exploring that now.

It seems like the integrations_jitsi_widget_url in config.json should help with this

https://github.com/matrix-org/matrix-react-sdk/blob/2d6317fdd0303ec575e64588e5490606207a1d3f/src/CallHandler.js#L419-L425

but I haven't quite got that working yet.

t3chguy commented 5 years ago

You could try just make a custom widget and stick a url to your local jitsi in there, riot won't know its jitsi but it should still work

Mitch9378 commented 5 years ago

That didn't quite work either as the custom widget didn't allow me to embed.

What I did do is the following:

I temporarily told the app to ignore cert errors (not great if connecting to outside matrix server):

app.commandLine.appendSwitch('ignore-certificate-errors', 'true');

I then pointed the src of the jitsi widget to my local server.

I was able to then start working with the following patch on lib-jitsi-meet:

diff --git a/modules/RTC/ScreenObtainer.js b/modules/RTC/ScreenObtainer.js
index 7a0dada3..1f730177 100644
--- a/modules/RTC/ScreenObtainer.js
+++ b/modules/RTC/ScreenObtainer.js
@@ -113,10 +113,15 @@ const ScreenObtainer = {
                     });
             };
         } else if (browser.isElectron()) {
+            if (browser.supportsGetDisplayMedia()) {
+                return this.obtainScreenFromGetDisplayMedia;
+            }
+
             return this.obtainScreenOnElectron;
+
         } else if (browser.isChrome() || browser.isOpera()) {
             if (browser.supportsGetDisplayMedia()
-                    && !options.desktopSharingChromeDisabled) {
+                && !options.desktopSharingChromeDisabled) {

                 return this.obtainScreenFromGetDisplayMedia;
             } else if (options.desktopSharingChromeDisabled
-- 
2.22.0

When I click the screen share the application is throwing a built in Jitsi exception saying I cancelled the screen picker window, although it never actually appeared:

https://github.com/jitsi/lib-jitsi-meet/blob/master/modules/RTC/ScreenObtainer.js#L305-L340

The jisi exception is masking the real error here so I will work on getting more information about that.

Initially I thought that it may be unhappy with the constraints that ScreenObtainer is trying to enforce so I removed them and simplified the code for debugging purposes and no change but things are still working in the browser directly.

The details are there but i'm too tired to take this further for now. Hopefully I will be able to provide some more information soon.

saghul commented 5 years ago

@Mitch9378 Thanks for your efforts here. As I said earlier, getDisplayMedia doesn't quite work on Electron 5 as you expect. You must pass the specific device (screen ID in this case) that you need to the getDisplayMedia call, but Jitsi won't do that. We rely on the fact that a native picker will show up and ask the user what they want to share. We "solved" this by implementing our own picker for Electron, using the Electron provided APIs for listing the available sources for desktop sharing.

I suggest you try to call navigator.mediaDevices.getDisplayMedia({ video: true }) in Electron Fiddle and see it for yourself.

Mitch9378 commented 5 years ago

@saghul Thanks for the suggestion. It looks like at one point someone attempted to implement it and it was removed in a subsequent commit:

https://github.com/vector-im/riot-web/commit/19f1489c9284171e5ea3e69121864697a4e97e8a#diff-c62222a7b0c834636c8b72d6fcb782d5

by @dbkr in hopes that getDisplayMedia will solve this problem, but at present it does not.

To me it seems this violates the contract of getDisplayMedia at least as moz intended it and how its supported in chrome proper.

https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/getDisplayMedia

The MediaDevices interface's getDisplayMedia() method prompts the user to select and grant permission to capture the contents of a display or portion thereof (such as a window) as a MediaStream. The resulting stream can then be recorded using the MediaStream Recording API or transmitted as part of a WebRTC session.

For UI consistency I could see an argument why you wouldn't use chrome's picker in electron. Although electron has gained the support of getDisplayMedia in electron 5 by being built on newer chrome there is no electron specific documentation at the moment on how they intend it to work in electron if at all.