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

Hot restart - camera no longer usable #1037

Open
EArminjon opened this issue Apr 23, 2024 · 22 comments
Open

Hot restart - camera no longer usable #1037

EArminjon opened this issue Apr 23, 2024 · 22 comments

Comments

@EArminjon
Copy link
Contributor

If i do a hot restart while camera is displayed, I no longer have the possibility to use it until app is closed.

MobileScannerException: code genericError, message: Called start() while already started

mobile_scanner: ^5.0.1

@dcarv01
Copy link

dcarv01 commented Apr 23, 2024

Here too, I need to reinstall the app. If I close and open it again, the same error persists.

mobile_scanner: ^5.0.0

@EArminjon
Copy link
Contributor Author

@dcarv01 can you tell me your device + os, that's weird to uninstall as a unique solution.

@laterdayi
Copy link

same #773 (comment)

@EArminjon
Copy link
Contributor Author

Personally I didn't have an issue with the hot reload, only the hot restart.

@navaronbracke
Copy link
Collaborator

@EArminjon We indeed need to provide better support for hot reload. I filed #773 for that in the past.

We might be able to use the reassemble hook from the framework, but I'm not sure.

I'm going to close this issue in favor of #773 so that we have only one tracking issue.

@navaronbracke navaronbracke closed this as not planned Won't fix, can't repro, duplicate, stale Apr 24, 2024
@EArminjon
Copy link
Contributor Author

EArminjon commented Apr 25, 2024

Hello @navaronbracke,

I didn't talk about hot reload but hot restart, can you confirm that your answer fit for hot restart ?

@navaronbracke
Copy link
Collaborator

Hot restart is a full restart of the app state. Perhaps if we fix hot reload, we might also fix it for hot restart?

@EArminjon
Copy link
Contributor Author

Personally i didn't have any issue around hot reload, only hot restart for both Android and iOS.

@EArminjon
Copy link
Contributor Author

EArminjon commented Apr 25, 2024

To be more precise : on hot restart camera work but not the preview on flutter side (black screen) : i can scan some document despite my black screen.

@dcarv01
Copy link

dcarv01 commented Apr 25, 2024

I too have problem only with Hot restart, hot reload is ok.

@dcarv01 can you tell me your device + os, that's weird to uninstall as a unique solution.

I tried again, and there was no need to uninstall, perhaps I was mistaken.

@EArminjon
Copy link
Contributor Author

EArminjon commented Apr 25, 2024

A hint :

I've removed some protections to force call the stop() method from MobileScanner.swift before instantiate the MobileScannerController() on my dart side.
I've added some protection inside this stop() method to only process some action if the variable is not nil.

Result :

On hot restart camera works.

Limit :

I can see the green camera OS icon after the hot restart, so camera is still used by something. Not normal.

Conclusion :

We need to find a way on hot restart to remove used resources or on start() to do so.

(maybe this topic can help once fixed : flutter/flutter#10437)

@EArminjon
Copy link
Contributor Author

On our app, we always dispose the MobileScannerController when we need to do 'pause' or to open a new page which potentially will contain an other MobileScannerController().

We use visibility detector to start() the camera and we delay it a bit to let other MobileScannerController() instance the time to dispose. That's an other issue which i will open later because it can be fixed maybe by this issue.

@EArminjon
Copy link
Contributor Author

EArminjon commented Apr 25, 2024

Based on these observations and as it's not hot reload directly linked, I think we can reopen :/ ?

@navaronbracke navaronbracke reopened this Apr 25, 2024
@navaronbracke
Copy link
Collaborator

I will keep this issue open for tracking. Perhaps if we fix the hot reload support, the other issue goes away, although I am not certain.

@navaronbracke
Copy link
Collaborator

@EArminjon I just now published a fix on the master channel that addresses the problems with hot reload in #1086

I plan on releasing a fix with this patch shortly. Could you verify on master (using a git override) if this issue is also fixed by that patch? I specifically targeted hot reload and not hot restart, hence the question.

@EArminjon
Copy link
Contributor Author

Hello @navaronbracke, still black on hot restart :'( !

  mobile_scanner:
    git:
      url: https://github.com/juliansteenbakker/mobile_scanner
  

@navaronbracke
Copy link
Collaborator

Interesting. Does hot reload work for you? (using lowercase r in the terminal when running a Flutter app)

In the case of hot restart (upper case R), do you see any errors in logcat / the terminal? Does the MobileScannerController throw any errors? Does the MobileScannerController.value.error provide you with an error?

@EArminjon
Copy link
Contributor Author

EArminjon commented Sep 30, 2024

Hot reload always worked for me. Today i use on my project 5.2.3and not issue with hot reload.

Actually no error despite the black screen. When i put some breakpoints on your package i saw "MobileScannerErrorCode.controllerAlreadyInitialized" (see code after). Maybe the previous instance can't kill the camera and so after app restart it can't create an other one.

 if (error.errorCode ==
          MobileScannerErrorCode.controllerAlreadyInitialized) {
        return;
      }

@navaronbracke
Copy link
Collaborator

navaronbracke commented Sep 30, 2024

Hmm, yes this seems like an issue with hot restart like you mentioned earlier in this thread.

I added the check on error.errorCode == MobileScannerErrorCode.controllerAlreadyInitialized to prevent issues with hot reload, as it might call start() on a new instance of the controller there.

I think you might be right that in this case, we should still do the above check, but we should actually stop the old controller and then just continue.

So we have two situations:

Hot Reload (r): The Dart snapshot is reloaded, but the app still runs. In this case, prevent the controller from starting again, using the check above, as the native code has not changed and the app is still running. Only the Dart snapshot has changed in memory. This is what the patch that I finished today does.
Hot Restart (R): The entire app reloads from scratch. The native side should be disposed, so that the app can create a new instance (like you indicated)

To fix the second case, we will probably need to know if the app restarted due to hot reload or hot restart.
Unless if we change the check above to always dispose the old instance before continuing as usual?

When I implemented the check above I wasn't sure if we should "ignore the additional calls to start and abort" or "detect it, dispose of the old instance and continue with starting a new instance"

So I think I made the wrong choice here? Although, now that we have the new check in place, adjusting it to fit the new situation should be easy (in the if statement, we await stop() and remove the return;)

What do you think about this option?

@EArminjon
Copy link
Contributor Author

EArminjon commented Oct 1, 2024

Sorry, for the delay.

The following code didn't fix the issue (even if i dispose my page and recreate it, camera is not longer available), it's more deep.

if (error.errorCode ==
          MobileScannerErrorCode.controllerAlreadyInitialized) {
        await stop();
        return;
      }

As i remember, Flutter didn't allow package to detect Hot Restart (only Hot Reload) : flutter/flutter#10437. I don't know so if it's possible to today to find a way for disposing the camera instance on hot restart.

@navaronbracke
Copy link
Collaborator

navaronbracke commented Oct 1, 2024

Shouldn't we do do this instead? (in mobile_scanner, not in your code)

if (error.errorCode == MobileScannerErrorCode.controllerAlreadyInitialized) {
  await stop();
}

// Continue with the rest of the start method, as if the scanner never started before.

But if this also does not fix the issue, then I agree that this can only be fixed when flutter/flutter#10437 is fixed, so that we can detect this case in the plugin, using an API.

@EArminjon
Copy link
Contributor Author

Shouldn't we do do this instead? (in mobile_scanner, not in your code)

if (error.errorCode == MobileScannerErrorCode.controllerAlreadyInitialized) {
  await stop();
}

// Continue with the rest of the start method, as if the scanner never started before.

But if this also does not fix the issue, then I agree that this can only be fixed when flutter/flutter#10437 is fixed, so that we can detect this case in the plugin, using an API.

I've tested that, it sadly didn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants