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

feat: Add playout delay header interceptor #151

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kevmo314
Copy link

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

@codecov
Copy link

codecov bot commented Jul 29, 2022

Codecov Report

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

❗ 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%) ⬇️
wasm 76.77% <60.00%> (-0.24%) ⬇️

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%) ⬆️

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

Copy link
Contributor

@mengelbart mengelbart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very familiar with the extension but have two questions: The document mentions that a sender may stop sending the extension if it receives a RR indicating that at it has been received at least once. Do we need that to save bytes?
And second question: Is it enough to send fixed limits, or do we need to be able to update the limits of the interceptor?


const playoutDelayURI = "http://www.webrtc.org/experiments/rtp-hdrext/playout-delay"

// BindLocalStream returns a writer that adds a rtp.TransportCCExtension
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy paste rtp.TransportCCExtension?

pkg/playoutdelay/header_extension_interceptor.go Outdated Show resolved Hide resolved
// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
// | ID | len=2 | MIN delay | MAX delay |
// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
type Extension struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be part of pion/rtp

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created pion/rtp#196 to add it to pion/rtp.

@kevmo314
Copy link
Author

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?

@mengelbart
Copy link
Contributor

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.

This interceptor adds the playout delay header extension following
http://www.webrtc.org/experiments/rtp-hdrext/playout-delay
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.

2 participants