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

Consider device key '' when evaluating for 'default' device #239

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kmteras
Copy link

@kmteras kmteras commented Jan 26, 2024

On Chrome, if no media device permissions are given, the devices are exposed with minimal info:

[
  {deviceId:'',kind:'audioinput',label:'',groupId:''},
  {deviceId:'',kind:'videoinput',label:'',groupId:''},
  {deviceId:'',kind:'audiooutput',label:'',groupId:''}
]

After the media device permissions have been granted, a devicechange event is triggered and the previous device '' is supposed to be removed but never is.

This resulted in duplicate audio for users who are presented with the microphone permissions on their first call as the previous output device '' never got properly removed.

Contributing to Twilio

All third party contributors acknowledge that any contributions they provide will be made under the same open source license that the open source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

Pull Request Details

Description

We were getting reports of clients complaining about echo from their side.

I managed to reproduce the issue when the user had not given any media permissions to the site before the call was started. And the audio playback was echoy/reverby. This does not come across on any of the recordings as the audio being sent to the user is right.

Tracked the issue down to devices changing on Chrome when permissions are granted and narrowed it down to the device '' not being removed properly when 'default' is available. This issue can be reproduced on Chrome or other Chromium based browser but does not exist on Firefox as Firefox presents the full devices list even without any permissions.

Burndown

Before review

  • Updated CHANGELOG.md if necessary
  • Added unit tests if necessary
  • Updated affected documentation
  • Verified locally with npm test
  • Manually sanity tested running locally
  • Ready for review

Before merge

  • Got one or more +1s
  • Squashed erroneous commits if necessary
  • Re-tested if necessary

On Chrome, if no media device permissions are given, the devices are
exposed with minimal info:
```
[
  {deviceId:'',kind:'audioinput',label:'',groupId:''},
  {deviceId:'',kind:'videoinput',label:'',groupId:''},
  {deviceId:'',kind:'audiooutput',label:'',groupId:''}
]
```

After the media device permissions have been granted, a `devicechange`
event is triggered and the previous device `''` is supposed to be
removed but never is.

This resulted in duplicate audio for users who are presented with
the microphone permissions on their first call as the previous output
device `''` never got properly removed.
const idToReplace = Array.from(pc.outputs.keys())[0] || 'default';
const activeDevice = Array.from(pc.outputs.keys())[0];
// The audio device key could also be '' on Chrome if no media device permissions are allowed
const idToReplace = activeDevice !== null && activeDevice !== undefined ? activeDevice : 'default';
Copy link
Author

Choose a reason for hiding this comment

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

Checking against undefined might be enough if guarding against outputs being empty?

@charliesantos
Copy link
Collaborator

Thanks for submitting @kmteras . We'll try to reproduce the issue first on our side then get back to this PR.

@kmteras
Copy link
Author

kmteras commented Feb 5, 2024

@charliesantos has anyone tried to reproduce this? I have not reproduced this with a starter guide or any minimal setup, but the problem should be noticeable there as well.
Can help with reproduction if needed.

@ostap0207
Copy link

Hello, any updates regarding this issue?

@kpchoy
Copy link
Collaborator

kpchoy commented Apr 2, 2024

Hi @kmteras , sorry for the delay in response. However, we are not able to reproduce. When we check device.audio.availableInputDevices and device.audio.availableOutputDevices, we cannot see the '' you are describing. Can you use our quickstart to reproduce, and provide any steps or code changes we need to make on our end.

@kmteras
Copy link
Author

kmteras commented Apr 4, 2024

When we check device.audio.availableInputDevices and device.audio.availableOutputDevices, we cannot see the '' you are describing.

I tested with the https://github.com/TwilioDevEd/voice-javascript-sdk-quickstart-node as well and could reproduce the '' devices, and the duplicate audio.

The steps I took to reproduce this:

  1. Open up a tab in Chrome (With microphone permissions not accepted!), and another tab in any other browser or incognito mode.
  2. Start up the devices, only press the "Seeing Unknown devices" in the non-Chrome tab/browser.
  3. At this point in the Chrome tab I made the device global in quickstart.js and device.audio.availableOutputDevices returns '' when the permissions are not granted.
  4. Make a call to the Chrome tab client name from the other device, accept the incoming call it in the Chrome tab and then accept the microphone permissions it asks for. (It must ask for permissions at this point!)
  5. The Chrome tab will be hearing duplicate audio, at this point device.audio.availableOutputDevices should return the actual devices.

If debugging further, then at step 5 in the Twilio SDK it should be clear that idToReplace evaluates to default instead of '', which is the Array.from(pc.outputs.keys())[0] value when the permissions are accepted and new devices are visible.

I did my current testing on MacOS 13.6.6 and Chrome 123.0.6312.107 with the other client being Firefox 124.0.1 but the issue should also be reproducible on Windows or Linux. The key part is that the duplicate audio and '' devices only happen on Chrome AFAIK. Firefox always shows all the devices and Safari does not show anything if audio permissions have not been granted.

@kmteras
Copy link
Author

kmteras commented Apr 4, 2024

To clarify point 5: I muted the non-main-Chrome tab, so the problem is not hearing audio from two tabs. This can also be reproduced with calling in from another device or a phone.

@charliesantos
Copy link
Collaborator

Thanks @kmteras for the detailed response. Can you please confirm that you cannot reproduce the echo issue with your PR?

@kmteras
Copy link
Author

kmteras commented Apr 4, 2024

Can you please confirm that you cannot reproduce the echo issue with your PR?

I tested with the custom build we have made (https://github.com/salemove/twilio-voice.js/blob/master/dist/twilio.min.js) with the quickstart guide and the issue does not exist there.
I tested the 2.10.2 version with the quickstart guide and the issue exists there.

Our custom build has been rolled out for our frontend release for over a month now and we have not received any more complaints from customers about this issue occurring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants