[camera_avfoundation] Implementation swift migration - part 12 #9781
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request continues the migration of the camera_avfoundation plugin to Swift by moving the FLTCam initialization logic into the Swift DefaultCamera class. This includes moving many properties from the Objective-C superclass to the Swift subclass, improving encapsulation. The changes are well-structured and the updates to tests and other parts of the codebase are consistent with the migration.
My review includes a few suggestions to improve maintainability and fix a potential issue in the new DefaultCamera initializer related to error handling and testability.
| videoCaptureSession.commitConfiguration() | ||
| captureDevice.unlockForConfiguration() |
There was a problem hiding this comment.
The catch block should use the mediaSettingsAVWrapper to commit the configuration and unlock the device, just like in the success path. Bypassing the wrapper here breaks the abstraction and could cause issues with testing.
| videoCaptureSession.commitConfiguration() | |
| captureDevice.unlockForConfiguration() | |
| mediaSettingsAVWrapper.commitConfiguration(for: videoCaptureSession) | |
| mediaSettingsAVWrapper.unlockDevice(captureDevice) |
There was a problem hiding this comment.
is mediaSettingsAVWrapper a wrapper around the session?
| if mediaSettings.framesPerSecond != nil { | ||
| // The frame rate can be changed only on a locked for configuration device. | ||
| try mediaSettingsAVWrapper.lockDevice(captureDevice) | ||
| mediaSettingsAVWrapper.beginConfiguration(for: videoCaptureSession) | ||
|
|
||
| // Possible values for presets are hard-coded in FLT interface having | ||
| // corresponding AVCaptureSessionPreset counterparts. | ||
| // If _resolutionPreset is not supported by camera there is | ||
| // fallback to lower resolution presets. | ||
| // If none can be selected there is error condition. | ||
| do { | ||
| try setCaptureSessionPreset(mediaSettings.resolutionPreset) | ||
| } catch { | ||
| videoCaptureSession.commitConfiguration() | ||
| captureDevice.unlockForConfiguration() | ||
| throw error | ||
| } | ||
|
|
||
| FLTSelectBestFormatForRequestedFrameRate( | ||
| captureDevice, | ||
| mediaSettings, | ||
| videoDimensionsForFormat) | ||
|
|
||
| if let framesPerSecond = mediaSettings.framesPerSecond { | ||
| // Set frame rate with 1/10 precision allowing non-integral values. | ||
| let fpsNominator = floor(framesPerSecond.doubleValue * 10.0) | ||
| let duration = CMTimeMake(value: 10, timescale: Int32(fpsNominator)) | ||
|
|
||
| mediaSettingsAVWrapper.setMinFrameDuration(duration, on: captureDevice) | ||
| mediaSettingsAVWrapper.setMaxFrameDuration(duration, on: captureDevice) | ||
| } | ||
|
|
||
| mediaSettingsAVWrapper.commitConfiguration(for: videoCaptureSession) | ||
| mediaSettingsAVWrapper.unlockDevice(captureDevice) | ||
| } else { | ||
| // If the frame rate is not important fall to a less restrictive | ||
| // behavior (no configuration locking). | ||
| try setCaptureSessionPreset(mediaSettings.resolutionPreset) | ||
| } |
There was a problem hiding this comment.
This initializer is quite long. To improve readability and maintainability, consider extracting this block of logic for setting up the resolution and frame rate into a separate private helper method.
Additionally, within that logic, using defer for unlockDevice and commitConfiguration would make the code more robust against potential errors.
| self?.inProgressSavePhotoDelegates.removeValue( | ||
| forKey: | ||
| settings.uniqueID) |
There was a problem hiding this comment.
For better readability, this call to removeValue(forKey:) could be on a single line.
self?.inProgressSavePhotoDelegates.removeValue(forKey: settings.uniqueID)
| videoCaptureSession.commitConfiguration() | ||
| captureDevice.unlockForConfiguration() |
There was a problem hiding this comment.
is mediaSettingsAVWrapper a wrapper around the session?
Migrates camera implementation as part of flutter/flutter#119109
This PR migrates the 9th chunk of
FLTCamclass to Swift:initFLTCamtoDefaultCameratextureIdfield fromFLTCam(it seemscameraIdis used as "texture id" and this field wasn't used for a while now)Some properties of the FLTCam have to be temporarily made public so that they are accessible in DefaultCamera.
Pre-Review Checklist
[shared_preferences]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 under1.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 under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3