-
-
Notifications
You must be signed in to change notification settings - Fork 605
fix: Fix discrepancy between barcode bounding box and barcode capture size #1169
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
fix: Fix discrepancy between barcode bounding box and barcode capture size #1169
Conversation
…g box for Android
…rcode in each result; update MacOS to use the new format for image data
## 5.2.2 | ||
|
||
Improvements: | ||
* [MacOS] Adds Swift Package Manager support. |
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 was added by a previous PR, but wasn't yet in the changelog.
"name" to "barcode", | ||
"data" to barcodes, | ||
"image" to image, | ||
"width" to width!!.toDouble(), |
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.
Since the width/height
is only for the output image data, I put them in a Map for the image data.
I also added safe access to the width / height. The method channel implementation handles null width/height by falling back to Size.zero
"driverLicense" to driverLicense?.data, "email" to email?.data, | ||
"geoPoint" to geoPoint?.data, "phone" to phone?.data, "sms" to sms?.data, | ||
"url" to url?.data, "wifi" to wifi?.data, "displayValue" to displayValue | ||
"calendarEvent" to calendarEvent?.data, |
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 sorted these properties alphabetically, to make it easier to follow. The line "size" to boundingBox?.size,
is new.
|
||
private val Rect.size: Map<String, Any?> | ||
get() { | ||
// Rect.isValid can't be accessed for some reason, so just do the check manually. |
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.
When inspecting the source code of android.graphics.Rect
, there was an isValid() method, but I could not access it. So I just took the check from there.
var scanner = BarcodeScanner.barcodeScanner() | ||
|
||
/// Return image buffer with the Barcode event | ||
var returnImage: Bool = 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.
This was moved
/// Gets called when a new image is added to the buffer | ||
public func captureOutput(_ output: AVCaptureOutput, didOutput sampleBuffer: CMSampleBuffer, from connection: AVCaptureConnection) { | ||
guard let imageBuffer = CMSampleBufferGetImageBuffer(sampleBuffer) else { | ||
print("Failed to get image buffer from sample buffer.") |
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 print doesn't add any value
if (error == nil && barcodesString != nil && newScannedBarcodes != nil && barcodesString!.elementsEqual(newScannedBarcodes!)) { | ||
return | ||
} else if (newScannedBarcodes?.isEmpty == 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.
Early return instead of else
|
||
var barcodesString: Array<String?>? | ||
|
||
// /// Convert image buffer to jpeg |
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.
Unused
} | ||
|
||
/// Start scanning for barcodes | ||
func start(barcodeScannerOptions: BarcodeScannerOptions?, returnImage: Bool, cameraPosition: AVCaptureDevice.Position, torch: Bool, detectionSpeed: DetectionSpeed, completion: @escaping (MobileScannerStartParameters) -> ()) throws { |
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 parameter is no longer needed
16bac88
to
3bb18bd
Compare
} else { | ||
return barcode.data | ||
} | ||
if error != 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.
Rewriting to use early returns
return | ||
} | ||
|
||
if (!MobileScannerPlugin.returnImage) { |
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 is new. Previously we would always send the image data, which is incorrect.
DispatchQueue.main.async { | ||
result(["name": "barcode", "data": barcodesMap]) | ||
} | ||
return |
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.
Early return
let corners = cornerPoints?.map({$0.cgPointValue.data}) | ||
return ["corners": corners, "format": format.rawValue, "rawBytes": rawData, "rawValue": rawValue, "type": valueType.rawValue, "calendarEvent": calendarEvent?.data, "contactInfo": contactInfo?.data, "driverLicense": driverLicense?.data, "email": email?.data, "geoPoint": geoPoint?.data, "phone": phone?.data, "sms": sms?.data, "url": url?.data, "wifi": wifi?.data, "displayValue": displayValue] | ||
return [ | ||
"calendarEvent": calendarEvent?.data, |
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.
Like on Android, I sorted these on alphabetical keys, and added the size
.
final List<Map<Object?, Object?>> barcodes = | ||
data.cast<Map<Object?, Object?>>(); | ||
|
||
if (defaultTargetPlatform == TargetPlatform.macOS) { |
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 updated the MacOS implementation to use the same keys / datatypes as iOS/Android. So this can be removed.
final double? width = event['width'] as double?; | ||
final double? height = event['height'] as double?; | ||
defaultTargetPlatform == TargetPlatform.iOS || | ||
defaultTargetPlatform == TargetPlatform.macOS) { |
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.
MacOS is added here, as the format has been updated to match the other platforms.
|
||
return BarcodeCapture( | ||
raw: data, | ||
raw: event, |
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.
MacOS used the event
Map as raw data. This wasn't true for iOS/Android, so I updated it to match MacOS.
Unlikely that a lot of people need the raw data anyway.
factory Barcode.fromNative(Map<Object?, Object?> data) { | ||
final Map<Object?, Object?>? calendarEvent = | ||
data['calendarEvent'] as Map<Object?, Object?>?; | ||
final List<Object?>? corners = data['corners'] as List<Object?>?; |
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.
Moved down, to be sorted on alphabetical names
|
||
var symbologies:[VNBarcodeSymbology] = [] | ||
|
||
// var analyzeMode: Int = 0 |
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 was unused
setScale(call, result) | ||
case "resetScale": | ||
resetScale(call, result) | ||
// case "analyze": |
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.
Unused
return | ||
} | ||
guard let imageBuffer = CMSampleBufferGetImageBuffer(sampleBuffer) else { | ||
print("Failed to get image buffer from sample buffer.") |
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.
Again, a print that didn't add any value.
} | ||
} else { | ||
|
||
if error != 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.
Refactored to use early returns.
return | ||
} | ||
|
||
let barcodes: [VNBarcodeObservation] = results.compactMap({ barcode in |
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.
There was actually a bug here, too. MacOS would call DispatchQueue.main.async {}
for every single barcode, instead of once, for all of them together. This has been corrected, by collecting the barcodes - that match the scan window - and passing them as a list.
}) | ||
|
||
DispatchQueue.main.async { | ||
if (!MobileScannerPlugin.returnImage) { |
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 is new, and similarly to iOS, we only pass the image data if requested.
result(nil) | ||
} | ||
|
||
// func switchAnalyzeMode(_ call: FlutterMethodCall, _ result: @escaping FlutterResult) { |
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.
Unused
} | ||
|
||
extension VNBarcodeObservation { | ||
public func toMap() -> [String: Any?] { |
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 is similar to the extension in the iOS implementation
} | ||
|
||
extension CGImage { | ||
public func jpegData(compressionQuality: CGFloat) -> Data? { |
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 needed this extension to get the (compressed) JPEG data from the input image.
The signature matches what the iOS implementation uses.
/// The list of scanned barcodes. | ||
final List<Barcode> barcodes; | ||
|
||
/// The bytes of the image that is embedded in the barcode. |
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.
Clarified a bunch of docs, that were incorrect.
@@ -1,3 +1,6 @@ | |||
/// @docImport 'package:mobile_scanner/src/mobile_scanner_controller.dart'; |
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.
New doc import annotation, for line 27, which has a doc reference to a different class.
"data": barcodes.map({ $0.toMap() }), | ||
"image": cgImage == nil ? nil : [ | ||
"bytes": FlutterStandardTypedData(bytes: cgImage!.jpegData(compressionQuality: 0.8)!), | ||
"width": Double(cgImage!.width), |
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 image on iOS uses doubles for width/height. On Android it uses integers, but we did convert them into doubles.
On MacOS, CGImage also uses integers, so I added a double conversion here.
import UIKit | ||
import Flutter | ||
|
||
@UIApplicationMain |
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.
UIApplicationMain is deprecated. This was updated by the Flutter tool project migrators.
This PR fixes a discrepancy between what is meant with the bounding box of a scanned barcode and the size of the barcode capture output image.
It also adds the following changes:
returnImage
on MacOSreturnImage
set to false would not do anything on iOSFixes #1162
cc @fulstadev