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

Avoid multiple calls to getUserMedia for getLocalDevices #311

Merged
merged 17 commits into from
Jul 28, 2022

Conversation

lukasIO
Copy link
Contributor

@lukasIO lukasIO commented Jul 7, 2022

fixes iOS safari video preview disappearing.
Alternative fix on the SDK level to livekit/livekit-react#60.
With this fix, we don't have to change anything in the react packages.

@lukasIO lukasIO requested a review from davidzhao July 7, 2022 15:01
@changeset-bot
Copy link

changeset-bot bot commented Jul 7, 2022

🦋 Changeset detected

Latest commit: de4ec44

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
livekit-client Patch

Not sure what this means? Click here to learn what changesets are.

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

Copy link
Member

@davidzhao davidzhao left a comment

Choose a reason for hiding this comment

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

I wonder if there's an alternative way to trigger the permission dialog without impacting normal devices.

Is it worth trying this approach:

  • in getDevices, calling navigator.mediaDevices.getUserMedia() but with constraints that wouldn't match any device.. for example, deviceId: "dummy".

Thinking out loud, the problem only exists if we acquire the same device twice, so if we could make it so that the getUserMedia attempt in discovery grants permissions, but fails to acquire an actual device.

@@ -30,7 +31,15 @@ export async function createLocalTracks(

const opts = mergeDefaultOptions(options, audioDefaults, videoDefaults);
const constraints = constraintsForOptions(opts);
const stream = await navigator.mediaDevices.getUserMedia(constraints);
Copy link
Member

Choose a reason for hiding this comment

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

this seems like it'd be problematic when we require different constraints? It seems that each attempt could be different.

Copy link
Member

Choose a reason for hiding this comment

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

ok, I totally misread the code.. it wasn't re-using it, but just caching the previous one.. clever!

this should work as well.

Copy link
Member

Choose a reason for hiding this comment

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

though I think there are still some edge cases as it relates to audio/video types.

@lukasIO
Copy link
Contributor Author

lukasIO commented Jul 7, 2022

nice idea! will test that approach with a dummy deviceId.
It might still fail as I think on iOS Safari acquiring multiple camera tracks isn't supported, but will verify.

@lukasIO
Copy link
Contributor Author

lukasIO commented Jul 8, 2022

@davidzhao your suggestion works like a charm also on iOS. It's a great workaround!

edit: ah. celebrated too early. behaviour is different for firefox. On firefox the permissions do not get granted or rather the user doesn't even get prompted for permissions if there is a OverConstrainedError. This leads to the devices list staying empty. MDN says:

If no camera exists with this resolution or higher, then the returned promise will be rejected with OverconstrainedError, and the user will not be prompted.

This seems to be in accordance with how the spec defines the steps: https://www.w3.org/TR/mediacapture-streams/#dom-mediadevices-getusermedia

If candidateSet is the empty set, jump to the step labeled NotFound Failure below.

The jump would mean skipping a user facing permission prompt.

We could:

  • use your suggestion for all browsers and make an exception for firefox
  • keep the old behaviour (the one prior to this PR) and only use the dummy-id workaround for Safari

As the spec says Firefox's behaviour is the "correct" one, I'd opt for only making the Safari case an exception. What do you think?

Copy link
Member

@davidzhao davidzhao left a comment

Choose a reason for hiding this comment

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

nice!!

lukasIO and others added 12 commits July 22, 2022 19:40
This reverts commit ca400b0.
This reverts commit 93b5100.
* fix sourcemaps in sample

* make overrides explicit or remove
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…#350)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…#353)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@lukasIO
Copy link
Contributor Author

lukasIO commented Jul 25, 2022

@davidzhao the map approach seems to work across browsers as long as the safari case is handled separately. tested across firefox/chrome/safari and iOS safari

Copy link
Member

@davidzhao davidzhao left a comment

Choose a reason for hiding this comment

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

awesome work!

@lukasIO lukasIO merged commit 61a41e0 into main Jul 28, 2022
@lukasIO lukasIO deleted the lukas/fix-ios-safari-multiple-acquire branch July 28, 2022 13:33
@github-actions github-actions bot mentioned this pull request Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants