-
Notifications
You must be signed in to change notification settings - Fork 3k
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] Add lens type information (iOS) #7653
base: main
Are you sure you want to change the base?
Conversation
0927fab
to
2515622
Compare
Please see https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changing-federated-plugins for information on how to make this PR pass CI checks, which is important for review. |
I made changes as specified by the docs, please let me if there's anything else I need to do |
This PR is currently failing a substantial number of our CI checks, so this checkbox state is not accurate. If you click through the details of each CI task failure you can see the specific issues that will need to be fixed before this would be ready for review. |
Hi @stuartmorgan, I fixed tests and most CI errors. One of the remaining errors is related to versions and the changelog not being updated. But I think that some packages don't require version bumps since there are no changes. Could you confirm if this assumption is correct?
The other issue is with formatting. I ran the format command using Flutter Plugin Tools, but there's still an error in |
Yes; for packages that don't require any changes at all, you can just revert the dependency_override changes in those packages, at which point the CI will stop flagging them as issue.
This is probably due to a |
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.
As a high level review note: as discussed here, we don't model platform interface APIs directly on one platform's APIs. A comparison of lens type information across at least iOS, Android, and web will be a necessary part of evaluating whether the values of this new enum are the ones we want.
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.
What is the best way to discuss this more in-depth? Is discord the best channel ?
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.
Discord or the GitHub issue are both options for discussing API design. For something like this, I think the best place to start would be a comparison table showing how each platform's lens information does or doesn't align with other platforms.
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.
Here is a comparison table @stuartmorgan
Lens Type | iOS | Android | Web | Windows | macOS | Linux |
---|---|---|---|---|---|---|
General Support | Available via AVCaptureDeviceType API |
Lens type info can be determined using Camera2 API. LENS_FACING for front/back/external, and field of view calculated using focal length (LENS_INFO_AVAILABLE_FOCAL_LENGTHS ) and sensor size (SENSOR_INFO_PHYSICAL_SIZE ). CameraX does not expose this level of detail. |
Lens type info not available. Web APIs (MediaDevices ) allow access to cameras but without detailed hardware info on lens types. |
Lens type info not available. No native API for detailed lens types, standard webcam info available via DirectShow/MediaFoundation APIs | Lens type info available via AVCaptureDevice API. If the attached camera (e.g., with telephoto or wide lens) supports detailed lens info, the API can retrieve it. However, most built-in or external webcams do not expose multiple lens types | Lens type info not available. No native API for detailed lens types, standard webcam info available via v4l2 (Video4Linux2) |
Ultra-Wide | Supported | Supported | No lens type info | No lens type info | Supported | No lens type info |
Telephoto | Supported | Supported | No lens type info | No lens type info | Supported | No lens type info |
Wide | Supported (default) | Supported (default) | No lens type info | No lens type info | Supported (default) | No lens type info |
- iOS avcapturedevicetype
- Android Camera 2 CameraCharacteristics
- On Web, Windows, Linux, and MacOS, there is camera access but no native support to access specific lens types or hardware details.
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.
Thanks; what about the other values in your enum? Are dual, dual-wide, and triple supported on Android? What does "continuity" mean conceptually as a lens type, and is there some more abstract definition of it that could apply to other platforms?
(The issue might be a better place for this, as the table is pretty hard to read in the review comment UI.)
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.
Hi @stuartmorgan , dual, dualWide, triple are not directly supported on Android. But it is possible to get the info, e.g. from the number of physical lenses via the Camera2 API CameraCharacteristics.LOGICAL_MULTI_CAMERA_PHYSICAL_IDS
for dual and triple. As for "continuity", I don't think we should include it here after all. It refers to a iPhone camera used as a webcam for a Mac.
I was thinking to change this to support only these three - Ultra-Wide, Wide, Telephoto - for simplicity, and because it covers most common use-cases, without handling more complex multi-lens configurations (e.g., dual, triple).
...amera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/CameraPlugin.m
Show resolved
Hide resolved
From triage: @lenzpaul Are you still planning on the discussion related to the enum values? |
Ran format script Fix Package.resolved
Note that there seems to be an error in the CI related to Package.resolved. I'm not sure how to resolve this. Could this maybe be related to the XCode version on the build machine? XCode 16.2 was just released... |
...amera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/CameraPlugin.m
Outdated
Show resolved
Hide resolved
...ios/camera_avfoundation/Sources/camera_avfoundation/include/camera_avfoundation/messages.g.h
Outdated
Show resolved
Hide resolved
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 after addressing comments
…and camera_description.dart
from `/packages/packages/camera` ran `dart run ../../script/tool/bin/flutter_plugin_tools.dart format --current-package`
This PR adds lens type information. The goal is to identify whether the lens type is wide, ultra-wide, telephoto, etc., as discussed in flutter/flutter#119908. The iOS implementation is complete, so lens data will now be populated on iOS.
Current CameraDescription for iPhone 11
New CameraDescription for iPhone 11
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.