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: Alternative mode for camera (Android only feature) #2953

Merged
merged 5 commits into from
Sep 7, 2022

Conversation

g123k
Copy link
Collaborator

@g123k g123k commented Sep 6, 2022

What

The MLKit plugin doesn't work on some devices with a byte array, but is OK when passing a file instead.
The native plugin (camera) is able to both provide a byte array or a file only on Android (as iOS seems OK).

The PR allows switching between the two modes.

Here is the screenshot of the new setting (only visible on Android)
1

2

Some notes:

  • The focus should now be a bit quicker than before (my last PR was a bit too conservative)
  • For the "normal mode", the native part only sends one image every 100 ms (some devices like the Pixel 6 Pro was able to send 3 images during this time). It should lower memory usage.

Related to #2741

@g123k g123k added 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… camera labels Sep 6, 2022
@g123k g123k self-assigned this Sep 6, 2022
@g123k g123k requested a review from a team as a code owner September 6, 2022 21:52
@g123k g123k changed the title feat: Alternative mode the camera (Android only feature) feat: Alternative mode for camera (Android only feature) Sep 6, 2022
@g123k
Copy link
Collaborator Author

g123k commented Sep 6, 2022

An open question: we have a list of whitelisted devices (eg: OnePlus smartphones).
Should we let the choice to change the mode?

@teolemon
Copy link
Member

teolemon commented Sep 7, 2022

  • What about a button to relaunch the app (not sure that's even possible ?)
  • What about a button to autogenerate an email with the device name ?

@teolemon
Copy link
Member

teolemon commented Sep 7, 2022

Or even log the device name when opening this settings, and using matomo, we should be able to see successful scans

@g123k
Copy link
Collaborator Author

g123k commented Sep 7, 2022

Or even log the device name when opening this settings, and using matomo, we should be able to see successful scans

This is a nice idea.
But first I want to be sure it works well.

To be honest I wonder if it shouldn't be the new default mode

@monsieurtanuki
Copy link
Contributor

To be honest I wonder if it shouldn't be the new default mode

Given all the pain in the neck around the camera, if you found a more reliable code I agree that we should use it as the default mode.

@g123k
Copy link
Collaborator Author

g123k commented Sep 7, 2022

The drawback with this new solution is that it consumes more I/O on the storage, but less memory.

Some devices use microSD and in that case, I'm not entirely sure what it can give in terms of performance.

Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

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

I'm not a big fan of the dynamic typing but thats fine, the code looks fine to me. Lets merge, test and then decide if we change to default mode

@g123k
Copy link
Collaborator Author

g123k commented Sep 7, 2022

That's idea here, a first implementation to test if the scanner works better and I will create a second PR to improve things accordingly 👍

@teolemon
Copy link
Member

teolemon commented Sep 7, 2022

ok, merging

@teolemon teolemon merged commit 15e1f57 into openfoodfacts:develop Sep 7, 2022
@g123k g123k deleted the camera_alternative_mode branch September 7, 2022 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
camera 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion…
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants