Add interceptor to aggregate CCFB reports #300
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #300 +/- ##
==========================================
+ Coverage 71.83% 72.56% +0.73%
==========================================
Files 79 84 +5
Lines 4743 5071 +328
==========================================
+ Hits 3407 3680 +273
- Misses 1199 1245 +46
- Partials 137 146 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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
force-pushed
the
ccfb-receiver
branch
2 times, most recently
from
271a101 to
1fb32cc
Compare
|
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? |
|
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
force-pushed
the
ccfb-receiver
branch
2 times, most recently
from
5244e8b to
7ad4eec
Compare
mengelbart
force-pushed
the
ccfb-receiver
branch
6 times, most recently
from
89e56ac to
b8e4acd
Compare
mengelbart
changed the title
mengelbart
force-pushed
the
ccfb-receiver
branch
from
1072695 to
abbd5eb
Compare
mengelbart
force-pushed
the
ccfb-receiver
branch
from
561a285 to
842f423
Compare
|
I tried to make all of the new linters happy, but I added |
mengelbart
force-pushed
the
ccfb-receiver
branch
from
dc4d48f to
27d47e8
Compare
|
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. |
| } | ||
| } | ||
|
|
||
| func twccConverterFactory(f func(feedback *rtcp.TransportLayerCC) (time.Time, map[uint32][]acknowledgement)) Option { |
There was a problem hiding this comment.
Looks like twcc is supported too (not just rfc8888), should we update the description and title of this PR?
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