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

Disable positional audio for Safari as a temporal workaround for its positional audio bug #4503

Closed
wants to merge 3 commits into from

Conversation

takahirox
Copy link
Contributor

@takahirox takahirox commented Aug 10, 2021

Currently audio in Hubs is choppy and distorted on Safari. It seems being caused by Safari Positional Audio bug. And the bug seems to have a bad impact to performance. (Refer to #4411).

We have reported the bug to Safari. And we have applied the temporal fix in #4442 by disabling positional audio for Hubs Cloud. Positional audio is one of the important features for us but non-positional audio should be much better than broken audio.

I would like to suggest to apply the same change to master (hubs.mozilla.com) because Safari users who tries hubs.mozilla.com may think Hubs is useless due to this problem and this problem blocks a lot of tests of the upstream on Safari.

@keianhzo Can you review to check if this temporal fix can affect new audio features you recently added?

┆Issue is synchronized with this Jira Task

@takahirox takahirox changed the title Disable positional audio for Safari as a workaround for its positional audio bug Disable positional audio for Safari as a temporal workaround for its positional audio bug Aug 10, 2021
Copy link
Contributor

@brianpeiris brianpeiris left a comment

Choose a reason for hiding this comment

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

The code changes look good to me, but please actually test this in Safari. The audio code in master is very different and I'm not sure if the audioOutputMode flag is working correctly there.

Copy link
Contributor

@keianhzo keianhzo left a comment

Choose a reason for hiding this comment

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

One thing that we need to do here is to check if the output mode is stereo before creating the THREE audio instances because I think historically we haven't really maintained the audioOutputMode path so we are not really considering that right now. since a couple of PRs ago we have stopped supporting that path.

So wherever we create a THREE audio instance we need to check first what the global audioOutputMode is to override the element's audio-params and create the right audio instance.

There is still some work to do to fully revert the support for audioOutputMode and hot changing that preference as right now we are not being consistent with the audio instances recreation in the new mode.

@keianhzo
Copy link
Contributor

keianhzo commented Aug 13, 2021

I'm requesting @johnshaughnessy review for my latest changes.

We should factor in this patch in the WIP the audio refactor and if we remove the audioOutputMode pref we will probably need to keep track of this case to instantiate the right THREE audio type.

@takahirox @brianpeiris I guess this updated patch should also be applied uplifted to HC?

Also it seems there are a number of media playback issues in Safari:

@takahirox
Copy link
Contributor Author

takahirox commented Aug 20, 2021

Sorry, a bit confused. Have we dropped autioOutputMode although /audiomode command still seems to exist?

@takahirox @brianpeiris I guess this updated patch should also be applied uplifted to HC?

I'm not sure if I understand the context correctly but I think Hubs Cloud still has audioOutputMode support because it seems to have been dropped just a few commits ago?

@johnshaughnessy
Copy link
Contributor

Hello @takahirox I would like to work on this problem with you.

It is news to me that the /audiomode command regressed a while back. I would like to help fix it. I have been working with Manuel on some of the other audio related code. It should be easy for us to fix the audioOutputMode pref and also to use Audios instead of PositionalAudios on safari if that's what we want to do.

The work that Manuel and I have been doing to simplify the way we handle audio nodes and audio settings is here: #4562

It would be convenient for me if we work together and avoid merge conflicts.

@johnshaughnessy
Copy link
Contributor

Takahiro and I paired to come up with an alternative fix (#4594), so I am closing this PR.

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

Successfully merging this pull request may close these issues.

4 participants