Skip to content
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

iOS camera provider enhancements #7857

Merged
merged 12 commits into from Apr 20, 2022
Merged

iOS camera provider enhancements #7857

merged 12 commits into from Apr 20, 2022

Conversation

RobertFlatt
Copy link
Contributor

@RobertFlatt RobertFlatt commented Apr 4, 2022

iOS camera provider enhancements, these are iOS only but implemented in the camera provider shared by iOS and MacOS.

The additional apis implement basic mobile camera functionality, this is not currently implemented.

To provide basic camera services on iOS, this PR adds

 def get_device_orientation(self):

 def change_camera_input(self, index):

 def zoom_level(self, level):

 def get_app_documents_directory(self):

 def save_texture(self, texture, filepath = '', quality = 0.9):
   # saves to iOS Photos App if filepath not specified,
   # or app documents directory if filepath is specified.

The fork also builds and runs on MacOS, indicating it is unlikely the changes have broken the MacOS camera provider.

TODO, but not part of this PR:

1) kivy-ios does not load the camera provider, the Camera widget fails. Assuming this PR is accepted in a timely fashion, I will implement kivy-ios PR that fixes the load and references this enhanced camera provider.

  1. iOS has native photo and video capture apis, these are not currently implemented in this camera provider.

@welcome
Copy link

welcome bot commented Apr 4, 2022

Thanks for opening your first pull request here! 💖 Please check out our contributing guidelines.

@RobertFlatt
Copy link
Contributor Author

kivy-ios does not load the camera provider, the Camera widget fails. Assuming this PR is accepted in a timely fashion, I will implement kivy-ios PR that fixes the load and references this enhanced camera provider.

Not required with kivy-ios master.

Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM at a first glance!

I will test it on runtime (and will try to write a demo App, unless you already have one to share) during the upcoming weekend 😃

Super-minor: How about adding some logging when functionalities are not expected to be available but are called on macOS or vice-versa? Comments are super-fine, but users tend to not read them immediately. 😆

@RobertFlatt
Copy link
Contributor Author

This example uses the new features https://github.com/Android-for-Python/INDEX-of-Examples#c4k_photo_example

It depends on the Preview widget not the Camera widget. For iOS only, the master version of Preview is required https://github.com/Android-for-Python/Camera4Kivy . Release waits on this PR and a kivy-ios that points to the new Kivy master.

The readme has not been updated for ios. Example shows switch between cameras, capture photo or screenshot, pinch-spread for zoom (on my SE only the back camera), device rotation behavior, mirrored front camera.

To enable use of the Camera

        <key>NSCameraUsageDescription</key>
	<string> </string>

To enable save to Photos App

	<key>NSPhotoLibraryAddUsageDescription</key>
	<string> </string>

To make contents of local storage visible to File Manager

	<key>UIFileSharingEnabled</key>
	<true/>
        <key>LSSupportsOpeningDocumentsInPlace</key>
	<true/>

Image saves default to Photos App. The Preview widget storage selection is the same as for Android, so to save to app storage in place of Photos App capture_photo(location = 'private') https://github.com/Android-for-Python/Camera4Kivy#location

[I started this because I wanted to try to port the tflite recipe, but my test example used a camera, so ...]

Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it on my iPhone 12 Pro, and generally works good!

Unfortunately, after some minor edits to your example and Camera4Kivy, I wasn't able to attach the ultra-wide or telephoto (back) lenses due to how camera selection is handled.

I've added some comments explaining what could be done to improve it.

NSArray *devices;
AVCaptureDevice *device;

devices = [AVCaptureDevice devicesWithMediaType:AVMediaTypeVideo];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

devicesWithMediaType has been deprecated in iOS 10.0, iPadOS 10.0, macOS 10.15, Mac Catalyst 13.1 and it doesn't provide access to ultra-wide or telephoto camera lenses.

Instead,AVCaptureDeviceDiscoverySession should be used.

Comment on lines +459 to +470
if (_cameraNum >= 0) {
int camNum = _cameraNum % [devices count];
if (camNum != _cameraNum) {
NSLog(@"Warning: Max Camera Num is %lu; Using camera %d\n",
(unsigned long)([devices count] - 1), camNum);
}
device = [devices objectAtIndex:camNum];
cameraNum = _cameraNum;
} else {
device = [AVCaptureDevice defaultDeviceWithMediaType:AVMediaTypeVideo] ;
cameraNum = 0;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new discoverySessionWithDeviceTypes:mediaType:position: allows a fairly more granular selection of the camera.

The devices array may be not consistent of returning the "correct" order of the cameras, so could be nice to create an enumerator on Kivy side, and pass the configuration requested from the user to discoverySessionWithDeviceTypes:mediaType:position:

@RobertFlatt
Copy link
Contributor Author

  1. The api mentioned depends on ios10+ , (last time I looked) we build with minimum 9 not 10. And this is not adjustable __IPHONE_OS_VERSION_MIN_REQUIRED fixed for toolchain build kivy-ios#691 On the subject of min api, the photo capture api gets good with ios11.

  2. I don't understand enough about deviceTypes https://developer.apple.com/documentation/avfoundation/avcapturedevicetype or Apple devices to implement this. It is not clear to me what the options do, or any side effects.
    I expect it is possible to synthesize some useful abstraction that extends Kivy's camera = [0,1] but at this point I don't know what it should be, or even what the usage model would be. So I would not try.

  3. This PR provides basic iOS functionality, which we don't have without the PR. We already know the camera provider could be improved, see the TODO at the top of the file. Support for ultrawide and telephoto should be added to that list. Approve this PR as a starting point. Anybody who understands the additional enhancement design issues can then jump in.

@misl6
Copy link
Member

misl6 commented Apr 10, 2022

  1. Yeah, the API mentioned depends on iOS 10+, and we're still targeting iOS 9 minimum. (Maybe is a good idea to higher that requirement?). But, considering that on iOS is always a good practice to check for the availability of APIs (or for the iOS version that introduced that feature), we can easily switch between the two implementations during runtime.

  2. Decrypting Apple's docs is always a nightmare, so not your fault 😄

  3. I agree that this PR is an improvement to the current situation, and will merge it as-is if you don't expect to continue working on it in the near/mid-term. In that case, I will open an issue that we'll need to likely target before releasing 2.2.x regarding the deprecated code and missing support for non-standard cameras

@RobertFlatt
Copy link
Contributor Author

Updated the comments at the top.
I have other priorities so this is as far as I will take it.
In the future maybe further enhancements will pop to the top of my stack.

@RobertFlatt
Copy link
Contributor Author

@misl6 Can you approve this, please.

Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you!
I will open a couple of issues targeting what we discussed during the review.

@misl6 misl6 merged commit 0e77e2a into kivy:master Apr 20, 2022
@welcome
Copy link

welcome bot commented Apr 20, 2022

Congrats on merging your first pull request! 🎉🎉🎉

@RobertFlatt
Copy link
Contributor Author

@misl6 You open to a kivy-ios PR that bumps the kivy version to include this change?

https://github.com/RobertFlatt/kivy-ios/blob/master/kivy_ios/recipes/kivy/__init__.py#L10

@misl6 misl6 added this to the 2.2.0 milestone Apr 21, 2022
@misl6
Copy link
Member

misl6 commented Apr 23, 2022

LGTM. Thank you!
I will open a couple of issues targeting what we discussed during the review.

See:
#7871

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants