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

Introduce MatrixRTCSession lower level group call primitive #3663

Merged
merged 89 commits into from
Sep 12, 2023

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Aug 18, 2023

This adds a lot of code, but the intention here is to ultimately replace much of the code in GroupCall, which has a number of jobs including managing media and state events. This code manages only state events, so make things a bit more modular. It is also fairly extensively tested from the beginning.

  • Adds a layer that is just about managing the events for a call, without any actual calling
  • Switches to experimental new design for call members events, removing m.call events

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

✨ Features

  • Introduce MatrixRTCSession lower level group call primitive (#3663).

dbkr and others added 22 commits June 14, 2023 12:11
* Put LK info into state

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

* Update to the new way the LK service works

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

---------

Signed-off-by: Šimon Brandner <[email protected]>
As per comment, so we can start digging ourselves out of the widget
API hole we're currently in.
Add comment on updating the livekit service URL
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Richard van der Hoff <[email protected]>
Co-authored-by: Michael Telatynski <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: RiotRobot <[email protected]>
Co-authored-by: Florian Duros <[email protected]>
Co-authored-by: Kerry <[email protected]>
Co-authored-by: David Baker <[email protected]>
Co-authored-by: Erik Johnston <[email protected]>
Co-authored-by: Valere <[email protected]>
Co-authored-by: Hubert Chathi <[email protected]>
Close IDB database before deleting it to prevent spurious unexpected close errors (#3478)
Fix export type `GeneratedSecretStorageKey` (#3479)
Fix order of things in `crypto-api.ts` (#3491)
Fix bug where switching media caused media in subsequent calls to fail (#3489)
fixes (#3515)
fix the integ tests, where #3509 etc fix the unit tests.
fix breakage on node 16 (#3527)
Fix an instance of failed to decrypt error when an in flight `/keys/query` fails. (#3486)
Fix `TypedEventEmitter::removeAllListeners(void)` not working (#3561)
add getOpenIdToken to embedded client backend
…nstead

and generally simplify a bit: change it to a single string rather than
an array of structs.
Don't update calls with no livekit URL
Copy link
Contributor

@daniel-abramov daniel-abramov left a comment

Choose a reason for hiding this comment

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

To not block the merge of the PR (as I understood you would prefer to merge it ASAP), I'll reply to the review request with a comment (GitHub allows replying with a comment, a request for changes or with an approve - I don't feel confident enough to approve it in a sense that there are some topics that IMHO should ideally be addressed before I can have a confidence that the code is sound, robust and does not exhibit any bugs). Some of the threads/questions have been addressed (thanks!), but I don't seem to have permissions to resolve conversations.

So far my primary concerns are the following:

  1. I believe that this PR could have been split into separate ones: one for the widget API, one for the OIDC, another one for the LiveKit URL specific stuff (setting the URL etc?), and one (arguably the most important in this context) for the matrix rtc session. This would allow me to provide better examples when pointing out to issues (I have a feeling that some of the problems that I mentioned aren't viewed as "that big of an issue"; perhaps with better examples and more elaborate explanations I could have proven my point a bit better).
  2. I'm not confident that we really need the rtc session manager at this point, I'd rather invested more on the rtc session itself. IMHO currently it's fairly complicated (for the amount of code that it contains it has a high accidental complexity) and feels somewhat brittle (my gut feeling is that there will be bugs and edge-cases with the changes introduced in the PR and the solutions for them won't be particularly elegant - e.g. the solution for the async/await function looks quite complicated atm and hard to understand and it's generally not a very idiomatic way of solving state access/synchronisation problems). I also think that the API surface of the matrix RTC session could resemble LiveKit's Room a bit more (conceptually it's the same thing and it would also help unifying/merging them once we work on a deeper integration to introduce cascading). I'm particularly picky on such topics because I think that the matrix RTC session is a foundation upon which the rest of the things are going to be built in the future. That being said, if the changes are considered temporary and we don't plan to build much stuff on top of that, it might be fine to leave it like this.
  3. The things with getContent() and data types. I know that it's a general issue of the Matrix JS SDK and not specific to this particular PR, so I can't complain much, though I feel like we would need to address this problem rather sooner or later as it adds a lot of complexity to the code (it's not clear where the types are guaranteed to have the right datatype vs when they must be manually checked).

@dbkr
Copy link
Member Author

dbkr commented Sep 12, 2023

I've created element-hq/element-call#1432 and element-hq/element-call#1433 to track your last two points (the first one obviously is just in relation to the size of the PR itself).

@dbkr dbkr added this pull request to the merge queue Sep 12, 2023
Merged via the queue into develop with commit 6836720 Sep 12, 2023
20 checks passed
@dbkr dbkr deleted the dbkr/matrixrtcsession branch September 12, 2023 15:22
dbkr added a commit that referenced this pull request Sep 13, 2023
Accidental change from merging #3663
@dbkr dbkr mentioned this pull request Sep 13, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants