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

@pionbot pionbot force-pushed the playout-delay-extension branch from aab5891 to 3ab918a Compare July 29, 2022 07:21
@kevmo314 kevmo314 force-pushed the playout-delay-extension branch from 3ab918a to 0aae0ae Compare July 29, 2022 07:22
@kevmo314 kevmo314 requested review from Sean-Der and mengelbart July 29, 2022 07:23
@pionbot pionbot force-pushed the playout-delay-extension branch from 0aae0ae to cc23b0e Compare July 29, 2022 07:23
@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.

@kevmo314 kevmo314 force-pushed the playout-delay-extension branch from cc23b0e to 4ad27e3 Compare July 29, 2022 07:26
@pionbot pionbot force-pushed the playout-delay-extension branch from 4ad27e3 to fde19d2 Compare July 29, 2022 07:27
@kevmo314 kevmo314 force-pushed the playout-delay-extension branch from fde19d2 to 380cc72 Compare July 29, 2022 07:27
@pionbot pionbot force-pushed the playout-delay-extension branch from 380cc72 to da7c156 Compare July 29, 2022 07:28
@kevmo314 kevmo314 force-pushed the playout-delay-extension branch from da7c156 to 9c19688 Compare July 29, 2022 07:30
@pionbot pionbot force-pushed the playout-delay-extension branch from 9c19688 to 39a4b2e Compare July 29, 2022 07:30
@kevmo314 kevmo314 force-pushed the playout-delay-extension branch from 39a4b2e to 3f392dc Compare July 29, 2022 07:31
@pionbot pionbot force-pushed the playout-delay-extension branch from 3f392dc to e69fdde Compare July 29, 2022 07:31
@kevmo314 kevmo314 force-pushed the playout-delay-extension branch from e69fdde to b16c5ce Compare July 29, 2022 07:55
@pionbot pionbot force-pushed the playout-delay-extension branch from b16c5ce to ea6e6a3 Compare July 29, 2022 07:56
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