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

Audio route to speaker for Android platform #295

Merged

Conversation

vtn-dev-prithipal
Copy link
Contributor

@vtn-dev-prithipal vtn-dev-prithipal commented Sep 24, 2020

Added a function that routes audio to the speaker for the Android platform

Issue: When Callkeep Active Call UI and custom Call UI are active simultaneously and the user tries to route the audio to the speaker from the earpiece or vice-versa from the custom call screen. The android connection service doesn't release the audio manager and the application is unable to change the state via the audio manager.

@jpotts18
Copy link
Contributor

jpotts18 commented Oct 8, 2020

What else can we do to get this merged in @manuquentin? Seems purely additive.

index.js Outdated
@@ -149,6 +149,8 @@ class RNCallKeep {

sendDTMF = (uuid, key) => RNCallKeepModule.sendDTMF(uuid, key);

toggleAudioRouteSpeaker = (uuid, useSpeaker) => RNCallKeepModule.toggleAudioRouteSpeaker(uuid, useSpeaker);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check to only call it on Android please ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, let me make this Android-specific function

@manuquentin
Copy link
Contributor

Thanks @vtn-dev-prithipal

I've made some comments in your code. Can you please check them ?

@danjenkins
Copy link
Collaborator

@jpotts18 I havent looked at it properly yet.... but the first thing I can see is theres no if iOS check on the JS api - right now, it would error out in ios.

I'm not even 100% sure this should be in here. For example... I use the react-native-call-manager package and I'm able to control the UI routing using that....

I don't see why this module should get involved with it.... what if I use an external module to control the audio session etc? I'm not saying no to the PR, I just need to understand it better

@manuquentin
Copy link
Contributor

manuquentin commented Oct 8, 2020

Hi Dan,
Regarding this feature in this library, we asked the same question with @sboily as we also use react-native-incall-manager.

But here, it seem to work directly on the VoiceConnection, I don't think that react-native-incall-manager could interact directly with the VoiceConnection. And maybe yes, this could belong here.

@danjenkins
Copy link
Collaborator

Yeah... @manuquentin I'm unsure why interacting on the voice connection directly is needed.... I don't want to say this shouldn't get merged.... but I'd argue to the docs need to be super clear why you'd use it here vs using say in-call-manager

@vtn-dev-prithipal
Copy link
Contributor Author

Hi Dan,
Regarding this feature in this library, we asked the same question with @sboily as we also use react-native-incall-manager.

But here, it seems to work directly on the VoiceConnection, I don't think that react-native-incall-manager could interact directly with the VoiceConnection. And maybe yes, this could belong here.

-- react-native-incall-manager is using AudioManager class for a toggle speaker. In our case, we are also using the AudioManager but its unable to route the audio to the speaker when we try to control the call actions from our app UI instead of native call UI.

Scenario: Whenever the user answer the calls and the app receives an answer event
RNCallKeep.addEventListener('answerCall', ({ callUUID }) => , we show our app with custom active call UI/UX
using RNCallKeep.backToForeground() function. When the user tries to toggle the speaker button on our app screen,  App tries to invoke AudioManager to set the speaker's phone on but it fails every time. I found out that the voice connection class has control of the audio manager and release after the end of the calls.

@geraintwhite
Copy link
Contributor

geraintwhite commented Dec 22, 2020

@vtn-dev-prithipal does a similar thing need to be done for muting?

In my app I use AudioManager.setSpeakerphoneOn(enabled); and AudioManager.setMicrophoneMute(enabled); from my custom call UI and muting works fine but speaker doesn't (as you explained).

Edit:

Nevermind - I just noticed that there already is setMutedCall!

@geraintwhite
Copy link
Contributor

I've tested this PR and it works well when using RNCallKeep.backToForeground().

Comment on lines 344 to 356
@ReactMethod
public void toggleAudioRouteSpeaker(String uuid, boolean useSpeaker) {
VoiceConnection conn = (VoiceConnection) VoiceConnectionService.getConnection(uuid);
if (conn == null) {
return;
}
if(useSpeaker){
conn.setAudioRoute(CallAudioState.ROUTE_SPEAKER);
}else{
conn.setAudioRoute(CallAudioState.ROUTE_EARPIECE);
}
}

This comment was marked as resolved.

index.d.ts Outdated
Comment on lines 118 to 123
/**
* @description toggleAudioRouteSpeaker method is available only on Android.
*/
static toggleAudioRouteSpeaker(uuid: string, useSpeaker: boolean) {

}

This comment was marked as resolved.

index.js Outdated
@@ -149,6 +149,8 @@ class RNCallKeep {

sendDTMF = (uuid, key) => RNCallKeepModule.sendDTMF(uuid, key);

toggleAudioRouteSpeaker = (uuid, useSpeaker) => RNCallKeepModule.toggleAudioRouteSpeaker(uuid, useSpeaker);

This comment was marked as resolved.

Copy link
Contributor Author

@vtn-dev-prithipal vtn-dev-prithipal left a comment

Choose a reason for hiding this comment

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

as per suggestion, changes are updated

@geraintwhite
Copy link
Contributor

@manuquentin is this any closer to being merged? I've been using it for a while in my app via patch-package and it works perfectly with RNCallKeep.backToForeground().

Copy link
Member

@sboily sboily left a comment

Choose a reason for hiding this comment

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

Hello, please add documentation and we'll merge it.

@sboily sboily added the waiting for feedback Waiting for feedback label Feb 19, 2021
@geraintwhite
Copy link
Contributor

I'm happy to add some documentation if @vtn-dev-prithipal is unable to?

@vtn-dev-prithipal
Copy link
Contributor Author

I'm happy to add some documentation if @vtn-dev-prithipal is unable to?

yes, please. I have added few lines. @grit96 please review and make changes if necessary.

Comment on lines +185 to +190
/**
* @description when Phone call is active, Android control the audio service via connection service. so this function help to toggle the audio to Speaker or wired/ear-piece or vice-versa
* @param {*} uuid
* @param {*} routeSpeaker
* @returns Audio route state of audio service
*/

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated here b0a2fda

please review

@sboily
Copy link
Member

sboily commented Mar 19, 2021

Thank you!

@sboily
Copy link
Member

sboily commented Mar 19, 2021

@manuquentin please review and merge it if everything is correct.

vtn-dev-prithipal and others added 2 commits March 19, 2021 19:22
Co-authored-by: Geraint White <geraintwhite@gmail.com>
Co-authored-by: Geraint White <geraintwhite@gmail.com>
if (conn == null) {
return;
}
if(useSpeaker){
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be routeSpeaker here.
Can you please also reformat code and add space before ( and after ).
Same for the else below, space before { and after }.

@manuquentin manuquentin merged commit 9e7b1f5 into react-native-webrtc:master Mar 22, 2021
@developer-vtnetzwelt
Copy link

Thanks @vtn-dev-prithipal

I've made some comments in your code. Can you please check them ?

Thanks @ vtn-dev-prithipal

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

Successfully merging this pull request may close these issues.

7 participants