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

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

Merged
merged 7 commits into from
Nov 20, 2023

Conversation

ywmoyue
Copy link
Contributor

@ywmoyue ywmoyue commented Oct 16, 2023

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
Copy link

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

@ywmoyue ywmoyue changed the title sipPlugin read rfc2833 dtmf and convert to sip info event SipPlugin read rfc2833 dtmf and convert to sip info event Oct 17, 2023
@ywmoyue
Copy link
Contributor Author

ywmoyue commented Oct 17, 2023

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

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

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

Apologies for the late feedback. I've added some notes inline.

src/plugins/janus_sip.c Outdated Show resolved Hide resolved
src/plugins/janus_sip.c Outdated Show resolved Hide resolved
src/plugins/janus_sip.c Outdated Show resolved Hide resolved
src/plugins/janus_sip.c Outdated Show resolved Hide resolved
src/plugins/janus_sip.c Outdated Show resolved Hide resolved
src/plugins/janus_sip.c Show resolved Hide resolved
src/plugins/janus_sip.c Outdated Show resolved Hide resolved
src/plugins/janus_sip.c Show resolved Hide resolved
@ywmoyue
Copy link
Contributor Author

ywmoyue commented Oct 27, 2023

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

@ywmoyue
Copy link
Contributor Author

ywmoyue commented Oct 30, 2023

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

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

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

Added a few more comments inline.

src/plugins/janus_sip.c Outdated Show resolved Hide resolved
src/plugins/janus_sip.c Outdated Show resolved Hide resolved
src/plugins/janus_sip.c Outdated Show resolved Hide resolved
src/plugins/janus_sip.c Outdated Show resolved Hide resolved
src/plugins/janus_sip.c Outdated Show resolved Hide resolved
src/plugins/janus_sip.c Outdated Show resolved Hide resolved
src/plugins/janus_sip.c Outdated Show resolved Hide resolved
src/plugins/janus_sip.c Outdated Show resolved Hide resolved
@lminiero lminiero added the multistream Related to Janus 1.x label Nov 16, 2023
@ywmoyue
Copy link
Contributor Author

ywmoyue commented Nov 20, 2023

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

@lminiero
Copy link
Member

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

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

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

Just tested this and it seems to be working nicely, thanks! I'm just reporting a couple of warnings my compiler gave: as soon as you can address those, this is good to merge for me.

src/plugins/janus_sip.c Outdated Show resolved Hide resolved
src/plugins/janus_sip.c Outdated Show resolved Hide resolved
@ywmoyue
Copy link
Contributor Author

ywmoyue commented Nov 20, 2023

I fixed the two errors, can you please check again

@lminiero
Copy link
Member

Thanks, merging then! 👍

@lminiero lminiero merged commit c751524 into meetecho:master Nov 20, 2023
@ycherniavskyi
Copy link
Contributor

ycherniavskyi commented Nov 28, 2023

@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
Copy link
Contributor Author

ywmoyue commented Nov 29, 2023

@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

mwalbeck pushed a commit to mwalbeck/docker-janus-gateway that referenced this pull request Dec 8, 2023
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [meetecho/janus-gateway](https://github.com/meetecho/janus-gateway) | minor | `v1.1.4` -> `v1.2.1` |

---

### Release Notes

<details>
<summary>meetecho/janus-gateway (meetecho/janus-gateway)</summary>

### [`v1.2.1`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v121---2023-12-06)

[Compare Source](meetecho/janus-gateway@v1.2.0...v1.2.1)

-   Added support for abs-capture-time RTP extension \[[PR-3161](meetecho/janus-gateway#3161)]
-   Fixed truncated label in datachannels (thanks [@&#8203;veeting](https://github.com/veeting)!) \[[PR-3265](meetecho/janus-gateway#3265)]
-   Support larger values for SDP attributes (thanks [@&#8203;petarminchev](https://github.com/petarminchev)!) \[[PR-3282](meetecho/janus-gateway#3282)]
-   Fixed rare crash in VideoRoom plugin (thanks [@&#8203;tmatth](https://github.com/tmatth)!) \[[PR-3259](meetecho/janus-gateway#3259)]
-   Don't create VideoRoom subscriptions to publisher streams with no associated codecs
-   Added suspend/resume participant API to AudioBridge \[[PR-3301](meetecho/janus-gateway#3301)]
-   Fixed rare crash in AudioBridge
-   Fixed rare crash in Streaming plugin when doing ICE restarts \[[Issue-3288](meetecho/janus-gateway#3288)]
-   Allow SIP and NoSIP plugins to bind media to a specific address (thanks [@&#8203;pawnnail](https://github.com/pawnnail)!) \[[PR-3263](meetecho/janus-gateway#3263)]
-   Removed advertised support for SIP UPDATE in SIP plugin
-   Parse RFC2833 DTMF to custom events in SIP plugin (thanks [@&#8203;ywmoyue](https://github.com/ywmoyue)!) \[[PR-3280](meetecho/janus-gateway#3280)]
-   Fixed missing Contact header in some dialogs in SIP plugin (thanks [@&#8203;ycherniavskyi](https://github.com/ycherniavskyi)!) \[[PR-3286](meetecho/janus-gateway#3286)]
-   Properly set mid when notifying about ended tracks in janus.js
-   Fixed broken restamping in janus-pp-rec
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

### [`v1.2.0`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v120---2023-08-09)

[Compare Source](meetecho/janus-gateway@v1.1.4...v1.2.0)

-   Added support for VP9/AV1 simulcast, and fixed broken AV1/SVC support \[[PR-3218](meetecho/janus-gateway#3218)]
-   Fixed RTCP out quality stats \[[PR-3228](meetecho/janus-gateway#3228)]
-   Default link quality stats to 100
-   Added support for ICE consent freshness \[[PR-3234](meetecho/janus-gateway#3234)]
-   Fixed rare race condition in VideoRoom \[[PR-3219](meetecho/janus-gateway#3219)] \[[PR-3247](meetecho/janus-gateway#3247)]
-   Use speexdsp's jitter buffer in the AudioBridge \[[PR-3233](meetecho/janus-gateway#3233)]
-   Fixed crash in Streaming plugin on mountpoints with too many streams \[[Issue-3225](meetecho/janus-gateway#3225)]
-   Support for batched configure requests in Streaming plugin (thanks [@&#8203;petarminchev](https://github.com/petarminchev)!) \[[PR-3239](meetecho/janus-gateway#3239)]
-   Fixed rare deadlock in Streamin plugin \[[PR-3250](meetecho/janus-gateway#3250)]
-   Fix simulated leave message for longer string ID rooms in TextRoom (thanks [@&#8203;adnanel](https://github.com/adnanel)!) [PR-3243](meetecho/janus-gateway#3243)]
-   Notify about count of sessions, handles and PeerConnections on a regular basis, when event handlers are enabled \[[PR-3221](meetecho/janus-gateway#3221)]
-   Fixed broken Insertable Streams for recvonly streams when answering in janus.js
-   Added background selector and blur support to Virtual Background demo
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4zNC4wIiwidXBkYXRlZEluVmVyIjoiMzcuODEuNCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9-->

Reviewed-on: https://git.walbeck.it/walbeck-it/docker-janus-gateway/pulls/122
Co-authored-by: renovate-bot <[email protected]>
Co-committed-by: renovate-bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multistream Related to Janus 1.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants