-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[camera_avfoundation] iOS: Fix crash when enableAudio == false by correcting guard condition
#9949
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
[camera_avfoundation] iOS: Fix crash when enableAudio == false by correcting guard condition
#9949
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
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.
Code Review
This pull request addresses a critical crash on iOS that occurs when the camera is used with enableAudio: false without the NSMicrophoneUsageDescription in Info.plist. The change corrects the logic in the guard statement within setUpCaptureSessionForAudioIfNeeded to ensure audio setup is only attempted when audio is explicitly enabled and has not already been configured. The fix is correct and directly resolves the reported issue by aligning the code's behavior with its intended logic.
|
can you add some tests so that it doesn't regress |
|
@hellohuanlin, added some test. |
|
/gemini review |
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.
Code Review
This pull request correctly fixes a crash on iOS that occurred when enableAudio was set to false. The logical condition in the guard statement has been properly corrected to prevent the audio setup from running when it's disabled. The addition of new unit tests is a great improvement, as they cover both the enabled and disabled audio scenarios, ensuring the fix is robust and preventing future regressions. The changes to the test mocks to support this are also well-implemented. Overall, this is a solid contribution.
| func test_setUpCaptureSessionForAudioIfNeeded_skipsAudioSession_whenAudioDisabled() { | ||
| let settings = FCPPlatformMediaSettings.make( | ||
| with: testResolutionPreset, | ||
| framesPerSecond: NSNumber(value: testFramesPerSecond), | ||
| videoBitrate: NSNumber(value: testVideoBitrate), | ||
| audioBitrate: NSNumber(value: testAudioBitrate), | ||
| enableAudio: false | ||
| ) | ||
|
|
||
| let wrapper = TestMediaSettingsAVWrapper(test: self, expectAudio: false) | ||
| let mockAudioSession = MockCaptureSession() | ||
|
|
||
| let configuration = CameraTestUtils.createTestCameraConfiguration() | ||
| configuration.mediaSettingsWrapper = wrapper | ||
| configuration.mediaSettings = settings | ||
| configuration.audioCaptureSession = mockAudioSession | ||
| let camera = CameraTestUtils.createTestCamera(configuration) | ||
|
|
||
| wait( | ||
| for: [ | ||
| wrapper.lockExpectation, | ||
| wrapper.beginConfigurationExpectation, | ||
| wrapper.minFrameDurationExpectation, | ||
| wrapper.maxFrameDurationExpectation, | ||
| wrapper.commitConfigurationExpectation, | ||
| wrapper.unlockExpectation, | ||
| ], | ||
| timeout: 1, | ||
| enforceOrder: true | ||
| ) | ||
|
|
||
| camera.startVideoRecording(completion: { _ in }, messengerForStreaming: nil) | ||
|
|
||
| wait( | ||
| for: [ | ||
| wrapper.audioSettingsExpectation, | ||
| wrapper.videoSettingsExpectation, | ||
| ], | ||
| timeout: 1 | ||
| ) | ||
|
|
||
| XCTAssertEqual( | ||
| mockAudioSession.addedAudioOutputCount, 0, | ||
| "Audio session should not receive AVCaptureAudioDataOutput when enableAudio is false" | ||
| ) | ||
| } | ||
|
|
||
| func test_setUpCaptureSessionForAudioIfNeeded_addsAudioSession_whenAudioEnabled() { | ||
| let settings = FCPPlatformMediaSettings.make( | ||
| with: testResolutionPreset, | ||
| framesPerSecond: NSNumber(value: testFramesPerSecond), | ||
| videoBitrate: NSNumber(value: testVideoBitrate), | ||
| audioBitrate: NSNumber(value: testAudioBitrate), | ||
| enableAudio: true | ||
| ) | ||
|
|
||
| let wrapper = TestMediaSettingsAVWrapper(test: self, expectAudio: true) | ||
| let mockAudioSession = MockCaptureSession() | ||
|
|
||
| let configuration = CameraTestUtils.createTestCameraConfiguration() | ||
| configuration.mediaSettingsWrapper = wrapper | ||
| configuration.mediaSettings = settings | ||
| configuration.audioCaptureSession = mockAudioSession | ||
| let camera = CameraTestUtils.createTestCamera(configuration) | ||
|
|
||
| wait( | ||
| for: [ | ||
| wrapper.lockExpectation, | ||
| wrapper.beginConfigurationExpectation, | ||
| wrapper.minFrameDurationExpectation, | ||
| wrapper.maxFrameDurationExpectation, | ||
| wrapper.commitConfigurationExpectation, | ||
| wrapper.unlockExpectation, | ||
| ], | ||
| timeout: 1, | ||
| enforceOrder: true | ||
| ) | ||
|
|
||
| camera.startVideoRecording(completion: { _ in }, messengerForStreaming: nil) | ||
|
|
||
| wait( | ||
| for: [ | ||
| wrapper.audioSettingsExpectation, | ||
| wrapper.videoSettingsExpectation, | ||
| ], | ||
| timeout: 1 | ||
| ) | ||
|
|
||
| XCTAssertGreaterThan( | ||
| mockAudioSession.addedAudioOutputCount, 0, | ||
| "Audio session should receive AVCaptureAudioDataOutput when enableAudio is true" | ||
| ) | ||
| } | ||
| } |
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.
The two new tests, test_setUpCaptureSessionForAudioIfNeeded_skipsAudioSession_whenAudioDisabled and test_setUpCaptureSessionForAudioIfNeeded_addsAudioSession_whenAudioEnabled, share a significant amount of setup and execution logic. To improve code clarity and maintainability, this duplicated code could be extracted into a private helper function.
This function could take enableAudio as a parameter, perform the common setup for the camera and mocks, and then return the mockAudioSession to be used for assertions in the individual tests.
Here is an example of how this could be structured:
private func performAudioSetupTest(enableAudio: Bool) -> MockCaptureSession {
// ... common setup for settings, wrapper, mockAudioSession, configuration, and camera ...
// ... first wait block for configuration ...
camera.startVideoRecording(completion: { _ in }, messengerForStreaming: nil)
// ... second wait block for recording start ...
return mockAudioSession
}
func test_setUpCaptureSessionForAudioIfNeeded_skipsAudioSession_whenAudioDisabled() {
let mockAudioSession = performAudioSetupTest(enableAudio: false)
XCTAssertEqual(
mockAudioSession.addedAudioOutputCount, 0,
"Audio session should not receive AVCaptureAudioDataOutput when enableAudio is false"
)
}
func test_setUpCaptureSessionForAudioIfNeeded_addsAudioSession_whenAudioEnabled() {
let mockAudioSession = performAudioSetupTest(enableAudio: true)
XCTAssertGreaterThan(
mockAudioSession.addedAudioOutputCount, 0,
"Audio session should receive AVCaptureAudioDataOutput when enableAudio is true"
)
}This refactoring would make the tests more concise and easier to maintain.
| @@ -1,3 +1,7 @@ | |||
| ## 0.9.21+4 | |||
|
|
|||
| * Fixes crash on iOS when `enableAudio` is false by correcting audio setup guard. | |||
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.
can remove by correcting audio setup guard since it's implementation details that audience doesn't care
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.
Done
juliendelarbre
left a comment
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.
Done
| @@ -1,3 +1,7 @@ | |||
| ## 0.9.21+4 | |||
|
|
|||
| * Fixes crash on iOS when `enableAudio` is false by correcting audio setup guard. | |||
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.
Done
|
Hello, |
LouiseHsu
left a comment
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.
LGTM
…alse` by correcting guard condition (flutter/packages#9949)
…alse` by correcting guard condition (flutter/packages#9949)
flutter/packages@8ca6416...260f381 2025-10-08 jessiewong401@gmail.com Add Versioning to Gradle 9 Deprecation Change (flutter/packages#10187) 2025-10-07 julien@delarbre.biz [camera_avfoundation] iOS: Fix crash when `enableAudio == false` by correcting guard condition (flutter/packages#9949) 2025-10-07 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump androidx.fragment:fragment from 1.8.8 to 1.8.9 in /packages/local_auth/local_auth_android/android (flutter/packages#10111) 2025-10-07 10687576+bparrishMines@users.noreply.github.com [interactive_media_ads] Adds support for accessing data for an Ad (flutter/packages#9972) 2025-10-07 stuartmorgan@google.com [webview_flutter] Add Android geolocation README (flutter/packages#10166) 2025-10-07 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump com.google.guava:guava from 33.4.8-android to 33.5.0-android in /packages/espresso/android (flutter/packages#10047) 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 flutter-ecosystem@google.com 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
Summary
This PR fixes an iOS crash that occurs when using the
cameraplugin withenableAudio: falseand noNSMicrophoneUsageDescriptioninInfo.plist.Root cause:
In
DefaultCamera.setUpCaptureSessionForAudioIfNeeded(), the guard currently allows audio setup to proceed even when audio is disabled:This evaluates to
truewhenenableAudio == false, so the audio setup runs regardless.Fix: Require audio to be enabled and not already set up before proceeding:
This aligns behavior with the intended logic (“don’t set up audio twice or when audio is disabled”) and prevents the crash path on iOS when the microphone usage description is absent.
Affected file:
camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/DefaultCamera.swiftProposed correction commit:
juliendelarbre@0635db4
Related Flutter issue & discussion:
flutter/flutter#174702 (comment)
Before / After
Before (buggy): audio setup runs when
enableAudio == false, causing a crash on iOS ifNSMicrophoneUsageDescriptionis missing.After (fixed): audio setup is skipped when
enableAudio == false; no crash, behavior matches API expectations.Reproduction steps
ios/Runner/Info.plistdoes not containNSMicrophoneUsageDescription.CameraControllerwithenableAudio: false.startVideoRecording().Linked Issues
Fixes flutter/flutter#174702
Documentation
No API changes; behavior now matches the documented intent. (Optional: add a short inline comment elaborating that audio setup is skipped when
enableAudio == false.)Pre-Review Checklist
pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or I have commented below to indicate which [version change exemption] this PR falls under[^1].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style], or I have commented below to indicate which [CHANGELOG exemption] this PR falls under[^1].///).