search

openfoodfacts / smooth-app

🤳🥫 The new Open Food Facts mobile application for Android and iOS, crafted with Flutter and Dart
https://world.openfoodfacts.org/open-food-facts-mobile-app?utm_source=off&utf_medium=web&utm_campaign=github-repo
Apache License 2.0
791 stars 272 forks source link

Upgrade the barcode scan to flutter 3.7 #3712

Closed monsieurtanuki closed 1 year ago

monsieurtanuki commented 1 year ago

What

Steps to reproduce the behavior

  1. Use the latest version of flutter.
  2. Run the app in debug mode
  3. The barcode scan does not work

Expected behavior

The barcode scan is supposed to work.

Why

The barcode scan is a key feature of Smoothie.

Additional context

Icing on the cake would be to refactor the barcode scan:

Adiii1436 commented 1 year ago

I found:

monsieurtanuki commented 1 year ago

Thank you @Adiii1436 for your research!

In that particular case the problem is not to have a barcode scanner that works - I did it in #3694 (based on qr_code_scanner), then had to revert it.

The idea is to refactor our current implementation so that it works in 3.7.

For some reason we have to support both MLKit (better but google related) and zxing (not google related so acceptable in no-google zones), in different app flavors.

We also have to deal with camera quality and performances, which are better with native packages, but barcode scan packages include both the camera and the image processor. As we need to support MLKit and zxing, we should have a native camera on one part, that calls an abstract image processor implemented by MLKit and zxing on another part. But that's not what the current issue is about...

The point is to get access to our barcode scanner code to make it work in 3.7 and to improve its maintainability.

abhinav52000 commented 1 year ago

I want to work on this problem. Can you please assign it to me?

monsieurtanuki commented 1 year ago

The idea is to refactor our current implementation so that it works in 3.7.

@abhinav52000 the first step is to understand the current implementation, so please have a look at it.

abhinav52000 commented 1 year ago

I went through it and tough and found that in order to update one Barcode scanner package we primarily need to update various other as well.

monsieurtanuki commented 1 year ago

@abhinav52000 The goal would be to code clean OOP, something like that:

So the first step would be to split the current implementation in 3: an autonomous widget called from the home page, that uses a camera preview and a barcode scan algorithm.

Could you have a look at it?

abhinav52000 commented 1 year ago

I went through it And however tried to get the packages but some of the packages are not being resolved even in flutter version 3.0.5. So is there any extra thing I am supposed to know in order to start the implementation for this?

monsieurtanuki commented 1 year ago

@abhinav52000 Try flutter clean. And it's not about starting an implementation from scratch but rather refactoring existing code.

abhinav52000 commented 1 year ago

Sure then I am on it try to do the required

M123-dev commented 1 year ago

Heyy @monsieurtanuki I've seen you just forked juliansteenbakker/mobile_scanner, just FYK we talked about the scanner situation in today's meeting, with the outcome to test different scanner implementations in the dev mode. Not fully implemented, that's too hard. Just dumping a MVP in it to test on android and especially iOS.

Also, it should be easy to "comment out" to not make the production app size bigger than needed

https://docs.google.com/document/d/1QKOqh7WOH9dY68rzGDy20l43iuUnrYe8tcHJaf06gGI/edit#heading=h.9mykibu6iylr

monsieurtanuki commented 1 year ago

@M123-dev Dammit, you're a real spy ! ;)

I've just read the google docs - I assume it includes the latest comments after your meeting. That's why I forked.

I can already say that mobile_scanner works very well with flutter 3.7 on my old smartphone.

As for the tap dancing about the size of the app, I'm not sure using the "light" version makes sense. It looks like whatever we don't integrate in the app, we'll have to download async'ly at init. That would mean no barcode scan immediately available... And besides, the "light" mode trick would have to be maintained. cf. https://developers.google.com/ml-kit/vision/barcode-scanning/android?hl=en

That said, if the size of the app is really problematic, I suggest to create a specific issue that tracks the app version sizes, and if possible the size of code vs. assets.

M123-dev commented 1 year ago

@M123-dev Dammit, you're a real spy ! ;)

GitHub just presents me some infos they think is interesting on the front page and since I follow you this was present directly in my view ;-D

I can already say that mobile_scanner works very well with flutter 3.7 on my old smartphone.

Sounds promising

As for the tap dancing about the size of the app, I'm not sure using the "light" version makes sense. It looks like whatever we don't integrate in the app, we'll have to download async'ly at init. That would mean no barcode scan immediately available... And besides, the "light" mode trick would have to be maintained. cf. https://developers.google.com/ml-kit/vision/barcode-scanning/android?hl=en

ohh I guess I didn't put it clearly. I don't mean downloading the stuff after the install, I just mean that we should put all the scanners we're testing into the app loose first. So that we can test it on iOS and don't need lots of test apks for android. However, making it easy for us to remove them temporally without effort.

We can upload 5 different scanners in test versions of the app, but should limit ourselves to one when we promote to production.

monsieurtanuki commented 1 year ago

@M123-dev I'm about to integrate mobile_scanner to Smoothie. And perhaps qr_code_scanner too, so we'll be fdroid compliant.

monsieurtanuki commented 1 year ago

I've managed to make both MLKit and ZXing work on my low-end smartphone, in flutter 3.7. Still some fine-tuning to do, though.

monsieurtanuki commented 1 year ago

I've managed to display zxing full screen. mlkit could look the same as zxing, after minor cosmetic changes.

@M123-dev I guess we could even add a "switch barcode scanner" button on that home page (only for dev mode users), that could switch among mlkit/zxing/no scanner/numeric keyboard.

mlkit zxing
Screenshot_2023-03-05-16-26-59 Screenshot_2023-03-05-14-55-54
M123-dev commented 1 year ago

@monsieurtanuki looks fine at a first glance, I guess the spacing will look not so crowded on a somewhat bigger display

Some things to consider;

monsieurtanuki commented 1 year ago

@M123-dev The screen will look as crowded as we want it to. The screenshots say roughly 45% for the scanner and 55% for the carousel.

The old implementation is not intact. You ask a good question: how should we test and transition?

The scanning rectangle is a parameter of both new scanners, and I assume that the focus point takes that into consideration.

monsieurtanuki commented 1 year ago

Issues about focus, scan and camera, that were created in the last 12 months. @M123-dev We may use this list in order to enroll focus/scan/camera testers.

3731 - User reviews in bulk

3674 - Pixelated camera

3670 - I can't scan anything anymore

3664 - Can't Scan item from camera properly ( Blur )

3427 - Barcode Scanner with fdroid build does not work (zxing)

3316 - We're getting black screens again

3212 - Can't scan on galaxy s21 (mlkit)

3196 - Infinite refocusing on a Pixel 6 with the dev branch (mlkit)

2927 - App does not read barcode from camera on Sony Xperia 10 IV

2917 - Camera crashes the app (Samsung) (mlkit)

2916 - List of brands or models being affected by the scan issue

2741 - We have a BIG scan issue

2251 - Samsung devices do not seem able to focus (mlkit)

1898 - Focus point is still in center

thestarsahil commented 1 year ago

[Edited for clarity]

Issues about focus, scan and camera, that were created in the last 12 months. @M123-dev We may use this list in order to enroll focus/scan/camera testers.

...

3664 - Can't Scan item from camera properly ( Blur )

  • ??? (@thestarsahil)

...

Redmi Note 8 pro

stephanegigandet commented 1 year ago

@monsieurtanuki Awesome! It would be great to have a test app with both new scanners (even better would be to also have the old scanner so that we can easily compare the 3).

Maybe to start with, we could just have a test APK, so that we can test it on the devices we have, see how well the new scanners are working on a variety of phones.

monsieurtanuki commented 1 year ago

@stephanegigandet Good idea! For the moment, I just need to:

I should be able to do that today.

monsieurtanuki commented 1 year ago

I should be able to do that today.

Maybe not, because of build confusion. I try flutter build apk --split-per-abi but that doesn't work, because of kotlin version incompatibilities. @M123-dev Are there specific things I should do before the build?

M123-dev commented 1 year ago

Perfect, otherwise there shouldn't be a problem copying the whole scanning files and renaming the modifies ones:

continuous_scan_page -> continuous_scan_page_zxing

To also test them on iOS

M123-dev commented 1 year ago

I should be able to do that today.

Maybe not, because of build confusion. I try flutter build apk --split-per-abi but that doesn't work, because of kotlin version incompatibilities. @M123-dev Are there specific things I should do before the build?

Does it work without --split-per-abi a fat apk shouldn't hurt for testing, does it?

monsieurtanuki commented 1 year ago

@M123-dev Finally I've managed to build (after some kotlin version fixes), as debug (because release needs signing). Now I have a 100Mb APK. I haven't changed the app name/path: wouldn't that enter in conflict with the "real" app? What do you suggest?

M123-dev commented 1 year ago

@monsieurtanuki here a trick to build release apk's without signing

Change signingConfigs.release to signingConfigs.debug in packages\smooth_app\android\app\build.gradle for the release build type

For the app id you are right, the icon & name will be the same no matter what, but due to the app id being the same you'll have to remove the normal play store variant thus lose all the data.

I'm not completely sure, but maybe it's enough to change the applicationId in the same file, otherwise there are some executable dart packages which should change the app id in all needed places

monsieurtanuki commented 1 year ago

I've clumsily created a new repository. The apk can be downloaded here: https://github.com/monsieurtanuki/barcode_scanner_test/blob/master/app-release.apk

In my tests, I managed to:

20230308_152109

I experienced some crashes with mlkit. Probably because of my low-end smartphone poor memory.

In this first version the camera is not full screen, as it became obvious that to have a focus you're better off with the center of the camera.

monsieurtanuki commented 1 year ago

@MKCOOL142 (OnePlus 7Pro), @teolemon (Pixel 6), @thestarsahil (Redmi Note 8 pro), @natrius (Xiaomi Mi 9 with Android 10), @M123-dev (Samsung galaxy s21), @raphael0202 (Oneplus 7T), @chk1 (Sony Xperia 10 IV), @monsieurtanuki (Samsung Galaxy Core Prime SM-G360F), @CharlesNepote (???), @g123k (ZFlip3), @stephanegigandet (Samsung Galaxy S8)

Guys, you all experienced problems (crash, blur, focus, no scan, ...) with the barcode scan in Smoothie on android. We are trying new libraries in order to improve the barcode scan experience.

I've created a lousy test app, available here (https://github.com/monsieurtanuki/barcode_scanner_test/blob/master/app-release.apk), that tests only the barcode scan, with MLKit or Zxing. Both will be implemented in Smoothie, for different play stores (something like Zxing for F-Droid and MLKit for all the other play stores).

If that's OK with you, I would like you to test the following use cases:

And then report how (not) well it went.

Thank you in advance.

The faster you answer the better, so if you guys could test it before the end of the week (2023-03-12) and send me your feedbacks we'll be able to fine-tune it and eventually integrate the new scanner into Smoothie soon.

ziovelvet commented 1 year ago

Hello @monsieurtanuki , I haven't originally reported an issue here but Smoothie scan won't work either on my Pixel 5 (no scan).
Here trying to help out.

I've tested 'barcode_scanner_test' with: • both MLKit and Zxing
• 3 different types of barcodes: flat - round and film
• on 4 angles 0°, 90°, 180°, 270°

Here my experience with a Pixel 5:

with MLKit: • On flat surface it works every time, but in most case it takes 2-4 seconds to scan.
• On round surface same as flat, it works but it's still slow.
• On film surface works on all angles, still slow.

with Zxing:
• On flat surface: Immediate scan at 0° and 180°. No scan at all at 90° and 270°
• On round surface: Immediate scan at 0° and 180°. Won't scan at 90° and 270°
• On film surface: Immediate scan at 0° and 180°. No scan at all at 90° and 270°

monsieurtanuki commented 1 year ago

Thank you @ziovelvet for your feedback! If I sum up:

About mlkit being slow, that can be fixed. I ran tests on my low-end smartphone, and as it sometimes crashed with mlkit I put a delay to 5s instead of 250ms. This delay is a mlkit parameter: the shorter the delay the more likely you have memory crashes.

I'll upload today a new version of the test app with various delay parameters.

monsieurtanuki commented 1 year ago

@ziovelvet The new version of the test app is available at the same place (https://github.com/monsieurtanuki/barcode_scanner_test/blob/master/app-release.apk).

Now there are 3 buttons for MLKit. The fastest being the 250ms button, which is the more likely to crash on low-end devices (it did crash when I tested it).

Mattis142 commented 1 year ago

@monsieurtanuki okay so I am using a OnePlus 7 Pro for this so I do not know the low end device performance,

flat and round both worked with with both libraries (mostly since most manufacturers do not put bar codes in a horizontal position on round products so finding a test product was challenging) I couldn't really think of anything challenging that would make sense to use for a test

now for the issues: Zxing seems to not be able to scan 45° rotated bar codes, only 180 and 0° worked for me, MLkit was fine with all directions. autofocus with Zxing was a lot slower compared to MLkit for me (as seen in the video). Zxing was slightly faster than MLkit(250ms) but barely noticable

also for some reason this black on brown barcode worked a lot worse with zxing:

https://user-images.githubusercontent.com/40364718/224060610-a8773550-c28e-442f-ad80-d847a29a68b7.mp4

I am not sure if this is simply Zxing not supporting this kind of barcode but this one never got scanned using Zxing even after multiple tries (tbf I'm not an expert on the matter so it could just be a completely different type of code idk) Screenshot_20230309-154940_SystemUI

monsieurtanuki commented 1 year ago

Thank you @MKCOOL142 for your feedback! Basically you seem to confirm that both mlkit and zxing work, except that zxing only works for horizontal barcodes. In theory mlkit and zxing support the same formats, and I limited both to ean8 and ean13 for the tests. I'll upload today a new version with all 1D formats for both.

Mattis142 commented 1 year ago

@monsieurtanuki that's what I'm surprised about tho, the barcode in that screenshot only worked with MLkit, would you be able to explain to me why that is since I'm kinda curious now

ziovelvet commented 1 year ago

@monsieurtanuki I've tried with 2 old devices, OnePlus 3T (6GB RAM) and Samsung A5 2017 (3GB RAM), no idea if low-end enough. Both can handle MLKit 250ms without crashing.

Mattis142 commented 1 year ago

maybe it would make sense to calculate some score based on hardware specs and assign a score which than would influence the delay, i.e mid rangers or phones with <3gb ram would get 2000ms delay, even slower devices an even higher delay

monsieurtanuki commented 1 year ago

that's what I'm surprised about tho, the barcode in that screenshot only worked with MLkit, would you be able to explain to me why that is since I'm kinda curious now

@MKCOOL142 I think MLKit cheats, as the format seems to be UPC-A, which was not supposed to be detected at the time by neither mlkit nor zxing (in the expected barcode formats I provided).

I've tried with 2 old devices, OnePlus 3T (6GB RAM) and Samsung A5 2017 (3GB RAM), no idea if low-end enough. Both can handle MLKit 250ms without crashing.

@ziovelvet 1GB RAM is apparently low-end enough ;)

maybe it would make sense to calculate some score based on hardware specs and assign a score which than would influence the delay, i.e mid rangers or phones with <3gb ram would get 2000ms delay, even slower devices an even higher delay

@MKCOOL142 I don't know how we could estimate that. The trick would probably be to switch to ZXing on 1GB RAM devices, or to provide ZXing with MLKit with a message to the user ("if the barcode scan crashes, please consider using the fallback barcode scanner", with a preference button).

I've just uploaded a new version (3) with all 1D barcode formats - where I guess the UPC-A should be detected by ZXing too. For the record that's where MLKit seems to crash very often on my low-end device, even with 5000ms. ZXing is still good.

The barcode formats that can now be detected are:

I don't know which barcode formats really make sense. And I believe that make my app crash on my device for MLKit, probably because it's a bit more to process (in the previous app version I only asked to detect 2 formats, ean-8 and ean-13).

Mattis142 commented 1 year ago

@MKCOOL142 I don't know how we could estimate that. The trick would probably be to switch to ZXing on 1GB RAM devices, or to provide ZXing with MLKit with a message to the user ("if the barcode scan crashes, please consider using the fallback barcode scanner", with a preference button).

I've just uploaded a new version (3) with all 1D barcode formats - where I guess the UPC-A should be detected by ZXing too. For the record that's where MLKit seems to crash very often on my low-end device, even with 5000ms. ZXing is still good.

maybe you could make it smart where if the app crashes because it ran out of memory the next time it would start you could display a message like "it seems that your device ran out of memory, to prevent crashes the barcode scanning method changed"

monsieurtanuki commented 1 year ago

@MKCOOL142 As long as you can detect an OOM crash, which I don't know how to do.

I would prefer a first time message like "You're currently using a very powerful barcode scanner, that uses a lot of memory. If you prefer a good barcode scanner that is less resource intensive, go to the preferences and select the failure-free barcode scanner".

Mattis142 commented 1 year ago

@MKCOOL142 As long as you can detect an OOM crash, which I don't know how to do.

I would prefer a first time message like "You're currently using a very powerful barcode scanner, that uses a lot of memory. If you prefer a good barcode scanner that is less resource intensive, go to the preferences and select the failure-free barcode scanner".

I asked a friend who said that they don't know a way to detect oom crashes, maybe Zxing should be the default for any phone that's below a certain amount of ram (maybe even completely block MLkit from being enabled in those cases to prevent people from locking themselves out of the app) and phones higher that just with the toggle unlocked in the settings and a small warning

raphael0202 commented 1 year ago

Here are the results on my cellphone (OnePlus 7T) with the v2 version: MLKIT (250ms): works in all angle, superfast ZXING 45: works in 0° and 180°, not in 90° and 270°

MLKIT with higher delay works as well, but is of course slower.

monsieurtanuki commented 1 year ago

Thank you @raphael0202 for your feedback! That confirms the other results.

chk1 commented 1 year ago

I can confirm @raphael0202's test results for my phones too

Sony Xperia 10 IV (Android 13), Xiami Redmi 4X (Android 11 custom ROM)

monsieurtanuki commented 1 year ago

Thank you @chk1! So far, the only downside I see with the upcoming PR is that the camera is not immersive anymore.

M123-dev commented 1 year ago

I can report the same, both working on my phone 👍🏼

So far, the only downside I see with the upcoming PR is that the camera is not immersive anymore.

Looking at your screenshots a few comments above, it looked really promising 💯

monsieurtanuki commented 1 year ago

Thank you @M123-dev for your tests!

The screenshots I shared were in immersive mode, but finally I had to put the camera only on top of the screen because the autofocus was not working correctly. Let's forget the immersive mode for this PR, as it's tagged as "low priority" anyway. If someone feels like trying to code the immersive mode later in a next PR, why not, but for the moment this is not a priority at all.

I'm still waiting until tomorrow for additional test results.

So far it looks very positive, so the next test would be inside the app itself. For that, that would make sense to create a new app version before I PR anything. We'll have a decent app with recent bug fixes in the play stores, which will give us room for barcode scanner tests on the debug or test version of the app after the PR is merged. @M123-dev @teolemon @stephanegigandet Is that OK if we create a new version now, so that we can test the "new" barcode scanners in real life conditions?

monsieurtanuki commented 1 year ago

Thank you @M123-dev @chk1 @raphael0202 @MKCOOL142 @ziovelvet for your tests and feedbacks! All of you had a positive experience with the new barcode scanners, which is great. I'm now in the process of putting the code of the test app into the Smoothie app, in flutter 3.7.

monsieurtanuki commented 1 year ago

For the record following are the apk sizes of my test app:

stephanegigandet commented 1 year ago

@M123-dev @teolemon @stephanegigandet Is that OK if we create a new version now, so that we can test the "new" barcode scanners in real life conditions?

That sounds like a good plan to me, what do you think @M123-dev @teolemon

M123-dev commented 1 year ago

Yes play store is no problem, but appstore is not working at the moment due to some credentials problem. We (mostly @teolemon ) is working on it