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: App crashes when Camera using scanner is closed #2213

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Hector-Chong
Copy link

@Hector-Chong Hector-Chong commented Nov 25, 2023

What

The App crashes when the Camera component with the code scanner after returning results is closed and unmounted.

Changes

The PR changes the destroyPreviewOutputSync and makes it run within the main dispatcher.

Tested on

OPPO A36, Android 11

Related issues

Note

I'm 100% sure this is the correct solution, but it works for me and others. I have not fully tested other features except for code scanner and photo capture which satisfy my needs, so I have not further debugged either.

What's more, I have another solution. This function aims to let configure compare out a difference, and forcing diff to be true can also resolve this issue. So that,

  suspend fun configure(lambda: (configuration: CameraConfiguration) -> Unit) {
    mutex.withLock {
      Log.i(TAG, "Updating CameraSession Configuration...")

      val config = CameraConfiguration.copyOf(this.configuration)
      lambda(config)
//      val diff = CameraConfiguration.difference(this.configuration, config)
      val diff = CameraConfiguration.Difference(true, true, true);

Of course, this is a more inappropriate solution, but just to give a clue of what's going on here.

Let me know if you need another PR or more information

Copy link

vercel bot commented Nov 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-vision-camera ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 25, 2023 1:32am

@mrousavy
Copy link
Owner

and forcing diff to be true can also resolve this issue

Interesting, maybe the true fix can be in Difference? I'll take a look soon, thanks for your PR!

@Hector-Chong
Copy link
Author

Hector-Chong commented Nov 28, 2023

destroyPreviewOutputSync

and forcing diff to be true can also resolve this issue

Interesting, maybe the true fix can be in Difference? I'll take a look soon, thanks for your PR!

If destroyPreviewOutputSync is called that means outputsChanged and sidePropsChanged must be true inside CameraConfiguration.difference, but deviceChanged not not be so, as a result, configureCameraDevice is not called. However, it does not make sense to me a little that forcing this function running on main thread can resolve this issue.

@mrousavy
Copy link
Owner

Yea, the problem with the Main Thread approach is that this queues the stop to be run on the Main Thread. The method destroyPreviewOutputSync() already returns, which tells Android "hey, you can now safely delete the SurfaceView".

So assuming Android deletes the SurfaceView now, and we still try to draw a Frame to it BEFORE the launch(Dispatchers.Main) { has actually executed, we are drawing to a deleted Surface, which will crash.

We need to be aware that this race condition can exist, so I think a true fix is to provide a single source of truth for the Difference and delete the Preview accordingly, I will take a look at that soon!

I think running this fix in your app for now is perfectly fine though, that's like a 0.001% chance of crashing.

@BrechtBD
Copy link

BrechtBD commented Dec 4, 2023

@mrousavy thank you in advance! Any update on this?

@mare95
Copy link

mare95 commented Dec 6, 2023

Are there updates regarding this PR?

@Sharf8351
Copy link

Facing the same issue.

@gabrielsalvi
Copy link

same problem here

@Gambitboy
Copy link
Contributor

I think I'm having this same problem. I'm scanning QR codes on an app. And sometimes after scanning the app will crash with a seg fault.

I've patched these changes into my project and can confirm its stopped crashing

@mschmid
Copy link

mschmid commented Apr 15, 2024

I got the same issue, looking forward to the merge of this PR. Thanks for your good 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

Successfully merging this pull request may close these issues.

🐛App crashed after scanning a qrcode
8 participants