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

Playout delay support #668

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

luc4s
Copy link

@luc4s luc4s commented Mar 15, 2022

Hi,

First, thanks for the awesome library :)

I needed playout delay header extension support for a low latency streaming application (https://webrtc.googlesource.com/src/+/refs/heads/main/docs/native-code/rtp-hdrext/playout-delay/README.md), so I thought I could share it.

src/aiortc/rtcrtpsender.py Outdated Show resolved Hide resolved
src/aiortc/rtcrtpsender.py Outdated Show resolved Hide resolved
src/aiortc/rtcrtpsender.py Outdated Show resolved Hide resolved
@jlaine
Copy link
Collaborator

jlaine commented Mar 25, 2022

This PR has no unit test?

@luc4s
Copy link
Author

luc4s commented Mar 25, 2022

True, I haven't added any, I will add some tests.

@luc4s
Copy link
Author

luc4s commented Mar 30, 2022

Ok so I added a test for the playout delay and adapted the ones for header extensions.

Also I was wondering: regarding setPlayoutDelay, is raising a ValueError when one of the delays is out of range ok for you?
I was also thinking of clamping them, but then I did not like having them 'silently' clamped.

@codecov
Copy link

codecov bot commented Apr 3, 2022

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (9f14474) 100.00% compared to head (13bf23d) 100.00%.

❗ Current head 13bf23d differs from pull request most recent head 2a5a978. Consider uploading reports for the commit 2a5a978 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #668   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           30        31    +1     
  Lines         5816      5801   -15     
=========================================
- Hits          5816      5801   -15     
Impacted Files Coverage Δ
src/aiortc/codecs/__init__.py 100.00% <ø> (ø)
src/aiortc/rtcrtpsender.py 100.00% <100.00%> (ø)
src/aiortc/rtp.py 100.00% <100.00%> (ø)

... and 16 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@luc4s
Copy link
Author

luc4s commented Apr 5, 2022

Is there a problem with the runners? Seems like the test never started:

2022-04-04T13:49:27.6313834Z Requested labels: macos-latest
2022-04-04T13:49:27.6313880Z Job defined at: aiortc/aiortc/.github/workflows/tests.yml@refs/pull/668/merge
2022-04-04T13:49:27.6313901Z Waiting for a runner to pick up this job...
2022-04-04T13:49:30.3209105Z Job is waiting for a hosted runner to come online.
2022-04-04T13:49:42.6601603Z Job is about to start running on the hosted runner: GitHub Actions 4 (hosted)

@luc4s luc4s requested a review from jlaine May 4, 2022 07:11
@DerGenaue
Copy link
Contributor

@jlaine Is there a chance for this PR to progress some time soon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants