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

Delay frame processor initialization until view creation on Android #1583

Closed

Conversation

jthure
Copy link
Contributor

@jthure jthure commented May 4, 2023

What

This PR fixes java.lang.NullPointerException in FrameProcessorRuntimeManager. on Android phones

Although I never was able to reproduce the issue myself, looking at the crashlogs from other users, this should fix it

Changes

Moved the creation FrameProcessorRuntimeManager to happen in onCreateViewInstance instead of inside CameraViewModule initialisation. This should make sure that the react context is fully initialised, which apparently is not the case right now

Inspired by @pke in #946 (comment)

Tested on

Not properly tested yet. I'm having trouble building and running the example app from this repo. I have tested it in another app which uses this library. Any help to test and verify the change is greatly appreciated

Related issues

Fixes #946

@vercel
Copy link

vercel bot commented May 4, 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 May 4, 2023 9:19am

@pke
Copy link

pke commented May 4, 2023

Thanks for creating this PR!

@mrousavy
Copy link
Owner

Hey! Thanks for creating this PR, but I don't think that this is the right approach to this problem. This just swallows the error and silently fails when you try to use FPs on the JS side later on.

Also, this elevates the internal setup from internal (CameraViewModule.kt) to external (CameraViewManager.kt; aka the CameraViewModule's caller) code.

@mrousavy mrousavy closed this Jun 19, 2023
@mrousavy
Copy link
Owner

In V3 I have a different solution here which is called from JS - so this is when the JS bundle is 100% ready. That's probably what we want.

Sorry for closing this PR please don't take it personal ❤️

@pke
Copy link

pke commented Jun 19, 2023

Hmm... this patch was written against current main, which is not yet V3. Why not allow this backport until V3 is production ready?

@mrousavy
Copy link
Owner

Because of this:

This just swallows the error and silently fails when you try to use FPs on the JS side later on.

the FP runtime will not be installed and will just not work. You can disable FPs at build time if that's what you want

@jthure
Copy link
Contributor Author

jthure commented Jun 20, 2023

Hey! Thanks for creating this PR, but I don't think that this is the right approach to this problem. This just swallows the error and silently fails when you try to use FPs on the JS side later on.

Also, this elevates the internal setup from internal (CameraViewModule.kt) to external (CameraViewManager.kt; aka the CameraViewModule's caller) code.

Okay sure, I get what you mean. Do you have another approach in mind on how to fix it? Before v3 is ready and adopted?

Edit: Actually, I'm not sure I fully agree. It doesn't just swallow the error. The error should not happen since the fp init happens at a later stage, right?

@mrousavy
Copy link
Owner

Do you have another approach in mind on how to fix it?

If you do not need Frame Processors, just disable them at build time.

If you do, then I am not sure - I think the solution in V3 is much more solid since it does init from JS side - but this is a big change

Actually, I'm not sure I fully agree. It doesn't just swallow the error. The error should not happen since the fp init happens at a later stage, right?

No, it is wrapped in a try/catch now and in the catch there's no handling - aka it silently fails.

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.

🐛 java.lang.NullPointerException in FrameProcessorRuntimeManager.<init>
3 participants