Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
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
4 changes: 4 additions & 0 deletions packages/camera/camera_avfoundation/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.9.17+2

* Fixes overwriting flag MixWithOthers set by video_player.

## 0.9.17+1

* Fixes a crash due to appending sample buffers when readyForMoreMediaData is NO
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -251,4 +251,27 @@ - (void)testDidOutputSampleBufferMustNotAppendSampleWhenReadyForMoreMediaDataIsN
CFRelease(videoSample);
}

- (void)testStartVideoRecordingWithCompletionShouldNotDisableMixWithOthers {
FLTCam *cam = FLTCreateCamWithCaptureSessionQueue(dispatch_queue_create("testing", NULL));

id writerMock = OCMClassMock([AVAssetWriter class]);
OCMStub([writerMock alloc]).andReturn(writerMock);
OCMStub([writerMock initWithURL:OCMOCK_ANY fileType:OCMOCK_ANY error:[OCMArg setTo:nil]])
.andReturn(writerMock);

[AVAudioSession.sharedInstance setCategory:AVAudioSessionCategoryPlayback
withOptions:AVAudioSessionCategoryOptionMixWithOthers
error:nil];

[cam
startVideoRecordingWithCompletion:^(FlutterError *_Nullable error) {
}
messengerForStreaming:nil];
XCTAssert(
AVAudioSession.sharedInstance.categoryOptions & AVAudioSessionCategoryOptionMixWithOthers,
@"Flag MixWithOthers was removed.");
XCTAssert(AVAudioSession.sharedInstance.category == AVAudioSessionCategoryPlayAndRecord,
@"Category should be PlayAndRecord.");
}

@end
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,8 @@ - (instancetype)initWithMediaSettings:(FCPPlatformMediaSettings *)mediaSettings
_videoFormat = kCVPixelFormatType_32BGRA;
_inProgressSavePhotoDelegates = [NSMutableDictionary dictionary];
_fileFormat = FCPPlatformImageFileFormatJpeg;
_videoCaptureSession.automaticallyConfiguresApplicationAudioSession = NO;
_audioCaptureSession.automaticallyConfiguresApplicationAudioSession = NO;

// To limit memory consumption, limit the number of frames pending processing.
// After some testing, 4 was determined to be the best maximum value.
Expand Down Expand Up @@ -673,7 +675,8 @@ - (void)captureOutput:(AVCaptureOutput *)output
[_videoWriter startWriting];
[_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;
}
Expand Down Expand Up @@ -1220,9 +1223,7 @@ - (BOOL)setupWriterForPath:(NSString *)path {
return NO;
}

if (_mediaSettings.enableAudio && !_isAudioSetup) {
[self setUpCaptureSessionForAudio];
}
[self setUpCaptureSessionForAudio];
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: setUpCaptureSessionForAudioIfNeeded since you moved condition inside


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

// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary route changes

What is a "route change"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://developer.apple.com/documentation/avfaudio/responding_to_audio_route_changes?language=objc

When category changes it can cause route change seems:

https://developer.apple.com/documentation/avfaudio/avaudiosessionroutechangereason/avaudiosessionroutechangereasoncategorychange?language=objc

Anyway the effect of setting AVAudioSessionCategoryPlayAndRecord multiple times even when the category is unchanged and there are even no headphones causes half of second or longer silence on my device. Also it causes the video_player plugin to pause and must be manually called play again.

// 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

haven't looked into the details. Does this have to be on main queue? Can it be on capture session queue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Audio session category is global state, it is changed based on old state so it must be synchronized with other plugins changing it.

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 set a breakpoint and check which thread the caller is in? This function is only called in one place, so if it's called in background, then we simply do a dispatch_async to avoid this unnecessary recursion.

Copy link
Contributor Author

@misos1 misos1 Jul 23, 2024

Choose a reason for hiding this comment

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

There is needed dispatch_sync. Category must be record before adding inputs/outputs to capture session otherwise there would be no audio. It is called on the capture session queue so not on the main thread. Seems also reportErrorMessage is currently called only on capture session queue but there is that FLTEnsureToRunOnMainQueue anyway. If there was similar method which used dispatch_sync it would not make any difference it would just be upgradeAudioSessionCategory->FLTEnsureToRunOnMainQueueSync->dispatch_sync instead of upgradeAudioSessionCategory->dispatch_sync->upgradeAudioSessionCategory so the same depth in stack. Can it be even called recursion if that second call is in another thread on another stack? I think it would be ok to change it to just dispatch_sync as when this is called from the main thread it will crash with an unhandled exception instead of deadlock so that would be obvious for someone who did such a change. Btw are there better options on how to synchronize between independent plugins? Ideally it would be called on the original thread but with some mutex shared between these plugins (and something like NSDistributedLock seems like overkill).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw many functions from FLTCam are called on the main thread in tests, now that dispatch_sync causes many tests to fail.

});
return;
}
NSSet *playCategories = [NSSet
setWithObjects:AVAudioSessionCategoryPlayback, AVAudioSessionCategoryPlayAndRecord, nil];
NSSet *recordCategories =
[NSSet setWithObjects:AVAudioSessionCategoryRecord, AVAudioSessionCategoryPlayAndRecord, nil];
NSSet *categories = [NSSet setWithObjects:category, AVAudioSession.sharedInstance.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.

what does this categories mean? why are we combining the set? Can you add some comments to explain?

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.

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.

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 requiredFinalCategories or something similar

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)setUpCaptureSessionForAudio {
// Don't setup audio twice or we will lose the audio.
if (_isAudioSetup) {
if (!_mediaSettings.enableAudio || _isAudioSetup) {
return;
}

Expand All @@ -1320,6 +1359,9 @@ - (void)setUpCaptureSessionForAudio {
// Setup the audio output.
_audioOutput = [[AVCaptureAudioDataOutput alloc] init];

upgradeAudioSessionCategory(AVAudioSessionCategoryPlayAndRecord,
AVAudioSessionCategoryOptionDefaultToSpeaker, 0);

if ([_audioCaptureSession canAddInput:audioInput]) {
[_audioCaptureSession addInput:audioInput];

Expand Down
2 changes: 1 addition & 1 deletion packages/camera/camera_avfoundation/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: camera_avfoundation
description: iOS implementation of the camera plugin.
repository: https://github.com/flutter/packages/tree/main/packages/camera/camera_avfoundation
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+camera%22
version: 0.9.17+1
version: 0.9.17+2

environment:
sdk: ^3.2.3
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 2.6.2

* Fixes audio recorded only with first recording.
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.


## 2.6.1

* Adds files to make include directory permanent.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -791,6 +791,40 @@ - (void)testPublishesInRegistration {
}

#if TARGET_OS_IOS
- (void)testVideoPlayerShouldNotOverwritePlayAndRecordNorDefaultToSpeaker {
NSObject<FlutterPluginRegistrar> *registrar = [GetPluginRegistry()
registrarForPlugin:@"testVideoPlayerShouldNotOverwritePlayAndRecordNorDefaultToSpeaker"];
FVPVideoPlayerPlugin *videoPlayerPlugin =
[[FVPVideoPlayerPlugin alloc] initWithRegistrar:registrar];
FlutterError *error;

[AVAudioSession.sharedInstance setCategory:AVAudioSessionCategoryPlayAndRecord
withOptions:AVAudioSessionCategoryOptionDefaultToSpeaker
error:nil];

[videoPlayerPlugin initialize:&error];
[videoPlayerPlugin setMixWithOthers:true error:&error];
XCTAssert(AVAudioSession.sharedInstance.category == AVAudioSessionCategoryPlayAndRecord,
@"Category should be PlayAndRecord.");
XCTAssert(
AVAudioSession.sharedInstance.categoryOptions & AVAudioSessionCategoryOptionDefaultToSpeaker,
@"Flag DefaultToSpeaker was removed.");
XCTAssert(
AVAudioSession.sharedInstance.categoryOptions & AVAudioSessionCategoryOptionMixWithOthers,
@"Flag MixWithOthers should be set.");

id sessionMock = OCMClassMock([AVAudioSession class]);
OCMStub([sessionMock sharedInstance]).andReturn(sessionMock);
OCMStub([sessionMock category]).andReturn(AVAudioSessionCategoryPlayAndRecord);
OCMStub([sessionMock categoryOptions])
.andReturn(AVAudioSessionCategoryOptionMixWithOthers |
AVAudioSessionCategoryOptionDefaultToSpeaker);
OCMReject([sessionMock setCategory:OCMOCK_ANY withOptions:0 error:[OCMArg setTo:nil]])
.ignoringNonObjectArgs();

[videoPlayerPlugin setMixWithOthers:true error:&error];
}

- (void)validateTransformFixForOrientation:(UIImageOrientation)orientation {
AVAssetTrack *track = [[FakeAVAssetTrack alloc] initWithOrientation:orientation];
CGAffineTransform t = FVPGetStandardizedTransformForTrack(track);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -708,7 +708,7 @@ - (int64_t)onPlayerSetup:(FVPVideoPlayer *)player frameUpdater:(FVPFrameUpdater
- (void)initialize:(FlutterError *__autoreleasing *)error {
#if TARGET_OS_IOS
// Allow audio playback when the Ring/Silent switch is set to silent
[[AVAudioSession sharedInstance] setCategory:AVAudioSessionCategoryPlayback error:nil];
upgradeAudioSessionCategory(AVAudioSessionCategoryPlayback, 0, 0);
#endif

[self.playersByTextureId
Expand Down Expand Up @@ -813,17 +813,57 @@ - (void)pausePlayer:(NSInteger)textureId error:(FlutterError **)error {
[player pause];
}

// 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
// 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;
}
NSSet *playCategories = [NSSet
setWithObjects:AVAudioSessionCategoryPlayback, AVAudioSessionCategoryPlayAndRecord, nil];
NSSet *recordCategories =
[NSSet setWithObjects:AVAudioSessionCategoryRecord, AVAudioSessionCategoryPlayAndRecord, nil];
NSSet *categories = [NSSet setWithObjects:category, AVAudioSession.sharedInstance.category, nil];
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];
}
#endif

- (void)setMixWithOthers:(BOOL)mixWithOthers
error:(FlutterError *_Nullable __autoreleasing *)error {
#if TARGET_OS_OSX
// AVAudioSession doesn't exist on macOS, and audio always mixes, so just no-op.
#else
if (mixWithOthers) {
[[AVAudioSession sharedInstance] setCategory:AVAudioSessionCategoryPlayback
withOptions:AVAudioSessionCategoryOptionMixWithOthers
error:nil];
upgradeAudioSessionCategory(AVAudioSession.sharedInstance.category,
AVAudioSessionCategoryOptionMixWithOthers, 0);
} else {
[[AVAudioSession sharedInstance] setCategory:AVAudioSessionCategoryPlayback error:nil];
upgradeAudioSessionCategory(AVAudioSession.sharedInstance.category, 0,
AVAudioSessionCategoryOptionMixWithOthers);
}
#endif
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: video_player_avfoundation
description: iOS and macOS implementation of the video_player plugin.
repository: https://github.com/flutter/packages/tree/main/packages/video_player/video_player_avfoundation
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+video_player%22
version: 2.6.1
version: 2.6.2

environment:
sdk: ^3.2.3
Expand Down