Bug 106782

Spec: http://dev.w3.org/2011/webrtc/editor/getusermedia.html

Ignore previous comment, wrong link.

Created attachment 185202 [details] Patch

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

I really can't see how this patch could have broken fast/workers/worker-document-leak.html...

> fast/workers/worker-document-leak.html It's a flaky test on apple-mac.

What is DTMF? I guess I should read the spec to find out.

http://en.wikipedia.org/wiki/DTMF Dual-tone multi-frequency signaling Bleepy tones when you press the phone keys. Really.

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.

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. :(

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

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

Yet another reflection is that the JS developer will never have to see or type these names so it's mostly an implementation detail.

Created attachment 185726 [details] Patch

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 on attachment 185726 [details] Patch Attachment 185726 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16269138

Comment on attachment 185726 [details] Patch Attachment 185726 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16251329

Created attachment 185749 [details] Patch

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?

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

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

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

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.

Created attachment 186330 [details] Patch (just rebased)

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

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

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

Created attachment 186787 [details] Patch for landing

Comment on attachment 186787 [details] Patch for landing Clearing flags on attachment: 186787 Committed r141984: <http://trac.webkit.org/changeset/141984>