(Android) Add support to detect QR codes only when inside a frame #693
Conversation
There was a problem hiding this comment.
Thanks @rawatnaresh for your first contribution!
I left a few feedbacks, but it's looking good
| val barcodeBoundingBox = barcode.boundingBox | ||
| if (barcodeBoundingBox === null) { | ||
| return@filter false | ||
| } |
There was a problem hiding this comment.
You can write that in one line. This will return early if variable is null
| val barcodeBoundingBox = barcode.boundingBox | |
| if (barcodeBoundingBox === null) { | |
| return@filter false | |
| } | |
| val barcodeBoundingBox = barcode.boundingBox ?: return@filter false |
| } | ||
|
|
||
| // val scaleX = viewFinder.width.toFloat() / imageSize.width | ||
| // val scaleY = viewFinder.height.toFloat() / imageSize.height |
| ) | ||
|
|
||
| // Check if the scaled bounding box is within the frame rectangle | ||
| barcodeFrame?.frameRect!!.contains(scaledBarcodeBoundingBox) |
There was a problem hiding this comment.
Avoid !! that would crash the app, prefer ? that would gracefully do nothing
But barcodeFrame shouldn't be optional as we check for it at the beginning, you might be able to remove it
| barcodeFrame?.frameRect!!.contains(scaledBarcodeBoundingBox) | |
| barcodeFrame.frameRect.contains(scaledBarcodeBoundingBox) |
| ) : ImageAnalysis.Analyzer { | ||
| @SuppressLint("UnsafeExperimentalUsageError") | ||
| @ExperimentalGetImage | ||
| override fun analyze(image: ImageProxy) { | ||
| val inputImage = InputImage.fromMediaImage(image.image!!, image.imageInfo.rotationDegrees) | ||
| val mediaImage = image.image ?: return |
| class QRCodeAnalyzer ( | ||
| private val onQRCodesDetected: (qrCodes: List<Barcode>) -> Unit | ||
| class QRCodeAnalyzer( | ||
| private val onQRCodesDetected: (qrCodes: List<Pair<Barcode, Size>>) -> Unit |
There was a problem hiding this comment.
In my comment, I was suggesting using a Pair to store the barcode bounding box. Turns out it's already in the Barcode object itself!
Here, as the imageSize is the same for every barcodes, I would sent it as another parameter, next to the barcodes
| private val onQRCodesDetected: (qrCodes: List<Pair<Barcode, Size>>) -> Unit | |
| private val onQRCodesDetected: (qrCodes: List<Barcode>, imageSize: Size) -> Unit |
This simplifies the code above, as the scale can be computed only once, and we don't have to "extract only the barcodes"
And below as we can just call onQRCodesDetected(result, Size(...)) without the map
| val analyzer = QRCodeAnalyzer { results -> | ||
| if (results.isNotEmpty()) { | ||
| val filteredResults = if (barcodeFrame !== null) { |
There was a problem hiding this comment.
We rarely, if ever, use triple equality in Kotlin. Same below with ===, prefer == instead
You can also create a shadow copy of barcodeFrame. This is not very sexy, but barcodeFrame is now a local val, and the compiler will allow using it later as a non optional.
Lastly, reverting the condition makes the code easier to read as we avoid having a one line else 20 lines down
That could looks something like that, with imageSize moved out
| val analyzer = QRCodeAnalyzer { results -> | |
| if (results.isNotEmpty()) { | |
| val filteredResults = if (barcodeFrame !== null) { | |
| val analyzer = QRCodeAnalyzer { (barcodes, imageSize) -> | |
| if (barcodes.isEmpty()) { | |
| return@QRCodeAnalyzer | |
| } | |
| val barcodeFrame = barcodeFrame | |
| if (barcodeFrame == null) { | |
| onBarcodeRead(barcodes) | |
| return@QRCodeAnalyzer | |
| } | |
| val filteredBarcodes = barcodes.filter { barcode -> | |
| ... | |
| } | |
| onBarcodeRead(filteredBarcodes) |
|
Hi @DavidBertet, thank you for your explanations and suggestions. I've made the changes accordingly. Please take a look |
There was a problem hiding this comment.
Nice! Thank you @rawatnaresh
PS: You might have a few linter issue, like space after if.
Summary
This PR fixes #689 and #484 based on the suggestions from #484 (comment).
Let me know if there's anything that needs to be improve in this PR—I’m happy to make changes!. Thanks.
How did you test this change?
VID_20250122_140752.mp4