Support publishing average loss rate

[kevmo314]

This change exposes an API for reading the loss rate.

[codecov[bot]]

Codecov Report

Merging #137 (532a4ca) into master (0748586) will decrease coverage by 0.23%. The diff coverage is 50.00%.

:exclamation: Current head 532a4ca differs from pull request most recent head a1d068c. Consider uploading reports for the commit a1d068c to get more accurate results

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
- Coverage   79.30%   79.07%   -0.24%     
==========================================
  Files          51       51              
  Lines        2489     2499      +10     
==========================================
+ Hits         1974     1976       +2     
- Misses        417      423       +6     
- Partials       98      100       +2     

Flag	Coverage Δ
go	79.07% <50.00%> (-0.24%)	:arrow_down:
wasm	77.75% <50.00%> (-0.12%)	:arrow_down:

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

Impacted Files	Coverage Δ
pkg/gcc/send_side_bwe.go	76.59% <50.00%> (-2.04%)	:arrow_down:
pkg/gcc/kalman.go	94.33% <0.00%> (-5.67%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0748586...a1d068c. Read the comment docs.

[mengelbart]

Hi @kevmo314 sorry I didn't reply to this earlier. I don't think we should add the loss rate to the BandwidthEstimator interface, because it is not clear if all implementations will use or even know about packet loss. Also, the implementation of the GCC estimator currently does not really reflect how the RTP packet loss rate should be calculated.

I just started work on a new interceptor to keep track of all kinds of RTP and RTCP-related statistics in #143 which includes loss rate in a cleaner implementation. Would that interceptor be a good alternative for your use case?

[kevmo314]

Hi @kevmo314 sorry I didn't reply to this earlier. I don't think we should add the loss rate to the BandwidthEstimator interface, because it is not clear if all implementations will use or even know about packet loss. Also, the implementation of the GCC estimator currently does not really reflect how the RTP packet loss rate should be calculated.

I just started work on a new interceptor to keep track of all kinds of RTP and RTCP-related statistics in #143 which includes loss rate in a cleaner implementation. Would that interceptor be a good alternative for your use case?

Seems reasonable, but aren't SR's sent at much lower frequency? The loss rate in our use case gets piped to the Opus/FEC encoder so something that's sent at 1 Hz might be too slow to respond to a sudden increase in loss. I guess I don't mind too much either way, I suppose the more direct feature request is access to the underlying congestion controller somehow :)

[mengelbart]

Mh, interesting, no idea if that will be helpful for you then. Anyway, I think the GCC interceptor allows you to get averageLoss via the GetStats, too, but I think that may change at some point or at least the way the value is calculated may change in the future. If you just want full control of the congestion controller, you can also pass your own BandwidthEstimator implementation, e.g. by forking the one in pkg/gcc and adding/removing whatever you need :)