-
Notifications
You must be signed in to change notification settings - Fork 54
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
Conversation
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'; |
There was a problem hiding this comment.
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?
Thanks for submitting @kmteras . We'll try to reproduce the issue first on our side then get back to this PR. |
@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. |
Hello, any updates regarding this issue? |
Hi @kmteras , sorry for the delay in response. However, we are not able to reproduce. When we check |
I tested with the https://github.com/TwilioDevEd/voice-javascript-sdk-quickstart-node as well and could reproduce the The steps I took to reproduce this:
If debugging further, then at step 5 in the Twilio SDK it should be clear that I did my current testing on MacOS |
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. |
Thanks @kmteras for the detailed response. 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. 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. |
On Chrome, if no media device permissions are given, the devices are exposed with minimal info:
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
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
npm test
Before merge