Enable group calls without video and audio track by configuration of MatrixClient #3162

EnricoSchw · 2023-02-16T18:32:11Z

Checklist

Tests written for new code (and old code if feasible)
Linter and other CI checks pass
Sign-off given on the changes (see CONTRIBUTING.md)

Allow Element Call to be started without audio / video interface for the sake of a screen-sharing only session element-hq/element-call#902

Here's what your changelog entry will look like:

✨ Features

Enable group calls without video and audio track by configuration of MatrixClient (#3162). Contributed by @EnricoSchw.

SimonBrandner · 2023-02-17T08:49:01Z

src/webrtc/groupCall.ts

+    }
+
+    public set initCallWithoutVideoAndAudio(allowCallWithOutVideoAndAudio: boolean) {
+        this.allowCallWithOutVideoAndAudio = allowCallWithOutVideoAndAudio;


Why not put this into opts and make it readonly and public?

In the beginning createGroupCall from the MatrixClient stopped me from doing that. But since the configuration is now done via the Matrix Client, there is no reason not to do it via the constructor. Thx 👍

SimonBrandner

I am not exactly confident having this as a flag on the client is the correct way to go though I can't think of alternatives 🤔 Could we quickly go over this in the EC internal room?

SimonBrandner · 2023-02-19T08:28:58Z

src/webrtc/groupCall.ts

+        // Because we need a Local Call Feed to establish a call connection, we avoid muting video in case of empty
+        // video track. In this way we go sure if a client implements muting we don't raise an error.
+        if (this.initCallWithoutVideoAndAudio && this.localCallFeed?.stream.getVideoTracks().length === 0) {
+            return false;
+        }


Sorry, it's somehow not clear to me what this piece of code does exactly

My implementation allows feeds that does not contain tracks at all. I have to make sure that the user of the SDK doesn't try to mute a feed that doesn't have a video track. This case must be caught. It would be logically correct if we would have no LocalFeed. This case is covert below. But you need a LocalFeed to innit a GroupCall. I can't go the way of PTT either, because I have to assume that no device exists.

Do we need to check for this.initCallWithoutVideoAndAudio? Wouldn't it always be an error to try & mute/unmute video if there was no video track?

i'll remove it

EnricoSchw · 2023-02-20T10:08:08Z

I am not exactly confident having this as a flag on the client is the correct way to go though I can't think of alternatives 🤔 Could we quickly go over this in the EC internal room?

Technically we could implement a new GroupCall Type in the same way we do this with PTT. After this we can bind it on EC internal room like we do it currently with PTT.

However, I don't think that's a good idea. The SDK is based on Feeds with a video and a audio track. PTT is already working around this and provide Feeds with only audio tracks. The feature I'm implemented should allows calls without Feeds at all. I'am working around this in a way thats I am provide Feeds without audio and without video tracks (but this is not a Feed anymore). However this is a hack, because a User is connected in the call, when we receive a Feed from the user means a Video or Audio Track from him. As long as we don't know exactly what is a Feed and what isn't, it's better to configure this feature separately via the client.

dbkr

I think basically either we add this flag (which does make the logic of calls a bit more complex) or accept that it's no longer an error condition to have a call with no local or remote feeds (ie. basically just make this behaviour the default. I think making it the default could be okay actually: it would be an API change (albeit not a very significant one) but I think that's OK?

dbkr · 2023-02-22T10:15:54Z

src/webrtc/groupCall.ts

+        // Because we need a Local Call Feed to establish a call connection, we avoid muting video in case of empty
+        // video track. In this way we go sure if a client implements muting we don't raise an error.
+        if (this.initCallWithoutVideoAndAudio && this.localCallFeed?.stream.getVideoTracks().length === 0) {
+            return false;
+        }


Do we need to check for this.initCallWithoutVideoAndAudio? Wouldn't it always be an error to try & mute/unmute video if there was no video track?

EnricoSchw · 2023-02-22T16:17:51Z

I think basically either we add this flag (which does make the logic of calls a bit more complex) or accept that it's no longer an error condition to have a call with no local or remote feeds (ie. basically just make this behaviour the default. I think making it the default could be okay actually: it would be an API change (albeit not a very significant one) but I think that's OK?

I didn't want to change that much and I wanted to keep it very simple, so we can close the ticket (only screen share calls)

This implementation works because no feed is expected if flag active. I have no idea how complicated it will be to add a new GrupenCall type.

dbkr · 2023-02-23T10:27:11Z

Agreed, I don't think adding a new group call type is the way to go. I feel okay about the flag since it leaves 1:1 calls in element web etc in a mode where the call will fail if no media is negotiated, which is probably the sensible thing to do. Perhaps we could put the option either in initLocalCallFeeds or a separate dedicated setter for the option. @SimonBrandner how would you feel about that?

SimonBrandner · 2023-02-23T10:52:37Z

Agreed, I don't think adding a new group call type is the way to go. I feel okay about the flag since it leaves 1:1 calls in element web etc in a mode where the call will fail if no media is negotiated, which is probably the sensible thing to do. Perhaps we could put the option either in initLocalCallFeeds or a separate dedicated setter for the option. @SimonBrandner how would you feel about that?

Yeah, that sounds good

EnricoSchw · 2023-02-24T08:36:49Z

Agreed, I don't think adding a new group call type is the way to go. I feel okay about the flag since it leaves 1:1 calls in element web etc in a mode where the call will fail if no media is negotiated, which is probably the sensible thing to do. Perhaps we could put the option either in initLocalCallFeeds or a separate dedicated setter for the option.

initCallWithoutVideoAndAudio change the behavior of the whole SDK and not only the behavior of the Feeds.. it should be a main property and not pass by a feed init method.

And because it change the behavior of the SDK its a good idea to path it by constructor, we already have discussed

EnricoSchw · 2023-02-24T08:40:11Z

Maybe the name initCallWithoutVideoAndAudio is misleading. I will change the name to allowCallWithoutVideoAndAudio. Its then consistent to EC.

@dbkr @SimonBrandner could you agree about it?

SimonBrandner

Can the code in groupcall.ts be simplified now?

src/webrtc/mediaHandler.ts

spec/unit/webrtc/groupCall.spec.ts

EnricoSchw · 2023-03-02T09:39:52Z

Can the code in groupcall.ts be simplified now?

@SimonBrandner Can you please be more specific? Do you mean setLocalVideoMuted and setMicrophoneMuted? I would like to use promise.then().catch() instead of try catch blocks, but the linter forbids that.

EnricoSchw · 2023-03-02T13:39:49Z

@dbkr PTAL

…all-without-video-and-audio

andybalaam · 2023-03-02T16:37:49Z

Apologies for jumping the queue with #3184 - the develop branch was broken due to me force-merging a PR that broke our quality guidelines (because it contained repeated code, to allow us to migrate smoothly from an old to new interface without breaking matrix-react-sdk). The PR I jumped to the front of the queue should unbreak us, meaning this PR can get merged when it gets to the front of the queue.

t3chguy · 2023-03-02T17:50:53Z

Due to a required parameter being added

t3chguy · 2023-03-02T23:18:10Z

#3186 brought back backwards compatibility - thanks @EnricoSchw

@EnricoSchw

* Implement MSC3758: a push rule condition to match event properties exactly ([\matrix-org#3179](matrix-org#3179)). * Enable group calls without video and audio track by configuration of MatrixClient ([\matrix-org#3162](matrix-org#3162)). Contributed by @EnricoSchw. * Updates to protocol used for Sign in with QR code ([\matrix-org#3155](matrix-org#3155)). Contributed by @hughns. * Implement MSC3873 to handle escaped dots in push rule keys ([\matrix-org#3134](matrix-org#3134)). Fixes undefined/matrix-js-sdk#1454. * Fix spec compliance issue around encrypted `m.relates_to` ([\matrix-org#3178](matrix-org#3178)). * Fix reactions in threads sometimes causing stuck notifications ([\matrix-org#3146](matrix-org#3146)). Fixes element-hq/element-web#24000. Contributed by @justjanne.

Enrico Schwendig added 2 commits February 16, 2023 19:14

groupCall: add configuration param to allow no audio and no camera

226ce4a

groupCall: enable datachannel to do no media group calls

9904a35

EnricoSchw requested review from a team as code owners February 16, 2023 18:32

EnricoSchw requested review from dbkr and SimonBrandner February 16, 2023 18:32

github-actions bot deployed to PR Documentation Preview February 16, 2023 18:34 View deployment

EnricoSchw added T-Enhancement T-Task Tasks for the team like planning labels Feb 16, 2023

EnricoSchw mentioned this pull request Feb 16, 2023

Allow Element Call to be started without audio / video interface for the sake of a screen-sharing only session element-hq/element-call#902

Closed

SimonBrandner reviewed Feb 17, 2023

View reviewed changes

Enrico Schwendig added 2 commits February 17, 2023 15:39

groupCall: changed call no media property as object property

cce6685

groupCall: fix existing unit tests

c57aadb

github-actions bot deployed to PR Documentation Preview February 17, 2023 14:48 View deployment

SimonBrandner reviewed Feb 19, 2023

View reviewed changes

EnricoSchw mentioned this pull request Feb 21, 2023

Allow Element Call to be started without audio / video interface element-hq/element-call#924

Merged

dbkr reviewed Feb 22, 2023

View reviewed changes

groupCall: remove not needed flag

53ec8e0

github-actions bot deployed to PR Documentation Preview February 22, 2023 16:27 View deployment

groupCall: rename property to allow no media calls

1cad2d7

github-actions bot deployed to PR Documentation Preview February 24, 2023 09:01 View deployment

groupCall: mute unmute even without device

9880a2e

EnricoSchw force-pushed the enricoschw/group-call-without-video-and-audio branch from 3775e7f to 9880a2e Compare February 24, 2023 13:50

github-actions bot deployed to PR Documentation Preview February 24, 2023 13:52 View deployment

groupCall: move permission check to device handler

913a231

github-actions bot deployed to PR Documentation Preview March 2, 2023 08:45 View deployment

SimonBrandner reviewed Mar 2, 2023

View reviewed changes

src/webrtc/mediaHandler.ts Outdated Show resolved Hide resolved

src/webrtc/mediaHandler.ts Outdated Show resolved Hide resolved

spec/unit/webrtc/groupCall.spec.ts Show resolved Hide resolved

spec/unit/webrtc/groupCall.spec.ts Show resolved Hide resolved

mediaHandler: add error in log statement

f7f0084

github-actions bot deployed to PR Documentation Preview March 2, 2023 09:45 View deployment

SimonBrandner approved these changes Mar 2, 2023

View reviewed changes

dbkr approved these changes Mar 2, 2023

View reviewed changes

EnricoSchw added this pull request to the merge queue Mar 2, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 2, 2023

EnricoSchw added this pull request to the merge queue Mar 2, 2023

Merge remote-tracking branch 'origin/develop' into enricoschw/group-c…

640da11

…all-without-video-and-audio

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 2, 2023

github-actions bot deployed to PR Documentation Preview March 2, 2023 16:07 View deployment

EnricoSchw enabled auto-merge March 2, 2023 16:11

EnricoSchw added this pull request to the merge queue Mar 2, 2023

andybalaam removed this pull request from the merge queue due to a manual request Mar 2, 2023

andybalaam added this pull request to the merge queue Mar 2, 2023

Merged via the queue into develop with commit e782a2a Mar 2, 2023

andybalaam deleted the enricoschw/group-call-without-video-and-audio branch March 2, 2023 17:17

t3chguy added the X-Breaking-Change label Mar 2, 2023

EnricoSchw mentioned this pull request Mar 2, 2023

Fix: breaking changes with GroupCall and ensure backwards-compatibility #3186

Merged

3 tasks

t3chguy removed T-Task Tasks for the team like planning X-Breaking-Change labels Mar 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable group calls without video and audio track by configuration of MatrixClient #3162

Enable group calls without video and audio track by configuration of MatrixClient #3162

EnricoSchw commented Feb 16, 2023 •

edited by github-actions bot

Loading

SimonBrandner Feb 17, 2023

EnricoSchw Feb 17, 2023

SimonBrandner left a comment

SimonBrandner Feb 19, 2023

EnricoSchw Feb 20, 2023

dbkr Feb 22, 2023

EnricoSchw Feb 22, 2023

EnricoSchw commented Feb 20, 2023

dbkr left a comment

dbkr Feb 22, 2023

EnricoSchw commented Feb 22, 2023 •

edited

Loading

dbkr commented Feb 23, 2023

SimonBrandner commented Feb 23, 2023

EnricoSchw commented Feb 24, 2023

EnricoSchw commented Feb 24, 2023 •

edited

Loading

SimonBrandner left a comment

EnricoSchw commented Mar 2, 2023

EnricoSchw commented Mar 2, 2023

andybalaam commented Mar 2, 2023

t3chguy commented Mar 2, 2023

t3chguy commented Mar 2, 2023 •

edited

Loading

Enable group calls without video and audio track by configuration of MatrixClient #3162

Enable group calls without video and audio track by configuration of MatrixClient #3162

Conversation

EnricoSchw commented Feb 16, 2023 • edited by github-actions bot Loading

Checklist

✨ Features

SimonBrandner Feb 17, 2023

Choose a reason for hiding this comment

EnricoSchw Feb 17, 2023

Choose a reason for hiding this comment

SimonBrandner left a comment

Choose a reason for hiding this comment

SimonBrandner Feb 19, 2023

Choose a reason for hiding this comment

EnricoSchw Feb 20, 2023

Choose a reason for hiding this comment

dbkr Feb 22, 2023

Choose a reason for hiding this comment

EnricoSchw Feb 22, 2023

Choose a reason for hiding this comment

EnricoSchw commented Feb 20, 2023

dbkr left a comment

Choose a reason for hiding this comment

dbkr Feb 22, 2023

Choose a reason for hiding this comment

EnricoSchw commented Feb 22, 2023 • edited Loading

dbkr commented Feb 23, 2023

SimonBrandner commented Feb 23, 2023

EnricoSchw commented Feb 24, 2023

EnricoSchw commented Feb 24, 2023 • edited Loading

SimonBrandner left a comment

Choose a reason for hiding this comment

EnricoSchw commented Mar 2, 2023

EnricoSchw commented Mar 2, 2023

andybalaam commented Mar 2, 2023

t3chguy commented Mar 2, 2023

t3chguy commented Mar 2, 2023 • edited Loading

EnricoSchw commented Feb 16, 2023 •

edited by github-actions bot

Loading

EnricoSchw commented Feb 22, 2023 •

edited

Loading

EnricoSchw commented Feb 24, 2023 •

edited

Loading

t3chguy commented Mar 2, 2023 •

edited

Loading