App icon indicating copy to clipboard operation
App copied to clipboard

[$500] Camera doesn't load when trying to smartscan a receipt

Open m-natarajan opened this issue 1 year ago β€’ 3 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.38-1 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @rlinoz Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1707336890268789

Action Performed:

  1. Tap request money
  2. Press the round button to take a picture
  3. Tap the back arrow
  4. Again press the Button to take a picture

Expected Result:

Able to scan the receipt again

Actual Result:

Camera doesn't load when trying to smartscan

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • [x] Android: Native
  • [ ] Android: mWeb Chrome
  • [ ] iOS: Native
  • [ ] iOS: mWeb Safari
  • [ ] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/38435837/9f09590f-fbcf-4c86-a4e2-0a300f3c9856

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0176ddbc1361cf082e
  • Upwork Job ID: 1755378138365923328
  • Last Price Increase: 2024-02-07

m-natarajan avatar Feb 07 '24 23:02 m-natarajan

Job added to Upwork: https://www.upwork.com/jobs/~0176ddbc1361cf082e

melvin-bot[bot] avatar Feb 07 '24 23:02 melvin-bot[bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt (External)

melvin-bot[bot] avatar Feb 07 '24 23:02 melvin-bot[bot]

Triggered auto assignment to @garrettmknight (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] avatar Feb 07 '24 23:02 melvin-bot[bot]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Camera fails to load after navigating away and then returning to it

What is the root cause of that problem?

When user captures the photo first time and navigate to next screen, camera is set inactive here.

https://github.com/Expensify/App/blob/9f37d3e2c1264420773c886ae6171748e5d512b8/src/pages/iou/request/step/IOURequestStepScan/NavigationAwareCamera/index.native.js#L13-L23

This causes the camera library to disable camera when users navigates away from TabNavigator. On refocus the video capture is re-enabled. However it throws below error.

 DEBUG  [warn] Error taking photo - {"name":"unknown/unknown","_code":"unknown/unknown","_message":"[unknown/unknown] Not bound to a valid Camera [ImageCapture:androidx.camera.core.ImageCapture-eab74056-9b89-4653-8288-e44bc9b65a66]"}

Though camera is enabled on refocus, photo capture fails. This looks like a bug in react-native-vision camera.

What changes do you think we should make in order to solve the problem?

Looks like a fix is available upstream https://github.com/mrousavy/react-native-vision-camera/pull/2339. But it is available only in release v3.70. We've pinned react-native-vision-camera at 2.16.5

https://github.com/Expensify/App/blob/9f37d3e2c1264420773c886ae6171748e5d512b8/package.json#L164

So one option is to upgrade to v3.7 which could be a major undertaking.

Instead what I found is that the camera gets re-enabled if we toggle the photo prop of Camera on refocus. This enables photo capture capabilities of camera on refocus and above error is not thrown any more.

    const isCameraActive = useTabNavigatorFocus({tabIndex: cameraTabIndex});
    return (
        <Camera
            ref={ref}
            // eslint-disable-next-line react/jsx-props-no-spreading
            {...props}
            isActive={isCameraActive}
            // Toggle the photo prop as well
            photo={isCameraActive}
        />
    );

Result

https://github.com/Expensify/App/assets/6880914/7a5007bf-ebfe-43aa-a61b-017dd4d94144

What alternative solutions did you explore? (Optional)

None

aswin-s avatar Feb 08 '24 03:02 aswin-s

Asked in thread here. Is this a deploy blocker if on staging only, or a fire if it's already on prod? It's a core flow, so if the camera can't load to take a receipt that's pretty bad!

trjExpensify avatar Feb 08 '24 10:02 trjExpensify

@aswin-s no we cannot simply update to v3, there are some feature missing unfortunately, like flash.

@janicduplessis @ishpaul777 any chance this could be stemming from this update? i see the camera worked for you both in the PR, not sure clearly ifyou have gone back and again to the camera, I would assume yes https://github.com/Expensify/App/pull/35014

mountiny avatar Feb 08 '24 12:02 mountiny

@mountiny I've detailed how we can fix this without a version upgrade

aswin-s avatar Feb 08 '24 12:02 aswin-s

We are looking into it whether we can find a better solution than the proposed workaround, give us a moment please!

~~(longterm we should look into upgrading V3!)~~ Tracking issue for that is here

hannojg avatar Feb 08 '24 12:02 hannojg

@janicduplessis @ishpaul777 any chance this could be stemming from this update?

It appears that the root cause is stemming from the react-native-vision-camera upstream, so i dont think this comes from autofocus PR, Also this is hard to reproduce (at least on my device) Camera turns active before i get the chance to click the capture button.

ishpaul777 avatar Feb 08 '24 13:02 ishpaul777

I can reproduce it on Samsung M 52. Build 1.4.38-2

https://github.com/Expensify/App/assets/115492554/96d5f2d2-b1c4-431d-abb6-954424797ea6

izarutskaya avatar Feb 08 '24 13:02 izarutskaya

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

github-actions[bot] avatar Feb 08 '24 13:02 github-actions[bot]

Triggered auto assignment to @youssef-lr (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

melvin-bot[bot] avatar Feb 08 '24 13:02 melvin-bot[bot]

Production -not reproduced

https://github.com/Expensify/App/assets/115492554/adfa887f-a7ed-46f2-bd8e-afcba4599c04

izarutskaya avatar Feb 08 '24 13:02 izarutskaya

QA has confirmed this is not repro in production https://expensify.slack.com/archives/C9YU7BX5M/p1707397831272999?thread_ts=1707394006.658679&cid=C9YU7BX5M

Curious what change from the recent deploy could have caused this

mountiny avatar Feb 08 '24 13:02 mountiny

This is a regression from https://github.com/Expensify/App/pull/32471 where react-native-vision-camera was upgraded from 2.16.2 to 2.16.5. I tested with 2.16.7 and that version is still buggy. Since this is a deploy blocker we should go with the workaround suggested by @aswin-s photo={isCameraActive && props.photo}.

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed Link to proposal

s77rt avatar Feb 08 '24 13:02 s77rt

Current assignee @youssef-lr is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] avatar Feb 08 '24 13:02 melvin-bot[bot]

For internal people, discussing here https://expensify.slack.com/archives/C035J5C9FAP/p1707394239307519

mountiny avatar Feb 08 '24 13:02 mountiny

I can take over, Youssef, if that is fine with you

mountiny avatar Feb 08 '24 13:02 mountiny

Please assign me here as it seems related to hybrid PR I reviewed

situchan avatar Feb 08 '24 13:02 situchan

Thank! I agree with @s77rt that given this is one of the last deploy blockers, the workaround is fine and we can add full fix upstream later.

mountiny avatar Feb 08 '24 13:02 mountiny

πŸ“£ @situchan πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

melvin-bot[bot] avatar Feb 08 '24 13:02 melvin-bot[bot]

πŸ“£ @aswin-s πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

melvin-bot[bot] avatar Feb 08 '24 14:02 melvin-bot[bot]

@aswin-s can you raise the PR asap? thank you!

mountiny avatar Feb 08 '24 14:02 mountiny

@mrousavy Since you are the maintainer of react-native-vision-camera, can you please check why this is bugging out?

shubham1206agra avatar Feb 08 '24 14:02 shubham1206agra

I think bug was introduced in https://github.com/mrousavy/react-native-vision-camera/pull/1955 which was included in 2.16.3

mkhutornyi avatar Feb 08 '24 14:02 mkhutornyi

Hey - yup, we (@margelo) are on it! (cc @hannojg)

Issue is that the onDetachedWindow is called when it can already be attached again, so the commit you linked @mkhutornyi is the problem πŸ‘

We'll revert that

mrousavy avatar Feb 08 '24 14:02 mrousavy

Should we hold @aswin-s's PR then?

situchan avatar Feb 08 '24 14:02 situchan

Yea, that PR is a workaround where the photo output is re-created each time the camera looses focus, which is a performance overhead as buffers need to be fully re-created. The true fix is reverting the PR 1955.

mrousavy avatar Feb 08 '24 14:02 mrousavy

Thanks. @mountiny I think we should wait for @margelo's patch PR, pausing @aswin-s's one

situchan avatar Feb 08 '24 14:02 situchan

@mrousavy @hannojg What is your ETA for a fix? Asking because the only other blocker has fix raised too and we would like to kick of the deploy as soon as possible.

I like doing a true fix but if that means postponing the deploy for 4+ hours maybe, I think the performance overhead of recreating the buffers should be fine

mountiny avatar Feb 08 '24 15:02 mountiny