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

Conversation

@keremcadirci
Copy link
Contributor

@keremcadirci keremcadirci commented Apr 17, 2025

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
}

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

@atoppi atoppi Apr 22, 2025

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

/*! \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

@atoppi atoppi Apr 22, 2025

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

@atoppi atoppi Apr 22, 2025

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

@atoppi atoppi Apr 22, 2025

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

@atoppi atoppi Apr 22, 2025

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

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

@atoppi atoppi Apr 22, 2025

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
#endif

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

@atoppi atoppi Apr 22, 2025

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
"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
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

@lminiero lminiero Apr 22, 2025

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
* 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

@atoppi atoppi May 29, 2025

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

@atoppi atoppi May 29, 2025

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

@lminiero lminiero lminiero left review comments

+1 more reviewer

@atoppi atoppi atoppi left review comments

Reviewers whose approvals may not affect merge requirements

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