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

Fix camera switching for ios, android and desktop #6691

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

mjamescompton
Copy link
Contributor

Summary

Closes #6529

Changes

  • Refactor Video component and separate into Camera, Stream, Video components.
  • Only display video stream based on videoinput with a deviceID

Testing

Testing
Log in the console on a mobile device
Go to register end device
Click on scan end device QR code
See the switch button at the bottom of the video
Click and see the camera changing

Regressions

...

Notes for Reviewers

I have separated this component into Camera, Stream, Video. I did this so that we would completely re render the Stream, Video components when a user selects another a new camera.

Problem 1 - samsung galaxy s20

The main discovery while testing was that on many devices (iPhone and some android) navigator.mediaDevices.enumerateDevices() was only able to show an empty videoinput object until the user gives permission to view the full list.

The problem with some android devices was that this stack overflow snippet we where using would assume that all android devices would have a device Id on page load.

const videoMode =
        cameras.length > 1
          ? ua.indexOf('safari') !== -1 && ua.indexOf('chrome') === -1
            ? { facingMode: { exact: 'environment' } }
            : { deviceId: rearCamera.deviceId }
          : { facingMode: 'environment' }

When we would request the back camera using the following we would get an empty device id which would then tell the phone to use its default camera, which in this case was the face camera.

  let rearCamera = cameras.find(device => device.label.toLowerCase().includes('back'))

Problem 2 - Android error on camera switch.

We where not using

if (stream) {
      stream.getTracks().forEach(track => {
        track.stop()
      })
    }

When switching cameras, so the previous camera feed was still active and this would cause an error on some android phone.

Problem 3 - iPhone

I think the root of the problem here was the amount of videoinputs on the newer iphones

WhatsApp Image 2023-11-07 at 14 09 20

When testing I believe we where just cycling through different back cameras making it seem it was not changing. This lead to us going down a rabbit whole where we ended up with no switching at all.

The switch button would not show on first render as at that point we did not have permission to view the list of cameras.

General Solution

As device Id is the only consistent way of identify the cameras, I have rebuilt this element to only display the video to the user once we have a deviceId

so the idea is that we request the back camera using the following

const userStream = await navigator.mediaDevices.getUserMedia({
  video: { facingMode: 'environment' },
})

This snippet seem to work generally across devices to return the back camera.
This requests permission to use the cameras and so gives us access to all camera data
At this point nothing is displayed to the user but using this stream we can get the deviceID for the camera the phone as assumed as back camera.

We then re-render the Stream component with a deviceId which displays Videos component to the user.

We now have a list of all devices for the switch button to use to change devices. Depending on the label names we either switch between cameras named back or front or we cycle through all the camera (This allows us to also cycle between multiple cameras on desktop for example if you using an external webcam ). As iPhone label names are pretty clean and consistent this means we should not cycle through all the different types of back cameras.

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • The steps/process to test this feature are clearly explained including testing for regressions.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@mjamescompton mjamescompton self-assigned this Nov 8, 2023
@github-actions github-actions bot added the ui/web This is related to a web interface label Nov 8, 2023
Copy link
Contributor

@kschiffer kschiffer left a comment

Choose a reason for hiding this comment

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

Nice drill-through. See my remarks.

pkg/webui/components/qr/input/video/index.js Outdated Show resolved Hide resolved
pkg/webui/components/qr/input/video/index.js Outdated Show resolved Hide resolved
pkg/webui/components/qr/input/video/index.js Outdated Show resolved Hide resolved
pkg/webui/components/qr/input/video/index.js Outdated Show resolved Hide resolved
pkg/webui/components/qr/input/video/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@kschiffer kschiffer left a comment

Choose a reason for hiding this comment

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

Thanks! See my last remark.

pkg/webui/components/qr/input/video/index.js Outdated Show resolved Hide resolved
@mjamescompton mjamescompton merged commit 18ecb74 into v3.28 Nov 13, 2023
13 checks passed
@mjamescompton mjamescompton deleted the fix/camera-input branch November 13, 2023 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui/web This is related to a web interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch camera button during device onboarding via mobile doesn't work
2 participants