search

meetecho / janus-gateway

Janus WebRTC Server
https://janus.conf.meetecho.com
GNU General Public License v3.0
8.25k stars 2.48k forks source link

SipPlugin read rfc2833 dtmf and convert to sip info event #3280

Closed ywmoyue closed 12 months ago

ywmoyue commented 1 year ago

Implement the features proposed by jkmchinese from https://janus.discourse.group/t/audiobridge-with-sip-participants/131/7

this PR get rfc2833 data from rtp stream and assemble a sip-info event push to user

januscla commented 1 year ago

Thanks for your contribution, @ywmoyue! Please make sure you sign our CLA, as it's a required step before we can merge this.

ywmoyue commented 1 year ago

Thanks for your contribution, @ywmoyue! Please make sure you sign our CLA, as it's a required step before we can merge this.

I have signed

ywmoyue commented 1 year ago

@lminiero Thank you very much for your review. I will revise and self-test and submit the code again next week.

ywmoyue commented 1 year ago

@lminiero I have accepted your comments and made the changes, can you please review this PR again?

ywmoyue commented 12 months ago

@lminiero Thanks for the review again. I have accepted your comments and made the changes.

lminiero commented 12 months ago

Thanks! I'll see if I can test this somehow.

ywmoyue commented 12 months ago

I fixed the two errors, can you please check again

lminiero commented 12 months ago

Thanks, merging then! :+1:

ycherniavskyi commented 11 months ago

@ywmoyue, I'm curious about the choice to set a hardcoded payload type of 101 for the telephone-event RTP payload format. According to RFC2833, this type is typically assigned dynamically. Could you elucidate the reasons for opting for a hardcoded value instead of retrieving the actual payload type dynamically from the SDP?

My concern stems from potential compatibility issues, such as with SIP calls initiated by the Janus SIP plugin. It appears that the WebRTC library used by browsers uses different (and currently hardcoded) payload types for telephone-event, which could lead to functional mismatches.

ywmoyue commented 11 months ago

@ycherniavskyi Yes, you are right to concern

The reason why I use 101 as the telephone-event payload is that the terminals I tested here use 101 as the telephone-event payload by default, except for webrtc, including yealink phone, linphone, real android phone with VOLTE, and others soft phone

The correct way is indeed to get the telephone-event payload value from sdp, I will see if I can submit a PR to fix this later