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 lost audio track on safari #11006

Merged
merged 1 commit into from
Nov 29, 2023
Merged

Fix lost audio track on safari #11006

merged 1 commit into from
Nov 29, 2023

Conversation

SystemKeeper
Copy link
Contributor

☑️ Resolves

After lots of manual trying, it looks like the safari bug was introduced between 12.0.0-alpha2 and 12.0.0-alpha3. After bisecting the commit abe5633 seems to be the problem. It belongs to the PR #5660 which replaces disabled tracks with a null track to reduce the amount of data being sent. Reverting the commit above makes the problem #6094 not reproducible anymore in safari.
I don't know if there's a way to use https://github.com/nextcloud/spreed/blob/main/src/mixins/browserCheck.js in peer.js as well, currently I just used ua-parser-js directly.

Based on the additional safari check, this should only effect safari and no other browsers.

🚧 Tasks

  • Check virtual backgrounds still working

🏁 Checklist

@Antreesy
Copy link
Contributor

I see the way like this: we can split this mixin into utilities (common) and composable (to use in components)

@ShGKme
Copy link
Contributor

ShGKme commented Nov 26, 2023

I see the way like this: we can split this mixin into utilities (common) and composable (to use in components)

We need neither mixin nor composable here. This functionality has nothing Vue-related nor anything reactive. No chance that "a web-browser will be changed during a process" =D

So we can make it an util.

@SystemKeeper
Copy link
Contributor Author

So we can make it an util.

Maybe we could do that first and then rebase this PR on top of it? I would like to keep this PR as isolated as possible (Safari 😅...)

@ShGKme
Copy link
Contributor

ShGKme commented Nov 27, 2023

So we can make it an util.

Maybe we could do that first and then rebase this PR on top of it? I would like to keep this PR as isolated as possible (Safari 😅...)

I'll make a PR in 0.5h

@nickvergessen
Copy link
Member

PR was closed by #11009 due to wrong comment. Reopening

@nickvergessen nickvergessen reopened this Nov 27, 2023
@nickvergessen
Copy link
Member

Needs a rebase now

@SystemKeeper
Copy link
Contributor Author

Needs a rebase now

Will take care

Signed-off-by: Marcel Müller <[email protected]>
@SystemKeeper SystemKeeper force-pushed the fix/6094/safari-sounds-2 branch from b176cea to 09360a8 Compare November 27, 2023 16:36
@SystemKeeper
Copy link
Contributor Author

Rebased on main.
Thanks @ShGKme for taking care of that browserCheck utility 👍

@SystemKeeper SystemKeeper added 3. to review feature: WebRTC 🚡 WebRTC connection between browsers and/or mobile clients feature: call 📹 Voice and video calls browser: Safari labels Nov 27, 2023
@SystemKeeper
Copy link
Contributor Author

/backport to stable28

Copy link
Member

@Ivansss Ivansss 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 works 👍

@nickvergessen nickvergessen merged commit 8bb7c87 into main Nov 29, 2023
36 checks passed
@nickvergessen nickvergessen deleted the fix/6094/safari-sounds-2 branch November 29, 2023 10:38
@SystemKeeper
Copy link
Contributor Author

/backport to stable27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review browser: Safari feature: call 📹 Voice and video calls feature: WebRTC 🚡 WebRTC connection between browsers and/or mobile clients
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS/macOS - Audio/video channel is deactivated on the server side after muting
5 participants