-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[video_player_avfoundation, camera_avfoundation] never overwrite but only upgrade audio session category #7143
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
Changes from 8 commits
787f41c
ef43db2
94a80df
f637396
86922ac
6746391
42bd00a
fde86f6
2f876dd
4ddc291
81a9449
4dd42c1
e02355c
6569354
1086400
9edbeef
6216653
59a7fab
8f013b6
acaad4c
3d4e638
948937e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -184,6 +184,8 @@ - (instancetype)initWithMediaSettings:(FCPPlatformMediaSettings *)mediaSettings | |
| _videoFormat = kCVPixelFormatType_32BGRA; | ||
| _inProgressSavePhotoDelegates = [NSMutableDictionary dictionary]; | ||
| _fileFormat = FCPPlatformImageFileFormatJpeg; | ||
| _videoCaptureSession.automaticallyConfiguresApplicationAudioSession = NO; | ||
| _audioCaptureSession.automaticallyConfiguresApplicationAudioSession = NO; | ||
|
stuartmorgan-g marked this conversation as resolved.
|
||
|
|
||
| // To limit memory consumption, limit the number of frames pending processing. | ||
| // After some testing, 4 was determined to be the best maximum value. | ||
|
|
@@ -673,7 +675,8 @@ - (void)captureOutput:(AVCaptureOutput *)output | |
| if (_isFirstVideoSample) { | ||
| [_videoWriter startSessionAtSourceTime:currentSampleTime]; | ||
| // fix sample times not being numeric when pause/resume happens before first sample buffer | ||
| // arrives https://github.com/flutter/flutter/issues/132014 | ||
| // arrives | ||
| // https://github.com/flutter/flutter/issues/132014 | ||
| _lastVideoSampleTime = currentSampleTime; | ||
| _lastAudioSampleTime = currentSampleTime; | ||
| _isFirstVideoSample = NO; | ||
|
|
@@ -1231,9 +1234,7 @@ - (BOOL)setupWriterForPath:(NSString *)path { | |
| return NO; | ||
| } | ||
|
|
||
| if (_mediaSettings.enableAudio && !_isAudioSetup) { | ||
| [self setUpCaptureSessionForAudio]; | ||
| } | ||
| [self setUpCaptureSessionForAudioIfNeeded]; | ||
|
|
||
| _videoWriter = [[AVAssetWriter alloc] initWithURL:outputURL | ||
| fileType:AVFileTypeMPEG4 | ||
|
|
@@ -1313,9 +1314,45 @@ - (BOOL)setupWriterForPath:(NSString *)path { | |
| return YES; | ||
| } | ||
|
|
||
| - (void)setUpCaptureSessionForAudio { | ||
| // this same function is also in video_player_avfoundation | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comments should be properly formatted sentences. |
||
| // 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
is it desired behavior?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. System itself is doing this at some point at or after |
||
| // ability to record audio but never disables it, that could affect other plugins | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 lags and silence | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (category == nil) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you explain what
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you rename this to |
||
| category = AVAudioSession.sharedInstance.category; | ||
| } | ||
| NSSet *playCategories = [NSSet | ||
| setWithObjects:AVAudioSessionCategoryPlayback, AVAudioSessionCategoryPlayAndRecord, nil]; | ||
| NSSet *recordCategories = | ||
| [NSSet setWithObjects:AVAudioSessionCategoryRecord, AVAudioSessionCategoryPlayAndRecord, nil]; | ||
| NSSet *categories = [NSSet setWithObjects:category, AVAudioSession.sharedInstance.category, nil]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Categories which should be combined. Upgrading means we need to combine categories so the new category has play flavour if any of them has it, and has record if any of them has it. Test whether any of these categories has play is done by intersection which means whether anything in the first set is present in the second set.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you rename this to |
||
| BOOL needPlay = [categories intersectsSet:playCategories]; | ||
| BOOL needRecord = [categories intersectsSet:recordCategories]; | ||
| if (needPlay && needRecord) { | ||
| category = AVAudioSessionCategoryPlayAndRecord; | ||
| } else if (needPlay) { | ||
| category = AVAudioSessionCategoryPlayback; | ||
| } else if (needRecord) { | ||
| category = AVAudioSessionCategoryRecord; | ||
| } | ||
| options = (AVAudioSession.sharedInstance.categoryOptions & ~clearOptions) | options; | ||
| if ([category isEqualToString:AVAudioSession.sharedInstance.category] && | ||
| options == AVAudioSession.sharedInstance.categoryOptions) { | ||
| return; | ||
| } | ||
| [AVAudioSession.sharedInstance setCategory:category withOptions:options error:nil]; | ||
| } | ||
|
|
||
| - (void)setUpCaptureSessionForAudioIfNeeded { | ||
| // Don't setup audio twice or we will lose the audio. | ||
| if (_isAudioSetup) { | ||
| if (!_mediaSettings.enableAudio || _isAudioSetup) { | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -1331,6 +1368,14 @@ - (void)setUpCaptureSessionForAudio { | |
| // Setup the audio output. | ||
| _audioOutput = [[AVCaptureAudioDataOutput alloc] init]; | ||
|
|
||
| dispatch_sync(dispatch_get_main_queue(), ^{ | ||
| upgradeAudioSessionCategory(AVAudioSessionCategoryPlayAndRecord, | ||
| AVAudioSessionCategoryOptionDefaultToSpeaker | | ||
| AVAudioSessionCategoryOptionAllowBluetoothA2DP | | ||
| AVAudioSessionCategoryOptionAllowAirPlay, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you explain why these options?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add the comments explaining this? |
||
| 0); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| }); | ||
|
|
||
| if ([_audioCaptureSession canAddInput:audioInput]) { | ||
| [_audioCaptureSession addInput:audioInput]; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is also called from |
||
|
|
||
| @end | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| ## NEXT | ||
| ## 2.6.2 | ||
|
|
||
| * Fixes audio recorded only with first recording. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This description doesn't make sense in the context of this package, which does not record anything. Please describe the fix in terms of what is changing generally, not the specific effect it has on a specific test app that readers of the changelog won't have context on. |
||
| * Updates minimum supported SDK version to Flutter 3.19/Dart 3.3. | ||
|
|
||
| ## 2.6.1 | ||
|
|
||
There was a problem hiding this comment.
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 going to be clear to a client of
camerawhat this means; please add more context about what the actual problem being solved is. (Also, it's fine to givevideo_playeras an example, but it shouldn't make it sound like this is specific tovideo_player; any other native code could collide.)