search

cordova-rtc / cordova-plugin-iosrtc

Cordova iOS plugin exposing the WebRTC W3C API
MIT License
690 stars 338 forks source link

fix: guard against empty senders array #764

Closed RaddishIoW closed 1 year ago

RaddishIoW commented 1 year ago

Guard against an empty senders array in PluginRTCDTMFSender init.

hthetiot commented 1 year ago

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

hthetiot commented 1 year ago

@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 commented 1 year ago

Hello @fippo, should createDTMF create audio sender automatically?

See pc.createDTMFSender() SHAM swift implementation of iosrtc here https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/95f21077e5c5af013cadf6395b2b9574b1c930d3/src/PluginRTCDTMFSender.swift#L24

Thx. Will send via Twitter if no reply after.

fippo commented 1 year ago

Isn't that the terrible old variant of creating DTMF on the peerconnection which got moved to the sender object to avoid that question? https://github.com/webrtchacks/adapter/issues/733 has some hints in that direction.

hthetiot commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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