Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MSC3401: Native Group VoIP Signalling #3401
base: main
Are you sure you want to change the base?
MSC3401: Native Group VoIP Signalling #3401
Changes from 18 commits
05fd5af
7f5ee49
083fd9a
5ee96fb
b90b85e
ed37a0d
33a64f2
7fd1ba6
669d471
48526ad
dfd4ffe
3c306cc
4d43aae
856ddc7
d109b54
07f9547
7a06ed7
32f566a
3fde32b
05b5db2
43dc42f
5635cee
b8ebe27
6b98d66
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshedding warning: I'm relatively new to the WebRTC/VoIP industry, but I have never heard the term focus used in place of SFU. Is this a commonly known term? Should we be using SFU in this spec instead? Including renaming
m.foci
->m.sfus
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason i originally went with
foci
is because the field originally described the (mxid, deviceid) tuples where a given mxid could be contacted - which might either be a local device (for full mesh) or an SFU.However, in the current simpler draft, the only time you include this field is if you are using a conferencing focus of some kind.
But, this proposal is not meant to just be for SFUs - the device you use to focus together your view of the conference could (in future) equally be an MCU as much as an SFU. Hence using the correct more generic term of 'focus' rather than making it specific to SFU technology. For instance, the server could advertise a stream which composites together a mosaic of different feeds for a non-E2EE call... at which point it's acting as a (hybrid) MCU.
The term 'focus' comes from SIP (e.g. https://datatracker.ietf.org/doc/html/rfc3840#section-10.18) and is the standard term there for "an endpoint you connect to which mixes together other endpoints". I'm slightly inclined to keep it, to keep thing flexible for future more sophisticated foci tech.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call it
call_focus
orstream_focus
or something a bit more descriptive than a not-well-known dictionary word?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
focus is a pretty well-known word, and foci is its plural. i don't particularly want to call it 'focuses', given that's a different word (the 3rd person present form of 'to focus'). not sure this is a showstopper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It definitely isn't a showstopper but I would like to come up with a better name if we can. It is also a bit of a red-flag that just about everything else in the MSC is calling it a SFU.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While focus is a well-known word, outside of Britain its plural is 'focuses', so I would expect that a lot of people are going to be similarly confused over its meaning. Even the Cambridge Dictionary lists 'focuses' as the plural, while listing 'foci' as the formal plural in the UK.
Might it be possible to at least mention in the spec that it's used in this sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm coming around to using "foci" as the word and there are references out there in the wild for "foci" being used in SIP terminology
https://books.google.com/books?id=CyYAEAAAQBAJ&pg=PT66&lpg=PT66&dq=SIP+%22foci%22&source=bl&ots=zMu58i8hrj&sig=ACfU3U2VD7ts63JbE1HXkjWNoVRXi_3prA&hl=en&sa=X&ved=2ahUKEwjg9cT_j7r2AhUyGDQIHTtpCFoQ6AF6BAgCEAM#v=onepage&q=SIP%20%22foci%22&f=false
https://datatracker.ietf.org/doc/html/rfc4575#section-3.8
I think we should keep foci.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should glare be handled at the group call level in the case where multiple parties actually didn't meant to set up separate group calls in a room but just meant to call each other? For example, we could dictate that calls that have the same purpose and name should be able to replace each other in case of glare?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very good question. Any idea @ara4n?
I think because the
m.call.invite
event includes theconf_id
this is a non issue? But we've also only defined them.call.invite
for group calls under to-device messages. I guess for them.ring
intent you also need to be able to send them.call.invite
with aconf_id
set as a regular message event?In any case, I think glare is a non issue for the
m.room
orm.prompt
intent types. You both created group calls and one of you needs to join the other in the UI. However, form.ring
that involves sending invite and if you both invite each other at the same time I think we should use the same glare resolution we have for regular calls in that we compareconf_id
values.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glare can happen with any call type though if two clients decide to set
m.call
at the same time, though. I suspect we should a) add an index somewhere to futureproof for more than one call per room, b) for two calls with the same index, tiebreak between them by prioritising the m.call event with the lexicographically lowest call ID.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
m.room.power_levels
state event specifies that posting state events requires a power level of 50 by default. From a user experience standpoint, I would think it is reasonable for normal users in a room to be able to start calls in that room by default, but with the current power_levels policy it would need them.call
power level set lower. It may be desirable for room creation UX in clients to present the option to set this level upfront.Perhaps there should be a way to specify a different power level requirement for different intents as well. A Discord user would expect to be able to start a room's call freely without disturbing other members of the room ala
m.room
intent. On the other hand, anm.ring
is a much more disruptive intent that would be reserved for smaller group chats and should not normally be allowed in other kinds of rooms.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imho outside of DMs (where both users have PL100 anyway usually) calls should not be allowed for normal users. It is still a vector of spam. Just imagine having calls being started in Matrix HQ. It would just cause issues imho.
Imho it is a sane default to restrict this and need active changes to allow it in a room.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I mean about the different call intents causing different levels of disruption. You're right, obviously
m.ring
has very different impact fromm.prompt
orm.room
and the default should be to disallow that. But a room's administration may want users to be able to start calls with one intent and not the other.Unless I'm misunderstanding the purpose of
m.room
? Is the idea form.room
intent that a room would always have a call "active", even if it has no participants, ala a "voice channel" in Discord, such that a level-0 user would typically not be able to end that call ergo not need to be able to publish state events for it other thanm.call.member
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, would there be a benefit to other call types? Or doing this more flexibly? (Allowing audioless default state)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? I think this was originally intended to be used to differentiate between the different UIs to display, but I ended up using it in matrix-js-sdk to help determine what user media constraints to use. So perhaps this needs to become an object containing the default media types to request (audio, video, datachannel)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Third Room, I think we do want to allow for joining a room with datachannel only and then upgrading the call to use audio. So either we need another type to handle this or we split it up like I commented above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like this would work?
Where
m.type
is used for displaying the correct room UI andm.audio
,m.video
, andm.datachannel
are used for specifying what the client should request from the user by default.m.audio_muted
andm.video_muted
specify whether your client should by default mute the microphone or video by default. Useful for large public rooms.Here's a voice room with audio requested by default.
Here's a voice room where audio isn't requested by default. Maybe you are listening to a presenter like in Twitter Spaces or Clubhouse style apps.
Or maybe you want everyone to be able to speak in the room so you request microphone permissions up front, you just want people to join muted.
Video and Voice Room where only audio is requested by default. This is similar to Discord where you can turn on your webcam or share your screen after you've joined.
Video Room where both audio and video are requested by default.
Third Room would use something like the following config:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this seems quite nice. m.audio and m.video work okay for 1:1 calls but get a bit foggier for multi-party, so I think it's fair to upgrade. I imagine we probably would want to avoid renegotiating to add video etc on mutli-party calls - renegotiating one connection is fine but we certainly want to avoid causing all parties to renegotiate at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea that the call should have a richer way to specify its expected UX (e.g. audio, video, datachannel, whether audio or video are muted by default etc). I'm a bit worried that the API shape starts to collide nastily with how each source advertises each stream (which in turn collides with simon's work for describing changes in each stream - i.e. signalling mute state). In other words, we have three similar related things here:
...which are all at risk of ending up with non-matching API shapes. I wonder if there is a way to unify them. For instance, this MSC currently proposes that each device in a call advertises the feeds that it's sending as:
(Which is somewhat similar to how #3077 advertises them in m.call.invite). Then #3291 adds on the ability to describe how they change over time:
So, i'm wondering whether a better way of describing the expected streams for participating in a call would be a list of feeds in the
m.call
, each with apurpose
andaudio_muted
, rather than yet another different shape.This would then also pave the way for the call to specify format intents (i.e. "you're expected to join this call with stereo audio and send 4K video") as opposed to ("you're expected to join this call with 8kHz mono audio and that's it").
TL;DR: we should be publishing the recommended media constraints in the
m.call
.FIXME: That said, do we want to support different send & receive constraints? Currently we assume calls are symmetric. Similarly, do we want to support proposing different constraints for different types of users? (e.g. Clubhouse presenters should start off unmuted, but Clubhouse listeners should start off muted)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is worth considering though, the UX nightmare might not be that bad (some clients might even work entirely with this possibility), and personally i think that putting the conf ID in a sub-field is just asking for problems (if the previous call information gets overridden by a person sending another state event for a "new" call while the last one is still in-progress.)
Why not move conf_id into the state_key, currently declare multiple calls UB and unsupported, while noting that speccing it and properly seating it would be a case for a future MSC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-opening this one because we've just had a glare-like bug on Element Call where multiple people entered the call at the same time (as you do) and multiple conferences got created in the same room. In general, we're going to want some way to handle glare of several people hitting the 'start conference call' button at the same time. Allowing multiple calls in a room means we need to handle this somehow. It's not impossible (eg. we could define some common ID for 'the' call in a room allowing you to use other IDs for other calls?) but I'd just like to check that we really want to deal with this complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also very much in favour of having the
state_key
be just""
because having multiple group calls in one room often leads to more problems rather than benefitsWith MSC3985 we now also have a separate method to create break-out rooms, so it feels like multiple calls in one room are no longer necessary
I also think we should be able to use the
m.termintated
to calculate the call lengthThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is still an issue with relying on
m.terminated
to determine the call length: If a client wants to display a timeline tile with the duration at the point where the call was ended, then it works, but if clients want to display the tile at the point that the call was started (like Element Web does), and we're reusing the same state key for all calls, it's difficult to get the duration from that event. In fact, if there's a call ongoing in the room, there's no way to tell whether a given call event is part of the current call or not, short of crawling the timeline, so clients won't know whether to label it with "call ended".With separate state keys, this is a lot easier, because it gives you a way to efficiently look up the current state of any call, current or historical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably ought to be
m.conf_id
to differentiate it from IDs of 1:1 calls and match the conf_id field in m.call.* to-device events?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous discussion at https://github.com/matrix-org/matrix-spec-proposals/pull/3401/files#r823313876
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: currently the
call_id
andconf_id
are not identical. This seems to be confusing if we're talking about the SFU calls (not sure how it's handled in a full-mesh).When working on an SFU recently, I realized that
conf_id
was the ID of a conference (or a call if you will) which was quite logical and expected. However, what I did not expect is that in addition to theconf_id
, each To-Device message has acall_id
which does not match theconf_id
and which seems to be uniquely generated by each participant.The thing is:
call_id
field does not make any sense for the SFU at the moment (see the SFU MSC), since the SFU does not know what thecall_id
is (it looks like a randomly generated string that is different for each participant who tries to join a conference), but at the same time, the SFU is essentially obligated to store thecall_id
because the To-Device messages from the SFU to the participants are expected to have thecall_id
that matches thecall_id
value sent from participants to the SFU when they contact the SFU (I tried settings thecall_id
to matchconf_id
when sending a message from the SFU to the client, but the client discarded the message if thecall_id
did not match thecall_id
that the client sent to the SFU). So essentially, there is aconf_id
the semantics of which is defined (it's the unique ID of a conference/call) and thecall_id
(which does not have any meaning for the SFU).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have trouble understanding what this is about. Is it perhaps about protecting against for example a user opening their same element web session in multiple tabs? In any case, it might be good to spell out more explicitly what exactly what this is supposed to guard against.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mostly to deal with users hitting the refresh button I believe, so we can ignore anything from previous instances of the app and terminate calls with the old instance when we see a new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: actually track here whether the participant is joined to the call or not(!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we still have an issue with tracking participants for a given group call for displaying in the UI. How are we going to check who is in a call and scale it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These IDs are only accurate if announcing to an SFU - in full mesh, each separate call will have its own IDs.
Therefore we should probably scope these to a given 1:1 call ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, perhaps this is a bit of a privacy violation - why do other people in a conference need to know what my devices are called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, right. Should we remove it? (I also don't seem to find a case where we would want to let others know what are devices are called when publishing 🤔)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can't really see a good reason we'd send this. The
purpose
ought to be enough information.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these
settings
required for some matrix-specific logic? (maybe bridges that may need this data about a stream or a track?)I'm just wondering if they are useful in a general case (like why would we need the information about a
facingMode
of a camera for intsance?). I.e. when we're talking about the SFU, the SFU does not really need to know the facing mode of a user I guess and I'm also not sure if the other call participants would benefit from this information.The only case where I assume the information about camera mode etc might be useful is when there is a specific app that runs over Matrix and needs to advertise the properties of the video/audio streams in order to implement a specific logic. But in this case, we're talking about application-specific data, i.e. something that must be the logic of the app rather than part of a [generic] Matrix protocol.
I think generally we only need the stream and track IDs, a purpose (for the use case of conference / using WebRTC for calls), and, perhaps basic information about certain tracks like the width and height of the video (theoretically it's not required, because we'll be able to access it when the track is received, but practically we would need it for the simulcast implementation on the SFU side, so such information would be useful for the conference use cases).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO we only need to submit the minimally required information about streams and tracks that would be enough for the calls to happen (with or without the SFU).
The details (
label
,displaySurface
,facingMode
, or evenbitrate
) are probably something that we generally speaking don't need. The apps that run on top of the Matrix could always exchange arbitrary metadata about their devices/tracks/streams if they need it: we anyway won't be able to describe all possible use cases for the matrixRTC in advance since we don't really know all of the use-cases - strictly speaking, video and audio tracks are not necessarily coming from a webcam or a microphone in a general [matrixRTC] case, they could be anything ranging from a mirrorless camera feed attached to the laptop to an audio output of digital instruments forwarded via a DAW).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the idea here is that clients should identify the sender (user_id and device_id) of the to_device through the
sender_key
of the olm-encrypted message to known which peer the message is from in a full-mesh group call? Perhaps it could be valuable to spell that out.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the spec to include this now, we send the sender's
device_id
along in them.call.invite
event. But I think this is a better method? I haven't seensender_key
yet, could you point me to the docs on that? Also we're not using olm-encryption just yet so it might still be better to include thedevice_id
field in the content? Not sure.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can read about
sender_key
here and here. The idea is that you query the keys for the given user (if not fetched already) and verify that the keys match the sender_key of the olm message you received. The advantage of doing it this way is that the user id and device id can almost not be spoofed, assuming you have marked the other device/user as trusted, either manually or through cross-signing.Perhaps the current impl doesn't encrypt with olm yet, but does it make sense to spec that? Is there a good reason to offer a non-encrypted version of the signalling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are using
m.ring
as the intent we don't have the recipient's device id. So how do we ring them? Do we send them.call.invite
as a regular message event?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was assuming that
m.ring
will ring every device in the roomThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right. So send to-device messages to each of the users and use
*
for to target all the user's devices. I'll add a clarification for that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the ringing happen by the mere fact that there is a non-terminated m.call state event with that intent? The to_device messages only start flowing once there are at least two
m.call.member
s in the call, e.g. you've picked up already with a specific device.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A "1:1 call" does not seem to be well defined within the context of this MSC. I assume what is meant here is a
m.ring
call (that could have 2 or more participants). Can we please use less ambiguous terminology?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we seem to have completely missed
seq
, needed to make up for todevice events having no intrinsic ordering otherwise: matrix-org/matrix-js-sdk@7f21f56There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a dumb question here: does it mean that the order of the To-Device messages is not guaranteed? I'm asking since I've processed the To-Device messages on the SFU under the assumption that they come at the same order in which they were sent by the client. If the order is not guaranteed, it may cause some interesting (undesired) effects, e.g. if the "Invite -> Hangup" sequence comes as "Hangup -> Invite" on a server, the end effect will not be what the user expects. Or
Invite -> SelectAnswer
asSelectAnswer -> Invite
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's not guaranteed and we should rely on the
seq
fieldThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's interesting. Should we document how to deal with it and what's the semantics? - I've noticed that a new commit has been pushed recently to add the
seq
to the request that says that it starts with 0 and gets incremented with each message. But where the counter is stored? I can imagine that it's a per-device counter (meaning that 2 different devices may generate the sameseq
)? And what happens when the overflow of the value occurs?It also has some practical implications: how are we (as receivers) expected to handle it in a proper way? - I.e. imagine that we receive a "New ICE candidates message" on the SFU with a
seq=15
while the previous message from the sender hadseq=5
. We probably don't want to handle the message withseq=15
right now if we have not yet received the previous 10 messages (since the current message withseq=15
may not even be useful by the moment we process the previous 10, or maybe it's related to the invite that has been sent inseq=10
). This means that in order to handle a message withseq=15
, we would need to buffer a couple more messages (messages that went beforeseq=15
) before we take a decision on whether to handle it.However, this poses certain questions, namely if we're communicating with 1000 devices (SFU use case), this means we would need to store the
lastStoredSeq
for 1000 users, the problem is that we don't really know when to release the counter for a particular user from memory (i.e. when to remove it from the map since we don't know in advance the pool of devices that would communicate with us and we may theoretically receive a message from any of them) which means that the memory usage would grow indefinitely and once the SFU is restarted, we'll have the counters lost.Another issue is that the sender can attack the receiver by sending a message with
seq=1
followed by a message withseq=99999999999
and then another withseq=99999999998
and another withseq=99999999997
and, knowing that the other side buffers them since it can't process them, it will send them until the other side gets killed due to the OOM.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to use a name other than
conf_id
? I think this is left over from them.conf
event which was renamed tom.call
. So maybegroup_call_id
is better?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, much prefer
group_call_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if
call_id
wouldn't be enough since we might want to transition to this MSC for all calls and thuscall_id
seems sufficientThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the to_device events sent between members of the group calls also have a
call_id
property, inherited from pre-3401 calls. We should probably address the existence of the other property and either plan a phase out or just use something less ambiguous likegroup_call_id
.It would also be good to use the same property name in both the
m.call.member
state event (nowm.call_id
) and to_device events (nowconf_id
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo on top of the raised mentioned improvements, this proposal and eventual resulting spec should stick to the wording "call" even in text to clarify that "conference" is not another concept at work here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empirically EC seems to expect
device_id
on all m.call.* events: https://github.com/matrix-org/matrix-js-sdk/blob/353d6bab47ab928aab089e897f5475942fcfa0ac/src/webrtc/callEventHandler.ts#L196-L212There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EC also sends device_id, sender_session_id and dest_session_id for every toDevice event: https://github.com/matrix-org/matrix-js-sdk/blob/353d6bab47ab928aab089e897f5475942fcfa0ac/src/webrtc/call.ts#L2008-L2010
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I need a better way of explaining both the device_id and sender_session_id here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the
m.call.select_answer
event makes sense for MSC 3401 calls.AIUI, the purpose of
m.call.select_answer
is to make your other devices stop ringing once you pick up on one of your devices. To_device messages only seem to be sent to devices already in the call (e.g. are present in them.call.member
state event for a given user), so they would not be sent to devices that haven't picked up yet.Perhaps to stop ringing, the other devices should just monitor the
m.call.member
event for their own user and see if it exists and a device has been added already.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plus that it happened in the past and who there-and-then participated in it, by correlating it corresponding
m.call.member
state events.