Skip to content

Add interceptor to aggregate CCFB reports#374

Merged
mengelbart merged 5 commits intomasterfrom
feat/rtpfb-receiver-2
Feb 4, 2026
Merged

Add interceptor to aggregate CCFB reports#374
mengelbart merged 5 commits intomasterfrom
feat/rtpfb-receiver-2

Conversation

@mengelbart
Copy link
Contributor

@mengelbart mengelbart commented Oct 15, 2025

Alternative but cleaner implementation for congestion control feedback receiver (#300). The main difference between this and #300 is that #300 organizes the feedback per stream, while this version organizes it as a single linear stream, which is easier to process. If one still needs to know which stream a packet was sent on, that information is still included in the reports for each packet.

@codecov
Copy link

codecov bot commented Oct 15, 2025

Codecov Report

❌ Patch coverage is 78.27476% with 68 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.78%. Comparing base (87546e8) to head (89b5044).
⚠️ Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
pkg/rtpfb/interceptor.go 73.94% 25 Missing and 6 partials ⚠️
pkg/rtpfb/twcc_receiver.go 69.11% 21 Missing ⚠️
pkg/rtpfb/history.go 81.39% 11 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #374      +/-   ##
==========================================
- Coverage   80.03%   79.78%   -0.26%     
==========================================
  Files          84       88       +4     
  Lines        4268     4581     +313     
==========================================
+ Hits         3416     3655     +239     
- Misses        681      741      +60     
- Partials      171      185      +14     
Flag Coverage Δ
go 79.78% <78.27%> (-0.26%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mengelbart mengelbart force-pushed the feat/rtpfb-receiver-2 branch 6 times, most recently from 0912350 to ceaeb98 Compare October 18, 2025 21:49
@mengelbart mengelbart marked this pull request as ready for review October 18, 2025 21:51
@mengelbart mengelbart requested review from JoTurk and Sean-Der October 27, 2025 20:31
Copy link
Member

@JoTurk JoTurk left a comment

Choose a reason for hiding this comment

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

Just two fast comments / nits, I'll read the RFC again and will actually review :)

}
}

return referenceTime.Sub(latestArrival), result
Copy link
Member

Choose a reason for hiding this comment

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

What will happen if latestArrival is never set? shouldn't we return 0, nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I think we should initialize latestArrival as latestArrival = referenceTime. That way it will be 0 if it is there were no packets received after the reference time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, this doesn't work because we are actually looking for the latest arrival before the referenceTime. So you're right, we have to add a check and return 0.

@mengelbart mengelbart force-pushed the feat/rtpfb-receiver-2 branch 2 times, most recently from e2d4aff to 82ff788 Compare December 3, 2025 14:30
@mengelbart mengelbart force-pushed the feat/rtpfb-receiver-2 branch 2 times, most recently from e5d1861 to 35b6c22 Compare January 28, 2026 15:44
@mengelbart mengelbart requested a review from JoTurk January 28, 2026 15:50
Copy link
Member

@JoTurk JoTurk left a comment

Choose a reason for hiding this comment

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

I'm so sorry for taking long with the second review, The code is great, and I think it works as expected would love if we can add more tests, esp around concurrency & life-cycle, but ofc this can be done in the future.

Thank you. sorry again.

@mengelbart
Copy link
Contributor Author

Thank you @JoTurk !

@mengelbart mengelbart force-pushed the feat/rtpfb-receiver-2 branch from fd061e7 to 89b5044 Compare February 4, 2026 13:04
@mengelbart mengelbart merged commit 048be58 into master Feb 4, 2026
19 checks passed
@mengelbart mengelbart deleted the feat/rtpfb-receiver-2 branch February 4, 2026 13:08
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