Skip to content

AudioBridge - PlainRTP Participant - Recognize RFC2833 DTMF and publish 'dtmf' event #3538

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

AudioBridge - PlainRTP Participant - Recognize RFC2833 DTMF and publish 'dtmf' event #3538

wants to merge 2 commits into from
+114 −2

Conversation

keremcadirci
Copy link
Contributor

This pull request adds support for handling RFC2833 DTMF signals from plain RTP participants in the AudioBridge plugin.

When a participant sends a DTMF signal using RFC2833, Janus will now emit the following event:

{
  "audiobridge": "event",
  "room": <numeric ID of the room>,
  "id": <unique ID assigned to the participant>,
  "result": {
    "event": "dtmf",
    "signal": "<DTMF signal received, values: 1-9, *, #, A-D>",
    "duration": <duration of the DTMF signal>
  }
}

To enable recognition of RFC2833 DTMF signals, the RTP payload type for DTMF must be specified using the dtmf_pt field in the rtp object of the join and/or configure request:

{
  "request": "join",
  ...
  "rtp": {
    ...
    "dtmf_pt": <RFC2833 RTP payload type used to receive DTMF signals (optional)>
  }
}

atoppi
atoppi reviewed Apr 22, 2025
View reviewed changes
Copy link
Contributor

@atoppi atoppi left a comment

Choose a reason for hiding this comment

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

Thanks for the effort!
Unfortunately the PR can not be merged in the current status. Please see my comments.

src/rtp.c Outdated
@@ -35,6 +35,13 @@ gboolean janus_is_rtp(char *buf, guint len) {
return ((header->type < 64) || (header->type >= 96));
}

gboolean janus_is_rfc2833_payload_type(int payload_type){
if( 96 > payload_type || 127 < payload_type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

style: remove extra space

src/rtp.h Outdated
@@ -154,6 +154,10 @@ int janus_videocodec_pt(janus_videocodec vcodec);
* @param[in] len Length of the buffer to inspect */
gboolean janus_is_rtp(char *buf, guint len);

/*! \brief Helper method to validate if payload type might be a RFC2833 (dtmf) between 96 and 127
* @param[in] int Payload_Type */
gboolean janus_is_rfc2833_payload_type(int payload_type);
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO the name choice (and the related comment) is unfortunate.
This is only checking that a pt is falling into the dynamic range [96, 127], whereas the current name seems to suggest a "match", like input pt is the one expected for dtmf tones.

src/plugins/janus_audiobridge.c Outdated
@@ -9586,6 +9687,10 @@ static void *janus_audiobridge_plainrtp_relay_thread(void *data) {
/* Handle as a WebRTC RTP packet */
packet.length = bytes;
janus_audiobridge_incoming_rtp(session->handle, &packet);
/* Handle rtp if rfc2833 event*/
if(janus_is_rfc2833_payload_type(header->type) && header->type==participant->plainrtp_media.dtmf_pt) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These checks seems to be already performed by janus_send_rfc2833_event ?

src/plugins/janus_audiobridge.c Outdated
if(janus_is_rfc2833_payload_type(dtmf_pt)) {
participant->plainrtp_media.dtmf_pt = dtmf_pt;
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

style: else must be in line with the closing curly bracket

src/plugins/janus_audiobridge.c Outdated
if(janus_is_rfc2833_payload_type(dtmf_pt)) {
participant->plainrtp_media.dtmf_pt = dtmf_pt;
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

style: else must be in line with the closing curly bracket

src/plugins/janus_audiobridge.c Outdated
@@ -2353,6 +2385,54 @@ static int janus_audiobridge_create_opus_encoder_if_needed(janus_audiobridge_roo
return 0;
}

static uint16_t dtmf_keys[] = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '*', '#', 'A', 'B', 'C', 'D'};
/* Check peer RTP has RFC2833 and push event */
static void janus_send_rfc2833_event(janus_audiobridge_participant *participant, char *buffer, int len) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The name must include the janus_audiobridge_ prefix

src/plugins/janus_audiobridge.c Outdated
@@ -1735,6 +1759,12 @@ static void janus_audiobridge_file_free(janus_audiobridge_file *ctx) {
}
#endif

/* In case we need to support plain RTP participants, this struct helps with that */
typedef struct janus_plainrtp_dtmf {
Copy link
Contributor

Choose a reason for hiding this comment

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

The name must include the janus_audiobridge_ prefix

src/plugins/janus_audiobridge.c Outdated
@@ -946,7 +946,8 @@ room-<unique room ID>: {
"port" : <port you want media to be sent to>,
"payload_type" : <payload type to use for RTP packets (optional; only needed in case Opus is used, automatic for G.711)>,
"audiolevel_ext" : <ID of the audiolevel RTP extension, if used (optional)>,
"fec" : <true|false, whether FEC from Janus to the user should be enabled for the Opus stream (optional; only needed in case Opus is used)>
"fec" : <true|false, whether FEC from Janus to the user should be enabled for the Opus stream (optional; only needed in case Opus is used)>,
"dtmf_pt" : <RFC2833 RTP payload type that dtmf signal will be received (optional)>
Copy link
Contributor

@atoppi atoppi Apr 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Please rephrase.
  2. Add dtmf_pt to rtp_parameters struct.

lminiero
lminiero reviewed Apr 22, 2025
View reviewed changes
src/plugins/janus_audiobridge.c Outdated
@@ -7292,6 +7381,18 @@ static void *janus_audiobridge_handler(void *data) {
const char *ip = json_string_value(json_object_get(rtp, "ip"));
uint16_t port = json_integer_value(json_object_get(rtp, "port"));
janus_mutex_lock(&participant->pmutex);
/* rfc2833 payload type is set */
json_t *dtmf_pt_json =json_object_get(rtp, "dtmf_pt");
Copy link
Member

Choose a reason for hiding this comment

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

Besides what Alessandro already said, all new API properties must be validated before they can be accessed. See the way "ip" and "port" are validated right now in the code, in the related parameters structure, and add the new property there too.

I also see a missing space before json_object_get (code style).

keremcadirci added a commit to keremcadirci/janus-gateway that referenced this pull request Apr 22, 2025
@keremcadirci
Audiobridge, rfc2733 dtmf event updates - meetecho#3538
a20268d
keremcadirci added a commit to keremcadirci/janus-gateway that referenced this pull request Apr 22, 2025
@keremcadirci
Merge pull request #19 from keremcadirci/AudioBridge-PlainRTP_RFC2833…
bec4b7c 
…_to_DTMF_Event2

Audiobridge, rfc2733 dtmf event updates - meetecho#3538
keremcadirci added a commit to keremcadirci/janus-gateway that referenced this pull request Apr 22, 2025
@keremcadirci
Audiobridge, rfc2733 dtmf event meetecho#3538 (comment)
cbd3e78
keremcadirci added a commit to keremcadirci/janus-gateway that referenced this pull request Apr 22, 2025
@keremcadirci
Audiobridge, rfc2733 dtmf event meetecho#3538 (comment)
54cab78
keremcadirci added a commit to keremcadirci/janus-gateway that referenced this pull request Apr 22, 2025
@keremcadirci
Audiobridge, rfc2733 dtmf event meetecho#3538 (comment)
14e6373
keremcadirci added a commit to keremcadirci/janus-gateway that referenced this pull request Apr 22, 2025
@keremcadirci
Revert "Audiobridge, rfc2733 dtmf event updates - meetecho#3538"
89b9042 
This reverts commit a20268d.
atoppi
atoppi reviewed May 29, 2025
View reviewed changes
src/plugins/janus_audiobridge.c
@@ -974,6 +975,17 @@ room-<unique room ID>: {
* If you're interested in supporting scenarios where the AudioBridge
* "dials out" (e.g., for outgoing INVITES to SIP endpoints) check the
* \ref aboffer section.
* if a maching rtp with rfc2833 payload type is received the fallowing dtmf event is published.
Copy link
Contributor

Choose a reason for hiding this comment

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

typos: start with a capital letter, maching, rtp (packet), fallowing

src/plugins/janus_audiobridge.c
"payload_type" : <payload type to use for RTP packets (optional; only needed in case Opus is used, automatic for G.711)>,
"audiolevel_ext" : <ID of the audiolevel RTP extension, if used (optional)>,
"fec" : <true|false, whether FEC should be enabled for the Opus stream (optional; only needed in case Opus is used)>,
"dtmf_pt": <RFC2833 RTP payload type that dtmf signal will be received (optional)>
Copy link
Contributor

Choose a reason for hiding this comment

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

please rephrase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atoppi atoppi atoppi left review comments

@lminiero lminiero lminiero left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@keremcadirci @atoppi @lminiero