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

feat: Upgrade VisionCamera to V4 #37483

Merged
merged 19 commits into from
Mar 28, 2024
Merged

Conversation

mrousavy
Copy link
Contributor

@mrousavy mrousavy commented Feb 29, 2024

Details

As many of you might've noticed, the upgrade to VisionCamera V3 took quite a long time already and eventually still wasn't merged because of a few blockers:

  1. It required minSdk 26, or at least 23 with some minor modifications, which wasn't acceptable for Expensify. The reason was that I was using custom SurfaceTexture APIs for the video pipeline, and OutputConfiguration APIs for the Capture Session that were introduced in SDK 26 or 23 respectively. Adding backwards compatibility here was painful and required a lot of effort.
  2. Up until 3.9.0, VisionCamera V3 didn't support flash on Android. This now works well on most devices, but there are a few devices where the vendor didn't implement flash or AE properly according to the Android Camera specification, which would require custom device specific workarounds from my side (if (manufacturer == "SAMSUNG") 🙄)
  3. Up until 3.9.0, VisionCamera V3 didn't support focus on Android. This now works well on most devices, but there are a few devices where the vendor didn't implement focus metering (AF) properly according to the Android Camera specification, which would require custom device specific workarounds from my side (if (manufacturer == "SAMSUNG") 🙄)
  4. Up until 3.9.0, VisionCamera V3 had some black-screen issues on some devices because the Camera CaptureSession could be destroyed by the Android OS, and I didn't have a good mechanism of safely restoring it. I did implement quite a good "PersistentCaptureSession" in 3.9.0, so that is theoretically fixed.

A bit of important backstory around V2 vs V3:

In VisionCamera V2 I used CameraX, which abstracted most of those quirks away, but back then (a year ago) was still insanely limiting and couldn't support all the features that VisionCamera had.

So in VisionCamera V3 I switched over to Camera2, the lower level Camera framework for Android where you have to do everything manually (e.g. capturing a photo requires an AF, AE, AWB precapture sequence, listen to status changes, manually override flash control, and only then you can capture photo metadata), and while that meant MUCH more complexity it also meant much more control and features.

Using Camera2 also gave me a great understanding of how Cameras work under the hood, and I noticed that a lot of vendors (e.g. Samsung) didn't implement the Camera HAL specification properly so I'd have to do a bunch of workarounds to avoid crashes, blackscreens or even full device reboots.
After adding workaround after workaround I decided to look into CameraX again - and to my surprise it actually did quite good over the last year. They added a ton of features, and have a bunch of those device specific quirk workarounds implemented already.

Also, they already have features like deferred surfaces and 10-bit HDR for video capture which I wanted to implement myself (those are really complex, HDR requires loading EGL OpenGL extensions).

So for VisionCamera V4 we're switching over to CameraX again. I kinda see it like this:

  • VisionCamera V2 was stable but a bit feature limited
  • VisionCamera V3 was a bit unstable at times, but introduced a ton of new features and exciting things
  • VisionCamera V4 will be fully stable again, but with all of the features and exciting things from V3

So yea - in short; this PR upgrades to VisionCamera V4 (currently in beta).

I will do a few more breaking changes in VisionCamera V4, but none of them will affect the expensify codebase (they're mostly for the video pipeline)
Also I promise I'll fix it in Expensify if I introduce a breaking change to VisionCamera V4 😄

Fixed Issues

$ #28341
PROPOSAL: #28341 (comment)

Tests

  • Open Scan Receipt screen
  • Check if Camera works and can take pictures (on iPhones as well as Android phones)
  • Verify that no errors appear in the JS console

Offline tests

QA Steps

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
      • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android

@mrousavy mrousavy requested a review from a team as a code owner February 29, 2024 10:24
@melvin-bot melvin-bot bot requested review from Beamanator and removed request for a team February 29, 2024 10:25
Copy link

melvin-bot bot commented Feb 29, 2024

@Beamanator 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]

@mrousavy mrousavy mentioned this pull request Feb 29, 2024
61 tasks
@mrousavy
Copy link
Contributor Author

(note; we are still testing this here in our office with the test devices - I'll report back with all devices tested in a few hours)

@allroundexperts
Copy link
Contributor

Thanks! I can review when this is ready since I am reviewing the stale v3 PR as well.

@mrousavy mrousavy marked this pull request as draft February 29, 2024 10:59
@mrousavy
Copy link
Contributor Author

Hold on we found an error on Android - marked this PR as draft, will fix it and ping you once it's ready again!

@mrousavy
Copy link
Contributor Author

mrousavy commented Mar 5, 2024

Status update: We are still encountering the error where CameraX fails to start. I am talking to Google about this, this might actually be a bug in CameraX. 😓

@mrousavy mrousavy force-pushed the feat/vision-camera-v4 branch from fadaf1a to 599c6d3 Compare March 18, 2024 15:25
@mrousavy
Copy link
Contributor Author

Update: I think I fixed the "Camera failed to start" blocker error in VisionCamera v4.0.0-beta.7! Testing in Expensify app now..

@Julesssss Julesssss self-requested a review March 19, 2024 09:59
@Julesssss
Copy link
Contributor

Let us know when you want us to generate AdHoc builds for testing

@melvin-bot melvin-bot bot requested a review from Julesssss March 27, 2024 08:42
@mananjadhav
Copy link
Collaborator

I've managed to upload the videos by cropping and compressing them, probably low network issues. As I mentioned earlier, the first time black screen still comes up on Android. But we've decided to create a new issue for that.

@mrousavy
Copy link
Contributor Author

Cool! 👍

@Julesssss
Copy link
Contributor

Created an issue for the blank camera view here. @mrousavy would you mind commenting on the issue so I can assign you? thanks

@Julesssss Julesssss dismissed their stale review March 27, 2024 11:44

Outdated

Julesssss
Julesssss previously approved these changes Mar 27, 2024
Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a separate issue for the bug

@Julesssss
Copy link
Contributor

@mananjadhav I think we're good to go now?

@mananjadhav
Copy link
Collaborator

Yes. I think we need to sync the latest main to get rid of the failing performance action ?

@Julesssss
Copy link
Contributor

Yes. I think we need to sync the latest main

Yeah normally I wouldn't care about that flakey check, but maybe it's a good idea given this is an important library change

@hannojg
Copy link
Contributor

hannojg commented Mar 27, 2024

Tested with beta 11, works well on all devices we have in the office!

@mananjadhav
Copy link
Collaborator

@Julesssss All yours.

Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks all

@Julesssss Julesssss merged commit c94be6a into Expensify:main Mar 28, 2024
18 checks passed
@melvin-bot melvin-bot bot added the Emergency label Mar 28, 2024
Copy link

melvin-bot bot commented Mar 28, 2024

@Julesssss looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

@Julesssss
Copy link
Contributor

There was not a failing test 😖

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/Julesssss in version: 1.4.58-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@johnmlee101
Copy link
Contributor

I think this introduced a regression here: #39241, basically the sound profile on Android isn't consistent with the settings both in OS and app-level.

@deetergp
Copy link
Contributor

I suspect this may have introduced this regression as well #39285. In this version of the app, the Android camera shows a black screen when attempting to SmartScan a receipt.

@OSBotify
Copy link
Contributor

OSBotify commented Apr 2, 2024

🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.58-8 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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

Successfully merging this pull request may close these issues.

9 participants