New issue
Advanced search Search tips
Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue webrtc:8908

Blocking:
issue 699846



Sign in to add a comment

WebRTC RTPSender should support "dtmf" attribute

Project Member Reported by hta@chromium.org, Feb 15 2018

Issue description

In the WebRTC spec, the RTCDTMFSender is an attribute on an audio RTPSender rather than being created with CreateDTMFSender().

We should support the spec way of doing it.

Related: We should deprecate and remove CreateDTMFSender() when the alternative is in place and the usage is low enough.

 

Comment 1 by guidou@chromium.org, Feb 15 2018

Owner: hbos@chromium.org
Status: Assigned (was: Untriaged)

Comment 2 by hta@chromium.org, Feb 20 2018

Owner: hta@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 20 2018

The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/52e58524b67a27c8bd6cdf6e66954dab996364f7

commit 52e58524b67a27c8bd6cdf6e66954dab996364f7
Author: Harald Alvestrand <hta@webrtc.org>
Date: Tue Feb 20 09:52:56 2018

Adjust DTMF min inter-tone gap to 30 ms

This brings it in line with the WEBRTC specification:
https://w3c.github.io/webrtc-pc/#dom-rtcdtmfsender-insertdtmf

Bug:  chromium:812587 
Change-Id: I705ac35cc94922f405e4951cfec813b74ed5dcab
Reviewed-on: https://webrtc-review.googlesource.com/55260
Reviewed-by: Henrik Lundin <henrik.lundin@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22096}
[modify] https://crrev.com/52e58524b67a27c8bd6cdf6e66954dab996364f7/pc/dtmfsender.cc
[modify] https://crrev.com/52e58524b67a27c8bd6cdf6e66954dab996364f7/pc/dtmfsender_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Feb 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f1bbe6226e8009c584a019ed943e1c8535197068

commit f1bbe6226e8009c584a019ed943e1c8535197068
Author: Harald Alvestrand <hta@chromium.org>
Date: Fri Feb 23 15:18:31 2018

Add the "dtmf" attribute on RTCRTPSender

This brings the API into conformance with the spec:
https://w3c.github.io/webrtc-pc/#dom-rtcdtmfsender

The CL also exposes the RTCDTMFToneChange event, which
was previously marked [ NoInterface ].

Bug:  812587 
Change-Id: I4122d1e4e336b811de234ed492f0de02dcc7a714
Reviewed-on: https://chromium-review.googlesource.com/926181
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Henrik Boström <hbos@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Commit-Queue: Harald Alvestrand <hta@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538782}
[modify] https://crrev.com/f1bbe6226e8009c584a019ed943e1c8535197068/content/browser/webrtc/webrtc_browsertest.cc
[modify] https://crrev.com/f1bbe6226e8009c584a019ed943e1c8535197068/third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCDTMFSender-helper.js
[modify] https://crrev.com/f1bbe6226e8009c584a019ed943e1c8535197068/third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCDTMFSender-insertDTMF.https-expected.txt
[modify] https://crrev.com/f1bbe6226e8009c584a019ed943e1c8535197068/third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCDTMFSender-insertDTMF.https.html
[delete] https://crrev.com/d32e0cf886e790e36aa393711861f98988e93cfa/third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCDTMFSender-ontonechange-long.https-expected.txt
[modify] https://crrev.com/f1bbe6226e8009c584a019ed943e1c8535197068/third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCDTMFSender-ontonechange.https-expected.txt
[modify] https://crrev.com/f1bbe6226e8009c584a019ed943e1c8535197068/third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCDTMFSender-ontonechange.https.html
[modify] https://crrev.com/f1bbe6226e8009c584a019ed943e1c8535197068/third_party/WebKit/LayoutTests/external/wpt/webrtc/interfaces.https-expected.txt
[add] https://crrev.com/f1bbe6226e8009c584a019ed943e1c8535197068/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-createDTMFSender.html
[modify] https://crrev.com/f1bbe6226e8009c584a019ed943e1c8535197068/third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/f1bbe6226e8009c584a019ed943e1c8535197068/third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/f1bbe6226e8009c584a019ed943e1c8535197068/third_party/WebKit/Source/modules/peerconnection/RTCDTMFSender.cpp
[modify] https://crrev.com/f1bbe6226e8009c584a019ed943e1c8535197068/third_party/WebKit/Source/modules/peerconnection/RTCDTMFToneChangeEvent.idl
[modify] https://crrev.com/f1bbe6226e8009c584a019ed943e1c8535197068/third_party/WebKit/Source/modules/peerconnection/RTCRtpSender.cpp
[modify] https://crrev.com/f1bbe6226e8009c584a019ed943e1c8535197068/third_party/WebKit/Source/modules/peerconnection/RTCRtpSender.h
[modify] https://crrev.com/f1bbe6226e8009c584a019ed943e1c8535197068/third_party/WebKit/Source/modules/peerconnection/RTCRtpSender.idl

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f3130195441d51855b9a5ca9bf591c71a47de4d6

commit f3130195441d51855b9a5ca9bf591c71a47de4d6
Author: Harald Alvestrand <hta@chromium.org>
Date: Thu Mar 01 15:03:04 2018

Add use counters for DTMF features we will deprecate

These counters measure non-standard features, except
for the "dtmf" attribute which measures the standard feature
that is replacing a non-standard feature (createDTMFSender).

Bug:  812587 
Change-Id: Iffd97a87fbc81a3c9b6f9a8afbd3c775220da1d1
Reviewed-on: https://chromium-review.googlesource.com/941802
Commit-Queue: Harald Alvestrand <hta@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540155}
[modify] https://crrev.com/f3130195441d51855b9a5ca9bf591c71a47de4d6/third_party/WebKit/Source/modules/peerconnection/RTCDTMFSender.idl
[modify] https://crrev.com/f3130195441d51855b9a5ca9bf591c71a47de4d6/third_party/WebKit/Source/modules/peerconnection/RTCRtpSender.idl
[modify] https://crrev.com/f3130195441d51855b9a5ca9bf591c71a47de4d6/third_party/WebKit/public/platform/web_feature.mojom
[modify] https://crrev.com/f3130195441d51855b9a5ca9bf591c71a47de4d6/tools/metrics/histograms/enums.xml

Status: Fixed (was: Started)
Blocking: 699846
I think you still have a nasty issue here.
In some of the adapter tests we got a failure because we only waited for iceconnectionstate to go to connected or completed before calling insertDtmf.
This threw a (very helpful) error about canInsertDTMF not being true.

Since chrome's iceconnectionstate is actually a combined ice/dtls state (aka connectionState) that should not happen.

See also https://github.com/w3c/webrtc-pc/issues/1619 -- I think canInsertDTMF needs to be updated before updating the ice connection state.
Blockedon: webrtc:8908
It was a nasty issue; I see I forgot to link to the WebRTC change I had to make in order to make this work.

See bugs.webrtc.org/8908 and the fix in https://webrtc-review.googlesource.com/c/src/+/55480

This led me to conclude that we need to implement connectionState, because it isn't the same thing as either ICE state or DTLS state.



Cc: hta@chromium.org
 Issue 674546  has been merged into this issue.
 Issue 806872  has been merged into this issue.
[bulk-edit: disregard if N/A] Can the owner please set milestone to this bug if applicable?

Sign in to add a comment