search

mrousavy / vision-camera-image-labeler

VisionCamera Frame Processor Plugin to label images using MLKit Vision
https://github.com/mrousavy/react-native-vision-camera
MIT License
119 stars 28 forks source link

Fix and upgrade example app #7

Closed yfunk closed 3 years ago

yfunk commented 3 years ago

This PR fixes some issues that lead to the example app not running properly on Android (and maybe also on iOS?). It also includes some dependency updates and changes that were recently added to the create-react-native-library template.

Unfortunately I don't have time to go into details about the individual changes right now, but the commits should do an okay job of highlighting all changes.

Currently the frame processor itself isn't working (at least on Android, I couldn't test iOS) and throws an error:

Frame Processor threw an error: Value is undefined, expected an Object

Trying to console.log() the frame object throws

Frame Processor threw an error: Exception in HostObject::get(propName:bytesPerRow): java.lang.NoSuchMethodError: no non-static method "Landroidx/camera/core/SettableImageProxy;.getPlanes()[Ljava/lang/Object;"

so maybe that gives some indication what the issue is.

I marked the PR as a draft because I'm unsure whether this is an issue with the plugin implementation, the example app or frame processors on Android in general.

Still to do

mrousavy commented 3 years ago

huh,

Frame Processor threw an error: Value is undefined, expected an Object

and

Frame Processor threw an error: Exception in HostObject::get(propName:bytesPerRow): java.lang.NoSuchMethodError: no non-static method "Landroidx/camera/core/SettableImageProxy;.getPlanes()[Ljava/lang/Object;"

makes me think that the frame processor is called with a "null" frame? Aka the image here:

https://github.com/mrousavy/react-native-vision-camera/blob/b5452ac4064fad1eb151d7ecc2a5b63e5f1be40c/android/src/main/java/com/mrousavy/camera/CameraView.kt#L416

is null? that doesn't make any sense... Maybe SettableImageProxy does not implement all the functions?

Thanks for your help!!! šŸ–¤šŸ–¤

mrousavy commented 3 years ago

While you're already at it - could you swap _log with console.log? VisionCamera supports console.log since 2.5.0, and _log seems to cause general confusion (see this issue: https://github.com/mrousavy/vision-camera-image-labeler/issues/8)

yfunk commented 3 years ago

While you're already at it - could you swap _log with console.log?

Already did that in the latest commit on here šŸ˜

I'll further debug / fix the current issue when I'm back on my main setup in a few days, could you take a look at the iOS stuff mentioned in the TODO above? I don't have a macOS development environment set up right now, so that's a bit hard for for me to do šŸ˜…

yfunk commented 3 years ago

@mrousavy Somehow the Value is undefined, expected an Object just went away without me changing anything, not sure how to feel about this šŸ˜•. I tested it on the same API 29 emulator as before, an API 30 one and a physical API 29 Galaxy M20, the frame processor ran without any issues on all of them.

The NoSuchMethodError is still thrown when trying to console.log(frame.bytesPerRow) or console.log(frame) (does this default to the bytesPerRow property?). This happens across all 3 devices I tested, maybe this is an issue with VisionCamera?

Another thing I noticed in the example app is that the globals.d.ts type declarations don't seem to be included in the bob output (./lib/typescript) and while they are usable that results in TypeScript / ESLint errors.

I'll add an UI element that displays the label with the highest confidence like in the README and when the iOS stuff is done this PR should be ready.

mrousavy commented 3 years ago

I'll add an UI element that displays the label with the highest confidence like in the README and when the iOS stuff is done this PR should be ready.

Awesome šŸ¤©šŸ¤©šŸ¤© I used a SharedValue and applied the text through a TextInput and useAnimatedProps in the initial example, was pretty fast

yfunk commented 3 years ago

Weird that that error just went away, did you test with JSC and Hermes? I guess nice that the FP now runs fine šŸ¤·

The example app was using JSC all along, I tried Hermes (on Android) and it worked the same. Maybe it was just some weird emulator quirk šŸ¤·

I still don't get why the NoSuchMethodError is being thrown, are you sure you have the latest version of VisionCamera installed in the Example as well as in the devDependencies of the lib? Maybe clean cache...

I tested it again on a fresh clone of the PR branch with the latest versions of VisionCamera and Reanimated in both projects and as before the error is still thrown when:

Using frame.bytesPerRow or console.log(frame), which does seem to somehow default to bytesPerRow according the error message (frame.toString() works fine):

Exception in HostObject::get(propName:bytesPerRow): java.lang.NoSuchMethodError: no non-static method "Landroidx/camera/core/SettableImageProxy;.getPlanes()[Ljava/lang/Object;"

Using frame.planesCount:

Exception in HostObject::get(propName:planesCount): java.lang.NoSuchMethodError: no non-static method "Landroidx/camera/core/SettableImageProxy;.getPlanes()[Ljava/lang/Object;"

I don't think the hidden variables/functions from globals.d.ts should be used outside of VisionCamera (I use them for debugging), so I don't think it makes sense to include them in the package - do you?

I agree that that there probably aren't many use cases outside debugging (especially with _log replaced by console.log()). What I was getting at is that they are in the API documentation and at least _WORKLET and _log() are also exposed as globals, but the type declarations are missing.

mrousavy commented 3 years ago

which does seem to somehow default to bytesPerRow according the error message

maybe console.log tries to JSON.stringify it to print all properties? Would be a bit weird if I extra override the toString method, but that might be what's happening here.. Either way, frame.bytesPerRow should not crash! Maybe I missed a cast here

mrousavy commented 3 years ago

Does frame.planesCount work?

yfunk commented 3 years ago

Does frame.planesCount work?

Nope, it throws the same error with propName:planesCount (see above). Are you able to reproduce the error in your environment?

mrousavy commented 3 years ago

I'll try to reproduce it soon, really busy with a contract right now and only got 1 week left

yfunk commented 3 years ago

I'll try to reproduce it soon, really busy with a contract right now and only got 1 week left

No worries šŸ™‚ Just to make sure it isn't an issue with my setup.

Edit: Looks like the error is coming from getClass()->getMethod<JArrayClass<jobject>()>("getPlanes") here and here.

mrousavy commented 3 years ago

@Simek did an amazing job getting the example app running on iOS, you might want to rebase your branch with master (https://github.com/mrousavy/vision-camera-image-labeler/commit/7746a92d044be7770fcd4b6744f84e0ee0baeb92) šŸš€šŸš€

mrousavy commented 3 years ago

btw: https://github.com/mrousavy/react-native-vision-camera/pull/380

yfunk commented 3 years ago

you might want to rebase your branch with master (7746a92) šŸš€šŸš€

@mrousavy Done, the PR should be ready to merge. You'll still need to update pods (and maybe test the example app) for iOS though since I upgraded a bunch of native dependencies.

Also can confirm the NoSuchMethodError error is gone in 2.6.1 and JS actually calls JSON.stringify for console.log(frame) as you suspected:

{"bytesPerRow": 640, "close": [Function hostFunction], "height": 480, "isValid": true, "planesCount": 3, "toString": [Function hostFunction], "width": 640}
yfunk commented 3 years ago

@mrousavy Is there anything still blocking this PR? lmk if I can help šŸ™‚

mrousavy commented 3 years ago

Oh sorry I've just been really busy - I promise I'll look into it today or tomorrow! Thank you again for the hard work šŸ–¤šŸ–¤

mrousavy commented 3 years ago

Just tested this on both iOS and Android, and both of them work smoothly! Awesome job @yfunk, thank you so much for the hard work! šŸ–¤šŸ–¤šŸ–¤ LGTM