Skip to content

Expose DataChannel buffer status events #654

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

Merged
merged 7 commits into from
Apr 18, 2023
Merged

Expose DataChannel buffer status events #654

merged 7 commits into from
Apr 18, 2023

Conversation

lukasIO
Copy link
Contributor

@lukasIO lukasIO commented Apr 14, 2023

(will revert the changes to example.ts, but helps with testing)

@changeset-bot
Copy link

changeset-bot bot commented Apr 14, 2023

🦋 Changeset detected

Latest commit: ca77ee6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
livekit-client Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@lukasIO lukasIO marked this pull request as ready for review April 14, 2023 10:11
@lukasIO lukasIO requested a review from davidzhao April 14, 2023 10:11
Copy link
Member

@davidzhao davidzhao left a comment

Choose a reason for hiding this comment

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

The implementation looks good!

Wanted to think through the API a bit more though. Is there any way for this to be easier to use? The current design is event -> push. which follows the data channel usage pretty closely. However, this seems kind of tricky to write programs for. Primarily because we'd leave the user to coordinate event handler code with their data source. (it's also difficult to know how much data to write at the same time)

@lukasIO
Copy link
Contributor Author

lukasIO commented Apr 17, 2023

Options to make the API easier would most likely just move the buffer to a different place in the code.
E.g. if the client sdk was responsible of sending data when the channel is ready to send more data, we would have to buffer incoming messages within the sdk instead. Not sure if that's preferable.

There are two different scenarios I can think of:

  1. Send a large amount of data as quickly as possible
  2. Send updates (e.g. positional data) as often as possible, it's important for the latest state to be sent, but less important that every state change gets sent out (e.g. I'd rather send the current position instead of sending outdated positions a couple of times while waiting for the buffer to clear)

@davidzhao
Copy link
Member

Options to make the API easier would most likely just move the buffer to a different place in the code. E.g. if the client sdk was responsible of sending data when the channel is ready to send more data, we would have to buffer incoming messages within the sdk instead. Not sure if that's preferable.

There are two different scenarios I can think of:

  1. Send a large amount of data as quickly as possible
  2. Send updates (e.g. positional data) as often as possible, it's important for the latest state to be sent, but less important that every state change gets sent out (e.g. I'd rather send the current position instead of sending outdated positions a couple of times while waiting for the buffer to clear)

Yeah that's fair. for 2. the current API should still work ok I guess. Agreed we shouldn't cause everything to be buffered and keeping the native data channel notifications is the low-friction path.

@lukasIO lukasIO merged commit 958eef2 into main Apr 18, 2023
@lukasIO lukasIO deleted the lukas/dc-buffer branch April 18, 2023 07:35
@github-actions github-actions bot mentioned this pull request Apr 18, 2023
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