Skip to content
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

MSC3077: Support for multi-stream VoIP #3077

Merged
merged 19 commits into from
Jun 20, 2023

Conversation

SimonBrandner
Copy link
Contributor

@SimonBrandner SimonBrandner commented Mar 29, 2021

@SimonBrandner SimonBrandner changed the title MSC: Support for multi-stream VoIP MSC3077: Support for multi-stream VoIP Mar 29, 2021
Signed-off-by: Šimon Brandner <[email protected]>
@SimonBrandner SimonBrandner marked this pull request as ready for review March 29, 2021 18:03
@turt2live turt2live added kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposal-in-review labels Mar 29, 2021
Copy link
Contributor

@ShadowJonathan ShadowJonathan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

screenshare when 👀

Copy link
Contributor

@ShadowJonathan ShadowJonathan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up: I would suggest adding some more explanation to what m.usermedia and m.screenshare actually means in this (new?) context, to understand what this adds exactly, but otherwise, this looks good.

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise this looks great - congrats on your first MSC :)

proposals/3077-multi-stream-voip.md Outdated Show resolved Hide resolved
I would be tempted to skip the capability advertisement for this and just say that the absence of stream metadata means clients just take the first or whatever MSC2746 says. I can't think of anything the capability advertisement caters for specifically? - Dave

Signed-off-by: Šimon Brandner <[email protected]>
proposals/3077-multi-stream-voip.md Outdated Show resolved Hide resolved
proposals/3077-multi-stream-voip.md Outdated Show resolved Hide resolved
proposals/3077-multi-stream-voip.md Show resolved Hide resolved

+ `purpose` - a string indicating the purpose of the stream. For compatibility
between clients values `m.usermedia` and `m.screenshare` are defined.
`m.usermedia` is the stream that contains the webcam and microphone tracks.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continuing thread on whether we should identify front/rear facing cameras with this, for purposes like when the user rotates the camera and the same stream changes to a different camera (eg. FaceTime displays an animation on the remote side to show that the camera has changed). I feel like this is probably something for separate MSC, unless we want two separate purposes for front/rear camera.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, unless we want a separate purpose for that probably a separate MSC. If we wanted a separate purpose, would we have m.usermedia, m.usermedia.front and m.usermedia.rear or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eg. FaceTime displays an animation on the remote side to show that the camera has changed

This bit seems more like a stream replacement problem rather than a purpose problem. But... maybe we could allow sending both and set a main stream which would tell the client on the other side to do the animation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion would be adding something like camere_type to further distinguish the cameras - this way not all clients need to support this and we avoid having two purposes that are almost the same

SimonBrandner and others added 4 commits June 17, 2023 18:28
Co-authored-by: Richard van der Hoff <[email protected]>
Co-authored-by: Richard van der Hoff <[email protected]>
Co-authored-by: Richard van der Hoff <[email protected]>
@mscbot
Copy link
Collaborator

mscbot commented Jun 20, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Jun 20, 2023
@turt2live turt2live merged commit ddad150 into matrix-org:old_master Jun 20, 2023
@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Jun 20, 2023
@SimonBrandner SimonBrandner deleted the msc/sdp-metadata branch June 20, 2023 16:22
turt2live pushed a commit that referenced this pull request Jun 21, 2023
* Draft of multi-stream MSC

Signed-off-by: Šimon Brandner <[email protected]>

* Remove unnecessary article and don't use CamelCase

Signed-off-by: Šimon Brandner <[email protected]>

* Fix naming and MSC number

Signed-off-by: Šimon Brandner <[email protected]>

* Make it prefixy

Co-authored-by: Tulir Asokan <[email protected]>

* Be more descriptive about keys

Signed-off-by: Šimon Brandner <[email protected]>

* Add more info about usermedia and screenshare

Signed-off-by: Šimon Brandner <[email protected]>

* Simplifie backwards compatibility

I would be tempted to skip the capability advertisement for this and just say that the absence of stream metadata means clients just take the first or whatever MSC2746 says. I can't think of anything the capability advertisement caters for specifically? - Dave

Signed-off-by: Šimon Brandner <[email protected]>

* Reword parts of backwards compatibility section

Signed-off-by: Šimon Brandner <[email protected]>

* Improve explanation of backwards compatibility

Signed-off-by: Šimon Brandner <[email protected]>

* Add missing spaces to unstable perifix table

Signed-off-by: Šimon Brandner <[email protected]>

* Remove support for stream-replacement

Signed-off-by: Šimon Brandner <[email protected]>

* Apply suggestions from code review

Co-authored-by: Andrew Morgan <[email protected]>

* Fix concerns

Signed-off-by: Šimon Brandner <[email protected]>

* Link to specific spec version

Co-authored-by: Richard van der Hoff <[email protected]>

* Be more readable

Co-authored-by: Richard van der Hoff <[email protected]>

* Clarify

Co-authored-by: Richard van der Hoff <[email protected]>

* Don't ref non-existing thing

Co-authored-by: Richard van der Hoff <[email protected]>

* Remove confusing words

---------

Signed-off-by: Šimon Brandner <[email protected]>
Co-authored-by: Tulir Asokan <[email protected]>
Co-authored-by: David Baker <[email protected]>
Co-authored-by: Andrew Morgan <[email protected]>
Co-authored-by: Richard van der Hoff <[email protected]>
dbkr added a commit to matrix-org/matrix-spec that referenced this pull request Jul 18, 2023
@turt2live
Copy link
Member

Spec PR: matrix-org/matrix-spec#1602

@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels Aug 1, 2023
@turt2live
Copy link
Member

Spec PR: matrix-org/matrix-spec#1735

@turt2live
Copy link
Member

Merged 🎉

@turt2live turt2live added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature MSC for not-core and not-maintenance stuff merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal voip
Projects
Status: Done to some definition
Status: Done for now
Development

Successfully merging this pull request may close these issues.

Support for multi-stream WebRTC calls (SPEC-252)