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

Show the device that's actually in use #1670

Draft
wants to merge 4 commits into
base: livekit
Choose a base branch
from

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Oct 2, 2023

Feeds the current track (if any), and therefore device it's using into the device selection dropdown and implement a some somewhat complex logic to observe when the 'default' device has changed to a different actual devbice between when we opened it and now, and display the device we're actually using as selected instead.

The comments should explain more, including the caveat that we can't do the same detective work if the device is one of several devices on the same bit of hardware. This is because is uses the same groupId comparison technique used by Livekit in normalizeDeviceId to do a smiliar thing.

Fixes #1646

Feeds the current track (if any), and therefore device it's using
into the device selection dropdown and implement a some somewhat
complex logic to observe when the 'default' device has changed to
a different actual devbice between when we opened it and now, and
display the device we're actually using as selected instead.

The comments should explain more, including the caveat that we can't
do the same detective work if the device is one of several devices
on the same bit of hardware. This is because is uses the same groupId
comparison technique used by Livekit in `normalizeDeviceId` to do
a smiliar thing.
@dbkr dbkr added the T-Defect Something isn't working: bugs, crashes, hangs, vulnerabilities, or other reported problems label Oct 2, 2023
@dbkr dbkr requested a review from a team as a code owner October 2, 2023 16:40
@codecov-commenter
Copy link

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (3e0bc8a) 29.94% compared to head (aff01d0) 29.94%.
Report is 10 commits behind head on livekit.

Additional details and impacted files
@@           Coverage Diff            @@
##           livekit    #1670   +/-   ##
========================================
  Coverage    29.94%   29.94%           
========================================
  Files           47       47           
  Lines         1877     1877           
  Branches       328      328           
========================================
  Hits           562      562           
  Misses        1274     1274           
  Partials        41       41           
Flag Coverage Δ
unittests 29.94% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

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

I am wondering if the other PR does obsolete this?
In the other PR the track is always updated in case the default device changes. So the track in use should always be matching the device and hence have the same/correct groupId.

I am wondering if this PR in combination with the other one would lead to the following behaviour:

  • default OS device is changed (actual-device-id:a -> b)
  • device id in the selected variable is still default
  • the group id of chromes default device changed to the groupId of b (also the label got updated to conatining the description of b)
  • track is restarted (becasue we do the group id check an see that default is a new device) (other PR) in this case its b now.
  • We reach the logic introduced here,
  • we pass the conditions (device.selectedId = "default")
  • we find (d) => d.groupId === usedGroupId && d.deviceId !== "default" (but there will be two devices with the correct groupId because the default device and the same device with deviceId = "someActualUUIDasTheID1234")
  • the found device will be the one where groupId == usedGroupId and deviceId will be the NOT "default" so the selector does not select the default device anymore
  • chainging the device in the os again (a -> b) works as normal but I am not sure EC still updates the device since now it does not check anymore if the group Id of the defualt device has changed.

So I am wondering if this would break EC following the system setting on chrome after the first system setting change.

Comment on lines 114 to 118
const usedGroupId =
trackUsedByRoom?.mediaStreamTrack.getSettings().groupId;
const devicesWithMatchingGroupId = devices.available.filter(
(d) => d.groupId === usedGroupId && d.groupId !== "default"
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the last pr. Could the label also work?

Copy link
Member Author

Choose a reason for hiding this comment

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

This might involve comparing here which is tricky because chrome prepends 'Default: ' so we'd have to work around that and however it i18ns on the user's system.

src/settings/SettingsModal.tsx Outdated Show resolved Hide resolved
src/settings/SettingsModal.tsx Outdated Show resolved Hide resolved
@toger5
Copy link
Contributor

toger5 commented Oct 5, 2023

Thinking about this topic does livekit already check if the order of the devices changes on firefox?

A Promise that is fulfilled with an array of MediaDeviceInfo objects. Each object in the array describes one of the available media input and output devices. The order is significant — the default capture devices will be listed first.

I am wondering if the ux would be much less broken with the following approach:

  • we have a checkbox: os default settings
  • if it is checked we only show what device we get as the first device of enumerateDevices and we react to order changes and update the media device accordnigly. (on chrome we always use id="default" and update using the groupId or label trick)
  • if it is unchecked a dropdown selector will appear to choose the device. We then NEVER use "default" as the device id and never react on os cahnges and instead only react to changes in the dropdown.

Co-authored-by: Timo <[email protected]>
@dbkr
Copy link
Member Author

dbkr commented Oct 5, 2023

I am wondering if the other PR does obsolete this?
Yes - hopefully :) Although the plan basically was to make sure the dropdown always shows the truth first, then implement the device switching, so even if the device switching doesn't work for whatever reason, it still shows the truth.

I am wondering if this PR in combination with the other one would lead to the following behaviour [...]

It will switch back to device a when you change the default device in the OS a -> b -> a (I tested). The group ID of the default device should change just the same in either case.

I am wondering if the ux would be much less broken with the following approach: [...]

Possibly! Do you think we should pause this and wait for product / design input?

@dbkr dbkr requested a review from toger5 October 5, 2023 17:21
@toger5
Copy link
Contributor

toger5 commented Oct 6, 2023

It will switch back to device a when you change the default device in the OS a -> b -> a (I tested). The group ID of the default device should change just the same in either case.

Then I have a thinko, just to confirm, you tested with both PR's being merged at the same.

@toger5
Copy link
Contributor

toger5 commented Oct 6, 2023

Yes - hopefully :) Although the plan basically was to make sure the dropdown always shows the truth first, then implement the device switching, so even if the device switching doesn't work for whatever reason, it still shows the truth.

Considering this,

Possibly! Do you think we should pause this and wait for product / design input?

I think pushing this to the next release and asking design is a good idea.

(and... add comments)
…im/element-call into dbkr/show_device_actually_in_use
@CLAassistant
Copy link

CLAassistant commented Sep 6, 2024

CLA assistant check
All committers have signed the CLA.

@hughns hughns marked this pull request as draft November 6, 2024 11:17
@hughns
Copy link
Member

hughns commented Nov 6, 2024

Making draft as this is stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect Something isn't working: bugs, crashes, hangs, vulnerabilities, or other reported problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Device Bug: Default device shown as used when re-added
5 participants