-
Notifications
You must be signed in to change notification settings - Fork 296
Update WebRTC message size limit #628
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -88,9 +88,9 @@ QUIC. The libp2p WebRTC message framing is not concerned with flow-control and | |||||||||||||||
| thus does not need the `RESET_STREAM` frame to be send in reply to a | ||||||||||||||||
| `STOP_SENDING` frame. | ||||||||||||||||
|
|
||||||||||||||||
| Encoded messages including their length prefix MUST NOT exceed 16kiB to support | ||||||||||||||||
| all major browsers. See ["Understanding message size | ||||||||||||||||
| limits"](https://developer.mozilla.org/en-US/docs/Web/API/WebRTC_API/Using_data_channels#understanding_message_size_limits). | ||||||||||||||||
| Encoded messages including their length prefix MUST NOT exceed 256kiB to support | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why 256kB, can we support more? Can we add links for the browser specific limts here? |
||||||||||||||||
| all major browsers. See ["Large Data Channel Messages"](https://blog.mozilla.org/webrtc/large-data-channel-messages/) | ||||||||||||||||
| and [WebRTC#40644524](https://issues.webrtc.org/issues/40644524). | ||||||||||||||||
|
Comment on lines
+91
to
+93
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on internal discussions, I believe we've decided to keep it at 16KiB to remain compatible with Kubo 0.30.0. @achingbrain what is the next step here? figure out next steps without breaking interop? (e.g. how to negotiate higher size limit per connection via something like RTCSctpTransport/maxMessageSize?)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’m doing some experiments in the test plans repo to validate that we do in fact see a throughput increase with larger messages. Marking this as a draft pending the outcome of that.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See linked performance graphs here: #628 (comment) |
||||||||||||||||
| Implementations MAY choose to send smaller messages, e.g. to reduce delays | ||||||||||||||||
| sending _flagged_ messages. | ||||||||||||||||
|
Comment on lines
94
to
95
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we account for forwards compatibility with future implementations that aren't arbitrarily limited like browsers currently?
Suggested change
A "be liberal in what you accept, conservative in what you emit" sort of thing.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer separate sentences since they're both important and about different things. |
||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
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.
Can you edit the webrtc-direct file to remove this line where we recommend a max-message size of 16kB
https://github.com/libp2p/specs/blob/1253e2a1deec908037fba924fcd350ec16df13a6/webrtc/webrtc-direct.md?plain=1#L79