Fixing the ability to choose default input in browsers when default changes #496
Conversation
| @@ -475,6 +481,15 @@ export default class DefaultDeviceController implements DeviceControllerBasedMed | |||
| return device && device.id ? device : null; | |||
| } | |||
|
|
|||
| private isGroudpIdSame(groupId: string, kind: string, device: Device): boolean { | |||
There was a problem hiding this comment.
nit: rename it to hasSameGroupId?
| private isGroudpIdSame(groupId: string, kind: string, device: Device): boolean { | ||
| if (device === null) return false; | ||
| if (typeof device !== 'string') | ||
| device = JSON.parse(JSON.stringify(device)).deviceId |
There was a problem hiding this comment.
Could we move JSON.stringify to a variable?
This comment has been minimized.
This comment has been minimized.
RidipDe
force-pushed
the
groupIdMethod2
branch
8 times, most recently
from
43d9d96 to
c6e4e75
Compare
RidipDe
force-pushed
the
groupIdMethod2
branch
3 times, most recently
from
29401b8 to
70467ec
Compare
There was a problem hiding this comment.
Questions on the PR overview:
When a meeting is active and we connect a bluetooth device to our system
- Is this specific to Bluetooth devices?
- What if the device is already connected, can the issue be reproduced?
- What browser does this impact?
then select the default (which is the bluetooth device) option from the audio input drop down
- Does this impact the SDK or just the meeting demo?
The constraint check for chooseInputDevice() now checks with the help of groupId in addition to deviceId.
- What does this mean specifically? How is the groupId used?
This call tells the user agent that the track's source is no longer needed by the MediaStreamTrack.
- Why does this resolve the issue?
How did you test these changes?
- Are there unit tests to test behavior with the workaround and without the workaround?
| @@ -520,9 +548,24 @@ export default class DefaultDeviceController implements DeviceControllerBasedMed | |||
| newDevice.stream = stream; | |||
| newDevice.constraints = proposedConstraints; | |||
| } else { | |||
| // TODO: remove check when the bug is fixed. | |||
| if ( | ||
| this.activeDevices[kind].constraints && | ||
| this.activeDevices[kind].constraints.audio && | ||
| JSON.parse(JSON.stringify(this.activeDevices[kind].constraints.audio)).deviceId |
| @@ -41,6 +41,11 @@ export default class DefaultDeviceController implements DeviceControllerBasedMed | |||
| private videoMaxBandwidthKbps: number = DefaultDeviceController.defaultVideoMaxBandwidthKbps; | |||
|
|
|||
| private useWebAudio: boolean = false; | |||
| private deviceIdGroupIdMap: { [mediaDeviceKind: string]: Map<Device, string> } = { | |||
There was a problem hiding this comment.
Isn't the group id information available in the deviceInfoCache and the activeDevices? Do we really need more state in order to solve the problem that this PR addresses?
There was a problem hiding this comment.
Few browsers like Safari does not have groupId as part of the device information. To make the solution oblivious of the browser I added this state so that it can have a mapping between the deviceId and the groupId (or '' if group id is missing)
There was a problem hiding this comment.
Do the browsers that have an empty group ID require the workaround logic?
There was a problem hiding this comment.
No browsers such as Safari does not have groupId. It has unique deviceId which is never reassigned like in the case of chromium based browser.
| device && | ||
| this.getDeviceIdStr(device) === 'default' | ||
| ) { | ||
| if ( |
There was a problem hiding this comment.
What is the reason for having two ifs? Can they be combined with &&?
The issue can occur whenever an external device is attached to the system in the middle of the meeting. Bluetooth was the external device used for testing here
If the device is already connected, and the default changes while in the middle of meeting, the issue can be reproduced.
Any browser that uses 'default' as the deviceId. Chrome and Opera are couple of examples
It affects both the SDK and the meeting demo
groupId is being used as a condition check along with the constraint parameter in this PR.
When the getUserMedia() is called with 'default' as the constraint deviceId, while the stream is active, the 'default' refers to the old 'default' even after the new device was connected in the middle of the meeting and it is chosen by the OS to become the 'default' |
There was a problem hiding this comment.
The issue can occur whenever an external device is attached to the system in the middle of the meeting. Bluetooth was the external device used for testing here
The PR overview makes it sound like this is fixing an issue with Bluetooth devices, but as far as what we have discussed it sounds like the type of device is not relevant.
If the device is already connected, and the default changes while in the middle of meeting, the issue can be reproduced.
The PR overview makes it sound like the device being newly connected is part of the issue repro. Could you clarify the minimal repro in the overview?
Any browser that uses 'default' as the deviceId. Chrome and Opera are couple of examples
Are there any non- Chromium-based browsers that have this ? Would it be worth putting the logic behind a BrowserBehavior flag so as not to change the logic for other browsers that do not have this issue?
It affects both the SDK and the meeting demo
The PR overview refers to the "drop down" menu item; however, from what we've discussed here it sounds like this issue lies in the SDK and the meeting demo is just one possible way to reproduce. Could you clarify the repro in terms of the actual sequence of events from the SDK perspective (e.g. chooseAudioInputDevice and other APIs)?
groupId is being used as a condition check along with the constraint parameter in this PR. If activeDevice (deviceId and groupId) = requestingDevice (deviceId and groupId) continue using the same device, else new create DeviceSelection() with the same deviceId and the groupId
Also, when in some browsers the groupId is undefined (like Safari), we assign a default groupId to make the solution oblivious of the browser.
Could you update the PR overview with this information?
When the getUserMedia() is called with 'default' as the constraint deviceId, while the stream is active, the 'default' refers to the old 'default' even after the new device was connected in the middle of the meeting and it is chosen by the OS to become the 'default'
Could you describe what we believe the browser to be doing under the hood and why releasing the media stream helps (and add to the PR overview)?
| @@ -41,6 +41,11 @@ export default class DefaultDeviceController implements DeviceControllerBasedMed | |||
| private videoMaxBandwidthKbps: number = DefaultDeviceController.defaultVideoMaxBandwidthKbps; | |||
|
|
|||
| private useWebAudio: boolean = false; | |||
| private deviceIdGroupIdMap: { [mediaDeviceKind: string]: Map<Device, string> } = { | |||
There was a problem hiding this comment.
Do the browsers that have an empty group ID require the workaround logic?
|
@aburnson @RidipDe Based on PR #490 (closed), please take a look at my minimal solution for choosing the default: 81f5462
|
RidipDe
force-pushed
the
groupIdMethod2
branch
4 times, most recently
from
1adaa14 to
5dcbf7b
Compare
RidipDe
force-pushed
the
groupIdMethod2
branch
9 times, most recently
from
7bc76eb to
60777c6
Compare
RidipDe
force-pushed
the
groupIdMethod2
branch
6 times, most recently
from
186b3c9 to
f75e759
Compare
Issue #:
When a new audio input device is connected to a system while the 'default' system microphone is in use such that the new input device becomes the system default, the call to the method getUserMedia() with audio constraint deviceId as 'default' in chromium based browsers continues to use the previously active media stream (system microphone) instead of the the expected new stream (new audio input device). However if we call the getUserMedia() with audio constraint deviceId with alpha-numeric deviceId of the new device, it returns the stream of the new device.
When a meeting is active and we connect an external device (lets say bluetooth device) to our system and then select the default which is the bluetooth device option from the audio input drop down, the switch does not take place. The app continues to use the audio input device that it has been using before the bluetooth was connected.
Description of changes:
Change 1:
In chromium browsers running on desktops and mobile devices, the system speaker is the 'default' device id.
With an external audio device connected (like a bluetooth audio device), the external audio device becomes the 'default' but the bowser browser don't automatically switch the device. The user has to manually reselect the default device.
When choosing the new device with deviceId 'default', while the the previous device with deviceId 'default' is active, the current logic in the SDK checks the constraints of the activeDevice and the new requesting device based on just the deviceId. So when requesting the new 'default' device, the current logic returns the already active media stream as it thinks that a request to the already active 'default' device is made.
Each device also has a groupId associated. While the deviceId is unique to each device, the groupId may/may not group multiple devices into single group. So instead of just using the deviceId as a constraint check for the condition, deviceId and the corresponding groupId should be used as a unique pair to validate the check. So when a new device id added the deviceId and groupId pair will be unique.
Change 2:
Following the above problem we have another issue that needs to be addressed to solve the issue. If we try to call the getUserMedia() on the 'default' device, since the browser is already using the 'default', it is going to create a new stream but will point to the old 'default' device. So we need to ensure that the old media stream is stopped/released before requesting the new 'default' device in the getUserMedia().
The understanding here is, that the mapping between the deviceId and the device is not updated until the track is stopped.
If we call the getUserMedia() with 'default' as deviceId in the constraint without stopping the active track, it returns the previously acquired MediaStream associated to the 'default' deviceId. But once we stop the active media track and then call the getUserMedia(), it will tell the user agent that the track's source is no longer needed by the MediaStreamTrack. This way, the browser will know to clean up the existing references and update the mapping.
Repro Steps using the demo meeting app:
The switch in the microphone does not take place. The SDK continues to use the old 'default' microphone
Testing
npm run build:releaselocally?Yes
Tested it manually. We can now select the default option in the drop down audio input menu or perform await app.audioVideo.chooseAudioInputDevice('default');
Inter browser tests have also been performed between Chrome and Firefox
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.