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

[Question || Bug] How to replace 'placeScreenSharingCall' by 'setScreensharingEnabled' to initiate a screensharing call w/o using Electron desktopCapturer #1898

Closed
menturion opened this issue Sep 9, 2021 · 9 comments

Comments

@menturion
Copy link

menturion commented Sep 9, 2021

matrix-js-sdk: v12.4.0

Hi,

I am trying to replace the former call.placeScreenSharingCall() by the newly introduced call.setScreensharingEnabled(true) (see #1685) to initiate a screensharing call (i.e. not to add screensharing to an existing voice call).

I am constantly getting an exception:

Set screensharing enabled? true using replaceTrack()
logger.js:59 Electron desktopCapturer is not available...
logger.js:59 Getting screen stream using getDisplayMedia()...
listener.js:78 Error: Failed to get screen-sharing stream: : TypeError: Cannot read properties of undefined (reading 'replaceTrack')
    at MatrixCall.setScreensharingEnabledWithoutMetadataSupport (call.js:1096)

Previously (v12.2.0), I just needed to call await call.placeScreenSharingCall() and to select the browser's native screen selector to start a screensharing call.

What is a simple replacement of ...

await call.placeScreenSharingCall();

using ...

await call.setScreensharingEnabled(
    !call.isScreensharing(), 
    ...
);

in case of the absence of the Electron desktopCapturer?

@SimonBrandner
Copy link
Contributor

We removed placeScreensharingCall() because of the reasons mentioned in #1685 (comment). Using placeScreensharingCall() was a super-messy approach to screen-sharing that doesn't have a place in the modern world and trying to make it work is both difficult and not very maintainable, imo.

@menturion, why do you want this feature?

@menturion
Copy link
Author

menturion commented Sep 10, 2021

@SimonBrandner

Thanks for your reply.
Sorry for my misleading description. I want to migrate my code to the new matrix-js-sdk (v12.4.0) and thus have to replace the current code segment ...

await call.placeScreenSharingCall();

by a working snippet using the new method ...

await call.setScreensharingEnabled(
    !call.isScreensharing(), 
    ...
);

I want to initiate a new screensharing call, using the browser's own screen picker.
I am get the below exception when using await call.setScreensharingEnabled(!call.isScreensharing()); for a newly created call:

Set screensharing enabled? true using replaceTrack()
logger.js:59 Electron desktopCapturer is not available...
logger.js:59 Getting screen stream using getDisplayMedia()...
listener.js:78 Error: Failed to get screen-sharing stream: : TypeError: Cannot read properties of undefined (reading 'replaceTrack')
    at MatrixCall.setScreensharingEnabledWithoutMetadataSupport (call.js:1096)trixCall.setScreensharingEnabledWithoutMetadataSupport (call.js:1096)

So my question is whether this is a bug or how do I have to establish a NEW screensharing call by using the new method call.setScreensharingEnabled(). The screen should be picked by the browser's own screen picker.

@SimonBrandner
Copy link
Contributor

SimonBrandner commented Sep 10, 2021

Ah, @menturion, could we perhaps move to #matrix-dev for faster communication...? Ping me there (@simon.brandner:envs.net)

@menturion
Copy link
Author

@SimonBrandner

Again many thanks for your reply.

In my experience, it is better to discuss such issues asynchronously here on GitHub. Issues are searchable for others who might have the same or similar issues, and on-topic discussion threads are not fragmented by other discussions (on Matrix) happening at the same time.

I just filed another bug report here #1912.
Perhaps these are related.

@SimonBrandner
Copy link
Contributor

I tend to prefer to discuss on Matrix (possibly in a DM to avoid talking over each other) and then posting the solution to GH if necessary :)

The way to do this should be something like:

const call = client.createCall(roomId);
// You can also place a voice call but you won't be able to screen-share if the other side doesn't support MSC3077 - multi-stream VoIP
await call.placeVideoCall();
// You might want to wait for the other side to pick up here, so that the js-sdk uses a separate stream for screen-sharing if possible
call.setScreensharingEnabled(true);

Does this explain the issues you're having?

@menturion
Copy link
Author

@SimonBrandner

Thanks for sharing this. I was able to test it now.

This ...

await call.placeVideoCall();
call.setScreensharingEnabled(true);

... is unfortunately not a replacement for the former placeScreenSharingCall method.
The caller's cam is and remains enabled and the video stream is not replaced by the screen sharing stream.

The removal of the placeScreenSharingCall method for initiating a fresh new screen sharing call without providing a full replacement is a backtrack and will break screen sharing functionality of clients that have implemented this.

@SimonBrandner
Copy link
Contributor

This was intended to be a breaking change behind which was reasoning:

  1. In all modern apps that support VoIP you always turn on screen-sharing during a call and you aren't forced to choose what type of call you want to make before actually making the call.
  2. All modern apps allow you to send both your video and screen
  3. There are two ways to do screen-sharing, the above-mentioned one and replacing the video track with the screen-sharing track. For the former to work both sides need to support MSC3077 and we can't detect that before the call starts, so if we were to keep placeScreenSharingCall(), it would be quite unclear what we want to do, combine the tracks into one stream or send two streams?
  4. Trying to keep placeScreenSharingCall() doesn't have much benefit since you can achieve the same result by doing a voice call and then starting screen-sharing. In fact, it would be quite hard to maintain in the future and it would lead to a lot of spaghetti code...

Hopefully, the above explains why we did this change

Also no need to ping me every time...

@menturion
Copy link
Author

I see.
Thanks a lot for the comprehensive explanation.

@SimonBrandner
Copy link
Contributor

Okay, let's close this then. Let me know if you have more questions

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

No branches or pull requests

2 participants