Opened 9 years ago

Last modified 4 years ago

#12617 new patch

Patch to add out-of-band DTMF support

Reported by: BigBrownChunx Owned by: darkrain42
Milestone: Patches Needing Review Component: libpurple
Version: 2.7.3 Keywords: dtmf media
Cc: EionRobb

Description

libpurple currently has no way to send out-of-band DTMF codes in media streams. This patch adds a new function to media.h and adds another blank function for prpls to fill in. A patch for XEP-0181 (Jingle DTMF) is on its way.

Attachments (10)

jabber_dtmf.2.patch (2.9 KB) - added by johntyree 9 years ago.
Fixed some typos and missing declarations.
media_dtmf.patch (5.6 KB) - added by BigBrownChunx 9 years ago.
Update to libpurple patch to add farsight2 backend support and volume/duration support
jabber_dtmf.patch (3.0 KB) - added by BigBrownChunx 9 years ago.
Update to fix typos and to add support for duration/volume
media_dtmf.2.patch (24.1 KB) - added by dwmw2 6 years ago.
Updated core patch
dtmf.patch (22.8 KB) - added by buzz 6 years ago.
Modified DTMF patch - create buttons in loop and fixes memory leak
dtmf-pad-with-key.patch (23.6 KB) - added by buzz 6 years ago.
DTMF pad + keyboard event handling
dtmf-pad-with-key-2.patch (21.7 KB) - added by dwmw2 6 years ago.
Updated patch to move 'PurpleMedia?' forward declaration earlier in media.h. See http://pidgin.im/pipermail/devel/2013-November/023237.html
0001-Add-out-of-band-DTMF-support-and-dialpad-to-use-it.patch (23.0 KB) - added by dwmw2 4 years ago.
Updated patch against trunk to add translations
pidgin2-dtmf.patch (10.3 KB) - added by dwmw2 4 years ago.
Minimal version of patch for 2.x branch
jabber_dtmf_2.patch (3.8 KB) - added by dwmw2 4 years ago.
Updated Jabber patch

Download all attachments as: .zip

Change History (33)

comment:1 Changed 9 years ago by darkrain42

The XEP-0181 spec allows for duration and volume -- those should probably also be included.

comment:2 Changed 9 years ago by BigBrownChunx

I suppose so. If the prpl can't handle duration/volume, then it can just ignore those values. I kind of think that this is a complexity that should be hidden away though.

comment:3 Changed 9 years ago by darkrain42

I don't think those two options increase the complexity too much (although I'm not sure what the point of exposing them in Jingle is; I'm also not really knowledgeable about it at all).

comment:4 Changed 9 years ago by BigBrownChunx

Should this be something that needs to be exposed by the prpl, but isn't exposed to the UI by libpurple, so that libpurple can store those details per-account and pass them to the prpl as needed?

comment:5 Changed 9 years ago by darkrain42

Errm, no (although I think I'd be okay with defaults, either globally or specified by the prpls). I can imagine these needing to vary from one tone to the next within one stream

comment:6 Changed 9 years ago by BigBrownChunx

Might also need to look at rfc2833 support... and possibly in-band DTMF in the UI somehow.

comment:7 Changed 9 years ago by rekkanoryo

  • Milestone set to Patches Needing Review
  • Owner set to darkrain42

Changed 9 years ago by johntyree

Fixed some typos and missing declarations.

comment:8 Changed 9 years ago by johntyree

jabber_dtmf.patch wasn't building for me. Looked like it had some mistakes. I made some educated guesses and came up with something that builds at least. Still can't really test it, of course.

Changed 9 years ago by BigBrownChunx

Update to libpurple patch to add farsight2 backend support and volume/duration support

Changed 9 years ago by BigBrownChunx

Update to fix typos and to add support for duration/volume

comment:9 Changed 9 years ago by BigBrownChunx

The new media_dtmf.patch should handle in-band/RFC4733/RFC2833 support too :)

comment:10 Changed 9 years ago by johntyree

How are you testing these with no UI? I tried hijacking the 'Hold' button to send a DTMF '1', but segfaulted :D I assume I did it incorrectly.

comment:11 Changed 9 years ago by johntyree

Also, unless I'm crazy, these still have typos like 'xmlnod' in them. No?

comment:12 Changed 9 years ago by darkrain42

Yeah, please to be fixing typos and making sure patches compile :)

comment:13 Changed 9 years ago by darkrain42

  • Milestone changed from Patches Needing Review to Patches Needing Improvement

BigBrownChunx?: Shouldn't purple_media_send_dtmf validate the character is a specific DTMF tone?

Please also document in the .h that they're sent in a protocol-specific way if the protocol has it, or as in-band if the media backend supports it. I'm also not sure that if the prpl returns FALSE, we should really fall back to in-band (I'm open to being convinced)

The newer Farsight code looks nice :)

comment:14 Changed 9 years ago by darkrain42

BigBrownChunx?, another update after banging my head against hiptobecubic's backtraces in #pidgin:

This code crashes when the media session is a GoogleSession, not JingleSession (i.e. in the expected use case of Google Voice). Two changes needed:

  • Only send XEP-0181 DTMF if the other side advertises support for the feature (see the spec)
  • Have a jabber function (the one actually set for the prpl) which checks if this is a JingleSession or GoogleSession (or something that otherwise keeps jingle_rtp_send_dtmf from trying to dereference the wrong type of variable).

Type safety in jingle_rtp_send_dtmf (g_return_if_fail(JINGLE_IS_SESSION(...)) probably also good )

Changed 6 years ago by dwmw2

Updated core patch

comment:15 Changed 6 years ago by dwmw2

I've just attached media_dtmf.2.patch which fixes various minor issues and makes it build, and also incorporates a UI patch to make it possible to dial DTMF digits while in a call.

I've tested this with PSTN calls made via Lync with the pidgin-sipe plugin, and it seems to work correctly.

Patch against release-2.x.y branch.

I don't care about jabber so I haven't looked at that part.

Last edited 6 years ago by dwmw2 (previous) (diff)

comment:16 Changed 6 years ago by dwmw2

I also wondered if we should have separate calls for start/stop instead of just a duration. The original UI patch from Jan-Michael Brummer did that, with functions for press/release of the buttons which would start/stop the DTMF directly:

+phone_dtmf_pressed_cb(GtkButton *button, gpointer user_data)
+{
+	PurpleMediaManager *manager = purple_media_manager_get();
+	GstElement *pipeline;
+
+	pipeline = purple_media_manager_get_pipeline(manager);
+
+	if (pipeline) {
+		GstStructure *structure;
+		GstEvent *event;
+		gint num = GPOINTER_TO_INT(user_data);
+
+		structure = gst_structure_new("dtmf-event",
+						"type", G_TYPE_INT, 1,
+						"number", G_TYPE_INT, num,
+						"volume", G_TYPE_INT, 25,
+						"start", G_TYPE_BOOLEAN, TRUE, NULL);
+
+		event = gst_event_new_custom(GST_EVENT_CUSTOM_UPSTREAM, structure);
+		gst_element_send_event(pipeline, event);
+	}
+}

comment:17 Changed 6 years ago by dwmw2

Note: The UI patch I used has now been filed against ticket 15575. Perhaps the generic part of this feature request should continue there and just the Jabber support remain here? cf. https://developer.pidgin.im/ticket/15575#comment:4

Changed 6 years ago by buzz

Modified DTMF patch - create buttons in loop and fixes memory leak

Changed 6 years ago by buzz

DTMF pad + keyboard event handling

Changed 6 years ago by dwmw2

Updated patch to move 'PurpleMedia?' forward declaration earlier in media.h. See http://pidgin.im/pipermail/devel/2013-November/023237.html

Changed 4 years ago by dwmw2

Updated patch against trunk to add translations

comment:18 Changed 4 years ago by Robby

  • Milestone changed from Patches Needing Improvement to Patches Needing Review

Changed 4 years ago by dwmw2

Minimal version of patch for 2.x branch

comment:19 Changed 4 years ago by dwmw2

As discussed on IRC yesterday, I've created a minimal version of the patch for the 2.x branch. It doesn't extend the prpl structure so protocols can't provide their own (not that we'd actually got anyone to test the XMPP implementation yet). But it *does* work for the case where the media backend provides the functionality, which is what we have with SIPE+FS2.

comment:20 Changed 4 years ago by elb

Comments on attachment:pidgin2-dtmf.patch:

I would like to see purple_media_send_dtmf() changed to take at least a 16-bit duration allowing a duration of up to 1 second. 256 ms is fairly short for some (particularly radio) applications. I would also like to see it ensure that the incoming characters are in [0-9A-D#*], since it specifies such in its documentation (as it should).

The invocation of dgettext in pidgin should be _(), I think. We don't call dgettext explicitly in Pidgin.

This patch otherwise looks suitable for inclusion to me.

comment:21 Changed 4 years ago by David Woodhouse <David.Woodhouse@…>

(In [e4c122196b08]):
Add out-of-band DTMF support and dialpad to use it

This resolves the non-Jabber-specific part of ticket 12617, by adding purple_media_send_dtmf(), and a corresponding method to the PurpleMediaBackendIface? and the farstream implementation thereof.

Fixes #15575 Refs #12617

comment:22 Changed 4 years ago by David Woodhouse <David.Woodhouse@…>

(In [723b8ecf31cd]):
Add media_send_dtmf() to prpl

This allows for prpls to have their own protocol-specific method for sending DTMF.

Refs #12617

Changed 4 years ago by dwmw2

Updated Jabber patch

comment:23 Changed 4 years ago by dwmw2

I've attached a new Jabber patch which at least *builds* against the current trunk. I am unable to test it, and the original had so many errors I'm not convinced it was ever even *built*, let alone tested.

I note at least that it doesn't check for the feature having been advertised in a Service Discovery, in accordance with §3 ("Determining Support") of XEP-0181, and doesn't check for any of the responses described in Examples 2-5 in §2.

I do not recommend applying this patch as-is, but I've updated it for posterity now that the base support has gone into trunk.

Last edited 4 years ago by dwmw2 (previous) (diff)
Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!