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.

Sign up for GitHub

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

Jump to bottom

[MM-52657] AV1 support #8037

Merged
merged 6 commits into from
Jul 26, 2024
Merged

[MM-52657] AV1 support #8037

merged 6 commits into from
Jul 26, 2024

Conversation

streamer45
Copy link
Contributor

Summary

PR adds experimental support for receiving AV1-encoded screen-sharing tracks. The change on this side is minimal as we only need to detect whether we support receiving AV1 or not. This is done through the new RTCPeer.getVideoCodec static method.

To simplify further, I also modified some code in calls-common to make use of WebRTC globals that can be conveniently registered using the provided registerGlobals function.

Related PRs

mattermost/mattermost-plugin-calls#795
mattermost/calls-common#34

Ticket Link

https://mattermost.atlassian.net/browse/MM-52657

Release Note

Calls: added experimental support for receiving AV1 encoded screen sharing tracks.

@streamer45 streamer45 added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Jun 24, 2024
@streamer45 streamer45 requested a review from cpoile June 24, 2024 14:46
@streamer45 streamer45 self-assigned this Jun 24, 2024
@streamer45 streamer45 added the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Jun 24, 2024
cpoile
cpoile approved these changes Jun 27, 2024
View reviewed changes
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Tried an old version of mobile with new server, new version of mobile with new server, and just in case: new version of mobile with old server. Everything works. Thank you!

app/products/calls/connection/connection.ts
Comment on lines +83 to +84
// Registering WebRTC globals (e.g. RTCPeerConnection)
registerGlobals();
Copy link
Member

Choose a reason for hiding this comment

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

Oh, nice! Didn't notice they had that. Kind of embarrassed I missed it, tbh...

@streamer45 streamer45 requested a review from enahum June 27, 2024 06:44
@streamer45 streamer45 removed the 2: Dev Review Requires review by a core commiter label Jun 27, 2024
@streamer45 streamer45 mentioned this pull request Jun 28, 2024
Merged
@DHaussermann DHaussermann added the Build Apps for PR Build the mobile app for iOS and Android to test label Jul 15, 2024
@DHaussermann
Copy link

Hi @streamer45,
Does this require testing beyond what receiving video with AV1 on as per this PR here?
mattermost/mattermost-plugin-calls#795

@streamer45
Copy link
Contributor Author

Hi @streamer45, Does this require testing beyond what receiving video with AV1 on as per this PR here? mattermost/mattermost-plugin-calls#795

@DHaussermann The idea is to run this mobile build against both the latest plugin release and the AV1 PR so we can make sure no regressions related to screen sharing exist.

@DHaussermann DHaussermann self-requested a review July 19, 2024 20:22
DHaussermann
DHaussermann approved these changes Jul 19, 2024
View reviewed changes
Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

  • Tested current release of Calls with prod mobile release
  • Tested current release of Calls with mobile builds from this PR
  • Tested Calls AV1 PR with AV1 enable in combination with these builds and production mobile builds
  • Tested Calls AV1 PR with AV1 disable in combination with these builds and production mobile builds

In all cases the Android and iOS clients had no issues receiving the video stream.

LGTM!

@DHaussermann DHaussermann added QA Review Done and removed Build Apps for PR Build the mobile app for iOS and Android to test 3: QA Review Requires review by a QA tester labels Jul 19, 2024
@streamer45 streamer45 added 4: Reviews Complete All reviewers have approved the pull request and removed Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) labels Jul 26, 2024
@streamer45 streamer45 merged commit 7682c5c into main Jul 26, 2024
4 checks passed
@streamer45 streamer45 deleted the MM-52657 branch July 26, 2024 08:01
@amyblais amyblais added this to the v2.20.0 milestone Jul 26, 2024
@amyblais amyblais added the Docs/Needed Requires documentation label Jul 26, 2024
@cwarnermm
Copy link
Member

@streamer45 - Is there product or developer documentation impact expected for this Engineering PR?

@streamer45
Copy link
Contributor Author

@streamer45 - Is there product or developer documentation impact expected for this Engineering PR?

@cwarnermm There's a new system console config setting to control this functionality that we should likely document.

@cwarnermm
Copy link
Member

@streamer45 - Can you direct me to those details please? I'm not seeing code in this PR that looks like a new config setting.

@streamer45
Copy link
Contributor Author

@streamer45 - Can you direct me to those details please? I'm not seeing code in this PR that looks like a new config setting.

@cwarnermm Yeah, it's the linked calls PR -> mattermost/mattermost-plugin-calls#795

@cwarnermm
Copy link
Member

Thanks, @streamer45!!

@cwarnermm cwarnermm mentioned this pull request Sep 16, 2024
Merged
@amyblais amyblais added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DHaussermann DHaussermann DHaussermann approved these changes

@enahum enahum enahum approved these changes

@cpoile cpoile cpoile approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Docs/Done Required documentation has been written QA Review Done release-note
Projects
None yet
Milestone
v2.20.0
Development

Successfully merging this pull request may close these issues.
7 participants
@streamer45 @DHaussermann @cwarnermm @cpoile @enahum @amyblais @mm-cloud-bot