[video_player_avfoundation, camera_avfoundation] never overwrite but only upgrade audio session category #7143

misos1 · 2024-07-16T18:02:06Z

Setting category of AVAudioSession was moved into a wrapper which ensures that only changes which do not disable the ability to play in silent mode (play) and ability to record (record) are considered. If either the new category or old category is play/record then also the set category will be play/record. This currently means that the video player will not overwrite PlayAndRecord with Playback which causes inability to record audio by camera. Options are treated selectively so the video player will no longer overwrite flags set by camera like DefaultToSpeaker and camera will not overwrite MixWithOthers set by video player setMixWithOthers. It will also only change category or options if there is change to prevent lags every time is constructed VideoPlayerController with non-null videoPlayerOptions which always causes call to setMixWithOthers.

Test testSeekToWhilePausedStartsDisplayLinkTemporarily is failing on the main branch on the tested device:

XCTAssertEqual([player position], 1234); // (([player position]) equal to (1234)) failed: ("0") is not equal to ("1234")

Fixes flutter/flutter#131553

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

hellohuanlin · 2024-07-21T17:50:05Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+                                        AVAudioSessionCategoryOptions clearOptions) {
+  if (!NSThread.isMainThread) {
+    dispatch_sync(dispatch_get_main_queue(), ^{
+      upgradeAudioSessionCategory(category, options, clearOptions);


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

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

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.

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

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

misos1 · 2024-07-23T18:08:34Z

I have no way how to test AVAudioSessionCategoryOptionOverrideMutedMicrophoneInterruption so this one was not added although it might prevent some errors for which camera may not be prepared: "Attempting to start audio input after the system mutes the microphone results in an error.".

misos1 · 2024-07-24T17:56:12Z

I removed AVAudioSessionCategoryOptionAllowBluetooth which basically just enables and prefers HFP profile over A2DP. This is not implicitly present with the playback category and has unintended consequences as output is not stereo and lower quality so if there ever is HFP support it should be opt-in from the dart side.

hellohuanlin · 2024-08-02T04:20:35Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

-  if (_mediaSettings.enableAudio && !_isAudioSetup) {
-    [self setUpCaptureSessionForAudio];
-  }
+  [self setUpCaptureSessionForAudio];


nit: setUpCaptureSessionForAudioIfNeeded since you moved condition inside

hellohuanlin · 2024-08-02T04:21:33Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+                              AVAudioSessionCategoryOptionDefaultToSpeaker |
+                                  AVAudioSessionCategoryOptionAllowBluetoothA2DP |
+                                  AVAudioSessionCategoryOptionAllowAirPlay,
+                              0);


Do we only pass 0 here? Should we just remove this parameter?

It looked like a good idea during development as there is this same function also in the player which uses that argument and it was easier to work on it in a single place and then copy it into the other plugin. One time this argument was actually used when I tried to use AVAudioSessionCategoryOptionAllowBluetooth it looked like a good idea to remove that option when the camera was no longer used.

hellohuanlin · 2024-08-02T04:22:55Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

@@ -1302,9 +1303,50 @@ - (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


enable ability to play in silent mode

is it desired behavior?

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.

hellohuanlin · 2024-08-02T04:23:26Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+// 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


unnecessary route changes

What is a "route change"?

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.

hellohuanlin · 2024-08-02T04:23:48Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+// 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
+// ability to record audio but never disables it, that could affect other plugins


affect other plugins

can you explicitly mention video player as an example?

hellohuanlin · 2024-08-19T21:05:53Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+    upgradeAudioSessionCategory(AVAudioSessionCategoryPlayAndRecord,
+                                AVAudioSessionCategoryOptionDefaultToSpeaker |
+                                    AVAudioSessionCategoryOptionAllowBluetoothA2DP |
+                                    AVAudioSessionCategoryOptionAllowAirPlay,


can you explain why these options?

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

Can you add the comments explaining this?

hellohuanlin · 2024-08-19T21:06:14Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+                                AVAudioSessionCategoryOptionDefaultToSpeaker |
+                                    AVAudioSessionCategoryOptionAllowBluetoothA2DP |
+                                    AVAudioSessionCategoryOptionAllowAirPlay,
+                                0);


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

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

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

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?

hellohuanlin · 2024-08-19T21:08:33Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+static void upgradeAudioSessionCategory(AVAudioSessionCategory category,
+                                        AVAudioSessionCategoryOptions options,
+                                        AVAudioSessionCategoryOptions clearOptions) {
+  if (category == nil) {


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?

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

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

hellohuanlin · 2024-08-19T21:08:59Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+      setWithObjects:AVAudioSessionCategoryPlayback, AVAudioSessionCategoryPlayAndRecord, nil];
+  NSSet *recordCategories =
+      [NSSet setWithObjects:AVAudioSessionCategoryRecord, AVAudioSessionCategoryPlayAndRecord, nil];
+  NSSet *categories = [NSSet setWithObjects:category, AVAudioSession.sharedInstance.category, nil];


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

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.

Can you rename this to requiredFinalCategories or something similar

hellohuanlin

Thanks for the comments

hellohuanlin · 2024-08-27T17:41:41Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+static void upgradeAudioSessionCategory(AVAudioSessionCategory category,
+                                        AVAudioSessionCategoryOptions options,
+                                        AVAudioSessionCategoryOptions clearOptions) {
+  if (category == nil) {


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

hellohuanlin · 2024-08-27T17:43:54Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+      setWithObjects:AVAudioSessionCategoryPlayback, AVAudioSessionCategoryPlayAndRecord, nil];
+  NSSet *recordCategories =
+      [NSSet setWithObjects:AVAudioSessionCategoryRecord, AVAudioSessionCategoryPlayAndRecord, nil];
+  NSSet *categories = [NSSet setWithObjects:category, AVAudioSession.sharedInstance.category, nil];


Can you rename this to requiredFinalCategories or something similar

hellohuanlin · 2024-08-27T17:46:05Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+    upgradeAudioSessionCategory(AVAudioSessionCategoryPlayAndRecord,
+                                AVAudioSessionCategoryOptionDefaultToSpeaker |
+                                    AVAudioSessionCategoryOptionAllowBluetoothA2DP |
+                                    AVAudioSessionCategoryOptionAllowAirPlay,


Can you add the comments explaining this?

hellohuanlin · 2024-08-27T17:47:22Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+                                AVAudioSessionCategoryOptionDefaultToSpeaker |
+                                    AVAudioSessionCategoryOptionAllowBluetoothA2DP |
+                                    AVAudioSessionCategoryOptionAllowAirPlay,
+                                0);


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?

misos1 · 2024-08-27T18:12:44Z

Also when will we need to pass a nil category param?

It means it should not be changed, used in player, AVAudioSession.sharedInstance.category could not be passed as argument when there was thread switch inside function, now that changed so I can remove that check and make player pass AVAudioSession.sharedInstance.category instead of nil.

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

No, not in camera at the moment.

stuartmorgan-g

Overall this looks like a good strategy to make the global state changes we unfortunately have less disruptive without causing any breaking changes.

(Long term we should still try to separate out the global changes into their own methods, and maybe even their own plugin, but that's a much larger undertaking that I don't expect us to get to for some time.)

stuartmorgan-g · 2024-10-07T19:18:36Z

packages/camera/camera_avfoundation/CHANGELOG.md


+* Fixes overwriting flag MixWithOthers set by video_player.


I don't think it's going to be clear to a client of camera what this means; please add more context about what the actual problem being solved is. (Also, it's fine to give video_player as an example, but it shouldn't make it sound like this is specific to video_player; any other native code could collide.)

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

stuartmorgan-g · 2024-10-07T19:21:34Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

@@ -1313,9 +1314,42 @@ - (BOOL)setupWriterForPath:(NSString *)path {
  return YES;
 }

- (void)setUpCaptureSessionForAudio {
+// this same function is also in video_player_avfoundation


Comments should be properly formatted sentences.

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

stuartmorgan-g · 2024-10-07T19:22:51Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+// 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 lags and silence


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.

stuartmorgan-g · 2024-10-07T19:23:58Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+// 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 lags and silence
+// https://github.com/flutter/flutter/issues/131553


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.

stuartmorgan-g · 2024-10-07T19:37:28Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

@@ -1331,6 +1365,19 @@ - (void)setUpCaptureSessionForAudio {
  // Setup the audio output.
  _audioOutput = [[AVCaptureAudioDataOutput alloc] init];

+  dispatch_block_t block = ^{
+    // Setup options implicit to AVAudioSessionCategoryPlayback to not disturb video_player.


This comment doesn't make sense to me in this context; this call is primarily about making a change, not not making a change. The comment on the function itself covers this.

This is to explain why there are such options: #7143 (comment)

Comment on the function does not cover this.

How about Upgrade options requested by camera without conflicting other plugins like video_player.

stuartmorgan-g · 2024-10-08T14:17:55Z

...ion/ios/camera_avfoundation/Sources/camera_avfoundation/include/camera_avfoundation/FLTCam.h

@@ -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;


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

It is also called from prepareForVideoRecordingWithCompletion in CameraPlugin.m.

stuartmorgan-g · 2024-10-08T14:19:17Z

packages/video_player/video_player_avfoundation/CHANGELOG.md


+* Fixes audio recorded only with first recording.


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.

stuartmorgan-g · 2024-10-08T14:21:28Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

@@ -705,10 +705,46 @@ - (int64_t)onPlayerSetup:(FVPVideoPlayer *)player frameUpdater:(FVPFrameUpdater
  return textureId;
 }

+// this same function is also in camera_avfoundation


This is the least important information in the comment, so should be last as a final note.

And it's not actually true, because they are slightly different due to clearOptions. So this should just be removed.

Before it was the same (it was removed due to a comment about it not being used in camera). It was supposed to be in a single place but I could not find a way to share source files between plugins so I put it twice for the time being until it is possibly moved elsewhere into a single place. I think such a comment makes sense (actually it may be important enough to be on top) because without this context making certain changes to it in one plugin without changing it also in other plugins can result in it no longer working as intended (like simple change in camera to call it on capture session queue instead of main, that change may seem reasonable without context). Maybe I should change it a little like "..similar function..." and that these two need to be "synchronized" resp. on the same queue.

The comments and implementation strategy seem overly focused on these two plugins to me. There could be 50 plugins in the ecosystem that set global audio state, only two of which we happen to control, so any solution that relies on these two plugins being exactly synchronized doesn't scale.

I would like the solution (and its comments) here to focus on things that are generally true:

AVAudioSession is not explicitly documented as being thread safe, so we should only use it from the main thread. We can mention that other plugin or application code could be using AVAudioSession.sharedInstance as well, and that we have to hope that they are doing the same.

We should upgrade rather than stomp the state because other code may be setting the global state as well, and we don't want to remove options they may have set.

Neither of those statements require exact synchronization with other code, and both generalize to the entire ecosystem. (Although as mentioned earlier, the real ecosystem-friendly solution is to move away from doing this in individual plugins at all, and leaving it for apps to do.)

It is not enough to just use AVAudioSession on the main thread, something like this would not work well:

dispatch_sync(main_queue, () => old_options = AVAudioSession.sharedInstance.categoryOptions); new_options = old_options | AVAudioSessionCategoryOptionMixWithOthers; dispatch_sync(main_queue, () => AVAudioSession.sharedInstance.categoryOptions = new_options);

AVAudioSession is not explicitly documented as being thread safe

Btw category and categoryOptions are atomic properties and I doubt that capture session is changing them on the main thread when automaticallyConfiguresApplicationAudioSession is true.

Just for curiosity, here it is changed by capture session and it is not possible that it happened on the main thread:

dispatch_async(dispatch_get_main_queue(), ^{ NSLog(@"%@", AVAudioSession.sharedInstance.category); dispatch_sync(_captureSessionQueue, ^{ [_audioCaptureSession addOutput:_audioOutput]; }); NSLog(@"%@", AVAudioSession.sharedInstance.category); });

AVAudioSessionCategoryPlayback AVAudioSessionCategoryPlayAndRecord

It is not enough to just use AVAudioSession on the main thread, something like this would not work well:

dispatch_sync(main_queue, () => old_options = AVAudioSession.sharedInstance.categoryOptions); new_options = old_options | AVAudioSessionCategoryOptionMixWithOthers; dispatch_sync(main_queue, () => AVAudioSession.sharedInstance.categoryOptions = new_options);

We don't need comments to tell people not to use basic threading anti-patterns. Reading, modifying, and writing back a value in a non-atomic ways is one of the most basic things people familiar with threading and locking learn that they should not do.

Is this the last change request still not fulfilled after the latest commits? Should that comment be further rewritten to not explicitly mention the other plugin but something more general? You mentioned "solution" but I am not sure what changes you mean to be done to that implementation.

stuartmorgan-g · 2024-10-08T14:21:38Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

+// 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 lags and silence
+// https://github.com/flutter/flutter/issues/131553


All the comments in the other file apply here as well.

hellohuanlin

Sorry for the delay. It's looking good. Just a few questions.

hellohuanlin · 2024-11-08T18:12:34Z

packages/camera/camera_avfoundation/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.9.17+5
+
+* Fixes changing global audio session category to be collision free.


nit: ...collision free across plugins

hellohuanlin · 2024-11-08T18:14:22Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

@@ -1313,9 +1314,42 @@ - (BOOL)setupWriterForPath:(NSString *)path {
  return YES;
 }

- (void)setUpCaptureSessionForAudio {
+// This function, although slightly modified, is also in video_player_avfoundation.
+// Both need to do the same thing and run on the same thread.


...run on the main thread?

"Same" is a requirement for correct synchronization. But if AVAudioSession must really be used only on the main thread and the fact that probably the only reasonable thread to use here between plugins is the main thread maybe it would be better to change it to "main".

hellohuanlin · 2024-11-08T18:18:43Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+  if (!NSThread.isMainThread) {
+    dispatch_sync(dispatch_get_main_queue(), block);
+  } else {
+    block();


is setUpCaptureSessionForAudioIfNeeded already guaranteed to be on background? Can you double check the caller of this function? If it's already on background, we can simply do

NSAssert(!NSThread.isMainThread); dispatch_sync(dispatch_get_main_queue()) { // your logic here };

It is called on the main thread in tests if I remember correctly #7143 (comment).

is it only called on main thread in tests? Can you give an example of such failure? I'd like to see if it's possible to run on background thread in those tests

Try to run tests with this:

//if (!NSThread.isMainThread) { dispatch_sync(dispatch_get_main_queue(), block); //} else { //block(); //}

And you will get this inside xcode:

Thread 1: EXC_BREAKPOINT (code=1, subcode=0x18082dc74)

When running outside of xcode with debugging there will be probably just crash, you can add NSLog(@"%@", NSThread.currentThread); to see that it runs on main thread <_NSMainThread: 0x2804b0080>{number = 1, name = main}.

You will probably need to run flutter pub upgrade first as without that I am getting this error during flutter run (win32 on macos?):

Could not build the precompiled application for the device. Error (Xcode): ../../../../../../../../.pub-cache/hosted/pub.dev/win32-5.2.0/lib/src/guid.dart:32:9: Error: Type 'UnmodifiableUint8ListView' not found.

It is called on the main thread in tests if I remember correctly #7143 (comment).

When running outside of xcode with debugging there will be probably just crash, you can add NSLog(@"%@", NSThread.currentThread); to see that it runs on main thread <_NSMainThread: 0x2804b0080>{number = 1, name = main}.

So setUpCaptureSessionForAudioIfNeeded is called on main thread somewhere (not just in test). Could you find the place it's called on main? The session setup shouldn't happen on main thread.

But that same situation is also with FLTEnsureToRunOnMainQueue.

Could you explain the problem with FLTEnsureToRunOnMainQueue? Maybe a concrete example?

How exactly is this a problem anyway? There are many other "adaptations" only for tests, like interfaces and factories just to simplify testing, in both camera and video_player.

I don't know why you are comparing with interfaces and factories.

Imagine you have this code:

func setupSession() { if is background thread { Logic A } else { Logic B } }

Here your production code only calls Logic A, and test code only calls Logic B. Then the test isn't really providing any confidence right?

What I suggest is:

func setupSession() { Assert(session must be setup on background); Logic A; }

Then in the test, you can do:

func testSetupSession() { dispatch_to_background { setupSession(); } }

This way, the test code actually covers Logic A, providing us with confidence in production.

Your assertion will fail because other tests call setupSession on the main thread. And would this not cause a deadlock due to dispatch_sync waiting for block dispatched on main queue while also waitForExpectation is waiting on main thread until dispatch_sync is done and expectation fulfilled? discussion_r1876564910

testFoo() { let expectation = ... sessionQueue.async { dispatch_sync(main_queue(), ...) expectation.fulfill() } waitForExpectation() }

What I proposed does not have such problems and tests also cover production logic as I wrote.

And would this not cause a deadlock due to dispatch_sync waiting for block dispatched on main queue while also waitForExpectation is waiting on main thread until dispatch_sync is done and expectation fulfilled?

Oh I overlooked the part that it's dispatch_sync to main, which leads to deadlock in test.

What I proposed does not have such problems and tests also cover production logic as I wrote.

It would be the same problem, that dispatch_sync(main) is inherently not testable - You simply moved the same problem to FLTEnsureToRunOnMainQueueSync (i.e. the dispatch_sync branch of this helper would not be testable for the same reason). So I'd accept the current code as it is.

It would be the same problem

Oh I did not think of that, but as you wrote that, I realized that means that also the async test should be untestable, maybe it actually waits in some non blocking way. But anyway I did not see that documented anywhere, maybe that async test is not 100% clean then?

I realized that means that also the async test should be untestable

dispatch_async is different and it can be tested. The problem with dispatch_sync is that it causes deadlock when dispatching to the same queue.

hellohuanlin · 2024-11-08T18:20:07Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

@@ -1331,6 +1365,19 @@ - (void)setUpCaptureSessionForAudio {
  // Setup the audio output.
  _audioOutput = [[AVCaptureAudioDataOutput alloc] init];

+  dispatch_block_t block = ^{
+    // Setup options implicit to AVAudioSessionCategoryPlayback to not disturb video_player.


How about Upgrade options requested by camera without conflicting other plugins like video_player.

stuartmorgan-g

LGTM!

Thanks for your patience on this review; I'm still catching up on reviews after being out of office for several weeks

auto-submit · 2025-01-13T17:46:46Z

auto label is removed for flutter/packages/7143, due to - The status or check suite ci.yaml validation has failed. Please fix the issues identified (or deflake) before re-applying this label.

The status or check suite Merge Queue Guard has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2025-01-13T17:56:04Z

auto label is removed for flutter/packages/7143, due to - The status or check suite Mac_arm64 macos_platform_tests master - packages has failed. Please fix the issues identified (or deflake) before re-applying this label.

…write but only upgrade audio session category (flutter/packages#7143)

flutter/packages@3c3bc68...d1fd623 2025-01-13 [email protected] [camera] Add API support query for image streaming (flutter/packages#8250) 2025-01-13 [email protected] [webview_flutter_android] Add additional WebSettings methods (flutter/packages#8270) 2025-01-13 [email protected] Roll Flutter from 864d4f5 to 72db8f6 (11 revisions) (flutter/packages#8421) 2025-01-13 [email protected] [video_player_avfoundation, camera_avfoundation] never overwrite but only upgrade audio session category (flutter/packages#7143) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…r#161597) flutter/packages@3c3bc68...d1fd623 2025-01-13 [email protected] [camera] Add API support query for image streaming (flutter/packages#8250) 2025-01-13 [email protected] [webview_flutter_android] Add additional WebSettings methods (flutter/packages#8270) 2025-01-13 [email protected] Roll Flutter from 864d4f5 to 72db8f6 (11 revisions) (flutter/packages#8421) 2025-01-13 [email protected] [video_player_avfoundation, camera_avfoundation] never overwrite but only upgrade audio session category (flutter/packages#7143) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

github-actions bot added p: camera platform-ios platform-macos p: video_player labels Jul 16, 2024

misos1 force-pushed the upgrade_audio_session branch from c0a1c93 to 0043944 Compare July 16, 2024 18:45

never overwrite but only upgrade audio session category

787f41c

misos1 force-pushed the upgrade_audio_session branch from 0043944 to 787f41c Compare July 16, 2024 19:00

misos1 marked this pull request as ready for review July 17, 2024 16:06

misos1 requested a review from hellohuanlin as a code owner July 17, 2024 16:06

Merge branch 'main' into upgrade_audio_session

ef43db2

hellohuanlin reviewed Jul 21, 2024

View reviewed changes

add more options implicitly present with Playback

94a80df

misos1 added 2 commits July 23, 2024 20:14

fix format

f637396

remove HFP and fix using audio session outside of critical section

86922ac

fix format

6746391

hellohuanlin requested a review from tarrinneal July 24, 2024 21:27

hellohuanlin reviewed Aug 2, 2024

View reviewed changes

misos1 added 2 commits August 15, 2024 18:39

remove dispatch_sync from upgradeAudioSessionCategory

42bd00a

Merge branch 'main' into upgrade_audio_session

fde86f6

hellohuanlin reviewed Aug 19, 2024

View reviewed changes

test for main thread because tests are running it on that

2f876dd

hellohuanlin reviewed Aug 27, 2024

View reviewed changes

misos1 added 2 commits September 6, 2024 21:12

remove nil check and rename variables and arguments

4ddc291

fix format

81a9449

hellohuanlin requested review from stuartmorgan-g and removed request for tarrinneal September 24, 2024 19:15

stuartmorgan-g requested changes Oct 8, 2024

View reviewed changes

misos1 added 2 commits October 15, 2024 18:35

update comments

e02355c

Merge branch 'main' into upgrade_audio_session

6569354

misos1 requested a review from stuartmorgan-g October 16, 2024 15:59

hellohuanlin reviewed Nov 8, 2024

View reviewed changes

misos1 added 3 commits November 28, 2024 19:41

change comments

1086400

Merge branch 'main' into upgrade_audio_session

9edbeef

fix format

6216653

misos1 requested a review from hellohuanlin November 28, 2024 18:53

hellohuanlin approved these changes Dec 11, 2024

View reviewed changes

misos1 and others added 5 commits December 11, 2024 18:53

Merge branch 'main' into upgrade_audio_session

59a7fab

Merge branch 'main' into upgrade_audio_session

8f013b6

Minor grammar fix

acaad4c

Re-bump camera version

3d4e638

Merge branch 'main' into upgrade_audio_session

948937e

stuartmorgan-g approved these changes Jan 13, 2025

View reviewed changes

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 13, 2025

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 13, 2025

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 13, 2025

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 13, 2025

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 13, 2025

auto-submit bot merged commit 5bb6a8c into flutter:main Jan 13, 2025
77 checks passed

engine-flutter-autoroll mentioned this pull request Jan 14, 2025

Roll Packages from 3c3bc6832b39 to d1fd6232ec33 (4 revisions) flutter/flutter#161597

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 14, 2025

5bb6a8ce7 [video_player_avfoundation, camera_avfoundation] never over…

1e8a87b

…write but only upgrade audio session category (flutter/packages#7143)

		@@ -1,3 +1,7 @@
		## 0.9.17+5

		* Fixes changing global audio session category to be collision free.

[video_player_avfoundation, camera_avfoundation] never overwrite but only upgrade audio session category #7143

[video_player_avfoundation, camera_avfoundation] never overwrite but only upgrade audio session category #7143

Uh oh!

Conversation

misos1 commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

misos1 Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

misos1 commented Jul 23, 2024

Uh oh!

misos1 commented Jul 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

misos1 Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

misos1 Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellohuanlin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

misos1 commented Jul 16, 2024 •

edited

Loading

misos1 Jul 23, 2024 •

edited

Loading

misos1 Aug 20, 2024 •

edited

Loading

misos1 Aug 20, 2024 •

edited

Loading