-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat: Upgrade VisionCamera to V3 #28914
Conversation
@MonilBhavsar Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosWebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
FYI, I'll be testing the android build on:
I will be testing the iOS build on:
|
@allroundexperts We need someone to test this with iPhone 15 due to a known issue |
@shubham1206agra I think that will be tested as part of #27820. No? |
To be safe, we should test v3 too. |
I have an iPhone 15 Pro test device |
Kicked off the test build |
What's up with the builds @mountiny? |
Not sure, I just retried it here https://github.com/Expensify/App/actions/runs/6424873394 |
Seems like Android is failing to build on the GitHub runner, are you seeing the same locally? https://github.com/Expensify/App/actions/runs/6424873394/job/17446462378#step:10:10762 |
@mrousavy Can you check this line? We are using |
Either we need to add I believe the funcs I'm using are in the std lib of Kotlin, this is no third party extensions dep or anything afaik. |
Waiting for the android build issue to resolve. |
d8e3491
to
0cad7c4
Compare
Update:
Since Cameras are hardware features and VisionCamera V3 now uses the low-level Camera2 API, we def. need to be quick with feedback here - I just know there's gonna be some weird Samsungs or something that break for some reason. If there are any issues with VisionCamera V3, just ping me on Slack or create an issue with details on the VisionCamera issues page! |
Okay was just Gradle cache from the WishList branch! Everything works for me now! :) cc @AndrewGable trim.A1EA3EBD-AA04-4EC0-A9A9-BB579B2EB78F.MOV |
059064c
to
9dd1ccf
Compare
I'm working on a real fix in VisionCamera in the meantime :)
@allroundexperts @mountiny @AndrewGable just created a hot-fix for the blackscreen issue. I identified the problem and can confirm that it will be fixed with this PR: mrousavy/react-native-vision-camera#1996 I pushed that VisionCamera patch here, so it should work fine now (confirmed on my Huawei something), but I'll now switch to VisionCamera to build a true solid fix for this. :) Do we want to wait for that VisionCamera update, or do we want to merge with that hot-patch? |
I think it'd be better to wait for the true VisionCamera fix. Just give me a few days and that's done :) |
I think I got mrousavy/react-native-vision-camera#2049 working pretty good - that fixes a ton of concurrency issues since now finally everything is happening under an atomic mutex 😄 on Android this was especially tricky because of the PreviewView since that could just go out of sync from time to time. I'll test more to see if this is still an issue or if everything is fixed now :) If everything's fixed, I can release this and we should be good to go with the upgrade here in this PR |
nice! |
Okay, I pushed the new changes and now we are using VisionCamera 3.6.x, which contains the new atomically single-lock Can we do another test build to gather some feedback? This is pretty early since I just released the new library, but from my testing so far it looks quite good. |
build running here https://github.com/Expensify/App/actions/runs/6628563393 |
🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪 |
minSdkVersion = 21 | ||
minSdkVersion = 26 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the following listed as high-level new features with Android SDK 26:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to discuss this. Merging the PR as is will block just under 7% of potential users from using the app.
Created a new issue for this here: #30732 |
update: I just got a prototype running on my end that works on API 21, so we don’t need to drop the minSdk requirement! 💪 |
Nice! |
Important update, I created a new PR: #37483 |
Can we close this in favour of #37483 |
Ja 👍 |
Details
Migrates from VisionCamera V2 to VisionCamera V3 ✨
Some important changes about V3:
useCameraDevice(..)
;useCameraFormat(..)
- always have a device, never null)Warning
Since the Android part is completely rewritten from CameraX to Camera2, there might be some rare edge cases on some phones. Often it's some Samsungs that report a feature as being available in their APIs, then the feature is not available for some reason (e.g. Samsung phones return 60 FPS as a supported FPS range, but just crash when you use 60 FPS. Aka; their APIs simply lie.)
We should carefully roll this out and report any issues straight to the VisionCamera repo so I can fix them asap! It's a huge help for me to test in a variety of devices.
Fixed Issues
$#28341
PROPOSAL: #28341 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android