-
Notifications
You must be signed in to change notification settings - Fork 232
Element Call: Add audio output selector handled by Android #4663
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
base: develop
Are you sure you want to change the base?
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4663 +/- ##
===========================================
- Coverage 80.47% 80.31% -0.16%
===========================================
Files 2140 2140
Lines 56756 56887 +131
Branches 7110 7150 +40
===========================================
+ Hits 45672 45688 +16
- Misses 8649 8765 +116
+ Partials 2435 2434 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2ef71bf
to
9ccfb63
Compare
17621c2
to
94e37e0
Compare
94e37e0
to
ed22465
Compare
After #4743 this should now be ready to review. |
I have tested this and I am loosing the audio output when selecting Actually it was due to the fact that this is not the same volume mapping. For earpiece, the |
Oh, that's tricky. I'm not sure if we can make they use the same volume rocker or have some other way to make this transition seamless.. |
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.
Changing audio output is working pretty well.
I have also checked that connecting a Bluetooth headset during a call automatically switch the audio output to the Bluetooth device.
But it seems that the audio input (in EX) was not working. I could not hear any voice coming to Element Web (sound alert was working so this is not a volume/mute problem on the browser)
AudioDeviceInfo.TYPE_WIRED_HEADPHONES -> "Wired headphones" | ||
AudioDeviceInfo.TYPE_BUILTIN_SPEAKER -> "Built-in speaker" | ||
AudioDeviceInfo.TYPE_BUILTIN_EARPIECE -> "Built-in earpiece" | ||
else -> "Unknown:" |
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.
else -> "Unknown:" | |
else -> "Unknown" |
(I am not sure)
*/ | ||
private fun setWebViewOnAudioDeviceSelectedCallback() { | ||
Timber.d("Adding callback in controls.onOutputDeviceSelect") | ||
webView.evaluateJavascript("controls.onOutputDeviceSelect = (id) => { onAudioDeviceSelectedCallback.setOutputDevice(id); };", null) |
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.
Maybe add the ref https://github.com/element-hq/element-call/blob/livekit/docs/controls.md#audio-devices
to the Kdoc or at the top of the class?
Did you have video output or was that also failing? I've had one call this morning where the encryption keys exchange seemed to fail and that resulted in the 2 users seeing each other as present in the call but no video or audio getting through to the other. |
I have tested it again and this is the status for a call with EW and EX and EW does not render incoming video nor audio. The other side is working (EX render video and play audio) |
Could you check you don't have set a custom Element Call URL? On EW you can use
So I have good news and bad news after checking other apps and how they handle this issue:
I'll check if there's any alternative to this, but I'm not really hopeful... |
I have not set any custom url on EW. For the volume I think it is acceptable if it's an OS limitation... But can be misleading for user (like me) thinking that the audio is not working when switching the output. There is maybe a way to read the volume level and warn the user that the volume is probably too low (I think I have seen other apps doing that, a long time ago, I do not remember which one). |
@bmarty could you retry with the latest changes? |
The latest changes should fix the issue with the audio for the speaker initially not following the right volume rocker, but now we need element-hq/element-call#3309. |
This way we can know when the EC page has been loaded and can start interacting with the JS code in it.
…anager` extension functions This component is responsible of tracking the audio device usage on both the native OS and the webview and coordinating them.
…ead of automatically detecting them
…ing the right audio stream
Otherwise, the default audio device won't use the right audio stream.
…` to keep up to date with Element Call
89bffd5
to
4446e33
Compare
Rebased after upgrading the Element Call version in #4832, which contains the changes needed to fix the audio for the device selected by 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.
Thanks for the update, I'll do some test and comment the PR again if everything is working fine on my side!
audioManager.selectAudioDevice(selectedDeviceId) | ||
}, | ||
onAudioPlaybackStarted = { | ||
MainScope().launch { |
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.
Provide a CoroutineScope in the constructor instead of using the MainScope?
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.
I think we'll still need to make sure it's called on Dispatchers.Main
, but good idea. This way everything can stop as soon as the UI is destroyed.
* Call this method when the call stops to disable in-call audio mode. | ||
* | ||
* It's the counterpart of [onCallStarted], and should be called as a pair with it once the call has ended. | ||
*/ |
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.
Kudos for all the comments, it helps to understand the code a lot!
|
Content
ElementCallActivity
.Motivation and context
Fixes #3625.
Screenshots / GIFs
audio-selector.mp4
Tests
It would be specially interesting to test this on Android 11, since I don't own a device with this OS version and the APIs are very different. Also testing it on Android 12 would be good, since there is a workaround needed for that version related to the AudioManager's comms mode.
Tested devices
Checklist