-
Notifications
You must be signed in to change notification settings - Fork 122
Data channel reliability #688
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
Conversation
fd561d6 to
38d4f36
Compare
Use system time and duration Add iterator method Organize Doc Add cleanup
- Root kind field applying to all event types - Detail enum - Optional completion tx channel - Prefer named fields - Publish data request gets its own type
- Prefer packet level participant identity and SID for user packet - Take packet value to avoid many unnecessary clones
This reverts commit 618534b.
Ensure all outgoing data packets have their participant identity and SID fields set by handling this as a low-level protocol detail in session
Install and run a LiveKit server before running tests
38d4f36 to
1993355
Compare
0e26219 to
9710265
Compare
|
Once this is merged, I will update Python. |
bcherry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm but would be good for @cnderrauber to confirm as well
| self._send_until_threshold(threshold, &mut reliable_buffered_amount, &mut reliable_queue); | ||
| self._send_until_threshold(DataPacketKind::Reliable, threshold, &mut reliable_buffered_amount, &mut reliable_queue); | ||
| // TODO: Ensure this is the proper quantity | ||
| let retry_min_amount = (threshold as usize * 5) / 4; // threshold * 1.25 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In js client it is the size of buffered messages (sent but not acked by sfu) to avoid lost message during reconnecting, is the threshold same as the js client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The threshold I used here was taken from the Swift implementation (DataChannelPair.swift:375); I will take a look at how this is handled in JS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cnderrauber, I did some stress testing, and it looks like the packet loss observed in Swift during testing cannot be reproduced in Rust, so I've gone ahead and removed the additional buffering.
cnderrauber
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
Summary of changes: