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

refactor(ios): audio session manager #3850

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

KrzysztofMoch
Copy link
Member

Summary

Create Audio Session Manager for better and more reliable audio session management

Motivation

In old implementation each component is changing audio session properties without carrying about other components - with audio session manager, components will respect other components "needs"

Note

This is very risky change (at last for me) and need tests to see if everything is working as it should

@@ -241,6 +242,7 @@ class RCTVideo: UIView, RCTVideoPlayerViewControllerDelegate, RCTPlayerObserverH

required init?(coder aDecoder: NSCoder) {
super.init(coder: aDecoder)
AudioSessionManager.shared.registerView(view: self)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no unregisterView call, is it really expected ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I will add unregisterView to deinit 👍
(but this I guess would be fine as it is because there is week object list so it would deinit anyway)

@freeboub
Copy link
Collaborator

Change make sense for me (this is really cleaner I think).
As I don't have ios device, I cannot really test, consider I approuve PR (but please check my small comment!)
Thank you @KrzysztofMoch

@YangJonghun
Copy link
Collaborator

First of all, thank you for your great work.
I would like to talk about two topics.

  1. why is the AudioSession handling within NowPlayingInfo? I think this processing should be a simple structure that is handled when the Player is mounted and unmounted. At the same time, the current audioSession is also handled within setPictureInPicture, so it seems like it could affect the management of audioSessions that work as application singletons.

  2. the work you're doing to tie the settings between videos together is great. However, since audioSession is a single instance throughout the application that determines the behavior of the app, I think that if this is necessary, it should be added to detect changes in the audioSession and reset it with a priority determined from the video.

@freeboub
Copy link
Collaborator

freeboub commented Jun 3, 2024

@KrzysztofMoch is it fixing: #3838 ?

@KrzysztofMoch
Copy link
Member Author

KrzysztofMoch commented Jun 3, 2024

It can (or at last will show warning), background audio and ignoreSilentSwitch="obey" wont work together as there iw audio session category conflict

@KrzysztofMoch
Copy link
Member Author

Hey @YangJonghun - Thanks for your feedback!

  1. PiP and NowPlayingManager (IIRC) needs playback category, so I set that video using it and then update audio session with those values "in mind". Also I am calling function on shared instance so I don't understand issue

  2. So we probably would need new iOS only prop for it? Something like audioSessionPriority?

@YangJonghun
Copy link
Collaborator

YangJonghun commented Jun 9, 2024

@KrzysztofMoch

  1. PiP and NowPlayingManager (IIRC) needs playback category, so I set that video using it and then update audio session with those values "in mind". Also I am calling function on shared instance so I don't understand issue

I wrote that because it is my understanding that setting up an AudioSession overwrites all previous settings. If I'm wrong, please let me know.

  1. So we probably would need new iOS only prop for it? Something like audioSessionPriority?

This is the same as 1., and I wrote it down because it seems like there's room for bugs due to interference from libraries other than react-native-video, but if I'm wrong about AudioSession, this is fine too.

@freeboub
Copy link
Collaborator

@KrzysztofMoch you can merge when you want !

@KrzysztofMoch
Copy link
Member Author

Okay, I will describe in docs how this will works to don't confuse peoples - I will do it this week

@freeboub
Copy link
Collaborator

freeboub commented Sep 6, 2024

additionnal comment, you need to replace all usage of: configureAudio here the systems are mixed

@frankusu
Copy link

Hi guys any updates on this ? trying to Play background music when video is muted. Stop background music when video is unmuted #4156
Do you guys know when this will get merged and if it will affect anything ?

let shouldEnableDucking = videoViews.allObjects.contains { view in
view._mixWithOthers == "duck"
}

Choose a reason for hiding this comment

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

I know in the original code theres no case for switching back to inherit, would it be possible to add it in ?
Because once you set it to 'mix' or 'duck' theres no way to set it back to 'inherit'

@gerson
Copy link

gerson commented Oct 28, 2024

Hey guys any updates on this? I'm having the same issue 😢

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.

5 participants