-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[camera_avfoundation] Return accurate values for lensType on iOS #10075
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
Conversation
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 implements mapping of native iOS camera device types (AVCaptureDevice.DeviceType) to the platform-agnostic CameraLensType. This is achieved by updating the Pigeon interface to include lens type information, modifying the native Swift code to perform the mapping, and adjusting the Dart implementation to use the new data. The changes include new unit tests for the mapping logic and refactoring of existing tests. I have one suggestion to improve the robustness of the native Swift code.
| switch device.deviceType { | ||
| case .builtInWideAngleCamera: | ||
| lensType = .builtInWideAngleCamera | ||
| case .builtInTelephotoCamera: | ||
| lensType = .builtInTelephotoCamera | ||
| case .builtInUltraWideCamera: | ||
| lensType = .builtInUltraWideCamera | ||
| default: | ||
| lensType = .unknown | ||
| } |
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.
For better forward-compatibility, it's recommended to use @unknown default instead of default when switching over non-frozen enums like AVCaptureDevice.DeviceType. This will cause the compiler to issue a warning if a new case is added in a future SDK version, reminding us to handle it explicitly. This pattern is already used for device.position in this same file.
| switch device.deviceType { | |
| case .builtInWideAngleCamera: | |
| lensType = .builtInWideAngleCamera | |
| case .builtInTelephotoCamera: | |
| lensType = .builtInTelephotoCamera | |
| case .builtInUltraWideCamera: | |
| lensType = .builtInUltraWideCamera | |
| default: | |
| lensType = .unknown | |
| } | |
| switch device.deviceType { | |
| case .builtInWideAngleCamera: | |
| lensType = .builtInWideAngleCamera | |
| case .builtInTelephotoCamera: | |
| lensType = .builtInTelephotoCamera | |
| case .builtInUltraWideCamera: | |
| lensType = .builtInUltraWideCamera | |
| @unknown default: | |
| lensType = .unknown | |
| } |
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.
In general, I'd agree with this suggestion, but in this specific case I think it's better to leave the "old" style default in place. Switching to @unknown default would require naming each of the current 15 values here. That would include some deprecated values, and also some non-camera devices like microphones and depth sensors. This might be something for closer consideration in a later PR.
But I'd welcome other perspectives from the reviewers.
|
From "Linux repo_checks" The only change in If I understand correctly, we will remove the dependency override after reviews are complete, and then camera/camera will have no changes. For this reason, I believe it's exempt from the versioning and change log requirements. |
ee60864 to
8f3d129
Compare
|
Hello code owners @bparrishMines @vashworth @hellohuanlin I'm unsure of the protocol here, so I just wanted to follow up, and flag this as being ready for review, when time permits. I've rebased a couple of times to keep up with new changes landing on Please let me know if there's anything I've missed, or if other actions are needed on my part. Thanks for all you do -- much appreciated. |
8f3d129 to
f7d6fba
Compare
Those instructions are for multi-package PRs; PRs that change only package don't need any overrides, so that change should be reverted. |
6894722 to
4059e9e
Compare
Previously, lensType was always .unknown on iOS. This CL implements a mapping from the platform's AVCaptureDevice.DeviceType to our more generic CameraLensType. [camera_avfoundation] bump min version of camera_platform_interface to 2.10.0 These changes depend on the CameraLensType that was added to camera_platform_interface in version 2.10.0. [camera] revert overrides in base camera package [camera_avfoundation] bump package version to 0.9.21+5
4059e9e to
baeabd0
Compare
Thanks @stuartmorgan-g. I've reverted that override, squashed, and rebased on |
Currently,
CameraDescription.lensTypeis always.unknownon iOS. This PR implements a mapping from the iOS nativeAVCaptureDevice.DeviceTypeto the existingCameraLensTypevalues defined in camera_platform_interface, ensuring that an accurate value for lensType will now be returned for all camera devices that camera_avfoundation presently supports.This PR is offered as a possible alternative to #7653, and #9959, in case those PRs have become stalled. I believe this accomplishes the same goals and aligns with the discussions there, and in related issues. This PR may also provide somewhat better test coverage.
Fixes flutter/flutter#174390
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 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].///).If you need help, consider asking for advice on the #hackers-new channel on Discord.