-
Notifications
You must be signed in to change notification settings - Fork 42
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.
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
DTMF Support #30
Comments
[Peter Thatcher] I like this idea, and I like the idea of changing the reference to track But would [Constructor(RTCRtpSender)] And can we call it DtmfSender? We have RtpSender and IceTransport and |
On 6 Feb 2014, at 21:14, aboba notifications@github.com wrote:
Just because it needs saying..... We don't actually have to add DTMF here. There are other ways to do it that T. |
I'm not sure that I agree @steely-glint. There's this uncomfortable middle ground where devices don't pull DTMF tones off the audio stream because they are RTP devices that support the DTMF "codec". What you propose would leave them in the rough. |
On 7 Feb 2014, at 17:23, Martin Thomson notifications@github.com wrote:
You really have one of those that can't do SIP info or inband tones but can do DTLS and full ice ? T. |
On Fri, Feb 7, 2014 at 12:34 PM, steely-glint notifications@github.comwrote:
Second, in-the-audio-stream DTMF has an issue in the presence of packet Finally, ORTC is supposed to allow implementation of current WebRTC spec on Bottom line, DTMF is a legacy feature, but while we need to interact with Roman Shpount |
I don't know, this is an old argument that I'm not sure I care for. In the grand scheme of things, I consider intermediation costs to be progressively higher for ICE, then DTLS, then codec stuff. And for some reason, there's a big gap in my mind between the last two. For instance, the former two don't add extra delay. But there's probably a good dollop of prejudice influencing that. |
On 7 Feb 2014, at 17:38, rshpount notifications@github.com wrote:
I understand and respect that POV. However it may turn out to be an excessive Tim. |
Please review the fixes relating to this issue in the latest ORTC Editor's draft: If there are any remaining concerns, please post these to the public-orca@w3.org mailing list. |
From: Justin Uberti juberti@google.com
To: public-orca@w3.org
Date: Thu, 6 Feb 2014 11:21:21 -0800
URL: http://lists.w3.org/Archives/Public/public-orca/2014Feb/0014.html
One thing absent from the current API is support for DTMF. Yes, it's a
relic from an earlier age, but unfortunately it's not something we can
ignore.
Fortunately, the 1.0 spec did a nice job factoring DTMF functionality out
into the RTCDTMFSender http://www.w3.org/TR/webrtc/#rtcdtmfsender object.
After calling PeerConnection.createDTMFSender(track), a RTCDTMFSender is
created and attached to |track|. This object then allows DTMF to be
injected into the audio stream for the specified track. Visually, it looks
like this (in 1.0):
MediaStreamTrack --> |Doohickey | |
| ^ |
| | |
|RTCDTMFSender| |
| |
|---------------------------|
So, we should be able to reuse this whole object in ORTC. All we need to do
is find a way to create a RTCDTMFSender for a given track, which we can do
just by adding a getDTMFSender() API to RTCRtpSender, and we end up with a
model like:
MediaStreamTrack --> RTCRtpSender ---> Transports
^
|
RTCDTMFSender
The API delta then becomes:
partial interface RTCRtpSender {
// gets/creates the RTCDTMFSender object for the current RTCRtpSender
RTCDTMFSender getDTMFSender();
}
and this taken from 1.0:
[NoInterfaceObject]
interface RTCDTMFSender {
readonly attribute boolean canInsertDTMF;
void insertDTMF (DOMString tones, optional long duration,
optional long interToneGap);
readonly attribute MediaStreamTrack track;
attribute EventHandler ontonechange;
readonly attribute DOMString toneBuffer;
readonly attribute long duration;
readonly attribute long interToneGap;
};
The only other edit we might want to make is to replace the |track|
reference with a reference to the RTCRtpSender the RTCDTMFSender is
attached to.
The text was updated successfully, but these errors were encountered: