Partial rewrite of TWCC sender #198
Conversation
Codecov ReportAttention:
... and 1 file with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
kcaffrey
force-pushed
the
rewrite-twcc
branch
2 times, most recently
from
97941d5
to
4799d60
Compare
|
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? |
There was a problem hiding this comment.
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.
kcaffrey
force-pushed
the
rewrite-twcc
branch
from
4799d60
to
0f32f12
Compare
kcaffrey
force-pushed
the
rewrite-twcc
branch
from
0f32f12
to
f89f02a
Compare
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
force-pushed
the
rewrite-twcc
branch
from
854b15e
to
7dddc3b
Compare
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.