Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
remove dispatch_sync from upgradeAudioSessionCategory
  • Loading branch information
misos1 committed Aug 15, 2024
commit 42bd00ab269a582d4ffa48f29085a185888d0998
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ - (void)prepareForVideoRecordingWithCompletion:
(nonnull void (^)(FlutterError *_Nullable))completion {
__weak typeof(self) weakSelf = self;
dispatch_async(self.captureSessionQueue, ^{
[weakSelf.camera setUpCaptureSessionForAudio];
[weakSelf.camera setUpCaptureSessionForAudioIfNeeded];
completion(nil);
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1223,7 +1223,7 @@ - (BOOL)setupWriterForPath:(NSString *)path {
return NO;
}

[self setUpCaptureSessionForAudio];
[self setUpCaptureSessionForAudioIfNeeded];

_videoWriter = [[AVAssetWriter alloc] initWithURL:outputURL
fileType:AVFileTypeMPEG4
Expand Down Expand Up @@ -1303,22 +1303,17 @@ - (BOOL)setupWriterForPath:(NSString *)path {
return YES;
}

// this same function is also in video_player_avfoundation
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comments should be properly formatted sentences.

https://google.github.io/styleguide/objcguide.html#comments

// configure application wide audio session manually to prevent overwriting
// flag MixWithOthers by capture session, only change category if it is considered
// as upgrade which means it can only enable ability to play in silent mode or
Copy link
Contributor

Choose a reason for hiding this comment

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

enable ability to play in silent mode

is it desired behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

System itself is doing this at some point at or after AVCaptureSession addInput or addOutput when automaticallyConfiguresApplicationAudioSession is YES which is default, this behaviour was there also before.

// ability to record audio but never disables it, that could affect other plugins
Copy link
Contributor

Choose a reason for hiding this comment

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

affect other plugins

can you explicitly mention video player as an example?

// which depend on this global state, only change category or options if there is
// change to prevent unnecessary route changes which can cause lags
// change to prevent unnecessary lags and silence
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not clear to me how this last part is different from what the comment already said so far (possibly because the sentence is a run-on by this point so it's hard to follow). Please reword to clarify if this is supposed to be adding new details.

// https://github.com/flutter/flutter/issues/131553
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't need an issue link; we generally only link to issues for TODOs, or cases that are not understandable from a comment (e.g., very subtle edge cases). The idea of only upgrading global mode is clear enough.

static void upgradeAudioSessionCategory(AVAudioSessionCategory category,
AVAudioSessionCategoryOptions options,
AVAudioSessionCategoryOptions clearOptions) {
if (!NSThread.isMainThread) {
dispatch_sync(dispatch_get_main_queue(), ^{
upgradeAudioSessionCategory(category, options, clearOptions);
});
return;
}
if (category == nil) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain what category means here? is it the category that we want to upgrade to? or the category that we want to upgrade from?

Copy link
Contributor Author

@misos1 misos1 Aug 20, 2024

Choose a reason for hiding this comment

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

Actually it is a category with which we need to combine an existing category. Newly set category should be such a combination.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you rename this to requestedCategory? Also when will we need to pass a nil category param? What does it mean by requesting a nil category? and why are we assigning category = AVAudioSession.sharedInstance.category when it's nil

category = AVAudioSession.sharedInstance.category;
}
Expand All @@ -1344,7 +1339,7 @@ static void upgradeAudioSessionCategory(AVAudioSessionCategory category,
[AVAudioSession.sharedInstance setCategory:category withOptions:options error:nil];
}

- (void)setUpCaptureSessionForAudio {
- (void)setUpCaptureSessionForAudioIfNeeded {
// Don't setup audio twice or we will lose the audio.
if (!_mediaSettings.enableAudio || _isAudioSetup) {
return;
Expand All @@ -1362,11 +1357,13 @@ - (void)setUpCaptureSessionForAudio {
// Setup the audio output.
_audioOutput = [[AVCaptureAudioDataOutput alloc] init];

upgradeAudioSessionCategory(AVAudioSessionCategoryPlayAndRecord,
AVAudioSessionCategoryOptionDefaultToSpeaker |
AVAudioSessionCategoryOptionAllowBluetoothA2DP |
AVAudioSessionCategoryOptionAllowAirPlay,
0);
dispatch_sync(dispatch_get_main_queue(), ^{
upgradeAudioSessionCategory(AVAudioSessionCategoryPlayAndRecord,
AVAudioSessionCategoryOptionDefaultToSpeaker |
AVAudioSessionCategoryOptionAllowBluetoothA2DP |
AVAudioSessionCategoryOptionAllowAirPlay,
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain why these options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DefaultToSpeaker is what it was setting also before with automatic session configuration, so better to not change that behaviour. It is also implicit default for AVAudioSessionCategoryPlayback along with AllowBluetoothA2DP and AllowAirPlay which cannot be turned off (it is not set by default for PlayAndRecord). This category uses the video player so it is better to not change player behaviour by just initializing the camera (with audio).

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the comments explaining this?

0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think it's better not to pass this param. It's making the function a bit complicated to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally that function would be in a separate file shared with video_player. Is it possible to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or you mean that better would be to have it as an objc selector rather than C function?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's possible to share video player and camera code. Our plugins are not setup to do so.

I meant it seems we only pass 0 and never other values to this param. So do we really need this param?

});

if ([_audioCaptureSession canAddInput:audioInput]) {
[_audioCaptureSession addInput:audioInput];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ NS_ASSUME_NONNULL_BEGIN
- (void)startImageStreamWithMessenger:(NSObject<FlutterBinaryMessenger> *)messenger;
- (void)stopImageStream;
- (void)setZoomLevel:(CGFloat)zoom withCompletion:(void (^)(FlutterError *_Nullable))completion;
- (void)setUpCaptureSessionForAudio;
- (void)setUpCaptureSessionForAudioIfNeeded;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this called outside the implementation? If not it should be moved to the private category in the .m file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is also called from prepareForVideoRecordingWithCompletion in CameraPlugin.m.


@end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -813,23 +813,18 @@ - (void)pausePlayer:(NSInteger)textureId error:(FlutterError **)error {
[player pause];
}

// this same function is also in camera_avfoundation
// do not overwrite PlayAndRecord with Playback which causes inability to record
// audio, do not overwrite all options, only change category if it is considered
// as upgrade which means it can only enable ability to play in silent mode or
// ability to record audio but never disables it, that could affect other plugins
// which depend on this global state, only change category or options if there is
// change to prevent unnecessary route changes which can cause lags
// change to prevent unnecessary lags and silence
// https://github.com/flutter/flutter/issues/131553
#if TARGET_OS_IOS
static void upgradeAudioSessionCategory(AVAudioSessionCategory category,
AVAudioSessionCategoryOptions options,
AVAudioSessionCategoryOptions clearOptions) {
if (!NSThread.isMainThread) {
dispatch_sync(dispatch_get_main_queue(), ^{
upgradeAudioSessionCategory(category, options, clearOptions);
});
return;
}
if (category == nil) {
category = AVAudioSession.sharedInstance.category;
}
Expand Down