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

Partial rewrite of TWCC sender #198

Merged
merged 2 commits into from
Sep 29, 2023
Merged

Conversation

kcaffrey
Copy link
Contributor

The previous implementation of the TWCC sender interceptor had some inconsistencies with libwebrtc. Namely, if there were missing packets between the last packet in the previous feedback interval and the first received packet in the following feedback interval, then those packets were never included as missing in any feedback.

This is an issue because libwebrtc uses data about lost packets from TWCC feedback in their congestion controller. This means that bursts of loss could go unnoticed by libwebrtc, causing the application to possibly send more data than it would if the loss was properly reported.

Another minor difference was that feedback packets with a single packet are in fact valid according to the RFC, but we were only returning feedback with at least two packets. libwebrtc has a check for minimum feedback size, which has now been added to the unit tests.

Just about all of the rewrite here has been ported from libwebrtc code, so it should now match the behavior fairly well.

@kcaffrey kcaffrey requested review from Sean-Der and mengelbart July 27, 2023 03:01
@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Attention: 25 lines in your changes are missing coverage. Please review.

Files Coverage Δ
pkg/twcc/arrival_time_map.go 100.00% <100.00%> (ø)
pkg/twcc/sender_interceptor.go 84.07% <100.00%> (ø)
pkg/twcc/twcc.go 86.97% <73.40%> (-8.11%) ⬇️

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@kcaffrey kcaffrey force-pushed the rewrite-twcc branch 2 times, most recently from 97941d5 to 4799d60 Compare July 27, 2023 03:08
@kcaffrey
Copy link
Contributor Author

I see #202 got merged, but I think this PR would actually replace it and do the same deferred sorting but in O(N) instead of O(N log N). This is because I walk through the sequence numbers in order to build feedback (note that since missing packets have to be added anyway, this adds no overhead). Simply keeping track of the start/end sequence numbers and storing arrival times in a circular buffer lets you do a simple for loop to get the sorted arrival times; no explicit sort needed.

@davidzhao @treyhakanson can you take a look and confirm that my logic above is correct, and that the proper way to resolve this change with #202 is to simply overwrite the changes?

@davidzhao davidzhao requested a review from boks1971 September 27, 2023 23:40
Copy link
Contributor

@boks1971 boks1971 left a comment

Choose a reason for hiding this comment

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

Great stuff @kcaffrey !! LGTM!

But, would be good to get some more 👀 . Will also ask @treyhakanson who recently worked on this.

Left a few comments for your consideration.

pkg/twcc/twcc_test.go Show resolved Hide resolved
pkg/twcc/arrival_time_map.go Show resolved Hide resolved
pkg/twcc/arrival_time_map.go Show resolved Hide resolved
pkg/twcc/twcc.go Show resolved Hide resolved
pkg/twcc/twcc.go Show resolved Hide resolved
pkg/twcc/twcc.go Outdated Show resolved Hide resolved
pkg/twcc/twcc.go Show resolved Hide resolved
pkg/twcc/twcc.go Show resolved Hide resolved
kcaffrey added a commit to kcaffrey/interceptor that referenced this pull request Sep 28, 2023
Reverted in favor of pion#198

This reverts commit dd00fe3.
The previous implementation of the TWCC sender interceptor
had some inconsistencies with libwebrtc. Namely, if there
were missing packets between the last packet in the previous
feedback interval and the first received packet in the
following feedback interval, then those packets were never
included as missing in any feedback.

This is an issue because libwebrtc uses data about lost packets
from TWCC feedback in their congestion controller. This means
that bursts of loss could go unnoticed by libwebrtc, causing
the application to possibly send more data than it would if
the loss was properly reported.

Another minor difference was that feedback packets with a
single packet are in fact valid according to the RFC, but
we were only returning feedback with at least two packets.
libwebrtc has a check for minimum feedback size, which has
now been added to the unit tests.

Just about all of the rewrite here has been ported from
libwebrtc code, so it should now match the behavior fairly
well.
@kcaffrey
Copy link
Contributor Author

Ok, I have reverted #202 and rebased this branch on top of master. I left it as two separate commits to clarify the revert of #202.

@adriancable adriancable merged commit 38fe7f5 into pion:master Sep 29, 2023
15 checks passed
adriancable pushed a commit that referenced this pull request Sep 29, 2023
Reverted in favor of #198

This reverts commit dd00fe3.
@kcaffrey kcaffrey deleted the rewrite-twcc branch September 29, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants