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

fix: guard against empty senders array #764

Closed

Conversation

RaddishIoW
Copy link

Guard against an empty senders array in PluginRTCDTMFSender init.

Guard against an empty senders array in PluginRTCDTMFSender init.
@hthetiot
Copy link
Contributor

hthetiot commented Feb 4, 2023

Thx @RaddishIoW, does it mean that using DTMF without sender will result in no DTMF?

@hthetiot hthetiot added the bug label Feb 4, 2023
@hthetiot hthetiot added this to the 8.0.x milestone Feb 4, 2023
@hthetiot
Copy link
Contributor

hthetiot commented Feb 11, 2023

@RaddishIoW again thx for pr, can you tell me as end user that OK for you if Promise DOT NOT fail if not sender available, meaning no audio tone sent to my understanding ? I think that an issue I need to see if it need to be created on demande, the webrtc-adapter maintainer would know, I need to ask him

@hthetiot
Copy link
Contributor

hthetiot commented Feb 11, 2023

Hello @fippo, should createDTMF create audio sender automatically?

See pc.createDTMFSender() SHAM swift implementation of iosrtc here

if !rtcPeerConnection.senders.isEmpty {

Thx. Will send via Twitter if no reply after.

@fippo
Copy link

fippo commented Feb 13, 2023

Isn't that the terrible old variant of creating DTMF on the peerconnection which got moved to the sender object to avoid that question? webrtcHacks/adapter#733 has some hints in that direction.

@hthetiot
Copy link
Contributor

hthetiot commented Feb 13, 2023

Thank you @fippo I will do some more reading based on the link, but creating a sender with dtmf attribute seams to be the unified plan strategy, I will release a patch for @RaddishIoW in the meantime to avoid crash.

Reference: https://groups.google.com/g/discuss-webrtc/c/zcvEBJUdFgs/m/yZroCB4-AwAJ

Related: https://github.com/react-native-webrtc/react-native-webrtc/pull/1293/files

@RaddishIoW
Copy link
Author

Hi @hthetiot I'm not using DTMF at all at the moment in my app - so I'm not sure whether it would affect that.

I think that SipJS has a separate method for sending DTMF via SIP INFO messages anyway, so that might be a workaround if I do.

@hthetiot
Copy link
Contributor

@RaddishIoW please can you test this PR, see instructions in description:

@hthetiot hthetiot closed this Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants