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

Factor out connection, peer management, and packet handling into VideoCallClient #137

Merged
merged 13 commits into from
Nov 4, 2023

Conversation

ronen
Copy link
Collaborator

@ronen ronen commented Aug 19, 2023

[Continuing down the path of #74]

This PR create a single VideoCallClient interface that internally takes care of the connection, packet handling, encryption, peer management, etc. -- moving all that code out of the Attendants and Host components.

Part of that cleanup included changing the encoders so that they too use the VideoCallClient in their interfaces

@ronen
Copy link
Collaborator Author

ronen commented Aug 20, 2023

But oh darn, I just tested e2ee (again) and it's not working now. So hold off on committing this, I need to debug.

@ronen
Copy link
Collaborator Author

ronen commented Sep 4, 2023

Hey, sorry for the long delay -- got distracted by my day job.

I've pushed a commit that fixed the problem, by replacing "callback hell" with a more straightforward approach for handling new peers. And did some code cleanup along the way (those went hand-in-hand :)

Then while I was in a code cleaning mood I tidied up a few other things in the client & peer_decoder_manager, in a few other commits.

Should be good to go now!

Copy link
Member

@darioalessandro darioalessandro left a comment

Choose a reason for hiding this comment

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

lgtm!!!!

@darioalessandro
Copy link
Member

I took a stab at rebasing this myself but it is not trivial!

@ronen
Copy link
Collaborator Author

ronen commented Nov 4, 2023

Hey @darioalessandro, @griffobeid any thoughts about how to handle the UI test suite failure?

It seems to be due to wasm-bindgen versionitis.

...and seems unrelated to my having just merged in main and resolved the conflicts? 🤷🏻

@darioalessandro
Copy link
Member

I think that @griffobeid is looking into this

@darioalessandro darioalessandro merged commit bb27fb4 into main Nov 4, 2023
2 of 3 checks passed
@darioalessandro darioalessandro deleted the client branch November 4, 2023 18:20
@griffobeid
Copy link
Contributor

@ronen @darioalessandro Yeah it is a version mismatch.

After doing

cargo install -f wasm-bindgen-cli --version 0.2.87

Tests are passing just fine, don't worry about it with your changes I can clean this up

darioalessandro added a commit that referenced this pull request Nov 18, 2023
darioalessandro added a commit that referenced this pull request Nov 18, 2023
darioalessandro added a commit that referenced this pull request Nov 18, 2023
…ndling into `VideoCallClient` (#137)" (#148)"

This reverts commit ad6dcc5.
darioalessandro added a commit that referenced this pull request Nov 18, 2023
…ndling into `VideoCallClient` (#137)" (#148)" (#149)

This reverts commit ad6dcc5.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants