-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[Camera] Add lens type information (iOS) #7653
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
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).
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.
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).
This seems like a reasonable starting place to me; we can always add more options in later PRs if there are compelling use cases and we can do something reasonable across platforms. @bparrishMines what do you think?
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.
Just to clarify, this PR adds CameraLensType
to CameraDescription
. And this value will only ever be set on iOS because none of the other platforms have a comparable distinction?
This is fine with me for the current architecture of the camera
plugin. I typically don't like adding an API to the platform interface that will seemingly only ever be set by one platform because this leads to a lot of confusion as more info is added to the class. I would prefer that camera_avfoundation
return a subclass of CameraDescription
with the value, but I don't want to set the precedent of importing camera_avfoundation
without a more in-depth look at the entire plugin structure.
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.
@bparrishMines Yes at this time I have only made the changes for iOS. The additional info will not be returned on any other platform. Although this sets the table so that we can expand support to Android or other platforms in the future without significant changes to the architecture.
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.
I typically don't like adding an API to the platform interface that will seemingly only ever be set by one platform
It seems plausible that a community PR would add this to camera_android
in the future, given the comments in the table that Camera2 supports it. I believe we still expect camera_android
to exist for the foreseeable future even though our efforts will be focused on camera_android_camerax
, because some complex use cases can't be supported via CameraX.
...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? |
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
Hi @stuartmorgan, this is now approved by @hellohuanlin. What are the next steps to merge it? And when would this be available for us to use? Thanks! |
@stuartmorgan @bparrishMines @ditman FYI I created #8723 |
Ran format script Fix Package.resolved
…and camera_description.dart
from `/packages/packages/camera` ran `dart run ../../script/tool/bin/flutter_plugin_tools.dart format --current-package`
Removed the changes to the pubspecs in the packages that have no other changes (the Android, web, and Windows)
Status update from triage: The platform interface PR landed, so this is waiting for the implementation package PR to be extracted. |
f197557
to
572c93d
Compare
572c93d
to
4b9335c
Compare
Remove Package.resolved file and update QueueUtils.h for consistent formatting
…0.9.19 for lens type querying support on iOS
13ef799
to
17240e4
Compare
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.