fix: fix barcode scanner leak on Android #1139
Conversation
…mage callback; format
| import androidx.camera.core.ImageProxy | ||
| import androidx.camera.core.Preview | ||
| import androidx.camera.core.TorchState | ||
| import androidx.camera.core.resolutionselector.AspectRatioStrategy |
There was a problem hiding this comment.
Unused import
| private val mobileScannerCallback: MobileScannerCallback, | ||
| private val mobileScannerErrorCallback: MobileScannerErrorCallback | ||
| private val mobileScannerErrorCallback: MobileScannerErrorCallback, | ||
| private val barcodeScannerFactory: (options: BarcodeScannerOptions?) -> BarcodeScanner = ::defaultBarcodeScannerFactory, |
There was a problem hiding this comment.
This new callback is mainly for tests. I'm adding a new test soon that needs this, in a follow up PR for the rounding bug #1134
| private var preview: Preview? = null | ||
| private var textureEntry: TextureRegistry.SurfaceTextureEntry? = null | ||
| private var scanner = BarcodeScanning.getClient() | ||
| private var scanner: BarcodeScanner? = null |
There was a problem hiding this comment.
This client was never disposed
| /** | ||
| * Create a barcode scanner from the given options. | ||
| */ | ||
| fun defaultBarcodeScannerFactory(options: BarcodeScannerOptions?) : BarcodeScanner { |
There was a problem hiding this comment.
Default for the barcode scanner creator function
|
|
||
| scanner.process(inputImage) | ||
| .addOnSuccessListener { barcodes -> | ||
| scanner?.let { |
There was a problem hiding this comment.
Scanner is nullable now, so used a let block
| it.process(inputImage).addOnSuccessListener { barcodes -> | ||
| if (detectionSpeed == DetectionSpeed.NO_DUPLICATES) { | ||
| val newScannedBarcodes = barcodes.mapNotNull { barcode -> barcode.rawValue }.sorted() | ||
| val newScannedBarcodes = barcodes.mapNotNull { |
There was a problem hiding this comment.
Formatting of mapNotNull to use braces
| return@addOnSuccessListener | ||
| } | ||
| if (newScannedBarcodes.isNotEmpty()) lastScanned = newScannedBarcodes | ||
| if (newScannedBarcodes.isNotEmpty()) { |
There was a problem hiding this comment.
Same formatting change here
| barcodeMap.add(barcode.data) | ||
| } | ||
| } else { | ||
| if (scanWindow == null) { |
There was a problem hiding this comment.
Inverting the check here made more sense
| val bitmap = Bitmap.createBitmap(mediaImage.width, mediaImage.height, Bitmap.Config.ARGB_8888) | ||
|
|
||
| val imageFormat = YuvToRgbConverter(activity.applicationContext) | ||
| if (isBarcodeInScanWindow(scanWindow!!, barcode, imageProxy)) { |
There was a problem hiding this comment.
Also invert the check here, which drops a redundant continue statement
| } | ||
|
|
||
| imageFormat.yuvToRgb(mediaImage, bitmap) | ||
| if (barcodeMap.isEmpty()) { |
There was a problem hiding this comment.
Early return 2x
navaronbracke
force-pushed
the
fix_barcode_scanner_leak
branch
from
39ec5a8 to
6b06c5d
Compare
| val owner = activity as LifecycleOwner | ||
| camera?.cameraInfo?.torchState?.removeObservers(owner) | ||
| // Release the camera observers first. | ||
| camera?.cameraInfo?.let { |
There was a problem hiding this comment.
This was a bug, we didn't remove all observers. removeObservers does nothing if the owner is not a listener, so calling this is safe for all three LiveData instances.
But we are listening to torch & zoom
| cameraProvider?.unbindAll() | ||
| textureEntry?.release() | ||
|
|
||
| cameraProvider = null |
There was a problem hiding this comment.
The following is just reflowing code so that the relevant things are together.
| cameraProvider = null | ||
|
|
||
| // Release the scanner. | ||
| scanner?.close() |
There was a problem hiding this comment.
This is new, we release the scanner when the users stops the camera. No need to keep it around when the preview / camera is gone.
| // Release the scanner. | ||
| scanner?.close() | ||
| scanner = null | ||
| lastScanned = null |
There was a problem hiding this comment.
Setting lastScanned to null is to keep things consistent.
| * Analyze a single image. | ||
| */ | ||
| fun analyzeImage(image: Uri, onSuccess: AnalyzerSuccessCallback, onError: AnalyzerErrorCallback) { | ||
| fun analyzeImage( |
There was a problem hiding this comment.
Analyze image now takes configurable options and uses a short lived scanner.
Using the short lived scanner is part of the fix for the retain cycle thing and allowing options is part of the fix for analyze image allowing all formats.
| fun dispose(activityPluginBinding: ActivityPluginBinding) { | ||
| methodChannel?.setMethodCallHandler(null) | ||
| methodChannel = null | ||
| mobileScanner?.dispose() |
There was a problem hiding this comment.
When the user calls MobileScannerController.dispose() we should also dispose the underlying camera / scanner. There is no guarantee that stop() is called before dispose().
We might get only dispose() when the user leaves a scanning page for example.
navaronbracke
changed the title
| @@ -1,3 +1,6 @@ | |||
| ## NEXT | |||
| * Fixed a leak of the barcode scanner on Android. | |||
There was a problem hiding this comment.
I'm planning on fixing the NaN error & analyze image format issue in the same release, so I'm using the NEXT tag here for a moment.
We should probably also look into doing the Impeller migration for this release.
…ode_scanner_leak fix: fix barcode scanner leak on Android
This PR adds the following changes:
analyzeImage()to take configurable barcode scanner options & use a short lived scannerFor future readers, iOS does not have an equivalent release method for the scanner in the MLKit library.
It's possible that uses an autorelease pool or destructor.
Fixes #988
Part of #1069
Part of #1134