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
[WIP] MSC3898: Native Matrix VoIP signalling for cascaded foci (SFUs, MCUs...) #3898
base: main
Are you sure you want to change the base?
[WIP] MSC3898: Native Matrix VoIP signalling for cascaded foci (SFUs, MCUs...) #3898
Changes from 4 commits
750087f
aa53398
de302cb
7474782
5cad46d
2cbc2d6
f542fcb
6f01a94
575e16c
33b1880
65faee4
9882c97
c66bbe4
1b2d740
feb064b
d96d101
d538e1e
91470a2
2ef7425
5a186e4
b461525
e49e80d
6b3fd47
bf52e02
f81dd9d
9c32b96
6f8c9d1
ecf2425
bf04b17
1896fc7
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.
Note that the
call_id
does not seem to be necessary.When the SFU sends To-Device messages to the clients, the
conf_id
is specified and given that theconf_id
is a unique identifier of a conference/call, there seem to be no need to have acall_id
in addition to that.Recently I've ran into an issue where I realized that
call_id
andconf_id
are not the same (despite MSC3401 giving me an impression that they are identical). Theconf_id
was the ID of a conference (as expected), but thecall_id
was another random string that was different for each single participant which forced us to use bothcall_id
andconf_id
when sending messages back to the clients (otherwise they would be rejected).It looks like
call_id
should either be removed or (if we want to keep it for the backward compatibility with the older MSC?) it must be equal to theconf_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 think this comment belongs on MSC3401 as this line is specified in other MSC, although I thinik the conclusion is just that there's confusion between call_id and conf_id and we should rename this to conf_id (there's no other conf ID in this event so it is necessary, not just for backwards compat).
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've also written a comment about it in MSC3401 😛
Basically, the problem is not only that they are called differently, but also that the value of
call_id
andconf_id
is different, so they are different for some reason (and on the SFU we are obligated to take both into account:conf_id
for a conference ID and thecall_id
to set a value in outgoing To-Device messages without which the client would discard the messages).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 you agree that the correct resolution is to change this to
m.conf_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.
Yes, that would be great! Though I wonder what the consequence of that would be (i.e. what is that value that the current
call_id
has? - It's not a conference ID, it's something different, or maybe it's a leftover from a previous implementation for 1:1s wherecall_id
meant something?)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 🙂 That's something that I discovered a week ago when deploying the first iteration of refactored SFU. I've just tried to join the SFU and the
conf_id
field is equal to1668002318158qFQmBWgVHHXTZsPA
, while thecall_id
is1670443502134bOWVqa3btIfDQMjJ
.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.
conf_id and call_id from where though? There will also be call_id in the individual calls which will definitely be different. Otherwise we need to work out what's going on 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.
From To-Device messages that the participants of the conference send to the SFU. We then reply with To-Device messages back (e.g. when we generate an answer), in which case we also set both
conf_id
orcall_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.
Do you mean https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/webrtc/call.ts#L2252?
conf_id
is the ID of the conference call (state key of the m.call event),call_id
is the ID of the 1:1 call between the individual group call participants.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, seems like this. But the thing is that, from the SFUs standpoint, the
call_id
does not have any semantics, but currently we're obligated to store bothconf_id
andcall_id
(which have different values), where thecall_id
is only used in order to send To-Device messages to the clients, i.e. when I e.g. want to send messages from the SFU to the client, I have to set both theconf_id
(the ID of a conference) and thecall_id
(the ID of the 1:1 call between individual group call participants).So my point is that we probably want to get rid of mandating
call_id
for the SFU calls since they don't seem any semantic value for this use case. And only use theconf_id
instead?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 specify the cascading specific logic in this MSC or would it be better to make a separate MSC for cascading?
Rationale: if we have a dedicated MSC for the SFU, we'll be able to finalize and merge it faster to master. Iterating with small MSCs might be a better idea given the amount of time it normally takes until the MSC is merged? (just a gut feeling)
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 issue is that the event fields used in a single focus case are quite different from the cascading case. I wonder if there is a way to avoid that issue
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.
Sorry, I did not fully get what you mean.
I think the reason why I initially commented is that it seems like we're not going to have the cascading implemented in the very nearest future (currently we don't really support it), so I thought maybe it would be faster to limit this MSC to the SFU and then create a cascading MSC after that (once we have a stable SFU). I was just afraid that otherwise the MSC would stay open (or in a draft state) for too long.
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 agree with that but I am not sure how to technically handle this - the MSC currently specifies an SFU selection algorithm and the fields it uses, if we wanted to split the MSC into two, we would need to completely different ways to specify the SFU, I think...
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.
(by @HarHarLinks from #3401 (comment))
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.
As I learn more about this topic, foci seem to not be authenticated.
As a server admin, I would like if not anyone could use the focus I host. It would appear logical to allow only user of one or more associated homeservers and at most also temporarily their remote call members if the algorithm deems the focus favourable.