Add support for multi-stream VoIP #1735
Merged
Conversation
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
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
zecakeh
commented
Feb 28, 2024
| a `purpose` of an unknown type (i.e. not `m.usermedia` or `m.screenshare`), it | ||
| should be ignored. | ||
|
|
||
| Clients implementing this specification will ignore any streamless tracks. Note |
There was a problem hiding this comment.
I know this is existing text, but it now seems even more confusing than before, so I think we need to clarify.
- What is the mechanism by which streamless tracks are ignored? Or is this an instruction to clients to ignore such tracks?
- what is that to do with
addTrack's parameters?- is it related to
sdp_stream_metadata? if not, it should probably not be in the middle of the text about it?
There was a problem hiding this comment.
- I believe it's the job of the client implementation to ignore streamless tracks. So the
willprobably needs to be changed to aMUST, like in other places in the spec. - I believe that the
addTracknote is a bit of help for client implementation. For context it refers toRTCPeerConnection: addTrack(). It's a method that takes a track and any number of streams. If there is no stream provided, it creates a streamless track, which is why the method should be called with a track and a stream. In my opinion the note could be removed. - Even if the whole section talks a lot about
sdp_stream_metadata, it is about streams, and it just happens that it is the only paragraph left that doesn't mention the field. It could be placed at the top or bottom of the section if you prefer.
There was a problem hiding this comment.
- Indeed. Can you change
willtoMUST(or, maybe more readably,should)? - Right. Yes. I think we should remove it.
- Yes, I think it would make more sense at the end of the section, after the note on backwards-compatibility.
zecakeh
commented
Feb 28, 2024
richvdh
reviewed
Mar 12, 2024
| @@ -0,0 +1,27 @@ | |||
| type: object | |||
There was a problem hiding this comment.
I don't think this belongs in core-event-schema
There was a problem hiding this comment.
I agree. Since it doesn't seem to belong directly in schemas either, because it only contains full event schemas, I took the liberty to create a components directory (named like this for similarity with the property that includes reusable bits in the OpenAPI spec)
richvdh
reviewed
Mar 12, 2024
| a `purpose` of an unknown type (i.e. not `m.usermedia` or `m.screenshare`), it | ||
| should be ignored. | ||
|
|
||
| Clients implementing this specification will ignore any streamless tracks. Note |
There was a problem hiding this comment.
- Indeed. Can you change
willtoMUST(or, maybe more readably,should)? - Right. Yes. I think we should remove it.
- Yes, I think it would make more sense at the end of the section, after the note on backwards-compatibility.
Comment on lines
23
to
25
| * `m.usermedia` - stream that contains the webcam and/or microphone | ||
| tracks | ||
| * `m.screenshare` - stream with the screen-sharing tracks |
There was a problem hiding this comment.
Suggested change
| * `m.usermedia` - stream that contains the webcam and/or microphone | |
| tracks | |
| * `m.screenshare` - stream with the screen-sharing tracks | |
| * `m.usermedia`: Stream that contains the webcam and/or microphone | |
| tracks. | |
| * `m.screenshare`: Stream with the screen-sharing tracks. |
Closed
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
This was referenced Mar 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
As per MSC3077.
This is built on top of #1602 and incorporates the feedback from the first review, as was suggested in the Matrix Spec and Docs Authoring Room.
Threads which are not resolved in the other PR are reopened here.
Preview: https://pr1735--matrix-spec-previews.netlify.app