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

New Codec selection logic #2296

Merged
merged 8 commits into from
Jul 3, 2023

Conversation

jallamsetty1
Copy link
Member

Use a configurable preferred codecs list to select the codecs.

Allow asymmetric codecs to be configured on the endpoints. This means that Firefox and Safari which have bugs with VP9 encode will now encode VP8 but decode VP9 coming in from Chromium endpoints.

…ct the codecs.

Allow asymmetric codecs to be configured on the endpoints. This means that Firefox and Safari which have bugs with VP9 encode will now encode VP8 but decode VP9 coming in from Chromium endpoints.
@jallamsetty1
Copy link
Member Author

Jenkins test this please.

Copy link
Member

@saghul saghul left a comment

Choose a reason for hiding this comment

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

Did a round!

JitsiConference.js Show resolved Hide resolved
JitsiConference.js Show resolved Hide resolved
JitsiConference.js Show resolved Hide resolved
modules/RTC/CodecSelection.js Show resolved Hide resolved
modules/RTC/CodecSelection.js Outdated Show resolved Hide resolved
modules/RTC/TraceablePeerConnection.js Outdated Show resolved Hide resolved
modules/RTC/TraceablePeerConnection.js Show resolved Hide resolved
modules/RTC/TraceablePeerConnection.js Show resolved Hide resolved
modules/browser/BrowserCapabilities.js Outdated Show resolved Hide resolved
service/RTC/CodecMimeType.js Outdated Show resolved Hide resolved
On participant join/leave, check if the new intersection of codecs are already configured to be the top n codecs.
@jallamsetty1
Copy link
Member Author

Failure seems to be a bridge issue where the bridge stops sending media to p2.

@jallamsetty1 jallamsetty1 requested a review from saghul June 26, 2023 18:22
saghul
saghul previously approved these changes Jun 27, 2023
Copy link
Member

@saghul saghul left a comment

Choose a reason for hiding this comment

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

LGTM with a question. Can you pl test mobile before landing?

JitsiConference.js Show resolved Hide resolved
…-lines.

Also, ignore remote codecs published in presence for p2p connections.
Fix an issue where p2p between mobile and desktop was broken.
Copy link
Member

@saghul saghul left a comment

Choose a reason for hiding this comment

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

One minor thing (RN would like to prefer H.264 for P2P) otherwise LGTM, very excited for this to land!

modules/RTC/CodecSelection.js Show resolved Hide resolved
Munge the initial offer sent out by RN clients since RN doesn't support RTCRtpTransceiver#setCodecPreferences.
@jallamsetty1 jallamsetty1 requested a review from saghul July 3, 2023 14:06
@saghul
Copy link
Member

saghul commented Jul 3, 2023

LGTM, awesome work! Can you pl give it a go on mobile to make sure everything is a-ok?

@jallamsetty1 jallamsetty1 merged commit 36a9712 into jitsi:master Jul 3, 2023
1 check passed
@jallamsetty1 jallamsetty1 deleted the feat-multiple-codecs branch July 3, 2023 16:16
nils-ohlmeier pushed a commit to nils-ohlmeier/lib-jitsi-meet that referenced this pull request Jul 10, 2023
* fix(codec-selection) Use a configurable preferred codecs list to select the codecs.
Allow asymmetric codecs to be configured on the endpoints. This means that Firefox and Safari which have bugs with VP9 encode will now encode VP8 but decode VP9 coming in from Chromium endpoints.

* fix(codec-selection) Add unit tests for the codec selection logic.

* feat(codec-selection): Introduce mobileCodecPreferenceOrder setting in config.js.

* fix(codec-selection) Avoid unnecessary renegotiations.
On participant join/leave, check if the new intersection of codecs are already configured to be the top n codecs.

* Address review comments

* fix: Strip the codecs that are not in the codec list from the video m-lines.
Also, ignore remote codecs published in presence for p2p connections.

* fix: Define default codec order for mobile and desktop.
Fix an issue where p2p between mobile and desktop was broken.

* fix: Add default codecs for both p2p and jvb on mobile devices.
Munge the initial offer sent out by RN clients since RN doesn't support RTCRtpTransceiver#setCodecPreferences.
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.

2 participants