Bug 106782 - MediaStream API: Implement DTMF support in RTCPeerConnection
Summary: MediaStream API: Implement DTMF support in RTCPeerConnection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on:
Blocks: 56459
  Show dependency treegraph
 
Reported: 2013-01-14 05:06 PST by Tommy Widenflycht
Modified: 2013-02-06 04:12 PST (History)
15 users (show)

See Also:


Attachments
Patch (74.01 KB, patch)
2013-01-29 01:52 PST, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (74.75 KB, patch)
2013-01-31 02:27 PST, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (78.65 KB, patch)
2013-01-31 04:39 PST, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (just rebased) (75.10 KB, patch)
2013-02-04 02:37 PST, Tommy Widenflycht
abarth: review+
Details | Formatted Diff | Diff
Patch for landing (74.32 KB, patch)
2013-02-06 01:34 PST, Tommy Widenflycht
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tommy Widenflycht 2013-01-14 05:06:39 PST
According to the latest spec (not yet published).
Comment 1 Tommy Widenflycht 2013-01-14 06:14:03 PST
Spec: http://dev.w3.org/2011/webrtc/editor/getusermedia.html
Comment 2 Tommy Widenflycht 2013-01-14 06:15:02 PST
Ignore previous comment, wrong link.
Comment 3 Tommy Widenflycht 2013-01-29 01:52:47 PST
Created attachment 185202 [details]
Patch
Comment 4 WebKit Review Bot 2013-01-29 02:05:01 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 5 Build Bot 2013-01-29 06:03:04 PST
Comment on attachment 185202 [details]
Patch

Attachment 185202 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16151190

New failing tests:
fast/workers/worker-document-leak.html
Comment 6 Tommy Widenflycht 2013-01-29 06:11:17 PST
I really can't see how this patch could have broken fast/workers/worker-document-leak.html...
Comment 7 Adam Barth 2013-01-29 12:42:01 PST
> fast/workers/worker-document-leak.html

It's a flaky test on apple-mac.
Comment 8 Adam Barth 2013-01-29 12:44:19 PST
What is DTMF?  I guess I should read the spec to find out.
Comment 9 Tommy Widenflycht 2013-01-30 00:46:04 PST
http://en.wikipedia.org/wiki/DTMF
Dual-tone multi-frequency signaling

Bleepy tones when you press the phone keys. Really.
Comment 10 Adam Barth 2013-01-31 01:34:27 PST
Comment on attachment 185202 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185202&action=review

> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:44
> +static const long minToneDuration = 70;
> +static const long defaultToneDuration = 100;
> +static const long maxToneDuration = 6000;
> +static const long minInterToneGap = 50;
> +static const long defaultInterToneGap = 50;

Isn't "static" is redundant with "const"?

> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:121
> +    if (duration != m_duration)
> +        m_duration = duration;
> +    if (interToneGap != m_interToneGap)
> +        m_interToneGap = interToneGap;

Why not just assign unconditionally?

> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:178
> +    events.clear();

There's no need to call clear() explicitly.  The Vector destructor will do it for you.

> Source/WebCore/Modules/mediastream/RTCDTMFToneChangeEvent.idl:27
> +] interface RTCDTMFToneChangeEvent : Event {

These API names are just terrible.  Why not DTMFToneChangeEvent or ToneChangeEvent?

Also, this event should have a DOM4-style constructor.  If you take a look at some of the other events, you can see the IDL attributes you'll need.
Comment 11 Adam Barth 2013-01-31 01:37:35 PST
The code itself looks fine.  My concerns are whether we really need to support DTMF.  You mentioned that this is something that's been a long discussion in the working group.  I haven't been participating in the working group, so I don't have much understanding of that discussion.  Are there some email threads I could read to learn about the working group's decision?

Also, would it be possible to use API names that weren't so ugly?  "RTCDTMF" is just such a mouthful to have in front of every name.  I realize that's probably bike shedding, but RTCDTMFToneChangeEvent is a really ugly color for the bikeshed.  :(
Comment 12 Tommy Widenflycht 2013-01-31 01:51:21 PST
(In reply to comment #11)
> The code itself looks fine.  My concerns are whether we really need to support DTMF.  You mentioned that this is something that's been a long discussion in the working group.  I haven't been participating in the working group, so I don't have much understanding of that discussion.  Are there some email threads I could read to learn about the working group's decision?
> 
> Also, would it be possible to use API names that weren't so ugly?  "RTCDTMF" is just such a mouthful to have in front of every name.  I realize that's probably bike shedding, but RTCDTMFToneChangeEvent is a really ugly color for the bikeshed.  :(

First for the standards part: The working group has discussed this back and forth but this time everyone has agreed that a) it should be done and b) it should be done this way. Most of the discussions have taken place during phone conferences and meetings but I can probably get hta@ to get you some meeting notes if you want.

And regarding the name: yes it is a mouthful but I have used the exact same name in WebCore as in the spec. And to be frank I don't particularly want to bring up a name change to the WG since most(everyone?) just want to put this behind them...

And there are actual use cases where this is useful (apart from interop with legacy systems) and that is remote camera control with sometimes uses DTMF.
Comment 13 Tommy Widenflycht 2013-01-31 01:58:40 PST
(In reply to comment #12)
> (In reply to comment #11)
> > Also, would it be possible to use API names that weren't so ugly?  "RTCDTMF" is just such a mouthful to have in front of every name.  I realize that's probably bike shedding, but RTCDTMFToneChangeEvent is a really ugly color for the bikeshed.  :(
> And regarding the name: yes it is a mouthful but I have used the exact same name in WebCore as in the spec. And to be frank I don't particularly want to bring up a name change to the WG since most(everyone?) just want to put this behind them...

Also I forgot to mention that the webRTC WG have decided to use RTC as a prefix for all classes to minimize eventual clashes with other standards.
Comment 14 Tommy Widenflycht 2013-01-31 02:05:22 PST
Yet another reflection is that the JS developer will never have to see or type these names so it's mostly an implementation detail.
Comment 15 Tommy Widenflycht 2013-01-31 02:27:22 PST
Created attachment 185726 [details]
Patch
Comment 16 Tommy Widenflycht 2013-01-31 02:29:03 PST
Comment on attachment 185202 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185202&action=review

>> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:44
>> +static const long defaultInterToneGap = 50;
> 
> Isn't "static" is redundant with "const"?

No, static is for limiting the visible scope. A const variable is still visible in the global WebCore namespace.

>> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:121
>> +        m_interToneGap = interToneGap;
> 
> Why not just assign unconditionally?

Fixed.

>> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:178
>> +    events.clear();
> 
> There's no need to call clear() explicitly.  The Vector destructor will do it for you.

Removed.

>> Source/WebCore/Modules/mediastream/RTCDTMFToneChangeEvent.idl:27
>> +] interface RTCDTMFToneChangeEvent : Event {
> 
> These API names are just terrible.  Why not DTMFToneChangeEvent or ToneChangeEvent?
> 
> Also, this event should have a DOM4-style constructor.  If you take a look at some of the other events, you can see the IDL attributes you'll need.

Added DOM4-style constructor.
Comment 17 WebKit Review Bot 2013-01-31 03:46:04 PST
Comment on attachment 185726 [details]
Patch

Attachment 185726 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16269138
Comment 18 Peter Beverloo (cr-android ews) 2013-01-31 04:33:36 PST
Comment on attachment 185726 [details]
Patch

Attachment 185726 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16251329
Comment 19 Tommy Widenflycht 2013-01-31 04:39:53 PST
Created attachment 185749 [details]
Patch
Comment 20 James Robinson 2013-01-31 09:38:57 PST
Aren't DTMF tones something an author could easily set up with web audio and pipe in to RTC as an input?  Why hardcode support for generating fairly simple tone sequences for a somewhat obscure format into the browser?
Comment 21 Harald Alvestrand 2013-01-31 10:08:04 PST
(In reply to comment #20)
> Aren't DTMF tones something an author could easily set up with web audio and pipe in to RTC as an input?  Why hardcode support for generating fairly simple tone sequences for a somewhat obscure format into the browser?

This particular variant of DTMF tones is carried as a special codec across the wire. There are many reasons why existing SIP phones and devices that emulate phones want to send these digital signals rather than the analog version of "beeps".

RFC 4733 is the specification that people want to have support for.
If you want to take out the requirement, please go to the IETF RTCWEB WG and try to generate consensus for taking it out. It's going to be hard.
Comment 22 Adam Barth 2013-01-31 11:02:52 PST
@hta: I'm sure the working group has good reasons for wanting to include this feature.  I'd just be interested in understanding them before reviewing the patch.  It sounds like this was a contentious decision in the working group.  Is there really no written record explaining the decision?
Comment 23 Harald Alvestrand 2013-01-31 14:44:31 PST
@abarth I've been quite unhappy with the IETF WG's documentation of its decisions.

The use case that requires it is the "1-800-Fedex" use case described in draft-ietf-rtcweb-use-cases-and-requirements, section 4.3.2:

   Alice uses her web browser with a service something like Skype to be
   able to phone PSTN numbers.  Alice calls 1-800-gofedex.  Alice should
   be able to hear the initial prompts from the fedex IVR and when the
   IVR says press 1, there should be a way for Alice to navigate the
   IVR.

The experience of the people with experience in SIP gateways clearly indicated that sending "I want this tone" rather than sending the actual DTMF audio was a much preferred mechanism.

The topic wasn't really controversial - the discussion there was basically done in November 2011, according to my archives; at that time the requirement for supporting RFC 4733 (DTMF) had entered what became draft-ietf-rtcweb-audio, and nobody's tried to remove it after that thread.

In the W3C, the discussion about how the API should look like went by fits and starts from July 2011, each fit and start causing the proposal to be changed more or less drastically, with the last proposal (the one that was edited into the spec) being proposed on December 14 of 2012, and the final decision to insert it was recorded in the minutes of our Dec 13 teleconference: DTMF section of http://www.w3.org/2012/12/13-webrtc-minutes.html

Hope that clarifies....
Comment 24 Adam Barth 2013-02-01 00:06:26 PST
Thanks, that context is very helpful.

> The experience of the people with experience in SIP gateways clearly indicated that sending "I want this tone" rather than sending the actual DTMF audio was a much preferred mechanism.

From Comment #20, it sounds like this is the core of the issue that jamesr and I are seeking to understand: why is it important to have built-in DTMF support rather than using a more general mechanism to generate the tone.
Comment 25 Tommy Widenflycht 2013-02-04 02:37:11 PST
Created attachment 186330 [details]
Patch (just rebased)
Comment 26 Tommy Widenflycht 2013-02-04 03:56:40 PST
(In reply to comment #20)
> Aren't DTMF tones something an author could easily set up with web audio and pipe in to RTC as an input?  Why hardcode support for generating fairly simple tone sequences for a somewhat obscure format into the browser?

Here's what I have learned about it (but I am not an expert; just bugged some)

It's not really possible to use WebAudio to mix in the tones of the outgoing audio for several reasons:

* Codec issues: The audio goes through several filters and most of them are optimized for the human voice and can distort the DTMF tones.

* Echo cancellation: same as above

* Packet loss: we can still understand audio even if several packets are lost and silence or the last received packet is replayed but it can distort DTMF if several packes is missing and silence is played instead in the middle of a tone.

To solve all these issues the spec that Harald referenced to states that the DTMF tones are sent as a separate payload (audio, video, data and DTMF are all different payloads) which avoids most of the issues above.

The fact that the spec should have DTMF has been uncontroversial from the start; deciding how the API should look like has taken extremely long time.

If it would be possible for everyone to rewrite their backends/legacy systems with WebRTC support it would have been much, much better to use data channels for communication like this. This is MY not-so-humble opinion (and I have been vocal about it more than once) but I also realize that there is a reality out there and since everyone have signed off on the current spec we have decided to implement it now.
Comment 27 Adam Barth 2013-02-06 00:10:35 PST
From your comment, it seems clear that you want to send this information digitally.

> If it would be possible for everyone to rewrite their backends/legacy systems with WebRTC support it would have been much, much better to use data channels for communication like this. This is MY not-so-humble opinion (and I have been vocal about it more than once) but I also realize that there is a reality out there and since everyone have signed off on the current spec we have decided to implement it now.

Ok.
Comment 28 Adam Barth 2013-02-06 00:20:25 PST
Comment on attachment 186330 [details]
Patch (just rebased)

View in context: https://bugs.webkit.org/attachment.cgi?id=186330&action=review

I think the main thing that I would like to change about this patch is the names used in the API.  RTCDTMF is just a mouthful.  IMHO, the WebRTC working group has gone a bit overboard in trying to structure these names.  However, I realize that a code review isn't the right place to raise those sorts of issues and that I'm just arguing about what color to paint the bike shed.

Thanks for your patience and your explanations above.

> Source/WebCore/CMakeLists.txt:227
> +    Modules/mediastream/RTCDTMFSender.idl
> +    Modules/mediastream/RTCDTMFToneChangeEvent.idl

Can we drop the "RTC" prefix from all these names?  We already have the DTMF prefix....

> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND ANY

APPLE -> GOOGLE

> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:40
> +static const long minToneDuration = 70;

minToneDuration <-- Can you add a unit?  minToneDurationMS for example

> Source/WebCore/Modules/mediastream/RTCDTMFToneChangeEvent.h:54
> +    RTCDTMFToneChangeEvent(const String& tone);
> +    RTCDTMFToneChangeEvent(const RTCDTMFToneChangeEventInit&);

I would have marked these explicit.

> Source/WebCore/Modules/mediastream/RTCDTMFToneChangeEvent.idl:28
> +] interface RTCDTMFToneChangeEvent : Event {

I probably would have called this event just ToneChangeEvent (or at least DTMFToneChangeEvent)
Comment 29 Tommy Widenflycht 2013-02-06 01:22:33 PST
Comment on attachment 186330 [details]
Patch (just rebased)

View in context: https://bugs.webkit.org/attachment.cgi?id=186330&action=review

I rather keep the names used in the specification

>> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:13
>> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND ANY
> 
> APPLE -> GOOGLE

Done. I wish all these licences could be removed. There are lots of variants in the tree...

>> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:40
>> +static const long minToneDuration = 70;
> 
> minToneDuration <-- Can you add a unit?  minToneDurationMS for example

Good idea. Done.

>> Source/WebCore/Modules/mediastream/RTCDTMFToneChangeEvent.h:54
>> +    RTCDTMFToneChangeEvent(const RTCDTMFToneChangeEventInit&);
> 
> I would have marked these explicit.

Done.
Comment 30 Tommy Widenflycht 2013-02-06 01:23:53 PST
(In reply to comment #29)
> (From update of attachment 186330 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186330&action=review
> 
> I rather keep the names used in the specification
> 

But I'll check if there is a possibility of an official name change though.
Comment 31 Tommy Widenflycht 2013-02-06 01:34:33 PST
Created attachment 186787 [details]
Patch for landing
Comment 32 WebKit Review Bot 2013-02-06 03:58:38 PST
Comment on attachment 186787 [details]
Patch for landing

Clearing flags on attachment: 186787

Committed r141984: <http://trac.webkit.org/changeset/141984>