-
Notifications
You must be signed in to change notification settings - Fork 50
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
Implement Transport Wide Congestion Control #40
Conversation
Codecov Report
@@ Coverage Diff @@
## master #40 +/- ##
==========================================
+ Coverage 79.16% 80.89% +1.72%
==========================================
Files 21 24 +3
Lines 600 806 +206
==========================================
+ Hits 475 652 +177
- Misses 85 102 +17
- Partials 40 52 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
pkg/twcc/header_extension.go
Outdated
if id == 0 { | ||
return nil, ErrInvalidExtensionHeaderID | ||
} | ||
return &HeaderExtensionInterceptor{extensionHeaderID: id}, nil |
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 id
will need to be determined in the Bind
This happens when a PeerConnection is created. Since we haven't done signaling yet we will not have negotiated IDs yet.
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.
Makes sense. I pushed another commit where I take the id
from the StreamInfo
in Bind
.
Fantastic work @mengelbart! We should get this merged ASAP I will do a quick review now and we can merged to master. We can use it without enabling it by default in |
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 !
I am sorry I dropped the ball on this @mengelbart Thank you so much @Antonito for being a great reviewer. I am in favor of turning this on by default and releasing a new minor version of Pion WebRTC (TWCC support is very exciting!) |
@mengelbart I would prefer you squash + merge, but it is totally your call! My goal is just to have commits in a format that are easily debuggable/readable for others in the future. After you squash+merge you should tag and experiment with turning it on by default in pion/webrtc :) |
Add one interceptor to add the header extension to outgoing packets and one interceptor to generate transport wide congestion control reports as described in https://datatracker.ietf.org/doc/html/draft-holmer-rmcat- transport-wide-cc-extensions-01
Description
https://datatracker.ietf.org/doc/html/draft-holmer-rmcat-transport-wide-cc-extensions-01