Add interceptor to aggregate CCFB reports

[mengelbart]

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.

[codecov]

Codecov Report

❌ Patch coverage is 82.83133% with 57 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.95%. Comparing base (b94d244) to head (266b5e5).

Files with missing lines	Patch %	Lines
pkg/rtpfb/interceptor.go	77.98%	26 Missing and 9 partials ⚠️
pkg/rtpfb/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   78.64%   78.95%   +0.31%     
==========================================
  Files          82       86       +4     
  Lines        5165     5493     +328     
==========================================
+ Hits         4062     4337     +275     
- Misses        929      974      +45     
- Partials      174      182       +8     

Flag	Coverage Δ
go	78.95% <82.83%> (+0.31%)	⬆️
wasm	76.55% <82.53%> (+0.40%)	⬆️

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.

[Sean-Der]

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

[jech]

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]

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]

I tried to make all of the new linters happy, but I added nolint in a few places...

[3DRX]

Hi @mengelbart what a cool PR! I'm curious to learn what's the relationship between this interceptor, the pacer, and bandwidth estimator GCC will be like by your design. For users using this interceptor, they need to make sure it goes after the pacer right? Otherwise the captured send time will be incorrect. I also take a brief look at the GCC refactor, and it seems like pacing is not included in it.

[3DRX]

Looks like twcc is supported too (not just rfc8888), should we update the description and title of this PR?

[mengelbart]

@3DRX right, pacing is not included in the GCC refactoring PR, because the new design only works on received feedback and everything it does is independent of the packet transmission part. Pacing can still be done in a new interceptor or even before writing packets to the track.

[JoeTurki]

looks good to me, thank you

[JoeTurki]

Correct me if I'm wrong, but could this panic if there's no history?
It also seems like it could be abused to trigger a panic for an SSRC that doesn’t have any history yet (though I’m not sure if that’s actually possible).
Additionally, is there a potential race condition here with BindLocalStream?

[mengelbart]

Good points. I added a check and skip for unknown SSRCs. I also made the lock an RWLock so we can concurrently read the history map for sending RTP packets and receive RTCP packets. And I added a lock around the missing header extension error case in the RTPWriter.

[JoeTurki]

Thanks!