-
Notifications
You must be signed in to change notification settings - Fork 385
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
Draft
SimonBrandner
wants to merge
30
commits into
main
Choose a base branch
from
SimonBrandner/msc/sfu
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
750087f
Native Matrix VoIP signalling for cascaded SFUs
SimonBrandner aa53398
Update MSC number
SimonBrandner de302cb
Link to diagrams from MSC3401
SimonBrandner 7474782
Use correct number for file
SimonBrandner 5cad46d
Update sub and unsub ops
SimonBrandner 2cbc2d6
Merge remote-tracking branch 'upstream/main' into SimonBrandner/msc/sfu
SimonBrandner f542fcb
Give a reason for specifying res in metadata
SimonBrandner 6f01a94
Specify foci by `device_id` too
SimonBrandner 575e16c
Fixup some json
SimonBrandner 33b1880
Typo
SimonBrandner 65faee4
Specify how to handle foci better
SimonBrandner 9882c97
Amend TODOs
SimonBrandner c66bbe4
Add rationale behind usage of data channels
daniel-abramov 1b2d740
Add TODO
SimonBrandner feb064b
Update event types
SimonBrandner d96d101
Add unstable prefixes
SimonBrandner d538e1e
Use `subscribe` instead of `select`
SimonBrandner 91470a2
`op` -> `event`
SimonBrandner 2ef7425
Fixup formatting
SimonBrandner 5a186e4
Use `content`
SimonBrandner b461525
Namespace things
SimonBrandner e49e80d
Further namespacing
SimonBrandner 6b3fd47
Update the events to match current Matrix
SimonBrandner bf52e02
Fix typo
SimonBrandner f81dd9d
Use `subscribe`/`unsbuscribe`
SimonBrandner 9c32b96
Add informational section on active/preferred foci.
dbkr 6f8c9d1
Change keepalives to ping/pong
dbkr ecf2425
Add empty line
SimonBrandner bf04b17
Fix event name
SimonBrandner 1896fc7
Remove encryption section as it's glossing over details
SimonBrandner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Specify how to handle foci better
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
- Loading branch information
commit 65faee445fa2108b79416433333aa1ad178a20e7
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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?