Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Group Calls (WIP)#6834

Closed
robertlong wants to merge 5 commits into
developfrom
robertlong/group-calls
Closed

Group Calls (WIP)#6834
robertlong wants to merge 5 commits into
developfrom
robertlong/group-calls

Conversation

@robertlong
Copy link
Copy Markdown

@robertlong robertlong commented Sep 18, 2021

Starting to implement group calls in Element. Will update these PR notes soon. This is following the work on Group Calls in matrix-js-sdk matrix-org/matrix-js-sdk#1902


This PR currently has no changelog labels, so will not be included in changelogs.

A reviewer can add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is, or add Type: [enhancement/defect/task] to the description and I'll add them for you.

Preview: https://6148fc9665c30d35772871e0--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

return [layout, toggleLayout];
}

export function GroupCallView({ groupCall, pipMode }: IProps) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
export function GroupCallView({ groupCall, pipMode }: IProps) {
const GroupCallView: React.FC<IProps> = ({ groupCall, pipMode }) => {

We tend to this rather than the other thing, you'll also need to export by default because of the dumb way component.index works

Copy link
Copy Markdown
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

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

There is one outstanding issue I see with this atm. The use of the GroupCallParticipants to display video elements is quite concerning to me. I find this solution quite ugly and not very scalable. I would very much prefer to stick to the current model of using CallFeeds and VideoFeed and AudioFeed components.

The are multiple reasons for this:

  • Using GroupCallParticipants feels like saying each participant is only sending one feed which isn't true
  • Using CallFeeds matches the current way of doing things and allows us to reuse already existing code
  • CallFeeds are screen-sharing friendly
  • CallFeeds are SFU friendly - with an SFU one GroupCallParticipant might be sending a bunch of streams, each one represented by a CallFeed (and corresponding components)

To be clear, I am not saying GroupCallParticipant should be removed or anything along those lines, I just think CallFeeds are better suited for being consumed by components for displaying video and playing audio

@SimonBrandner
Copy link
Copy Markdown
Contributor

Replaced by #6848

@robertlong
Copy link
Copy Markdown
Author

Thanks @SimonBrandner for letting me know about the CI integration being dependent on branch name 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants