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

[expo-av][iOS] - Fix AVAudioSession deactivation #16578

Merged
merged 3 commits into from
Mar 10, 2022
Merged

[expo-av][iOS] - Fix AVAudioSession deactivation #16578

merged 3 commits into from
Mar 10, 2022

Conversation

hirbod
Copy link
Contributor

@hirbod hirbod commented Mar 10, 2022

Why

After ample discussion, testing and consideration, it makes no sense to constantly disable AudioSession as it is a synchronous and blocking process.

There were no regressions in various tests. With this change, pauseAsync() function calls are effectively instant without frame drops.
To avoid triggering unnecessary setActive:YES calls, we still set the variable without actually disabling the session.

We agreed to go this way and in case of doubt look for another approach in a regression.
See #15873

fixes #15873
fixes #5297
fixes #16262
fixes #12613 (even though already closed but never fixed)

How

We're not going to disable the AudioSession anymore, because it's not needed. This make expo-av behave like almost all other apps that work with video and audio (thus making pausing videos fast and non blocking)

Test Plan

Tested in Simulator and on two real devices running iOS 14 and iOS 15.

Checklist

  • Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md).
  • This diff will work correctly for expo build (eg: updated @expo/xdl).
  • This diff will work correctly for expo prebuild & EAS Build (eg: updated a module plugin).

After ample discussion, testing and consideration, it makes no sense to constantly disable AudioSession as it is a synchronous and blocking process. 

There were no regressions in various tests. With this change, pauseAsync() function calls are effectively instant without frame drops. 
To avoid triggering unnecessary setActive:YES calls, we still set the variable without actually disabling the session. 

We agreed to go this way and in case of doubt look for another approach in a regression.
See #15873

fixes #15873
fixes #5297
fixes #16262
fixes #12613 (even though already closed but never fixed)
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Mar 10, 2022
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Mar 10, 2022
@hirbod hirbod marked this pull request as ready for review March 10, 2022 11:32
@hirbod hirbod requested a review from bbarthec as a code owner March 10, 2022 11:32
@hirbod
Copy link
Contributor Author

hirbod commented Mar 10, 2022

@bbarthec here we go.

Copy link
Contributor

@bbarthec bbarthec left a comment

Choose a reason for hiding this comment

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

That looks superb 🎉
Thank you @hirbod for taking care of it and fixing this whole set of issues 👏
I'll release it as soon as the CI goes green 😁

packages/expo-av/CHANGELOG.md Outdated Show resolved Hide resolved
@hirbod
Copy link
Contributor Author

hirbod commented Mar 10, 2022

@bbarthec thank you very much, but I basically did nothing big here :D
Kudos goes to @mnightingale for finding the root cause. I just did the testing and PR :D

@bbarthec
Copy link
Contributor

Yeah! 🎉 @mnightingale, you're doing amazing job for this package 👏
@mnightingale, @hirbod thank you very much for your work here 😁

@tiagotwistag
Copy link

Hey @hirbod, thank you for your awesome work and time to get this one fixed, after updating to the version 11.1.0 I lost my audio on videos, is there something that might be missing on the changes you made? Sorry, my knowledge in Object-C is pretty much none

@hirbod
Copy link
Contributor Author

hirbod commented Mar 18, 2022

@tiagotwistag, the changes we've pushed are most likely not causing issues with audio.
Are you sure that your silent switch isn't active? Because if you want to play music with activated silent switch, you need to trigger something like this

import { Audio, InterruptionModeAndroid, InterruptionModeIOS, Video } from 'expo-av';
...
  useEffect(() => {
    const enableAudio = async () => {
      try {
        await Audio.setAudioModeAsync({
          allowsRecordingIOS: false,
          interruptionModeIOS: InterruptionModeIOS.DoNotMix,
          playsInSilentModeIOS: true, // this one here
          staysActiveInBackground: false,
          interruptionModeAndroid: InterruptionModeAndroid.DoNotMix,
          shouldDuckAndroid: false,
        });
      } catch {}
    };
    enableAudio();
  }, []);

@tiagotwistag
Copy link

@hirbod Yeah you were right I had the wrong value at interruptionModeIOS for some reason I left the android one over there, it does indeed works if properly configured, sorry for that and thanks for the time 👏

@hirbod
Copy link
Contributor Author

hirbod commented Mar 18, 2022

@tiagotwistag you did have a wrong value (old value) because the values changed in v11 :D. I guess you had v10 values.
I just knew they did change because I am spying Expos PRs like a detective :D

This one:
#16145

@IliasseWahbi
Copy link

I confirm that the delay after pausing the video is no longer existing. Thank you very much for your work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
5 participants