-
-
Notifications
You must be signed in to change notification settings - Fork 530
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: Ability to scan both normal codes and inverted codes #1215
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks okay, but I have feedback for the implementation.
Will you be looking into color inversion for the Vision API (for MacOS) as well?
android/src/main/kotlin/dev/steenbakker/mobile_scanner/MobileScanner.kt
Outdated
Show resolved
Hide resolved
ios/Classes/MobileScanner.swift
Outdated
|
||
func invertImage(image: UIImage) -> UIImage { | ||
let ciImage = CIImage(image: image) | ||
let filter = CIFilter(name: "CIColorInvert") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not use a magic string here, you should be using https://developer.apple.com/documentation/coreimage/cifilter/3228292-colorinvert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@navaronbracke can you give me a hand here?
I'm trying to make it API available, as I've seen there are a few statements for iOS 13 somewhere in the project, but despite setting my project deployment target to 15, I keep seeing this:
I've done just a little Swift in my life, it's been more Android
Co-authored-by: Navaron Bracke <brackenavaron@gmail.com>
Co-authored-by: Navaron Bracke <brackenavaron@gmail.com>
…canner.kt Co-authored-by: Navaron Bracke <brackenavaron@gmail.com>
Co-authored-by: Navaron Bracke <brackenavaron@gmail.com>
# Conflicts: # android/src/main/kotlin/dev/steenbakker/mobile_scanner/MobileScannerHandler.kt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juliansteenbakker This entire implementation would omit MacOS, even though that should be supported there also, considering the API's we use.
Since this is a draft, can we close this PR and reopen this on the dev branch instead?
@@ -116,6 +116,11 @@ abstract class MobileScannerPlatform extends PlatformInterface { | |||
throw UnimplementedError('updateScanWindow() has not been implemented.'); | |||
} | |||
|
|||
/// Set inverting image colors in intervals (for negative Data Matrices). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Set inverting image colors in intervals (for negative Data Matrices). | |
/// Enable or disable the inverting of image colors. | |
/// | |
/// This is useful when working with negative-color Data Matrices. | |
/// See also: https://en.wikipedia.org/wiki/Negative_(photography) |
/// | ||
/// Defaults to false and is only supported on Android and iOS. | ||
/// | ||
/// Defaults to false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second Defaults to false.
can be removed.
Future<void> setShouldConsiderInvertedImages(bool shouldConsiderInvertedImages) async { | ||
await methodChannel.invokeMethod<void>( | ||
'setShouldConsiderInvertedImages', | ||
{'shouldConsiderInvertedImages': shouldConsiderInvertedImages}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just pass in a boolean flag instead of a Map here? The native implementations will need to be updated accordingly.
@@ -79,7 +83,17 @@ class MobileScanner( | |||
@ExperimentalGetImage | |||
val captureOutput = ImageAnalysis.Analyzer { imageProxy -> // YUV_420_888 format | |||
val mediaImage = imageProxy.image ?: return@Analyzer | |||
val inputImage = InputImage.fromMediaImage(mediaImage, imageProxy.imageInfo.rotationDegrees) | |||
|
|||
// Invert every other frame. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Invert every other frame. | |
// Invert every other frame, by flipping the flag on every input frame. |
|
||
// Invert every other frame. | ||
if (shouldConsiderInvertedImages) { | ||
invertCurrentImage = !invertCurrentImage // so we jump from one normal to one inverted and viceversa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invertCurrentImage = !invertCurrentImage // so we jump from one normal to one inverted and viceversa | |
invertCurrentImage = !invertCurrentImage |
// Convert YUV_420_888 image to NV21 format | ||
// based on our util helper | ||
val bitmap = Bitmap.createBitmap(image.width, image.height, Bitmap.Config.ARGB_8888) | ||
YuvToRgbConverter(activity).yuvToRgb(image, bitmap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds another usage of the deprecated RenderScript API's. Can we avoid this?
See #1142
/// Sets the zoomScale. | ||
private func setShouldConsiderInvertedImages(_ call: FlutterMethodCall, _ result: @escaping FlutterResult) { | ||
let shouldConsiderInvertedImages = call.arguments as? Bool | ||
if (shouldConsiderInvertedImages == nil) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we guard mobileScanner.setShouldConsiderInvertedImages(shouldConsiderInvertedImages!)
to not do anything if the argument is null ? Otherwise this does not match Android.
ios/Classes/MobileScanner.swift
Outdated
@@ -48,6 +48,12 @@ public class MobileScanner: NSObject, AVCaptureVideoDataOutputSampleBufferDelega | |||
|
|||
var detectionSpeed: DetectionSpeed = DetectionSpeed.noDuplicates | |||
|
|||
var shouldConsiderInvertedImages: Bool = false | |||
// local variable to invert this image only this time, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be a one line comment.
// local variable to invert this image only this time, | |
// This flag is used to determine if the current frame should be color-inverted. |
Sounds good to me @navaronbracke. The android implementation can be a lot faster, i will work on it. I'll first merge the changes from master into develop, before fixing the merge conflicts on this PR. |
# Conflicts: # android/src/main/kotlin/dev/steenbakker/mobile_scanner/MobileScanner.kt # android/src/main/kotlin/dev/steenbakker/mobile_scanner/MobileScannerHandler.kt # ios/Classes/MobileScanner.swift # ios/Classes/MobileScannerPlugin.swift # lib/src/mobile_scanner_controller.dart # lib/src/objects/start_options.dart
@navaronbracke do you see any usecase for the |
@juliansteenbakker I agree, you'd probably want to set it up on the controller itself. That follows the same pattern for the other options. |
Started on #1214 to improve #1071 but with a few enhancements:
intervalInvertImage
instead of choosing whether to invert the image constantly or not. This way we can read both types, for example:Both types of codes (example)