MSC4196: Voice and video calling application for MatrixRTC - #4196
MSC4196: Voice and video calling application for MatrixRTC#4196hughns wants to merge 31 commits into
Conversation
hughns
changed the title
m.call
turt2live
added
A-VOIP
ara4n
added
the
matrix-2.0
github-project-automation
Bot
moved this to Tracking for review
in Spec Core Team Workflow
fkwp
changed the title
m.callm.call
gewitternacht
left a comment
gewitternacht
left a comment
There was a problem hiding this comment.
I only realised while adding comments that this seems to generally be rather outdated. I'm still leaving them here in case they help at all, but if it's planned to revise this in a more general way anyway, feel free to ignore.
Johennes
changed the title
m.call
robintown
left a comment
robintown
left a comment
There was a problem hiding this comment.
Thanks for fleshing this out, it's looking really good to me. Just got a couple suggestions for how to make things less surprising, and nits.
| ### Injecting `m.rtc.slot` events on the server | ||
|
|
||
| Instead of having clients pass in the initial `m.rtc.slot` event via `initial_state` on | ||
| [`/createRoom`] requests, this logic could also be implemented by the server. This would | ||
| further complicate the already complex steps the server has to run through during room | ||
| creation though. |
There was a problem hiding this comment.
Hm, I kind of was expecting this to be the server's responsibility in the case of the private_chat or trusted_private_chat presets. It seems like exactly what presets are for…
There was a problem hiding this comment.
I think you could argue the same about enabling room encryption at room creation time. Currently clients have to do this manually by specifying m.room.encryption in initial_state. I'm not sure what the history of that is to be honest though.
Other than adding to the existing complexity of /createRoom, one more potential issue I can see is that slots might contain further configuration options in future. Things like different encryption schemes (such as shared key encryption) or application specific settings (such as voice-only calls). It might be difficult to reflect all of that via presets?
There was a problem hiding this comment.
I suppose with m.room.encryption there's the problem that if it were included in the preset, clients would have no way of disabling encryption with those presets. initial_state can only add state events, not take them away, and to disable encryption you need the room to never have had an m.room.encryption event at all. With m.rtc.slot that's not a problem, as you can add one to initial_state with status = closed to override the defaults.
It would just be a preset, though expecting reconfiguration to be quite common in the future still sounds like a decent argument for not bothering with this. Also, on second thought, maybe calls aren't such a core feature of the protocol that they should be governed by presets that otherwise deal with just access, power, and history visibility.
Maybe let's just wait and see if this comes up with any other reviewers?
There was a problem hiding this comment.
My questions above also made me think whether it wouldn't be cleaner for the server to handle this, but I can very much see the reconfiguration and not-a-core-feature arguments
Co-authored-by: Robin <robin@robin.town>
Co-authored-by: Robin <robin@robin.town>
Co-authored-by: Robin <robin@robin.town>
0637a27 to
16cb63a
Compare
There was a problem hiding this comment.
Implementation requirements:
- Client
There was a problem hiding this comment.
m.callapplication inm.rtc.slotevents: Use MatrixRTC slots behind labs flag element-hq/element-web#34392m.callapplication inm.rtc.memberevents: https://github.com/matrix-org/matrix-js-sdk/blob/e16b0bcc06d0b17889f3eedef972f3cae9790f08/src/matrixrtc/MembershipManager.ts#L1102-L1125- Connecting to LiveKit rooms & subscribing: https://github.com/element-hq/element-call/blob/d765267c431685c655c1dad0b6bddc611362c16a/src/state/CallViewModel/remoteMembers/Connection.ts#L188
- Publishing call media to LiveKit: https://github.com/element-hq/element-call/blob/d765267c431685c655c1dad0b6bddc611362c16a/src/state/CallViewModel/localMember/Publisher.ts
- Rendering subscribed video tracks: https://github.com/element-hq/element-call/blob/d765267c431685c655c1dad0b6bddc611362c16a/src/tile/MediaView.tsx#L188-L189
- Rendering subscribed audio tracks: https://github.com/element-hq/element-call/blob/d765267c431685c655c1dad0b6bddc611362c16a/src/livekit/MatrixAudioRenderer.tsx#L56
Note
The implementation uses m.call.intent instead of intent.
Supplying an m.rtc.slot event for m.call in initial_state on /createRoom is not currently implemented (but trivial). There is an open discussion about whether this should happen on the client or the server.
| When clients create rooms with a `preset` of `private_chat` or `trusted_private_chat` in | ||
| [`/createRoom`], they SHOULD by default include an open `m.rtc.slot` event for `m.call` in | ||
| `initial_state`. Clients MAY let the user override this default behaviour. | ||
|
|
||
| As per [MSC4143], encryption of MatrixRTC sessions is mandatory in encrypted rooms and forbidden | ||
| in unencrypted rooms. Therefore, if [`m.room.encryption`] is also present in `initial_state`, the | ||
| `encryption` content block on the initial slot event MUST be set to `{ "type": "m.per_member" }`. | ||
| Otherwise, the `encryption` property MUST be omitted. |
There was a problem hiding this comment.
So, if someone "manually" provides contradicting m.rtc.slot and m.room.encryption events in initial_state, what happens? Does room creation fail, or slot creation fail? Does any of this depend on the order in which the two events are given?
Which reminds me: Did we specify in MSC4143 what happens with unencrypted slots when encryption gets enabled in a previously unencrypted room (I can't find anything like that right now), and does this influence the behaviour here?
(I'm not sure whether all of this needs to be specified here or whether it should be "obvious", I'm just unsure myself.)
| ### Injecting `m.rtc.slot` events on the server | ||
|
|
||
| Instead of having clients pass in the initial `m.rtc.slot` event via `initial_state` on | ||
| [`/createRoom`] requests, this logic could also be implemented by the server. This would | ||
| further complicate the already complex steps the server has to run through during room | ||
| creation though. |
There was a problem hiding this comment.
My questions above also made me think whether it wouldn't be cleaner for the server to handle this, but I can very much see the reconfiguration and not-a-core-feature arguments
| Some users might not be comfortable with disclosing whether their camera is on or off via | ||
| the `intent` property on `m.rtc.member` events. Given that any room member can join the | ||
| session, this information is effectively obtainable by all room members anyway (though the | ||
| join would at least be a visible choice). In either case, users can opt not to fill `intent` | ||
| given that it is an optional property. |
There was a problem hiding this comment.
Given that any room member can join the session
Is that so? What if they lack the power level to send m.rtc.member events? Then they at least aren't "officially" part of the session...
the join would at least be a visible choice
... but I think this is also not true, because they could also be "unofficially" be part of a session (see here).
So I think the whole argumentation here isn't really appropriate. But maybe it isn't even necessary, and
In either case, users can opt not to fill
intentgiven that it is an optional property.
is enough?
7 participants
Rendered