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

Add WHIP ICE Restart Support #267

Merged
merged 1 commit into from
May 17, 2024
Merged

Conversation

Sean-Der
Copy link
Contributor

No description provided.

@Sean-Der Sean-Der requested a review from a team as a code owner May 13, 2024 20:34
@Sean-Der
Copy link
Contributor Author

Marking as draft. Should be tested against a client before worth reviewing

@Sean-Der Sean-Der marked this pull request as draft May 13, 2024 20:38
Copy link
Contributor

@biglittlebigben biglittlebigben left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. I understand this is not finalized, but a few comments below.

pkg/whip/server.go Outdated Show resolved Hide resolved
pkg/whip/utils.go Show resolved Hide resolved
pkg/whip/utils.go Outdated Show resolved Hide resolved
pkg/whip/whip_handler.go Outdated Show resolved Hide resolved
@Sean-Der
Copy link
Contributor Author

Sean-Der commented May 13, 2024

Thanks for the review @biglittlebigben I will fix that up now!

Here is a jsfiddle of how I was testing https://jsfiddle.net/phn51xz9/

@Sean-Der Sean-Der force-pushed the ice-restart branch 4 times, most recently from 894bba3 to b9e0933 Compare May 14, 2024 03:04
@Sean-Der
Copy link
Contributor Author

Ok I feel pretty good about that @biglittlebigben

Testing with clients should happen Tuesday/Wednesday!

Copy link
Contributor

@biglittlebigben biglittlebigben left a comment

Choose a reason for hiding this comment

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

Thanks. Look good to me after updating the protocol version and decreasing the log level of some of the errors.

pkg/whip/server.go Outdated Show resolved Hide resolved
@Sean-Der Sean-Der force-pushed the ice-restart branch 2 times, most recently from 85f9b4c to 79f141a Compare May 14, 2024 18:50
@Sean-Der Sean-Der marked this pull request as ready for review May 14, 2024 19:32
@biglittlebigben
Copy link
Contributor

Please let me know when you happy with your testing and this is ready to merge.

@Sean-Der
Copy link
Contributor Author

@biglittlebigben testing went well, this is good for merge. I had to make one additional change though.

We now close the session when goes to failed (30 seconds of no activity) instead of disconnected (5 seconds of no activity)

The client side is waiting for 'disconnected'.

@biglittlebigben biglittlebigben merged commit a0cdd12 into livekit:main May 17, 2024
3 checks passed
biglittlebigben added a commit that referenced this pull request May 20, 2024
 ## Changelog

 ### Added

- Add WHIP ICE Restart Support (#267)
@biglittlebigben biglittlebigben mentioned this pull request May 20, 2024
biglittlebigben added a commit that referenced this pull request May 21, 2024
 ## Changelog

 ### Added

- Add WHIP ICE Restart Support (#267)
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.

2 participants