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

Fix AV1 and VP9 codec matching #2772

Merged
merged 2 commits into from
Jun 25, 2024
Merged

Fix AV1 and VP9 codec matching #2772

merged 2 commits into from
Jun 25, 2024

Conversation

aler9
Copy link
Member

@aler9 aler9 commented May 27, 2024

Description

Currently, AV1 or VP9 formats are matched regardless of the profile parameter. This was not noticeable until browsers started advertising multiple VP9 formats at the same time, each with a different profile ID, in order to allow the counterpart to send different streams on the basis of supported profiles.

This causes two issues: first, the library includes in the SDP all formats passed by the browser, regardless of the fact that the profile ID is registered in the API or not. Then, the library is unable to choose the correct format for streaming, causing an intermittent failure.

This patch fixes the matching algorithm and also covers the case in which the profile ID is missing, by using values dictated by specifications.

Tests were refactored since previous ones covered the same lines multiple times.

Related to #1912

@aler9 aler9 changed the title Fix codec matching in case of AV1 or VP9 with different profiles Fix codec matching with AV1 or VP9 and different profiles May 27, 2024
Copy link

codecov bot commented May 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.10%. Comparing base (fc3521e) to head (c605ab8).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2772      +/-   ##
==========================================
+ Coverage   78.99%   79.10%   +0.10%     
==========================================
  Files          87       89       +2     
  Lines        8204     8249      +45     
==========================================
+ Hits         6481     6525      +44     
  Misses       1257     1257              
- Partials      466      467       +1     
Flag Coverage Δ
go 80.72% <100.00%> (+0.10%) ⬆️
wasm 64.98% <89.28%> (+0.49%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aler9 aler9 changed the title Fix codec matching with AV1 or VP9 and different profiles Fix AV1 and VP9 codec matching May 27, 2024
Currently, AV1 or VP9 formats are matched regardless of the profile
parameter. This was not noticeable until browsers started advertising
multiple VP9 formats at the same time, each with a different profile
ID, in order to allow the counterpart to send different streams on the
basis of supported profiles.

This causes two issues: first, the library includes in the SDP all
formats passed by the browser, regardless of the fact that the profile
ID is registered in the API or not. Then, the library is unable to
choose the correct format for streaming, causing an intermittent
failure.

This patch fixes the matching algorithm and also covers the case in
which the profile ID is missing, by using values dictated by
specifications.

Tests were refactored since previous ones covered the same lines
multiple times.
Copy link
Member

@edaniels edaniels left a comment

Choose a reason for hiding this comment

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

This LGTM. Just one comment. You can merge if you don't want to address it :)

internal/fmtp/av1.go Show resolved Hide resolved
@aler9
Copy link
Member Author

aler9 commented Jun 25, 2024

@edaniels thanks for the review, the issue in editing this patch is that is has already been merged inside the v3 branch (#2773), therefore editing it would de-synchronize the master branch from the v3 branch. I'll create a dedicated patch that adds the comment you requested.

@aler9 aler9 merged commit 31c2c0d into pion:master Jun 25, 2024
17 checks passed
@aler9 aler9 deleted the patch-av1-vp9 branch June 25, 2024 20:10
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