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: ML Kit scanner #893

Merged
merged 10 commits into from
Jan 12, 2022
Merged

feat: ML Kit scanner #893

merged 10 commits into from
Jan 12, 2022

Conversation

M123-dev
Copy link
Member

@M123-dev M123-dev commented Jan 7, 2022

What

Now using ML Kit as default scanning engine (Default so that all testers even without knowing about the dev mode can test it)

Why

Caveats

  • When we produce 100% free versions, we'll have to be careful to remove binary blobs.

@github-actions github-actions bot added 🍎 iOS iOS specific issues or PRs scan labels Jan 7, 2022
end

Copy link
Member Author

@M123-dev M123-dev Jan 7, 2022

Choose a reason for hiding this comment

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

the iOS native changes here are not tested

@@ -41,6 +41,8 @@ dependencies:
path: ../smooth_ui_library
url_launcher: ^6.0.17
visibility_detector: ^0.2.2
google_ml_barcode_scanner: ^0.0.2
Copy link
Member Author

Choose a reason for hiding this comment

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

FYK

This is a standalone barcode scanner from the original google_ml_kit plugin.

Reason as to why I extracted the original plugin is because of unnecessary imports creates a huge file size of app during build.

Comment on lines 90 to 100
final Size size = MediaQuery.of(context).size;
// calculate scale depending on screen and camera ratios
// this is actually size.aspectRatio / (1 / camera.aspectRatio)
// because camera preview size is received as landscape
// but we're calculating for portrait orientation
double scale = size.aspectRatio * _controller!.value.aspectRatio;

// to prevent scaling down, invert the value
if (scale < 1) {
scale = 1 / scale;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This calculation is from here:

https://stackoverflow.com/a/61487358/13313941

without that the camera looks really streched when moving around

Copy link
Contributor

Choose a reason for hiding this comment

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

If so please put that in the code's comment, not here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough

if (!mounted) {
return;
}
_controller?.setFocusMode(FocusMode.auto);
Copy link
Member Author

Choose a reason for hiding this comment

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

We can also set it to manual and allow for focusing on a certain focus offset as there is the option in the native android app, which is relevant for devices with poor autofocus
@teolemon

@M123-dev M123-dev requested review from monsieurtanuki and jasmeet0817 and removed request for monsieurtanuki January 9, 2022 13:12
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @M123-dev!
I would have used 2 different Widgets rather than 2 different pages: not enough abstraction in this PR for my personal taste.
But at least now we can test if ML Kit works better.
And @teolemon will at last scan with his new expensive smartphone ;)

Comment on lines +73 to +74
ScaffoldMessenger.of(context)
.showSnackBar(const SnackBar(content: Text('Ok')));
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea

Comment on lines 90 to 100
final Size size = MediaQuery.of(context).size;
// calculate scale depending on screen and camera ratios
// this is actually size.aspectRatio / (1 / camera.aspectRatio)
// because camera preview size is received as landscape
// but we're calculating for portrait orientation
double scale = size.aspectRatio * _controller!.value.aspectRatio;

// to prevent scaling down, invert the value
if (scale < 1) {
scale = 1 / scale;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If so please put that in the code's comment, not here.

packages/smooth_app/lib/pages/scan/ml_kit_scan_page.dart Outdated Show resolved Hide resolved
@M123-dev
Copy link
Member Author

M123-dev commented Jan 9, 2022

@monsieurtanuki just made ee0249b, this is most abstraction which makes sense, since the QRView from qr_code_scanner needs the constraints from the LayoutBuilder.
I am actually unsure if the code was prettier before or now

@teolemon
Copy link
Member

teolemon commented Jan 9, 2022

Great :-)
As mentioned orally:

  • It's not a 100% can't scan bug for me. It's an 30% autofocus bug.
  • We need to have both QR and MLKit to evaluate performance (and potentially others in the future)
  • From what I understand
    • uncouple the videostream acquisition from the barcode decoding library.
    • Probably give as many options on the initial videostream (back camera, front camera, manual focus, autofocus…).
    • The diversity of hardware is such that trusting one scanner might not work (
    • Some people don't want MLKit or Scandit, but purely open, so we should have a runtime switch, and probably a compile time switch as well
  • @gspencergoog do you have any suggestions ? would you know a barcode scanning expert who could suggest things to be careful about ?

@teolemon
Copy link
Member

Additional interesting link on a behaviour change from Android 12 which points how CameraX and Camera2 are impacting 3rd party apps differently on various devices https://developer.android.com/about/versions/12/features#camera2-extensions

@M123-dev
Copy link
Member Author

M123-dev commented Jan 12, 2022

Thanks for your review @monsieurtanuki

@teolemon I'm just going to merge so that we can test this scanner

@teolemon
Copy link
Member

teolemon commented Jan 12, 2022

arf, merge conflict.
edit: solved.

@M123-dev
Copy link
Member Author

@M123-dev M123-dev merged commit 4a85c62 into develop Jan 12, 2022
@M123-dev M123-dev deleted the ml-kit branch January 12, 2022 17:54
@teolemon teolemon removed the scan label Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍎 iOS iOS specific issues or PRs 🤳 MLKit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants