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

fix: Close barcode scanner and free it's resources #1181

Merged
merged 7 commits into from
Mar 10, 2022

Conversation

M123-dev
Copy link
Member

@M123-dev M123-dev commented Mar 4, 2022

I wondered why it would fail but only on the second attempt, maybe this fixes it, even if not its good for freeing unused resources

cc: @cli1005

@M123-dev M123-dev requested a review from monsieurtanuki March 4, 2022 12:32
@M123-dev M123-dev requested a review from a team as a code owner March 4, 2022 12:32
@github-actions github-actions bot added 🤳 MLKit 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… labels Mar 4, 2022
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.

You close the barcodeScanner but you never reopen it...
I guess the open/close behavior should be somehow similar to the controller's.

@M123-dev
Copy link
Member Author

M123-dev commented Mar 4, 2022

Added documentation @monsieurtanuki

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.

@M123-dev You're missing the point: how do you expect the barcodeScanner to work after being closed? This stop method is called on an onPause callback, with a corresponding onResume callback. If onResume is called after onPause, the barcodeScanner will be closed.

@M123-dev
Copy link
Member Author

M123-dev commented Mar 4, 2022

No I understand what you mean @monsieurtanuki, its just that there is no way to manually start the scanner. ML Kit just has three public methods: https://developers.google.com/android/reference/com/google/mlkit/vision/barcode/BarcodeScanner?hl=en

The scanner is "started" when giving it it's first image

@monsieurtanuki
Copy link
Contributor

there is no way to manually start the scanner

Then your solution cannot work. After you close the barcodeScanner with onPause, will it be automagically restarted with the next onResume or the next scan? Maybe onPause and onResume are not correct names, they should be onCreate and onDestroy in order to leave no hope of resuming.

@M123-dev
Copy link
Member Author

M123-dev commented Mar 4, 2022

I think we are talking past one another here @monsieurtanuki

The lifecycle looks like it:

idle...

while(running){
      image => ML Kit
      ML Kit starts
      ML Kit => barcode
      ML Kit goes back to idle
}



dispose()
{
  stopImageStream()
  stopMLKit() // Just frees remaining recources
}

There isn't even a public start method from ml kit, it's all done internally

On android it worked fine without closing the scanner but maybe this differs from android to iOS maybe they have a check that the scanner can't handle a new pic if the last one is still in there without responsing or so, honestly idk but it's worth trying

@monsieurtanuki
Copy link
Contributor

@M123-dev If you want to destroy the barcode scanner, I guess the correct syntax would be:

await barcodeScanner?.close();
barcodeScanner = null;

Then come my previous remarks about the difference between onPause/onResume (e.g. we temporarily sleep while a dialog is displayed but will go back again) and onCreate/onDestroy (e.g. when the app is killed because another app needs tons of memory: if we go back again, it will be from scratch). Here the LifeCycleManager is about onPause/onResume.

In this current PR, assuming that the mere presence of the barcode scanner is a nuisance, I suggest that you just create a new BarcodeScanner in _startLiveFeed and close/nullify it in _stopImageStream.

@M123-dev M123-dev requested a review from monsieurtanuki March 10, 2022 16:39
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.

It would be cleaner with no default value at construction time:

BarcodeScanner? barcodeScanner;

instead of

BarcodeScanner? barcodeScanner = GoogleMlKit.vision.barcodeScanner();

But that's ok too I guess.

@M123-dev M123-dev merged commit d490682 into develop Mar 10, 2022
@M123-dev M123-dev deleted the ml-kit-ios-second-scan-attempt-fix branch March 10, 2022 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤳 MLKit 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion…
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants