interceptor icon indicating copy to clipboard operation
interceptor copied to clipboard

feat: Add playout delay header interceptor

Open kevmo314 opened this issue 2 years ago • 3 comments

This interceptor adds the playout delay header extension following https://webrtc.googlesource.com/src/+/refs/heads/main/docs/native-code/rtp-hdrext/playout-delay

kevmo314 avatar Jul 29 '22 07:07 kevmo314

Codecov Report

Merging #151 (ea6e6a3) into master (a82b843) will decrease coverage by 0.17%. The diff coverage is 60.00%.

:exclamation: Current head ea6e6a3 differs from pull request most recent head 3c49840. Consider uploading reports for the commit 3c49840 to get more accurate results

@@            Coverage Diff             @@
##           master     #151      +/-   ##
==========================================
- Coverage   79.05%   78.88%   -0.18%     
==========================================
  Files          63       65       +2     
  Lines        3189     3234      +45     
==========================================
+ Hits         2521     2551      +30     
- Misses        555      565      +10     
- Partials      113      118       +5     
Flag Coverage Δ
go 78.88% <60.00%> (-0.18%) :arrow_down:
wasm 76.77% <60.00%> (-0.24%) :arrow_down:

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

Impacted Files Coverage Δ
pkg/playoutdelay/header_extension_interceptor.go 58.62% <58.62%> (ø)
pkg/playoutdelay/playout_delay.go 62.50% <62.50%> (ø)
pkg/gcc/kalman.go 100.00% <0.00%> (+5.66%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Jul 29 '22 07:07 codecov[bot]

Do we need that to save bytes?

Yes, although since it's only a few bytes I figured I'd do it in a followup PR 😅

And second question: Is it enough to send fixed limits, or do we need to be able to update the limits of the interceptor?

The limits of the interceptor should be able to change but I'm actually not sure of a good way to do that. Since the developer only instantiates a factory, wouldn't they need access to the underlying interceptor to update the parameters on the fly? Do you have suggestions for such an API?

kevmo314 avatar Jul 29 '22 16:07 kevmo314

Yes, although since it's only a few bytes I figured I'd do it in a followup PR sweat_smile

Alright, follow up PR sounds fine to me :)

The limits of the interceptor should be able to change but I'm actually not sure of a good way to do that. Since the developer only instantiates a factory, wouldn't they need access to the underlying interceptor to update the parameters on the fly? Do you have suggestions for such an API?

In the bandwidth estimation and similar interceptors, you can register a callback that is called whenever the factory creates a new interceptor. The callback receives the instance of the interceptor for the PeerConnection. I am not sure if this is the best design we can have but I don't know of a better way.

mengelbart avatar Jul 29 '22 16:07 mengelbart