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

Add interceptor to aggregate CCFB reports #300

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

mengelbart
Copy link
Contributor

@mengelbart mengelbart commented Jan 16, 2025

Description

This interceptor keeps track of outgoing RTP packets and reads incoming CCFB (RFC8888) RTCP packets. For each incoming CCFB packet, it creates a report of the included acknowledgments and stores it in the interceptor attributes. Applications reading RTCP packets from it can read the report, e.g., to perform bandwidth estimation.

This is a PoC to improve bandwidth estimation in Pion. The current GCC implementation is hard to use and test. Giving applications access to the feedback reports could simplify and decouple the actual bandwidth estimation from the interceptors.

TODO

  • Add unit tests
  • Implement TWCC converter (currently, only RFC 8888 is supported)
  • Remove old entries from history
  • Document the semantics of the generated reports (currently, it generates a report for every packet included in the feedback packet, including the sequence number, size, departure timestamp, status (received or not), arrival timestamp, and ECN bits.

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 83.75000% with 52 lines in your changes missing coverage. Please review.

Project coverage is 72.26%. Comparing base (c06f448) to head (1072695).

Files with missing lines Patch % Lines
pkg/ccfb/interceptor.go 77.44% 22 Missing and 8 partials ⚠️
pkg/ccfb/twcc_receiver.go 70.83% 21 Missing ⚠️
internal/test/mock_stream.go 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #300      +/-   ##
==========================================
+ Coverage   71.38%   72.26%   +0.87%     
==========================================
  Files          79       84       +5     
  Lines        4526     4842     +316     
==========================================
+ Hits         3231     3499     +268     
- Misses       1158     1199      +41     
- Partials      137      144       +7     
Flag Coverage Δ
go 72.13% <83.75%> (+0.88%) ⬆️
wasm 70.42% <83.43%> (+0.93%) ⬆️

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.

@Sean-Der
Copy link
Member

This is so exciting! I am glad to see you back @mengelbart :)

@jech and @JoeTurki are getting deep into this world if you are looking for people to work with/quick reviews

@mengelbart mengelbart force-pushed the ccfb-receiver branch 2 times, most recently from 271a101 to 1fb32cc Compare January 16, 2025 15:51
@jech
Copy link
Member

jech commented Jan 16, 2025

Could you please give some background? Why not simply provide a function that parses the reports?

To my untrained eyes, it looks like your interceptor aims to allow a client to do:

_, attr, _ := track.ReadRTCP()
data := attr.Get(CCFBAttributesKey)

Without an interceptor, the same code would look like this:

p, _, _ := track.ReadRTCP()
var data CCFBReportPacket
data.Unmarshal(p.Data)

Now, perhaps I'm biased against interceptors (which are causing me a fair amount of suffering), but the second code looks clearer to me. What am I missing?

@mengelbart
Copy link
Contributor Author

The interceptor also keeps track of outgoing RTP packets, timestamps them, and matches RTCP reports to the history of sent packets. The report you get from the attributes then contains departure and arrival timestamps for each packet that was reported on in the RTCP. Without the interceptor, you have to keep track of the departure timestamps yourself.

@mengelbart mengelbart force-pushed the ccfb-receiver branch 2 times, most recently from 5244e8b to 7ad4eec Compare January 19, 2025 17:09
@mengelbart mengelbart mentioned this pull request Jan 20, 2025
@mengelbart mengelbart force-pushed the ccfb-receiver branch 5 times, most recently from 6cf49f5 to 89e56ac Compare January 27, 2025 15:12
@mengelbart mengelbart marked this pull request as ready for review January 27, 2025 15:33
@mengelbart mengelbart changed the title WIP: Add interceptor to aggregate CCFB reports Add interceptor to aggregate CCFB reports Jan 27, 2025
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