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

Added Speakerphone funcionality #379

Closed
wants to merge 1 commit into from
Closed

Added Speakerphone funcionality #379

wants to merge 1 commit into from

Conversation

CSantosM
Copy link
Contributor

@CSantosM CSantosM commented Sep 5, 2019

Hi @hthetiot ,

By default, the audio output is always set to Earpiece speaker. It would be interesting be able to choose whether or not to enable the speakerphone.

This enhancement allows us it, to enable the iPhone speakerphone, exporting the enableSpeakerphone() method. Invoking this we will choose speakerphone as audio output.

Take a look.

Added enableSpeakerphone to choose speakerphone as a audio output
@hthetiot hthetiot self-requested a review September 9, 2019 22:06
@hthetiot hthetiot added this to the 5.0.x milestone Sep 9, 2019
@hthetiot
Copy link
Contributor

hthetiot commented Sep 9, 2019

In the past iosrtc had this feature, but it was removed in favor or following cordova plugin:

Original iosrtc implementation entrypoint:

The problem is your solution is only one way speaker on while "cordova-plugin-audioroute" allow to change back to earpiece and detect change when user plug a device.

I do understand your needs and for me cordova-plugin-audioroute is the real solution for now.

The ideal iosrtc solution would be to SHIM setSinkId and add "audiooutput" to enumerateDevices, that would be an acceptable solution.

Would you consider improving the PR in that direction?

"The idea is to not expose stuff not purely related to the WebRTC API."
#271 (comment)

Copy link
Contributor

@hthetiot hthetiot left a comment

Choose a reason for hiding this comment

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

@hthetiot
Copy link
Contributor

@CSantosM
Copy link
Contributor Author

CSantosM commented Sep 11, 2019

Hello @hthetiot ,

I think your proposal is reasonable. However I don't have enough time to address it. Perhaps in the future I can modify it to meet the requirements.

@saghul
Copy link
Collaborator

saghul commented Sep 11, 2019

Sorry @CSantosM but that's not how things work. @hthetiot I suggest you don't land this unless it meets your criteria.

Everyone is busy with their projects, but if you contribute, please, see it through. "I have no time, so here is a hack that works for me, deal with it" is not good enough, sorry.

@hthetiot You are the chief, do as you see fit, this is just my 2 cents.

@CSantosM
Copy link
Contributor Author

Sorry @saghul, I meant that I will try to improve the code in the future.

@hthetiot hthetiot modified the milestones: 5.0.x, 6.0.x Sep 17, 2019
@hthetiot hthetiot closed this Sep 17, 2019
@hthetiot
Copy link
Contributor

@CSantosM I made the changes on 6.0.0 branch but if you want it soon you may make a PR based on the comment above in master for 5.0.4+

@CSantosM
Copy link
Contributor Author

Thank you @hthetiot, I will take a look.

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.

3 participants