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
MSC2746: Improved VoIP Signalling #2746
MSC2746: Improved VoIP Signalling #2746
Changes from 2 commits
cf50137
a9b17fc
37c1f98
5156caf
fe8b1eb
6c4a077
25ed29a
bec62ab
019bcdd
66179f1
9e8c829
e224af3
2561820
63cecd1
c6f6ca1
e9fe3af
563dba5
070451e
7bca76e
e361fa9
722ee0d
8e76616
7e742d4
8da4b7c
18200d0
9e22601
a446629
1326a22
a669828
0fa0770
599ad3c
6478f9d
b62f842
9156c80
834bc3b
ec2c7fe
a572eb8
6592023
996adab
91428c8
e42dd41
29485c4
f69ae72
51e02b2
312ffe5
4617af5
289fb3f
1392eae
46bfbde
9dcda02
6dc85a8
f138bfe
2fd97c9
04eaee2
c52a845
6dcf65c
859cf6d
340e769
8be57ed
a09be95
5d38f15
3162911
097fa58
4eaa0b4
db5ca80
48527fc
312cdf7
8286d10
a5e963f
b09af73
1fc6b37
c9f0574
0880475
e45c1e0
bdf9639
082f216
ce0e338
2919112
c949b32
7d8d527
3925586
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.
as this is an MSC which touches event schemas while Extensible Events is on the battlefield, just a heads up that a v3 of call events is somewhat on the horizon to make them legal in extensible event-supported rooms. I don't think this MSC needs to do anything specific to solve the conflict (unless you want it to, but that means putting an even longer delay on it landing), but implementations of calls should be aware that calls will be changing again (sorry).
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 is probably fine. I think we would want this version of calls defined in a version of the spec in any case.
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.
So we treat
3
as version1
? Won't that cause issues in the future? Why do we do this?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 intent is that newer versions will have their own MSC. This is describing what clients should do should they encounter an unknown version,
0
and1
being the only known versions in this context.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.
Well, it currently reads like version
3
should be treated as1
, which sounds wrong. If a client implements it that way now, you can't make a new version ever, but I guess that is not the intention 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.
New versions mean implementation changes - in which case you can also change what it considers to be the most recent known version from
1
to something else. It's a bit like with room versions: currently a homeserver will refuse to create/join a room in a version it doesn't know about, but that doesn't mean it will never be able to support this new version. The only difference here is the fallback; in the case of a room version the server refuses to perform the action, in the case of a call the client substitutes it with the most recent known version.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.
So future versions will be compatible enough, that this doesn't cause issues?
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.
Yep, version
3
should be treated identically to1
by anything implementing this spec. Something implementing a future spec may treat it differently.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.
That sounds like it would cause issues, whenever we actually need to make a breaking change. How would that work? Shouldn't clients instead negotiate the lowest common version instead of treating every newer version as an older version?
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 need to make a breaking change, probably just easier to move to a whole new set of event types.
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 discussed on Matrix, I think it'd be better if
lifetime
s (both inm.call.negotiate
andm.call.invite
events) were replaced by absolute timestamps for when the call should time out. That way we don't rely on an arbitrary field set by the server to determine when the call should expire, and we have a single source of truth for when this expiration should happen (i.e. the content set by the client) rather than hoping the client and the server agree on the time (which they often do, but if they don't then it can get complicated). The concern then becomes that clients can get out-of-sync time-wise but in my experience client terminals (i.e. desktops, mobiles, tablets, etc) are much more likely to be time-synced out of the box than servers, so I'm not sure it should be that big of a concern.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 might also have covered this on Matrix at the time, but this is basically designed to avoid ever assuming the client clocks will be synced, but assuming those on the HSes are correct (at least within a second or so). It is somewhat interesting that since clients are usually on consumer devices which are managed by the device / OS vendor who sets up NTP, they're now often more likely to have correct clocks than servers which are subject to the server admin forgetting to start ntpd.
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.
@babolivier, what's your view on this?
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.
Since this copies what has already been done in a different place, I'd suggest sticking with this for now and rethinking it in either a new MSC or if we ultimately phase this out in favour of MSC3401.