Hi Jack.
I think it would be better if you provided a patch created with "git format-patch", so your authorship could be preserved ;-)

Created attachment 36104 [details] [review]
patch

Done - http://www.sfu.ca/~jdbates/tmp/telepathy/201006070/0001-Implement-TP_IFACE_CHANNEL_INTERFACE_DTMF-so-Empathy.patch

Thanks Pablo!

*** Bug 28242 has been marked as a duplicate of this bug. ***

Review of attachment 36104 [details] [review]:

Thanks for the patch; generally looks good! Does it work with the Empathy dialpad? A couple of small things.

For starters, git commit messages are normally formatted with a shortish (less than 60 characters) "subject", followed by a blank line, and then the body of the message. So maybe something like:

  Implement Channel.Interface.DTMF

  This allows the Empathy dialpad to be used when calling Asterisk with Jingle. Fixes https://bugs.freedesktop.org/show_bug.cgi?id=28413

A smoke test would be nice. I would cargo-cult from tests/twisted/jingle/call-state.py, tearing out the CallState-specific stuff, and checking:

• the channel has the DTMF interface (where the test currently looks for cs.CHANNEL_IFACE_CALL_STATE in chan_props['Interfaces'], but probably using the assertContains() helper function from servicetest, which didn't exist when this test was written);
• calling StartTones() and StopTones() with invalid stream IDs returns an error;
• calling them with valid IDs causes the signals to be emitted on the StreamHandler.

(Actually, in recent spec. versions the semantics of the DTMF interface changed slightly to say that DTMF events should be sent on all audio streams — see http://telepathy.freedesktop.org/spec/org.freedesktop.Telepathy.Channel.Interface.DTMF.html#org.freedesktop.Telepathy.Channel.Interface.DTMF.StartTone — so if you were feeling keen you could change the implementation to do that.)

::: src/media-stream.c
@@ +1852,3 @@
+gabble_media_stream_start_telephony_event (GabbleMediaStream *self, guchar event)
+{
+  DEBUG ("called");

These DEBUG messages aren't particularly helpful unless they say which stream and event they apply to. (Yes, Gabble is riddled with such not-enormously-helpful debug messages. ☺)

I've picked this up and brought it up to date with current telepathy-spec. See "URL" field for my branch.

Recent versions of telepathy-spec add a MultipleTones method, which can be used to send a pre-composed string of DTMF, so I ended up writing a small class to "play" DTMF strings. This should really be in telepathy-glib, for which I'll clone a bug.

Things in my branch I'm unsure about:

• It might try to send DTMF before a stream is connected, which I won't work
• The timings in media-channel.c (stop StartTone tones after at most 10 seconds; send MultipleTones with 200ms tones and 100ms gaps between them) are entirely arbitrary, and someone who knows more about telephony might be able to suggest better numbers

(In reply to comment #4)
> For starters, git commit messages are normally formatted with a shortish (less
> than 60 characters) "subject", followed by a blank line, and then the body of
> the message.

I've re-worded the commit message slightly to fit the "subject"/"body" pattern, I hope you don't mind!

> A smoke test would be nice.

Added.

> (Actually, in recent spec. versions the semantics of the DTMF interface changed
> slightly to say that DTMF events should be sent on all audio streams — see
> http://telepathy.freedesktop.org/spec/org.freedesktop.Telepathy.Channel.Interface.DTMF.html#org.freedesktop.Telepathy.Channel.Interface.DTMF.StartTone
> — so if you were feeling keen you could change the implementation to do that.)

I did indeed feel keen.

> These DEBUG messages aren't particularly helpful unless they say which stream
> and event they apply to

... which they now do.

12:06 < pessi> smcv: 'p' and perhaps ','  for 3 second pause?
12:07 < smcv> pessi: both, with the same semantics, or what?
12:08 < pessi> for sending dtmfs, yes
12:08 < barisione> smcv, pessi: I'm not entirely sure what you are talking 
                   about, but there are other things, like X and W
12:09 < smcv> pessi: is it always always 3 seconds, or should the CM be able to 
              override?
2:10 < pessi> ',' as first pause means "send rest as dtmf when call is 
               alerting" and "p" (or X) send as dtmf when call was connected 
12:10 < smcv> pessi: and indeed can you offer any opinion on the beep lengths 
              that Gabble uses? it'scurrently a 200ms tone and a 100ms gap
12:11 < pessi> and "W" means that stop here and let user decide when to send 
               rest of the string
12:11 < smcv> pessi: so for Telepathy, I don't think "wait" really makes sense
12:11 < pessi> smcv: we use same values
12:12 < smcv> wow, my randomly guessed values were right!
12:12 < pessi> smcv: it is more an ui issue
12:12 < smcv> pessi: so the plan I had for initial DTMF was:
12:12 < pessi> but there is a stupid 3gpp feature called FDN where you have to 
               tell modem in advance which DTMF digits you plan to send
12:13 < smcv> we alter telepathy-glib so it keeps the TargetID in the mapping 
              given to CMs
12:13 < smcv> more or less all CMs ignore it
12:13 < smcv> but telephony CMs can look at it and chop it up into phone number 
              and initial DTMF blob if they want to
12:13 < sjoerd> pessi: so you tell the modem what you plan to tell it, and 
                later on you tell it, now actually send these ?
12:13 < smcv> e.g.
12:14 < pessi> sjoerd: yes
12:14 < smcv> if you want to call +44123456 and send ##ABCD, you'd dial 
              "+44123456p##ABCD" right?
12:14 < sjoerd> pessi: awesome...
12:15 < smcv> then our idea was that -ring would look at the TargetID and go 
              "there's more here than I thought there would be", and treat it 
              as if it had been TargetID = +4412346, InitialTones ##ABCD
12:15 < pessi> sjoerd: it has added benefit of including all those DDTMF things 
               in your call history 
12:15 < smcv> (I haven't implemented InitialTones in Gabble yet either)
12:15 < smcv> then the TpDTMFPlayer would only have to play the ##ABCD part I 
              think?
12:16 < smcv> does that sound workable, and if we do that, do we still need p, 
              w, comma?
12:16 < pessi> smcv: yep, that sounds ok
12:16 < smcv> indeed, are there any other things we might need? barisione 
              mentioned X?
12:17 < pessi> X is sip thing, it means same as p, send dtmf digits once call 
               is connected
12:17 < pessi> it is never in the InitialTones part
12:17 < smcv> so P and X are synonyms, and mean "wait for call to connect, then 
              play"
12:17 < smcv> and the TpDTMFPlayer should never need to see them
12:17 < pessi> ..but later p means pause
12:18 < smcv> ugh, ok
12:18 < pessi> ;)
12:18 < smcv> so the first p (or x) is the break point between the phone number 
              and the InitialTones
12:18 < pessi> or ","
12:18 < pessi> or "w"
12:19 < smcv> pessi: should we just treat any member of [pxwPXW,] as "sleep 3"?
12:20 < smcv> pessi: (and, implementation detail, ring also uses them to cut 
              the InitialTones off the TargetID, but when it does that it 
              should delete the separator)
12:23 < pessi> smcv: 'w' should probably send something to ui, and ui should 
               decide when to continue
12:23 < pessi> but for [pPxX,] yes
12:23 < smcv> pessi: W doesn't sound like it should be in Telepathy at all tbh
12:24 < smcv> pessi: oh, except you want to be able to pass it through the 
              TargetID?
12:24 < pessi> yep
12:25 < smcv> ugh, in that case we need a TonesWaiting(s) signal and some way 
              to resume
12:25 < smcv> I'd be inclined to say it goes like this:
12:25 < smcv> suppose InitialTones (or tail of TargetID) is 1234w5678w90
12:26 < smcv> when the call connects, the DTMF beeper sends 1234
12:26 < smcv> then emits TonesWaiting("5678w90")
12:26 < smcv> at this point the DTMF player is idle
12:27 < pessi> smcv: sounds fine this far
12:27 < smcv> the UI can either ignore the signal, or respect the signal by 
              (waiting for the user and) calling SendMultipleTones("5678w90")
12:27 < smcv> at that point the CM plays 5678 and emits TonesWaiting("90")
12:27 < smcv> and the UI can call SendMultipleTones("90") to finish the 
              sequence off?
12:27 < smcv> this is much more complicated than I hoped it'd be
12:28 < smcv> let me note that on the bug
12:28 < Robot101> smcv: is that really easier than just saying -> 
                  TonesWaiting("true") <- CarryOn() ...?
12:28 < smcv> Robot101: it reduces it to an already-solved problem? :-)
12:28 < smcv> the UI can either play the suggested tones or do something else
12:28 < smcv> oh, bah, state recovery though
12:29 < smcv> if the UI doesn't exist yet, it'll miss the signal
12:29 < smcv> the other possibility would be that the DMTF player holds more 
              state (the fact that it has a "w" string ready to go)
12:30 < smcv> and calling SendMultipleTones or StartTone will fail, until you 
              have either called StopTone to cancel the "w" string, or called 
              CarryOn
12:33 < pessi> smcv: I like the TonesWaiting signal (and property?) better..
12:33 < smcv> pessi: yeah it'll have to have a mutable property :-(

(In reply to comment #5)
> I've picked this up and brought it up to date with current telepathy-spec. See
> "URL" field for my branch.

I now also have a subset of that branch, <http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=shortlog;h=refs/heads/dtmf-from-the-past>, which doesn't implement MultipleTones. We could use that branch to get basic DTMF support merged without deciding on MultipleTones' exact semantics.

That subset doesn't have a regression test, but I could backport the one from smcv/dtmf (with the MultipleTones-related bits deleted) if required.

> • It might try to send DTMF before a stream is connected, which won't work

Still unresolved, but not very important for StartTone/StopTone - it becomes more signficant when we implement InitialTones.

> • The timings in media-channel.c (stop StartTone tones after at most 10
> seconds; send MultipleTones with 200ms tones and 100ms gaps between them) are
> entirely arbitrary

Pekka seems to think 200ms and 100ms are entirely reasonable, so let's keep them.

You don't have to pause between DTMF events when sending them to farsight2. Farsight2 will do the queuing internally and guarantee the minimal tone duration and inter-tone intervals to make sure they are recognizable on the other end. So you can just do Start(),Stop(),Start(),Stop() as fast as dbus can transmit them.

(In reply to comment #7)
> I now also have a subset of that branch,
> <http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=shortlog;h=refs/heads/dtmf-from-the-past>,
> which doesn't implement MultipleTones. We could use that branch to get basic
> DTMF support merged without deciding on MultipleTones' exact semantics.

Your branch looks good. Just two things:

 * The spec says InvalidArgument is only used for an invalid stream ID
   but now that's deprecated. However, surely it can be used for a bad
   event type?

 * When the channel is closed and events are currently being sent then
   StoppedTones(False) should be fired.

> That subset doesn't have a regression test, but I could backport the one from
> smcv/dtmf (with the MultipleTones-related bits deleted) if required.

I haven't looked at your dtmf branch yet, but yeah a test would be nice.

Now some comments on your dtmf branch:

 * gabble_dtmf_player_play has loads of g_return_val_if_fail to check for bad
   arguments, but doesn't check whether self is what it should be, so you could
   easily pass NULL into it, etc.

 * Just a nitpick, but the sig_id_started_tone IDs are fine, but just different
   from (I believe) everywhere in gabble and tp-glib. Why didn't you do the
   standard thing of creating a signals array and enum?

The rest of the branch looks totally fine, and I can't see why GabbleDTMFPlayer can't become TpDTMFPlayer.

Also, it turns out you fix all my comments about your dtmf-from-the-past branch I gave above in your dtmf branch, so there's no point in wasting time and trying to fix them.

I have already ++'d your spec branch, so I say merge that, and merge your dtmf branch. It's up to you what you want to do about the new spec stuff -- either merge your dtmf branch now and leave the commented out parts commented out, or wait until a new spec release, then new tp-glib release, and uncomment them out before merging the dtmf branch.

Way to go!

I just implemented DTMF in Call on top of your dtmf branch using the DTMF sender. Check out bug #31051 for more details.

*** Bug 31051 has been marked as a duplicate of this bug. ***

(In reply to comment #8)
> You don't have to pause between DTMF events when sending them to farsight2.
> Farsight2 will do the queuing internally and guarantee the minimal tone
> duration and inter-tone intervals to make sure they are recognizable on the
> other end. So you can just do Start(),Stop(),Start(),Stop() as fast as dbus can
> transmit them.

I'm sure I responded to this already, but something must have eaten it. Anyway:

If we rely on Farsight to do this, then we can't emit CurrentlySendingTones(FALSE) correctly, unless we invent a way for Farsight to feed back "please wait" and "OK, finished now"; we also can't implement the [pPxX,] functionality (3 second pause).

What timings does Farsight use? If Gabble uses 200ms tones, 100ms gaps, and 3s pauses when a pause is explicitly requested, will Farsight interfere?

(I've merged Jonny's call-dtmf branch into my dtmf branch, and will merge it to master at the same time.)

(In reply to comment #10)
>  * gabble_dtmf_player_play has loads of g_return_val_if_fail to check for bad
>    arguments, but doesn't check whether self is what it should be, so you could
>    easily pass NULL into it, etc.

Fair enough, I'll poke at that.

>  * Just a nitpick, but the sig_id_started_tone IDs are fine, but just different
>    from (I believe) everywhere in gabble and tp-glib. Why didn't you do the
>    standard thing of creating a signals array and enum?

No particularly compelling reason; I suspect the signal pseudo-ID enum is mostly cargo-culted from the property ID enum, which is made necessary by GObject's API (signal IDs are provided by GObject, whereas property IDs are provided by each class with properties, hence the discrepancy), so I'm trying out a different approach.

I'm not sure that having the signal pseudo-ID being an index into an array containing the *actual* IDs gains us any clarity - it just means we have an extra sort of small integer involved - but for some reason it's become conventional, and if you'd prefer consistency here, I can easily do that.

> I have already ++'d your spec branch, so I say merge that, and merge your dtmf
> branch. It's up to you what you want to do about the new spec stuff

I'm inclined to merge dtmf now, then uncomment the new stuff after the next spec/tp-glib release (which I want to do pretty soon anyway, so they might both land in the same Gabble release, in practice). I might also delete GabbleDTMFPlayer in favour of TpDTMFPlayer at that point.

(In reply to comment #14)
[snip]
> and if you'd prefer consistency here, I can easily do that.

Meh, if you feel that strongly about it.

> I'm inclined to merge dtmf now, then uncomment the new stuff after the next
> spec/tp-glib release (which I want to do pretty soon anyway, so they might both
> land in the same Gabble release, in practice). I might also delete
> GabbleDTMFPlayer in favour of TpDTMFPlayer at that point.

Cool, knock yourself out.

I'm going to change this to be in terms of TpDTMFPlayer and implement TonesDelayed immediately.

Branch updated, with two new commits that port it to the latest telepathy-glib.

Wowzer, gogogo!

Fixed in git for 0.11.0. Thanks, everyone!