fix(liveness): Fix oval render when switching cameras #5954

esauerbo · 2024-10-24T20:14:19Z

Description of changes

Fix for an issue where the oval renders in the wrong location when switching cameras to unblock data collection
Add an event listener to make sure video metadata (which includes video.videoHeight and video.videoWidth) is loaded before attempting to draw the oval
Remove drawStaticOval from state machine because we already call it in the LivenessCameraModule. Calling from multiple places caused weird/unpredictable behavior

Issue #, if available

Description of how you validated changes

Deployment https://testing.d2pnskzbmchcgk.amplifyapp.com/
Compare to https://alpha.d3v0f004886eky.amplifyapp.com/

Checklist

Have read the Pull Request Guidelines
PR description included
yarn test passes and tests are updated/added
PR title and commit messages follow conventional commit syntax
If this change should result in a version bump, changeset added (This can be done after creating the PR.) This does not apply to changes made to docs, e2e, examples, or other private packages.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

changeset-bot · 2024-10-24T20:14:22Z

⚠️ No Changeset found

Latest commit: 61308ec

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

jordanvn

This makes sense to me

calebpollman · 2024-10-24T23:49:57Z

...es/react-liveness/src/components/FaceLivenessDetector/LivenessCheck/LivenessCameraModule.tsx

+        if (isMetadataLoaded) {
+          send({
+            type: 'UPDATE_DEVICE_AND_STREAM',
+            data: { newDeviceId, newStream },
+          });
+        }


How does this function run if setIsMetaDataLoaded is set to false in the same closure on line 275?

This is updated now so it just sets IsMetaDataLoaded to false. I took out the step where it draws the oval from the state machine since we're already drawing it in the camera component.

calebpollman · 2024-10-24T23:52:53Z

...es/react-liveness/src/components/FaceLivenessDetector/LivenessCheck/LivenessCameraModule.tsx

+      canvasRef?.current &&
+      videoRef?.current &&
+      videoStream &&
+      isStartView &&
+      isMetadataLoaded


Concerned abt fragility/maintainability with so many boolean values used for a condition

Yeah fair, maybe we can extract them into one variable for readability?

calebpollman · 2024-10-24T23:57:22Z

...es/react-liveness/src/components/FaceLivenessDetector/LivenessCheck/LivenessCameraModule.tsx


  React.useLayoutEffect(() => {
-    if (isCameraReady) {
+    if (isCameraReady && isMetadataLoaded) {


Using isMetadataLoaded to gate behavior in three different side effects creates complexity in readability and refactoring, can we isolate some of this behavior?

I think that would be hard to do without a lot of refactoring. But I was actually able to remove this particular gate

reesscot · 2024-10-30T21:57:31Z

...ss/src/components/FaceLivenessDetector/LivenessCheck/__tests__/LivenessCameraModule.test.tsx

@@ -258,7 +269,9 @@ describe('LivenessCameraModule', () => {
      />
    );
    const videoEl = screen.getByTestId('video');
-    videoEl.dispatchEvent(new Event('canplay'));
+    await waitFor(() => {


thanks for fixing these!

calebpollman · 2024-10-31T17:28:50Z

...es/react-liveness/src/components/FaceLivenessDetector/LivenessCheck/LivenessCameraModule.tsx

-      if (
-        e.matches &&
-        canvasRef?.current &&
-        videoRef?.current &&
-        videoStream &&
-        isStartView
-      ) {
-        drawStaticOval(canvasRef.current, videoRef.current, videoStream);
+      if (e.matches && shouldDrawOval) {
+        drawStaticOval(canvasRef.current, videoRef.current!, videoStream);
      }


Nice cleanup!

calebpollman · 2024-10-31T17:30:02Z

...es/react-liveness/src/components/FaceLivenessDetector/LivenessCheck/LivenessCameraModule.tsx

+    canvasRef,
+    videoRef,


We don't need to track the refs in the dependency array, their references maintain stability since we access .current inside the body of the hook

Suggested change

canvasRef,

videoRef,

removed canvasRef but react gets mad if we take out videoRef, I think because the ref itself might change since it comes from the useMediaStreamInVideo hook

amplify-ui/packages/react-liveness/src/components/FaceLivenessDetector/LivenessCheck/LivenessCameraModule.tsx

Line 131 in 4393c78

const { videoRef, videoWidth, videoHeight } = useMediaStreamInVideo(

Ahh got it. For sure out of scope for this PR (and tbh think it will be a non-trivial change) but we should refactor to keep videoRef scoped to wherever it's used directly. Passing refs around leads to unexpected behaviors

Yeah makes sense I'll add that to our backlog. It definitely made this like 10x harder to fix

…fy-ui into fix-oval-render

* chore(liveness): allow selecting all cameras, allow camera selection … (#5833) * chore(liveness): allow selecting all cameras, allow camera selection on mobile * fix(liveness): fix camera select showing up with one camera, fix camera changing weirdness with hair check screen (#5845) * fix(liveness): Only apply transform style on user-facing video (#5953) * fix(liveness): Fix oval render when switching cameras (#5954) --------- Co-authored-by: thaddmt <68032955+thaddmt@users.noreply.github.com> Co-authored-by: Caleb Pollman <cpollman@amazon.com> Co-authored-by: Emma Sauerborn <70536670+esauerbo@users.noreply.github.com> Co-authored-by: Scott Rees <6165315+reesscot@users.noreply.github.com>

* chore(liveness): allow selecting all cameras, allow camera selection … (#5833) * chore(liveness): allow selecting all cameras, allow camera selection on mobile * only show on mobile with facemovement challenge only * relax e2e tests on alpha * make more reaadable * fix * update names * refactor, add tests * Update packages/react-liveness/src/components/FaceLivenessDetector/LivenessCheck/LivenessCameraModule.tsx Co-authored-by: Caleb Pollman <cpollman@amazon.com> * refactor --------- Co-authored-by: Caleb Pollman <cpollman@amazon.com> * fix(liveness): fix camera select showing up with one camera, fix camera changing weirdness with hair check screen (#5845) * fix(liveness): Only apply transform style on user-facing video (#5953) Co-authored-by: Scott Rees <6165315+reesscot@users.noreply.github.com> * fix(liveness): Fix oval render when switching cameras (#5954) * chore(liveness): update video constraints for selected camera (#6009) --------- Co-authored-by: thaddmt <68032955+thaddmt@users.noreply.github.com> Co-authored-by: Caleb Pollman <cpollman@amazon.com> Co-authored-by: Emma Sauerborn <70536670+esauerbo@users.noreply.github.com> Co-authored-by: Scott Rees <6165315+reesscot@users.noreply.github.com>

Add event handler for loaded metadata

875c409

esauerbo requested a review from a team as a code owner October 24, 2024 20:14

esauerbo temporarily deployed to ci October 24, 2024 20:14 — with GitHub Actions Inactive

jordanvn previously approved these changes Oct 24, 2024

View reviewed changes

Fix unit test

6b4258c

esauerbo dismissed jordanvn’s stale review via 6b4258c October 24, 2024 22:30

esauerbo temporarily deployed to ci October 24, 2024 22:30 — with GitHub Actions Inactive

esauerbo requested review from jordanvn and a team October 24, 2024 22:31

esauerbo temporarily deployed to ci October 24, 2024 22:39 — with GitHub Actions Inactive

calebpollman reviewed Oct 24, 2024

View reviewed changes

Remove drawoval from state machine

ad53e49

esauerbo temporarily deployed to ci October 25, 2024 17:32 — with GitHub Actions Inactive

esauerbo temporarily deployed to ci October 25, 2024 17:42 — with GitHub Actions Inactive

Remove metadata gate from set dom and camera details

8d594e5

esauerbo temporarily deployed to ci October 25, 2024 19:12 — with GitHub Actions Inactive

esauerbo requested a review from calebpollman October 25, 2024 19:14

esauerbo temporarily deployed to ci October 25, 2024 19:20 — with GitHub Actions Inactive

esauerbo temporarily deployed to ci October 25, 2024 19:21 — with GitHub Actions Inactive

esauerbo requested a review from reesscot October 30, 2024 20:35

reesscot reviewed Oct 30, 2024

View reviewed changes

reesscot previously approved these changes Oct 30, 2024

View reviewed changes

calebpollman previously approved these changes Oct 31, 2024

View reviewed changes

Remove canvasRef from dependency array

97f047d

esauerbo dismissed stale reviews from calebpollman and reesscot via 97f047d October 31, 2024 17:37

esauerbo requested review from reesscot and calebpollman October 31, 2024 17:42

calebpollman previously approved these changes Oct 31, 2024

View reviewed changes

reesscot previously approved these changes Oct 31, 2024

View reviewed changes

Merge branch 'liveness/alpha' of https://github.com/aws-amplify/ampli…

61308ec

…fy-ui into fix-oval-render

esauerbo dismissed stale reviews from reesscot and calebpollman via 61308ec October 31, 2024 18:06

esauerbo requested review from reesscot and calebpollman October 31, 2024 18:06

esauerbo temporarily deployed to ci October 31, 2024 18:06 — with GitHub Actions Inactive

esauerbo temporarily deployed to ci October 31, 2024 18:14 — with GitHub Actions Inactive

reesscot approved these changes Oct 31, 2024

View reviewed changes

calebpollman approved these changes Oct 31, 2024

View reviewed changes

esauerbo merged commit abbf118 into liveness/alpha Oct 31, 2024
32 checks passed

esauerbo deleted the fix-oval-render branch October 31, 2024 20:38

ayush987goyal mentioned this pull request Nov 1, 2024

Merge liveness/alpha into liveness dca v2 #5992

Merged

5 tasks

ayush987goyal mentioned this pull request Nov 6, 2024

liveness dca v2 #6016

Merged

5 tasks

ayush987goyal mentioned this pull request Nov 7, 2024

liveness dca v2: Update selecter userCamera metadata #6024

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(liveness): Fix oval render when switching cameras #5954

fix(liveness): Fix oval render when switching cameras #5954

esauerbo commented Oct 24, 2024 •

edited

Loading

changeset-bot bot commented Oct 24, 2024 •

edited

Loading

jordanvn left a comment

calebpollman Oct 24, 2024 •

edited

Loading

esauerbo Oct 25, 2024 •

edited

Loading

calebpollman Oct 24, 2024

esauerbo Oct 25, 2024

calebpollman Oct 24, 2024

esauerbo Oct 25, 2024

reesscot Oct 30, 2024

calebpollman Oct 31, 2024

calebpollman Oct 31, 2024

esauerbo Oct 31, 2024 •

edited

Loading

calebpollman Oct 31, 2024

esauerbo Oct 31, 2024

fix(liveness): Fix oval render when switching cameras #5954

fix(liveness): Fix oval render when switching cameras #5954

Conversation

esauerbo commented Oct 24, 2024 • edited Loading

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

changeset-bot bot commented Oct 24, 2024 • edited Loading

⚠️ No Changeset found

jordanvn left a comment

Choose a reason for hiding this comment

calebpollman Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

esauerbo Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esauerbo Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esauerbo commented Oct 24, 2024 •

edited

Loading

changeset-bot bot commented Oct 24, 2024 •

edited

Loading

calebpollman Oct 24, 2024 •

edited

Loading

esauerbo Oct 25, 2024 •

edited

Loading

esauerbo Oct 31, 2024 •

edited

Loading