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

feat: Support focus depth/distance on android #2937

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HugoGresse
Copy link

@HugoGresse HugoGresse commented Jun 4, 2024

This PR add support for focus depth/distance on android.

This is based on this work: https://github.com/Fabi019/hid-barcode-scanner/blob/a38601ee94941304eaf515baa89cd5dbfc9ee092/app/src/main/java/dev/fabik/bluetoothhid/ui/model/CameraViewModel.kt#L60

After/if this PR is merged (🤞 ), I'll work on the iOS equivalent.

Changes

  • New method on the CameraSession to set the focus depth. It also change the autofocus to off, which is convenient bacause the end user can use the app with autofocus, then decide to focus manually. The focus max need to be aligned with device.minFocusDistance already available.

Tested on

Samsung S23. The feature was made for the Pl@ntNet plant identification app, so I intend to test the feature on my coworkers devices, before going to 20k users in beta.

Related issues

Relates to #1809

Demo

az_recorder_20240604_180249_edited.mp4

Copy link

vercel bot commented Jun 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-vision-camera ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 4, 2024 3:58pm

@HugoGresse
Copy link
Author

HugoGresse commented Jun 6, 2024

Tested on multiple devices (pixel 6,cheap readmi, s23) and it worked without issues!

@mrousavy
Copy link
Owner

Hey - thank you for your contribution this looks really cool! ❤️

I do think however that focusDepth should not be a function, but instead a view property on <Camera>. That way it can be animated using Reanimated/Native Props and you can create cool sliders or similar.

Also I can only merge this if it has both iOS and Android implementations.

Can you make those changes? Thanks!

@HugoGresse
Copy link
Author

ok for moving it as a prop, I've duplicated the logic initialy from the focus method, so I thought it made sense to do it that way.
I'll work on the iOS equivalent locally and update the PR later next week.

Quick question, do you think I should adjust the auto contrast stuff? It seems that on some device, the PR disable the auto exposure (AWB?), but not on all devices. I may add a button to toggle between a few exposure settings in Pl@ntNet but let me know what you think.

@mrousavy
Copy link
Owner

Ah yeah I can see how it is very imperative, maybe a prop isn't the right idea after all. Is there no way to set focus distance via CameraX?
I kinda want to avoid such Camera2 interop features...

@mrousavy
Copy link
Owner

It seems that on some device, the PR disable the auto exposure (AWB?), but not on all devices

That's why I want to avoid Camera2 interop 😅

I went through a lot of bullshit in VisionCamera V3 when I used Camera2 directly. It's such a wanky API that I want to stay away from it and handle everything through CameraX. If there's no way to use CameraX for it I don't know if I really want to merge this into main...

@HugoGresse
Copy link
Author

I looked hard into cameraX and found no similar method, only having to use the interop methods.
I understand how you feel with visionV3, i thought it could help us too, went it prod, and had a lot of feedback. But v4 seems to work great so far (already in prod, no issue)

@mrousavy
Copy link
Owner

I really like the feature and I really want to have this in VisionCamera, but I am honestly not sure if it's a good idea to use Camera2.. sorry...

Maybe we can create a feature request at the CameraX issue tracker and hope they add it to CameraX, then easily add it to VisionCamera?

I'll think about this a bit. Maybe I can merge it, after all it's entirely optional anyways.

From a user perspective, a prop sounds more logical. From an implementation perspective, it's an imperative function.

Either way thank you for the contribution, I'll take a few days to think about this and then maybe pick this up again - maybe I'll also just do the iOS implementation then.

Btw.; great news hearing that VisionCamera powers your app on thousands of devices out there. 💪

@HugoGresse
Copy link
Author

Ticket created: https://issuetracker.google.com/issues/346918497

@mrousavy
Copy link
Owner

Awesome, thank you! Let's hear what they say

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

Successfully merging this pull request may close these issues.

2 participants