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

Upgrade the barcode scan to flutter 3.7 #3712

Closed
monsieurtanuki opened this issue Feb 17, 2023 · 70 comments · Fixed by #3767
Closed

Upgrade the barcode scan to flutter 3.7 #3712

monsieurtanuki opened this issue Feb 17, 2023 · 70 comments · Fixed by #3767
Assignees
Labels
🐛 bug Something isn't working 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion…

Comments

@monsieurtanuki
Copy link
Contributor

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:

  • the current barcode scan doesn't work on my smartphone (crashes)
  • the current barcode scan code is somehow hidden in github
  • I coded a fresh barcode scan based on zxing that worked perfectly on my smartphone in fix: 3674 - simplified the barcode scan with native solution #3694, and the development was much faster (initial flutter run start time) as - I suppose - the code was local and less complex. The PR worked very well and fixed a dozen of issues, including P0, but I had to close it because it discarded most of the current barcode scan code. The current barcode scan code is embedded in the rest of the code - you cannot touch it without touching the rest of the app - and should be completely isolated in a package instead.
  • beyond the barcode scan issues, the flutter camera package doesn't seem to work very well either (cf. Pixelated camera #3674), and I recommend using native camera code.
  • I cannot code or test on this issue - I have no idea where the code is hidden (I'm not very good with github), I cannot test on a device (the initial flutter run takes 30 minutes and when I reload the app it crashes)
@Adiii1436
Copy link
Contributor

Adiii1436 commented Feb 18, 2023

I found:

@monsieurtanuki
Copy link
Contributor Author

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
Copy link

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

@monsieurtanuki
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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

  • a widget that displays the camera preview and scans barcode, with a callback
  • that widget would be called by the home page
  • in that widget we would have a camera preview widget and a barcode scan from camera preview widget
  • we could decide to use a native camera preview or a flutter camera preview
  • we could decide to use mlkit or zxing as barcode scanner

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
Copy link

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
Copy link
Contributor Author

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

@abhinav52000
Copy link

abhinav52000 commented Feb 26, 2023

Sure then I am on it try to do the required

@M123-dev
Copy link
Member

M123-dev commented Mar 2, 2023

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
Copy link
Contributor Author

@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
Copy link
Member

M123-dev commented Mar 2, 2023

@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 monsieurtanuki self-assigned this Mar 3, 2023
@monsieurtanuki
Copy link
Contributor Author

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

@monsieurtanuki
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

M123-dev commented Mar 5, 2023

@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;

  • Is the "old" implementation still intact (testing the new one without stopping us from releasing)
  • Did you find a way to change the focus point to be in the middle of the scanning square

@monsieurtanuki
Copy link
Contributor Author

@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
Copy link
Contributor Author

monsieurtanuki commented Mar 5, 2023

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

  • Redmi Note Pro 11

#3674 - Pixelated camera

  • OnePlus 7Pro (@MKCOOL142)

#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)

  • Xiaomi Mi 9 with Android 10 (@natrius)

#3316 - We're getting black screens again

  • Huawei Honor 10
  • RedMi Note 10 Pro

#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

  • Sony Xperia 10 IV (@chk1)

#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
Copy link

thestarsahil commented Mar 5, 2023

[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 )

...

Redmi Note 8 pro

@stephanegigandet
Copy link
Contributor

@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
Copy link
Contributor Author

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

  • add a button on home page that switches from mlkit to zxing
  • create locally an APK with a different name
  • attach the APK to the current issue in a comment
  • ping all of you as testers

I should be able to do that today.

@monsieurtanuki
Copy link
Contributor Author

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
Copy link
Member

M123-dev commented Mar 6, 2023

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
Copy link
Member

M123-dev commented Mar 6, 2023

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
Copy link
Contributor Author

@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
Copy link
Member

M123-dev commented Mar 6, 2023

@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
Copy link
Contributor Author

@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, the current develop version of Smoothie now includes the "new" barcode scanner.
Please test it as much as possible, in real life conditions. If something strange happens it's better to know asap so we can fix it.
If you don't know how to install the develop version of the app, I don't either, but I guess @M123-dev does know.

@Mattis142
Copy link

@monsieurtanuki i am confused on where to select which of the two (Zxing MLkit) to use for scanning, I found nothing in the settings mby I overlooked it

@thestarsahil
Copy link

@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, the current develop version of Smoothie now includes the "new" barcode scanner.
Please test it as much as possible, in real life conditions. If something strange happens it's better to know asap so we can fix it.
If you don't know how to install the develop version of the app, I don't either, but I guess @M123-dev does know.

Still Facing the same Autofocus problem

@monsieurtanuki
Copy link
Contributor Author

@MKCOOL142 @thestarsahil @natrius Be sure to use the latest develop version - well, 4.6.0 is the latest published version.

If you're familiar with flutter, you can

  • flutter upgrade # should be something like flutter 3.7 / dart 2.19
  • get the latest smooth_app code
  • cd smooth-app/packages/smooth_app
  • flutter run -t lib/entrypoints/android/main_google_play.dart # for mlkit (to be deployed in almost all play store)
  • flutter run -t lib/entrypoints/android/main_fdroid.dart # for zxing (to be deployed only on FDroid)

I'm currently trying to create an apk; I'll keep you posted.

@thestarsahil
Copy link

@MKCOOL142 @thestarsahil @natrius Be sure to use the latest develop version - well, 4.6.0 is the latest published version.

If you're familiar with flutter, you can

  • flutter upgrade # should be something like flutter 3.7 / dart 2.19
  • get the latest smooth_app code
  • cd smooth-app/packages/smooth_app
  • flutter run -t lib/entrypoints/android/main_google_play.dart # for mlkit (to be deployed in almost all play store)
  • flutter run -t lib/entrypoints/android/main_fdroid.dart # for zxing (to be deployed only on FDroid)

I'm currently trying to create an apk; I'll keep you posted.

Thanks for the update men

@monsieurtanuki
Copy link
Contributor Author

@thestarsahil Do you mean it finally works?

I'm currently trying to create an apk; I'll keep you posted.

Unfortunately I don't seem to be able to create that stupid apk. There are tons of thing I ignore about compiling - and I intend to keep it that way.

M123-dev pushed a commit that referenced this issue Mar 18, 2023
Impacted files:
* `pubspec.lock`: wtf
* `pubspec.yaml`: upgraded mlkit (bug fix)
@M123-dev M123-dev reopened this Mar 18, 2023
@M123-dev
Copy link
Member

(Re-opening for the discussion in here)

Here a build apk ready to test, not for looks but for functionality.
You can now switch the scanning engine (mlkit and zxing) in the dev settings (how to enter it)

https://drive.google.com/file/d/1R2b_rbQuq9Lv-vGErU-8xdvmumQYwhcz/view?usp=sharing

@Mattis142
Copy link

mhhh so I did some testing with ML Kit and it is more prone to scanning incorrectly (these screenshots are both from the same bar code)
Screenshot_20230318-130019_SystemUI
Screenshot_20230318-130049_OpenFoodFacts

@thestarsahil
Copy link

@thestarsahil Do you mean it finally works?

I'm currently trying to create an apk; I'll keep you posted.

Unfortunately I don't seem to be able to create that stupid apk. There are tons of thing I ignore about compiling - and I intend to keep it that way.

Not working properly Have some time to update

@monsieurtanuki
Copy link
Contributor Author

@MKCOOL142 Possible reasons/fixes:

  • mlkit may not be mature enough - please try with zxing, is that better?
  • do you experience the same incorrect barcode detection with my test app?
  • perhaps confusion is added with too many formats checked - which barcode formats are we supposed to detect?
  • incorrect barcode detections are possible anyway - that would make sense to give the user more feedback possibility ("that's not correct!" would send a sentry msg, "let me try with another barcode scanner" would switch to zxing, "let me input the correct barcode" would open a numeric keyboard)
  • btw there's an unexpected rrect window on your screenshots - any idea what it comes from?

@monsieurtanuki
Copy link
Contributor Author

Not working properly Have some time to update

@thestarsahil Could you please be a little more specific about what works and what does not work, and with mlkit and zxing?

@thestarsahil
Copy link

Not working properly Have some time to update

@thestarsahil Could you please be a little more specific about what works and what does not work, and with mlkit and zxing?

Actually, When we try to scan the barcode the scanner couldn't recognize that information from the product
So Update on the camera feature! That Camera Focusing focuses on the object barcode

@chk1
Copy link

chk1 commented Mar 19, 2023

(Re-opening for the discussion in here)

Here a build apk ready to test, not for looks but for functionality. You can now switch the scanning engine (mlkit and zxing) in the dev settings (how to enter it)

https://drive.google.com/file/d/1R2b_rbQuq9Lv-vGErU-8xdvmumQYwhcz/view?usp=sharing

Test on Sony Xperia 10 IV:

  • On first launch, after tapping through the welcome wizard, viewfinder stays black (small red text "ML KIT"). Locking and unlocking phone somehow fixed that without app restart (I think this was an issue with the production version too afair)

MLKIT:

  • Scanning works great
  • Switching to profile and back to scan view causes an error banner message, but does not have an impact on anything else (scanning still works) "Something went wrong (start)! MobileScannerException: code genericError, message: Called start() while already started"
    Screenshot of error message mentioned in previous point

Zxing:

  • Feels about the same as MLKIT, maybe a little slower and more picky about angles
  • Unlike with the test app mentioned earlier in this topic, I can scan barcodes at almost any angle

@monsieurtanuki
Copy link
Contributor Author

Thank you @chk1 for your positive feedback!
About the red snackbar, it's being dealt with in #3782 #3787.
The only problem seems to be the initial non-display of the scanner after onboarding: I'll have a look at it again.

@teolemon teolemon added the 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… label Mar 20, 2023
@monsieurtanuki
Copy link
Contributor Author

@chk1 Could not reproduce the "no scanner after onboarding" bug with the latest code, with mlkit as default.
Perhaps it's a side effect of the new (and temporary) mlkit/zxing test switch.
I'll keep that bug in mind, hoping (?) I will reproduce it.

@raphael0202
Copy link

I just installed the new version (4.6.0) using internal release on my cellphone (OnePlus 7T), it works really well using MLKit, scanning is superfast, good job! 🎉

@monsieurtanuki
Copy link
Contributor Author

@raphael0202 Sounds good but unfortunately there's a possible confusion with versions, so please test with https://drive.google.com/file/d/1R2b_rbQuq9Lv-vGErU-8xdvmumQYwhcz/view?usp=sharing

@juliansteenbakker
Copy link

Hi all, as the developer of the mobile_scanner package i am happy to help with any issues regarding the package.

A quick note about some people having trouble getting the correct barcode from a scan: The MLKit framework decides which resolution is used for an image to be analyzed, and i have found out that this can be to low on some devices.

I'm currently trying to find a solution by using some kind of 'quality presets', so we can improve scan accuracy on all devices.

@monsieurtanuki
Copy link
Contributor Author

I'm currently trying to find a solution by using some kind of 'quality presets', so we can improve scan accuracy on all devices.

Thank you @juliansteenbakker! If you could make it optional we'll have a better assessment of the actual impact of that fine-tuning.

@monsieurtanuki
Copy link
Contributor Author

This issue is closed now: we do have 2 barcode scanners (mlkit and zxing) working on flutter 3.7.
Changes and improvements can be dealt with in different issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion…
Development

Successfully merging a pull request may close this issue.