Closed Bug 1291715 Opened 8 years ago Closed 8 years ago

Enable voice/audio support for DTMF in WebRTC

Categories

(Core :: WebRTC: Audio/Video, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: dminor, Assigned: dminor)

References

Details

Attachments

(7 files, 1 obsolete file)

This bug covers enabling support for DTMF in the WebRTC code base, outside of any signaling changes required, which are covered by Bug 1291714.
Rank: 15
I just like to note that the work around in https://hacks.mozilla.org/2015/11/webrtc-sending-dtmf-in-firefox/ works some of the time but does not work with gateways that don't do inband DTMF which we see more and more of. It also some times ends up with double press events but that is a bug with the gateways that do inband DTMF not a problem with the work around or FF. If you are trying to build a softphone with WebRTC, you pretty much need DTMF to be able to work with any IVR system and the inband approach that the work around uses does not reliably work.
Thanks Cullen - For the reasons you cite I've prioritized this as a Q3 goal, which means I expect it to land in Nightly by the end of September.
Looks like the first step is adding DOM support for RTCDTMFSender in RTCRtpSender.
Status: NEW → ASSIGNED
This patches the webrtc.org dtmf sample to make use of the rtcSender.dtmf object instead of using createDTMFSender.
I've posted a WIP to MozReview. Lots of work to do, but this is sufficient to generate tones on the (modified) webrtc.org sample. Feedback welcome :)
My plan is to get the RTCDTMFToneChange event working and then circle back and make things work properly for concurrent senders.
Events are working. Next step is to handle concurrent senders properly.
I've pushed the webrtc sample changes to: https://github.com/dminor/webrtc-samples/tree/update-dtmf-sample
Comment on attachment 8785439 [details]
Bug 1291715 - Add RTCDTMFSender and RTCDTMFToneChangeEvent to webidl;

https://reviewboard.mozilla.org/r/74640/#review77170
Attachment #8785439 - Flags: review?(bugs) → review+
Comment on attachment 8785440 [details]
Bug 1291715 - Add RTCDTMFSender to PeerConnection.js;

https://reviewboard.mozilla.org/r/74642/#review77184

Looks good! Just a few fixes, and misc style nits.

::: dom/media/PeerConnection.js:1072
(Diff revision 3)
> +    //TODO: our sender is not a proper RTCRtpSender due to when we grab
> +    //      the reference.

This is because you have the inner object in this sandwich:

https://dxr.mozilla.org/mozilla-central/rev/f5d043ce6d36a3c461cbd829d4a4a38394b7c436/dom/media/PeerConnection.js#1053-1055

Use

    sender.__DOM_IMPL__

to get the outer object.

::: dom/media/PeerConnection.js:1074
(Diff revision 3)
>    },
>  
> +  _insertDTMF: function(sender, tones, duration, interToneGap) {
> +    //TODO: our sender is not a proper RTCRtpSender due to when we grab
> +    //      the reference.
> +    var sender = this._senders.find(function (s) {return s.track == sender.track});

If two senders are sending the same track, this will return the wrong sender, so good riddance.

::: dom/media/PeerConnection.js:1075
(Diff revision 3)
>  
> +  _insertDTMF: function(sender, tones, duration, interToneGap) {
> +    //TODO: our sender is not a proper RTCRtpSender due to when we grab
> +    //      the reference.
> +    var sender = this._senders.find(function (s) {return s.track == sender.track});
> +    if (sender) {

always true

::: dom/media/PeerConnection.js:1606
(Diff revision 3)
> +  this.duration = 100.0;
> +  this.interToneGap = 70.0;

Style nit: I find superfluous decimals non-idiomatic in JS. Applies throughout, unless you have a strong precedent for it.

::: dom/media/PeerConnection.js:1628
(Diff revision 3)
> +  set ontonechange(handler) {
> +    this.__DOM_IMPL__.setEventHandler("ontonechange", handler);
> +  },
> +
> +  insertDTMF: function(tones, duration, interToneGap) {
> +    if (duration === null) duration = 100.0;

Can't happen, as the duration argument is non-nullable and has a numeric default (duration is already 100 here unless a different number was passed in, thanks to the WebIDL binding code. 0 is worst-case.

::: dom/media/PeerConnection.js:1629
(Diff revision 3)
> +    if (duration < 40) duration = 40;
> +    if (duration > 6000) duration = 6000;
> +    this.duration = duration;

How about:

    this.duration = Math.max(40, Math.min(duration, 6000));

?

::: dom/media/PeerConnection.js:1633
(Diff revision 3)
> +    if (interToneGap === null) interToneGap = 70.0;
> +    if (interToneGap < 30) interToneGap = 30;
> +    this.interToneGap = interToneGap;

same here

::: dom/media/PeerConnection.js:1639
(Diff revision 3)
> +    if (interToneGap < 30) interToneGap = 30;
> +    this.interToneGap = interToneGap;
> +
> +    tones = tones.toUpperCase();
> +
> +    if (tones.match(/[^0-9A-D#*,]/) !== null) {

Style guide says: In JavaScript, == is preferred to ===.

match() returns something on success, or nothing, so I'd much rather read:

    if (tones.match(/[^0-9A-D#*,]/)) {

rather than risk it testing against the wrong nothing and take this if().

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators

::: dom/media/PeerConnection.js:1640
(Diff revision 3)
> +      this._sender._pc._insertDTMF(this._sender, "", duration, interToneGap);
> +      throw new this._sender._pc._win.DOMException("Invalid DTMF characters",
> +                                                   "InvalidCharacterError");

We should just throw and not touch the buffer here.

See https://github.com/w3c/webrtc-pc/issues/775

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2517
(Diff revision 3)
>  
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +PeerConnectionImpl::InsertDTMF(mozilla::dom::RTCRtpSender& sender,

mozilla::dom:: isn't needed in this .cpp file thanks to `using` statements, though they're used inconsistently. just fyi
Comment on attachment 8785441 [details]
Bug 1291715 - Add DTMF support to PeerConnection and AudioConduit;

https://reviewboard.mozilla.org/r/74644/#review77316

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:4113
(Diff revision 5)
> +void
> +PeerConnectionImpl::DTMFSendTimerCallback_m(nsITimer* timer, void* closure)
> +{
> +  auto state = static_cast<DTMFState* >(closure);
> +
> +  RefPtr<AudioSessionConduit> conduit = state->mPeerConnectionImpl->mMedia->GetAudioConduit(state->mLevel);

mMedia can be null here.
Comment on attachment 8788617 [details]
Bug 1291715 - Generate RTCDTMFToneChangeEvent;

https://reviewboard.mozilla.org/r/77044/#review77318

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:4132
(Diff revision 4)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +#if !defined(MOZILLA_EXTERNAL_LINKAGE)
> +  if (state->mNextTone >= 0) {
> +    //TODO: https://www.w3.org/TR/webrtc/#rtcdtmftonechangeevent says

We should send the empty string to indicate that all queued tones have been played.
Comment on attachment 8785441 [details]
Bug 1291715 - Add DTMF support to PeerConnection and AudioConduit;

https://reviewboard.mozilla.org/r/74644/#review77188

This is the right direction! r- for data race, and needing to address removeTrack. Misc style nits.

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:225
(Diff revision 3)
> +bool
> +WebrtcAudioConduit::InsertDTMFTone(int channel, int eventCode, bool outOfBand,
> +                                   int lengthMs, int attenuationDb) {

Assert thread.

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:228
(Diff revision 3)
>  
> +
> +bool
> +WebrtcAudioConduit::InsertDTMFTone(int channel, int eventCode, bool outOfBand,
> +                                   int lengthMs, int attenuationDb) {
> +  if (mVoiceEngine) {

Nit: Early-bail seems preferable here (less indentation).

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2521
(Diff revision 3)
>  PeerConnectionImpl::InsertDTMF(mozilla::dom::RTCRtpSender& sender,
>                                 const nsAString& tones, double duration,
>                                 double interToneGap) {
> +#if !defined(MOZILLA_EXTERNAL_LINKAGE)

We should do a PC_AUTO_ENTER_API_CALL(false); on entry here I think.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2527
(Diff revision 3)
> +  if (jrv.Failed()) {
> +    NS_WARNING("Failed to retrieve DTMF object on RTCRtpSender!");
> +    return NS_ERROR_UNEXPECTED;

We should pass out jrv here.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2534
(Diff revision 3)
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +
> +  // Attempt to locate state for the DTMFSender
> +  DTMFState *state = nullptr;
> +  for (uint32_t i = 0; i < mDTMFStates.Length(); ++i) {

Better to use:

    for (auto& dtmfState : mDTMFStates) {

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2542
(Diff revision 3)
> +      break;
> +    }
> +  }
> +
> +  // No state yet, create a new one
> +  if (state == nullptr) {

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style says

  "When testing a pointer, use (!myPtr) or (myPtr); don't use myPtr != nullptr or myPtr == nullptr."

Applies throughout.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2543
(Diff revision 3)
> +    DTMFState newState;
> +    newState.mPeerConnectionImpl = this;
> +    newState.mDTMFSender = dtmfSender;
> +    newState.mSendTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
> +    MOZ_ASSERT(newState.mSendTimer);

Suggestion: DTMFState may deserve a constructor for encapsulation, even if the members are public.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2548
(Diff revision 3)
> +    DTMFState newState;
> +    newState.mPeerConnectionImpl = this;
> +    newState.mDTMFSender = dtmfSender;
> +    newState.mSendTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
> +    MOZ_ASSERT(newState.mSendTimer);
> +    nsresult timerRv = newState.mSendTimer->SetTarget(mSTSThread);

This fires the timer directly on STS thread, which means the toneBuffer (mTones) is accessed on both the STS thread and the main thread below without any locking.

The spec [1] also accesses toneBuffer from both inside "Playout task" and outside it, but it says to "queue a task", which means on the same thread (not "in parallel").

So I think we should fire the timer on the main thread, read toneBuffer, then copy it in a runnable for the STS thread, so we won't need any locking. Have you considered using lambda capture?

This should make it simpler to reason about shutdown as well.

[1] http://w3c.github.io/webrtc-pc/#dom-rtcdtmfsender-insertdtmf

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2551
(Diff revision 3)
> +    mDTMFStates.AppendElement(newState);
> +    state = &mDTMFStates[mDTMFStates.Length() - 1];

I see no code that ever removes state or stops the timer except on close.

If someone calls:

    sender.dtmf.insertDTMF("ABCABCABCABCABCABCABC");
    pc.removeTrack(sender);

what should happen?

http://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-removetrack says we should "stop" the sender, "which indicates that it will no longer send any media".

Right now it looks like we leave the timer running and keep firing.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2555
(Diff revision 3)
> +
> +    mDTMFStates.AppendElement(newState);
> +    state = &mDTMFStates[mDTMFStates.Length() - 1];
> +  }
> +
> +  MOZ_ASSERT(state != nullptr);

MOZ_ASSERT(state);

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2556
(Diff revision 3)
> +    mDTMFStates.AppendElement(newState);
> +    state = &mDTMFStates[mDTMFStates.Length() - 1];
> +  }
> +
> +  MOZ_ASSERT(state != nullptr);
> +  state->mSendTimer->Cancel();

http://w3c.github.io/webrtc-pc/#peer-to-peer-dtmf says:

  "If a Playout task is scheduled to be run; abort these steps; otherwise queue a task that runs the following steps (Playout task):"

so we probably shouldn't stop the timer here.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2559
(Diff revision 3)
> +  // Retrieve track
> +  RefPtr<MediaStreamTrack> mst = sender.GetTrack(jrv);
> +  if (jrv.Failed()) {
> +    NS_WARNING("Failed to retrieve track for RTCRtpSender!");
> +    return NS_ERROR_UNEXPECTED;
> +  }

This is supposed to be invariant, but it's good to be safe, though we should probably do it higher up, before clearing toneBuffer.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2561
(Diff revision 3)
> +  if (jrv.Failed()) {
> +    NS_WARNING("Failed to retrieve track for RTCRtpSender!");
> +    return NS_ERROR_UNEXPECTED;

return jrv

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2568
(Diff revision 3)
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +
> +  auto trackPairs = mJsepSession->GetNegotiatedTrackPairs();
> +  state->mLevel = -1;
> +  for (auto j = trackPairs.begin(); j != trackPairs.end(); ++j) {

for ( : )

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2568
(Diff revision 3)
> +  for (auto j = trackPairs.begin(); j != trackPairs.end(); ++j) {
> +    JsepTrackPair& trackPair = *j;

for (auto& trackPair : trackPairs)

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2572
(Diff revision 3)
> +        state->mLevel = static_cast<uint16_t>(*trackPair.mBundleLevel);
> +      } else {
> +        state->mLevel = static_cast<uint16_t>(trackPair.mLevel);

What's significant about uint16_t? Why not have mLevel be size_t instead of casting?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2580
(Diff revision 3)
> +  for (uint32_t i = 0; i < tones.Length(); ++i) {
> +    uint16_t c = tones.CharAt(i);
> +    if (c == ',') {
> +      // , is a special character indicating a 2 second delay
> +      state->mTones.AppendElement(-1);
> +    } else {
> +      const char* i = strchr(DTMF_TONECODES, c);
> +      // the input tones should have been validated prior to calling this function
> +      MOZ_ASSERT(i != nullptr);
> +      if (i) {
> +        state->mTones.AppendElement(i - DTMF_TONECODES);
> +      }
> +    }
> +  }

Does this conversion from string to array<int> buy us anything? Seems redundant. Why not just operate on the string directly?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2627
(Diff revision 3)
> +    for (uint32_t i = 0; i < state->mTones.Length(); ++i) {
> +      if (state->mTones[i] == -1) {
> +        outToneBuffer.Append(',');
> +      } else {
> +        outToneBuffer.Append(DTMF_TONECODES[state->mTones[i]]);
> +      }
> +    }

Would remove the need for back-conversion as well.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2957
(Diff revision 3)
> +  for (uint32_t i = 0; i < mDTMFStates.Length(); ++i) {
> +    mDTMFStates[i].mSendTimer->Cancel();
> +  }

for ( : )

Also, we may want to move this to CloseInt() with the other cleanup, though it may not matter these days as peer connection can no longer reach closed state on its own.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:4108
(Diff revision 3)
> +void
> +PeerConnectionImpl::DTMFSendTimerCallback_m(nsITimer* timer, void* closure)
> +{
> +  auto state = static_cast<DTMFState* >(closure);

Assert thread. Note that `_m` is for main thread, and `_s` is for STS thread.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:4111
(Diff revision 3)
>  
> +
> +void
> +PeerConnectionImpl::DTMFSendTimerCallback_m(nsITimer* timer, void* closure)
> +{
> +  auto state = static_cast<DTMFState* >(closure);

no need for space before >

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:4113
(Diff revision 3)
> +void
> +PeerConnectionImpl::DTMFSendTimerCallback_m(nsITimer* timer, void* closure)
> +{
> +  auto state = static_cast<DTMFState* >(closure);
> +
> +  RefPtr<AudioSessionConduit> conduit = state->mPeerConnectionImpl->mMedia->GetAudioConduit(state->mLevel);

wordwrap
Attachment #8785441 - Flags: review?(jib) → review-
Comment on attachment 8788617 [details]
Bug 1291715 - Generate RTCDTMFToneChangeEvent;

https://reviewboard.mozilla.org/r/77044/#review77500

r- for returning wrong number of characters, and maybe "" events between tones? Otherwise all good!

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:4132
(Diff revision 4)
> +    //TODO: https://www.w3.org/TR/webrtc/#rtcdtmftonechangeevent says
> +    //      that an empty string for mTone indicates that the previous tone
> +    //      has stopped playing, but doesn't say if we are required to
> +    //      generate events in this case.

Good catch. Want to file an issue with the spec?

To me, it seems safe to assume they meant to fire "" events for each pause between tones. We should probably resolve this before landing, so we have consistent behavior out the gate.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:4138
(Diff revision 4)
> +    //      that an empty string for mTone indicates that the previous tone
> +    //      has stopped playing, but doesn't say if we are required to
> +    //      generate events in this case.
> +    RTCDTMFToneChangeEventInit init;
> +    MOZ_ASSERT(state->mNextTone < strlen(DTMF_TONECODES));
> +    init.mTone = DTMF_TONECODES[state->mNextTone];

Won't this return more than one character in most cases?
Attachment #8788617 - Flags: review?(jib) → review-
Comment on attachment 8785441 [details]
Bug 1291715 - Add DTMF support to PeerConnection and AudioConduit;

https://reviewboard.mozilla.org/r/74644/#review77188

> This fires the timer directly on STS thread, which means the toneBuffer (mTones) is accessed on both the STS thread and the main thread below without any locking.
> 
> The spec [1] also accesses toneBuffer from both inside "Playout task" and outside it, but it says to "queue a task", which means on the same thread (not "in parallel").
> 
> So I think we should fire the timer on the main thread, read toneBuffer, then copy it in a runnable for the STS thread, so we won't need any locking. Have you considered using lambda capture?
> 
> This should make it simpler to reason about shutdown as well.
> 
> [1] http://w3c.github.io/webrtc-pc/#dom-rtcdtmfsender-insertdtmf

We actually only access mNextTone (an int) from both the STS thread and the main thread. We update it from the main thread and read it from the STS thread. mTones is only used on the main thread. I used to have a comment to that effect, but removed it :/.

I tried your suggestion of having the timer on the main thread and then dispatch it as a runnable on the STS thread but unfortunately that throws off the intertone timing quite noticeably and so I don't think that will work in this case.

You're right about a potential data race here, I'll update the code to set mNextTone atomically.
Comment on attachment 8788617 [details]
Bug 1291715 - Generate RTCDTMFToneChangeEvent;

https://reviewboard.mozilla.org/r/77044/#review77500

> Good catch. Want to file an issue with the spec?
> 
> To me, it seems safe to assume they meant to fire "" events for each pause between tones. We should probably resolve this before landing, so we have consistent behavior out the gate.

Looking at the steps for insertDTMF at https://w3c.github.io/webrtc-pc/#rtcdtmfsender, step 1 is to fire an event containing an empty string when the tone buffer is empty and abort the steps. So it sounds like we get one empty string event per call to insertDTMF after the last tone in the buffer is played, regardless of how many tones were enqueued in the buffer by the call. If that doesn't match your interpretation, please let me know, and we should definitely file that issue :)
Comment on attachment 8788617 [details]
Bug 1291715 - Generate RTCDTMFToneChangeEvent;

https://reviewboard.mozilla.org/r/77044/#review77500

> Won't this return more than one character in most cases?

This works as expected, operator= is defined for char_type so it constructs a string from the single character passed into it. It won't compile if passed a char* string.
(In reply to Dan Minor [:dminor] from comment #36)
> Comment on attachment 8788617 [details]
> Bug 1291715 - Generate RTCDTMFToneChangeEvent;
> 
> https://reviewboard.mozilla.org/r/77044/#review77500
> 
> > Good catch. Want to file an issue with the spec?
> > 
> > To me, it seems safe to assume they meant to fire "" events for each pause between tones. We should probably resolve this before landing, so we have consistent behavior out the gate.
> 
> Looking at the steps for insertDTMF at
> https://w3c.github.io/webrtc-pc/#rtcdtmfsender, step 1 is to fire an event
> containing an empty string when the tone buffer is empty and abort the
> steps. So it sounds like we get one empty string event per call to
> insertDTMF after the last tone in the buffer is played, regardless of how
> many tones were enqueued in the buffer by the call. If that doesn't match
> your interpretation, please let me know, and we should definitely file that
> issue :)

Filed https://github.com/w3c/webrtc-pc/issues/799, as the steps in insertDTMF don't agree with the description of the DTMFToneChangeEvent itself.
Comment on attachment 8785440 [details]
Bug 1291715 - Add RTCDTMFSender to PeerConnection.js;

https://reviewboard.mozilla.org/r/74642/#review79034

::: dom/media/PeerConnection.js:1618
(Diff revision 5)
> +  set ontonechange(handler) {
> +    this.__DOM_IMPL__.setEventHandler("ontonechange", handler);
> +  },
> +
> +  insertDTMF: function(tones, duration, interToneGap) {
> +    this.duration = Math.max(40, Math.min(duration, 6000));

I don't think this is quite right -- duration is optional in insertDTMF, which means it may be undefined here. You need to either do an `if (duration)` or change your rhs to something like `Math.min(6000, Math.max(40, duration || 100))`

::: dom/media/PeerConnection.js:1621
(Diff revision 5)
> +
> +  insertDTMF: function(tones, duration, interToneGap) {
> +    this.duration = Math.max(40, Math.min(duration, 6000));
> +
> +    if (interToneGap < 30) interToneGap = 30;
> +    this.interToneGap = interToneGap;

Similarly, interToneGap may come in as undefined. This code would set it to 30 under such circumstances, when it should be 70. Again `if(interToneGap)` or `Math.max(40, interToneGap || 70)`.

::: dom/media/PeerConnection.js:1625
(Diff revision 5)
> +    if (interToneGap < 30) interToneGap = 30;
> +    this.interToneGap = interToneGap;
> +
> +    tones = tones.toUpperCase();
> +
> +    if (tones.match(/[^0-9A-D#*,]/)) {

Lowercase a-d are allowed and treated the same as uppercase. Either add an `i` flag to the regex or normalize the input stream to all-uppercase.
Comment on attachment 8785440 [details]
Bug 1291715 - Add RTCDTMFSender to PeerConnection.js;

https://reviewboard.mozilla.org/r/74642/#review79034

> I don't think this is quite right -- duration is optional in insertDTMF, which means it may be undefined here. You need to either do an `if (duration)` or change your rhs to something like `Math.min(6000, Math.max(40, duration || 100))`

In an earlier revision I had code here like you suggest, but jib pointed out that a default value is specified in the IDL file, so we should never receive an undefined value here.

> Lowercase a-d are allowed and treated the same as uppercase. Either add an `i` flag to the regex or normalize the input stream to all-uppercase.

I normalize it to uppercase just above this line :)
Attachment #8788617 - Attachment is obsolete: true
Comment on attachment 8794790 [details]
Bug 1291715 - Add support for 44100 and 48000 Hz sample rates to DtmfInband;

https://reviewboard.mozilla.org/r/81086/#review79768

::: media/webrtc/trunk/webrtc/voice_engine/output_mixer.cc:605
(Diff revision 1)
> +    if (!(_audioFrame.sample_rate_hz_ == 8000 ||
> +          _audioFrame.sample_rate_hz_ == 16000 ||
> +          _audioFrame.sample_rate_hz_ == 32000)) {
> +        return -1;

So we may frequently (or even much of the time) have sample rates of 44100 or 48000 hz... Can we log the failure?  Can we work around it even if it's ugly (upsample)? 48000 = 3*16000.  44100 is ugly, but doable with a resampler.
Comment on attachment 8794790 [details]
Bug 1291715 - Add support for 44100 and 48000 Hz sample rates to DtmfInband;

https://reviewboard.mozilla.org/r/81086/#review79768

> So we may frequently (or even much of the time) have sample rates of 44100 or 48000 hz... Can we log the failure?  Can we work around it even if it's ugly (upsample)? 48000 = 3*16000.  44100 is ugly, but doable with a resampler.

A little digging turned up this [1] which I think provides enough detail on their algorithm to add extra coefficient tables for 44100 and 48000 to [2], which has the assertion I was trying to work around. I'll add the extra tables and log something if we see something other than 8000, 16000, 32000, 44100, 48000.

[1] https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/audio_coding/neteq/dtmf_tone_generator.cc
[2] https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/voice_engine/dtmf_inband.cc
Comment on attachment 8794792 [details]
Bug 1291715 - Add pref for DTMF;

https://reviewboard.mozilla.org/r/81090/#review79792

ok, this is just modifying a webidl change which was done in a different patch in this bug.
Attachment #8794792 - Flags: review?(bugs) → review+
Comment on attachment 8785440 [details]
Bug 1291715 - Add RTCDTMFSender to PeerConnection.js;

https://reviewboard.mozilla.org/r/74642/#review79856
Attachment #8785440 - Flags: review?(jib) → review+
Comment on attachment 8794790 [details]
Bug 1291715 - Add support for 44100 and 48000 Hz sample rates to DtmfInband;

https://reviewboard.mozilla.org/r/81086/#review79768

> A little digging turned up this [1] which I think provides enough detail on their algorithm to add extra coefficient tables for 44100 and 48000 to [2], which has the assertion I was trying to work around. I'll add the extra tables and log something if we see something other than 8000, 16000, 32000, 44100, 48000.
> 
> [1] https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/audio_coding/neteq/dtmf_tone_generator.cc
> [2] https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/voice_engine/dtmf_inband.cc

Good.  We won't see anything else to speak of, since other code limits to those frequencies in the webrtc.org code
Comment on attachment 8785441 [details]
Bug 1291715 - Add DTMF support to PeerConnection and AudioConduit;

https://reviewboard.mozilla.org/r/74644/#review79858

Thanks for the updates, and sorry for the delay in reviewing. I found some more issues I didn't catch initially, sorry.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:857
(Diff revisions 5 - 6)
>    uint16_t mMaxReceiving[SdpMediaSection::kMediaTypes];
>    uint16_t mMaxSending[SdpMediaSection::kMediaTypes];
>  
>    // DTMF
> -  struct DTMFState {
> +  struct DTMFState : public LinkedListElement<DTMFState> {
> +    explicit DTMFState(PeerConnectionImpl* impl);

aImpl

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:861
(Diff revisions 5 - 6)
> -  struct DTMFState {
> +  struct DTMFState : public LinkedListElement<DTMFState> {
> +    explicit DTMFState(PeerConnectionImpl* impl);
> +    PeerConnectionImpl* mPeerConnectionImpl;
>      nsCOMPtr<nsITimer> mSendTimer;
>  #if !defined(MOZILLA_EXTERNAL_LINKAGE)
>      RefPtr<dom::RTCDTMFSender> mDTMFSender;

Should have caught this earlier... PeerConnectionImpl doesn't participate in cycle-collection (it can't because in XPCOM one has to pick between cycle collection and threadsafe locking, and it needs to be threadsafe), which means we can't have strong references back up to PeerConnection.js, or all RTCPeerConnection instances effectively become impervious to garbage collection even if all references are dropped on a page.

To work around this, we keep a single weak back pointer: mPCObserver [1]. Rather than fire events directly from c++, we might want to consider defining a method on PCObserver to do it, and write the last bit in JavaScript instead, to match how other back traffic is done.

[1] https://dxr.mozilla.org/mozilla-central/rev/c55bcb7c777ea09431b4d16903ed079ae5632648/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h#756

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:866
(Diff revisions 5 - 6)
> -    int mNextTone;
> +    Atomic<int> mNextTone;
>      double mDuration;

Why is mNextTone atomic while mDuration is not? Both seem read on STS the same way.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2491
(Diff revisions 5 - 6)
>  PeerConnectionImpl::RemoveTrack(MediaStreamTrack& aTrack) {
>    PC_AUTO_ENTER_API_CALL(true);
>  
>    std::string trackId = PeerConnectionImpl::GetTrackId(aTrack);
> +
> +  for (auto dtmfState : mDTMFStates) {

auto&

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2529
(Diff revisions 5 - 6)
> +static const char* DTMF_TONECODES = "0123456789*#ABCD";
> +
> +static int GetDTMFToneCode(uint16_t c)
> +{

The conversion to ToneCode has been pushed down a bit, but not all the way, which means I see we still have to back-convert for the tonechange event.

Why don't we push the conversion all the way down, so that GetDTMFToneCode() is only called from conduit->InsertDTMFTone()? This would remove the need to ever back-convert, and then the DTMF_TONECODES static above can be moved inside the GetDTMFToneCode() function, which should please the platform team who frown on global initialized static data, however small.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2543
(Diff revisions 5 - 6)
> +PeerConnectionImpl::DTMFState::DTMFState(PeerConnectionImpl* impl)
> +  : mPeerConnectionImpl(impl)
> +{
> +
> +}

Feel free to inline in the .h file if you want.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2565
(Diff revisions 5 - 6)
> -    return NS_ERROR_UNEXPECTED;
> +    return jrv.StealNSResult();
>    }
>  
>    // Attempt to locate state for the DTMFSender
>    DTMFState *state = nullptr;
> -  for (uint32_t i = 0; i < mDTMFStates.Length(); ++i) {
> +  for (auto dtmfState : mDTMFStates) {

auto&

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2589
(Diff revisions 5 - 6)
> +    state->mTrackId = GetTrackId(*mst.get());
> +    state->mDTMFSender = dtmfSender;
> +    state->mSendTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
> +    MOZ_ASSERT(state->mSendTimer);
> +    nsresult timerRv = state->mSendTimer->SetTarget(mSTSThread);
> +    NS_ENSURE_SUCCESS(timerRv, timerRv);

Nit: We're not supposed to use NS_ENSURE_SUCCESS anymore in new code, as it was deemed to "hide code flow". Use if (NS_FAILED(rv)) { return rv; } instead. It's also common to reuse the rv var name.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2613
(Diff revisions 5 - 6)
> +    state->mSendTimer->InitWithFuncCallback(DTMFSendTimerCallback_s, state, 0,
>                                              nsITimer::TYPE_ONE_SHOT);

You'd mentioned you could hear timing issues when more than one thread was involved... I know we're not doing that anymore, but it's perhaps still concerning that it was within hearing range, since timer errors accumulate with one-shots.

Should we consider TYPE_REPEATING_PRECISE_CAN_SKIP ?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2633
(Diff revisions 5 - 6)
> -    return NS_ERROR_UNEXPECTED;
> +    return jrv.StealNSResult();
>    }
>  
>    // Attempt to locate state for the DTMFSender
> -  DTMFState* state = nullptr;
> -  for (uint32_t i = 0; i < mDTMFStates.Length(); ++i) {
> +  DTMFState *state = nullptr;
> +  for (auto dtmfState : mDTMFStates) {

auto&

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3070
(Diff revisions 5 - 6)
>  nsresult
>  PeerConnectionImpl::CloseInt()
>  {
>    PC_AUTO_ENTER_API_CALL_NO_CHECK();
>  
> +  for (auto dtmfState : mDTMFStates) {

auto&

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:4121
(Diff revisions 5 - 6)
> +  auto state = static_cast<DTMFState*>(closure);
>  
> -  RefPtr<AudioSessionConduit> conduit = state->mPeerConnectionImpl->mMedia->GetAudioConduit(state->mLevel);
> +  if (!state->mPeerConnectionImpl->mMedia) {

When a timer is cancelled on the same thread, I understand we're guaranteed not to get any stray callbacks after the fact, but when the target thread is different, is it possible for this to race with ~PeerConnectionImpl() calling CloseInt() and mSendTimer.Cancel()? If so, what guarantee do we have that this isn't use-after-free?

We'd need to add some checks here, or, alternatively...

Instead of, on main, starting a timer to trigger on STS thread, and when it triggers on STS, insert DTMF tone there, then dispatch to main thread, where we update buffer, fire DOM events and another timer to go off on STS thread again, might an alternative be to:

On main, start a recurring timer on main thread to trigger on main thread, and when it triggers on the main thread, update buffer, dispatch a fire-and-forget runnable to STS to insert the actual DTMF tone, but don't otherwise wait for it, instead immediately fire DOM events and be done?

This would seem to have several benefits:
- Would follow the spec more closely [1]
- Should avoid any race with destruction of PeerConnectionImpl.
- If we pack the tone and duration (both 16 bit) into a ulong, we can put that in the closure, effectively a copy, no longer needing to reference dtmfState from two threads, thus no need for atomics or linked list, and we can now safely remove state in RemoveTrack rather than accumulate state over time.

Thoughts?

[1] http://w3c.github.io/webrtc-pc/#dom-rtcdtmfsender-insertdtmf

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:4130
(Diff revisions 5 - 6)
> -    //TODO: channel, inband, attenuation...
> +    //TODO: We default to channel 0, not inband, and no attenuation here.
> +    //      We might want to revisit these choices in the future.

Maybe add a bug # for TODOs, otherwise they get lost?
Attachment #8785441 - Flags: review?(jib) → review-
Comment on attachment 8794791 [details]
Bug 1291715 - Add mochitest for DTMF;

https://reviewboard.mozilla.org/r/81088/#review80076

Lgtm. Do we want to test the tonechange events as well?

We should also test that we fire InvalidStateError (not sure we do yet) according to the lastest spec update:

https://rawgit.com/w3c/webrtc-pc/c594cd214f0d71ff3c75a28e16be5dd695da15a6/webrtc.html#dom-rtcdtmfsender-insertdtmf

::: dom/media/tests/mochitest/test_peerConnection_insertDTMF.html:20
(Diff revision 2)
> +function insertdtmftest(pc) {
> +  ok(pc.getSenders().length > 0, "have senders");
> +  var sender = pc.getSenders()[0];
> +  ok(sender.dtmf, "sender has dtmf object");
> +
> +  ok(sender.dtmf.toneBuffer == "", "sender should start with empty tonebuffer");

This is one of the rare cases I'd recommend ===

::: dom/media/tests/mochitest/test_peerConnection_insertDTMF.html:23
(Diff revision 2)
> +  ok(sender.dtmf, "sender has dtmf object");
> +
> +  ok(sender.dtmf.toneBuffer == "", "sender should start with empty tonebuffer");
> +
> +  sender.dtmf.insertDTMF("A", 10);
> +  ok(sender.dtmf.duration == 40, "minimum duration is 40");

If you use:

    is(sender.dtmf.duration, 40, "minimum duration is 40");

then if duration is ever something other than 40, the value will get reported, which is a nice touch.

Applies throughout where two values are compared.

::: dom/media/tests/mochitest/test_peerConnection_insertDTMF.html:26
(Diff revision 2)
> +  sender.dtmf.insertDTMF("A", 70, 10);
> +  ok(sender.dtmf.interToneGap == 30, "minimum interToneGap is 30");

Maybe also verify that duration is 70 here.

Maybe also test setting a legal value for interToneGap, and in light of https://github.com/w3c/webrtc-pc/issues/836 we might want to do so with an empty buffer at the same time, e.g.:

    sender.dtmf.insertDTMF("", 70, 40);

::: dom/media/tests/mochitest/test_peerConnection_insertDTMF.html:34
(Diff revision 2)
> +  var threw = false;
> +  try {
> +    sender.dtmf.insertDTMF("bad tones");
> +  } catch (ex) {
> +    threw = true;
> +    ok(ex.code == DOMException.INVALID_CHARACTER_ERR, "Expected InvalidCharacterError");

is(ex.code, DOMEXception.INVALID_CHARACTER_ERR, "...");
Attachment #8794791 - Flags: review?(jib) → review+
Comment on attachment 8794792 [details]
Bug 1291715 - Add pref for DTMF;

https://reviewboard.mozilla.org/r/81090/#review80088

::: dom/media/tests/mochitest/test_peerConnection_insertDTMF.html:48
(Diff revision 2)
>    ok(sender.dtmf.toneBuffer.indexOf("A") != -1, "lowercase characters should be normalized");
>  }
>  
>  runNetworkTest(() => {
>  
> +  SpecialPowers.setBoolPref("media.peerconnection.dtmf.enabled", true);

I guess there's no way this would race in practice, but it'd still be cleaner to wait for this. 

E.g. see:

https://dxr.mozilla.org/mozilla-central/rev/c55bcb7c777ea09431b4d16903ed079ae5632648/dom/media/test/test_mediarecorder_principals.html#46
Attachment #8794792 - Flags: review?(jib) → review+
Comment on attachment 8785441 [details]
Bug 1291715 - Add DTMF support to PeerConnection and AudioConduit;

https://reviewboard.mozilla.org/r/74644/#review79858

> When a timer is cancelled on the same thread, I understand we're guaranteed not to get any stray callbacks after the fact, but when the target thread is different, is it possible for this to race with ~PeerConnectionImpl() calling CloseInt() and mSendTimer.Cancel()? If so, what guarantee do we have that this isn't use-after-free?
> 
> We'd need to add some checks here, or, alternatively...
> 
> Instead of, on main, starting a timer to trigger on STS thread, and when it triggers on STS, insert DTMF tone there, then dispatch to main thread, where we update buffer, fire DOM events and another timer to go off on STS thread again, might an alternative be to:
> 
> On main, start a recurring timer on main thread to trigger on main thread, and when it triggers on the main thread, update buffer, dispatch a fire-and-forget runnable to STS to insert the actual DTMF tone, but don't otherwise wait for it, instead immediately fire DOM events and be done?
> 
> This would seem to have several benefits:
> - Would follow the spec more closely [1]
> - Should avoid any race with destruction of PeerConnectionImpl.
> - If we pack the tone and duration (both 16 bit) into a ulong, we can put that in the closure, effectively a copy, no longer needing to reference dtmfState from two threads, thus no need for atomics or linked list, and we can now safely remove state in RemoveTrack rather than accumulate state over time.
> 
> Thoughts?
> 
> [1] http://w3c.github.io/webrtc-pc/#dom-rtcdtmfsender-insertdtmf

So, I've tried exactly this and it throws off the timing of the tones to a very noticeable extent. I think the problem is that it takes a variable amount of time for the runnable to actually start on the STS thread. I agree that if we can make it work, it would make the rest of the implementation much nicer.

I think there must have been a misunderstanding when we talked on IRC, I never had any problems with timers throwing off timing between tones, it was when I tried to dispatch a runnable on another thread that things went badly. I'm willing to try again, especially if you have any suggestions on how we could reduce the variance associated with dispatching the runnable. Of course, it's always possible I made a mistake with my implementation the first time I tried this...

Otherwise, I think we're stuck with adding whatever checks to prevent a UAF. Could we make mPeerConnection atomic and set it to nullptr in ~PeerConnectionImpl?
(In reply to Dan Minor [:dminor] from comment #61)
> Otherwise, I think we're stuck with adding whatever checks to prevent a UAF.
> Could we make mPeerConnection atomic and set it to nullptr in
> ~PeerConnectionImpl?

Basically no... the issue is the memory pointed to by mPeerConnection, not the pointer itself.  You need a lock or an owning reference, or avoid needing it.

Anything which depends on accurate timing of runnables that touch mainthread will see variance especially on loaded systems.  If this is a dom-visible thing like an event firing that is expected and impossible to avoid.  

Generally I'd want DTMF generation timing to be entirely handled off-main-thread, and fire runnables as needed to MainThread to notify it of what's happening (tone played, tone queue empty, etc - I didn't read back to see what the exact issue here is).
For what it's worth, if I run the timer on the main thread, and fire the runnable to the STS Thread with NS_DISPATCH_SYNC the timing issues playing the tones that I notice with NS_DISPATCH_NORMAL disappear. I'm guessing that this kind of synchronization is not a good idea, but please let me know otherwise.
(In reply to Dan Minor [:dminor] from comment #63)
> For what it's worth, if I run the timer on the main thread, and fire the
> runnable to the STS Thread with NS_DISPATCH_SYNC the timing issues playing
> the tones that I notice with NS_DISPATCH_NORMAL disappear. I'm guessing that
> this kind of synchronization is not a good idea, but please let me know
> otherwise.

DISPATCH_SYNC from MainThread must be used with great care.... it will spin the event loop, and spinning the event loop violates JS safety if called from JS code (or with a JS event/code on the stack).  Basically, it can cause JS to become reentrant, leading to all sorts of evil failures.
Comment on attachment 8794790 [details]
Bug 1291715 - Add support for 44100 and 48000 Hz sample rates to DtmfInband;

https://reviewboard.mozilla.org/r/81086/#review81092
Attachment #8794790 - Flags: review?(rjesup) → review+
Note that until the DISPATCH_SYNC/mPeerConnection stuff above is resolved, I'm not sure we can land this.
Comment on attachment 8785441 [details]
Bug 1291715 - Add DTMF support to PeerConnection and AudioConduit;

https://reviewboard.mozilla.org/r/74644/#review81554

With the negative delay and the types fixed, this lgtm.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:862
(Diff revisions 7 - 8)
>      double mDuration;
>      double mInterToneGap;

I didn't catch these before, but the WebIDL defines these as unsigned long, so the binding code will ensure these are uint32_t for this implementation, so that's what we should use throughout.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2542
(Diff revisions 7 - 8)
>                                 const nsAString& tones, double duration,
>                                 double interToneGap) {

s/double/uint32_t/

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2556
(Diff revisions 7 - 8)
>    if (jrv.Failed()) {
> -    NS_WARNING("Failed to retrieve DTMF object on RTCRtpSender!");
> +    NS_WARNING("Failed to retrieve track for RTCRtpSender!");
>      return jrv.StealNSResult();
>    }
>  
> +  std::string senderTrackId = GetTrackId(*mst.get());

nit:

    s/*mst.get()/*mst/

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2616
(Diff revisions 7 - 8)
>    if (jrv.Failed()) {
> -    NS_WARNING("Failed to retrieve DTMF object on RTCRtpSender!");
> +    NS_WARNING("Failed to retrieve track for RTCRtpSender!");
>      return jrv.StealNSResult();
>    }
>  
> +  std::string senderTrackId = GetTrackId(*mst.get());

*mst

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:4121
(Diff revisions 7 - 8)
> +    int tone = GetDTMFToneCode(toneChar);
> +
> +    state->mTones.Cut(0, 1);
> +
> +    if (tone == -1) {
> +      timer->SetDelay(2000 - state->mDuration - state->mInterToneGap);

Duration alone can be 8000 ms, which'll make this negative, causing a really long unsigned delay.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:4134
(Diff revisions 7 - 8)
>  
> -  RefPtr<AudioSessionConduit> conduit =
> +      RefPtr<AudioSessionConduit> conduit =
> -    state->mPeerConnectionImpl->mMedia->GetAudioConduit(state->mLevel);
> +        state->mPeerConnectionImpl->mMedia->GetAudioConduit(state->mLevel);
> +
> -  if (conduit) {
> +      if (conduit) {
> -    //TODO: We default to channel 0, not inband, and no attenuation here.
> +        double duration = state->mDuration;

uint32_t

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:4142
(Diff revisions 7 - 8)
>  #if !defined(MOZILLA_EXTERNAL_LINKAGE)
> -  if (state->mNextTone >= 0) {
> -    RTCDTMFToneChangeEventInit init;
> -    MOZ_ASSERT(state->mNextTone < (int)strlen(DTMF_TONECODES));
> -    init.mTone = DTMF_TONECODES[state->mNextTone];
> -
> +      RefPtr<PeerConnectionObserver> pco = do_QueryObjectReferent(state->mPeerConnectionImpl->mPCObserver);
> +      if (!pco) {
> +        NS_WARNING("Failed to dispatch the RTCDTMFToneChange event!");
> +        return;
> +      }
> -    RefPtr<RTCDTMFToneChangeEvent> event =
> -      RTCDTMFToneChangeEvent::Constructor(state->mDTMFSender,
> -                                          NS_LITERAL_STRING("tonechange"),
> -                                          init);
>  
> -    event->SetTrusted(true);
> +      nsString trackId;
> +      trackId.AssignASCII(state->mTrackId.c_str());
> +      nsString tone;
> +      tone.Assign(toneChar);
> +      JSErrorResult jrv;
> +      pco->OnDTMFToneChange(trackId, tone, jrv);
>  
> -    nsresult rv;
> +      if (jrv.Failed()) {
> -    rv = EventDispatcher::DispatchDOMEvent(state->mDTMFSender, nullptr,
> -                                           event, nullptr, nullptr);
> -    if (NS_FAILED(rv)) {
> -      NS_WARNING("Failed to dispatch the RTCDTMFToneChange event!");
> +        NS_WARNING("Failed to dispatch the RTCDTMFToneChange event!");
> -      return;
> +        return;

It seems like this code (with a toneChar) could be combined with almost the same code (without a toneChar) below, to reduce the size of this function.
Attachment #8785441 - Flags: review?(jib) → review+
Comment on attachment 8785439 [details]
Bug 1291715 - Add RTCDTMFSender and RTCDTMFToneChangeEvent to webidl;

https://reviewboard.mozilla.org/r/74640/#review81584

::: dom/webidl/RTCDTMFSender.webidl:17
(Diff revision 8)
> +    readonly attribute long         duration;
> +    readonly attribute long         interToneGap;

Btw, these should be unsigned long.

https://github.com/w3c/webrtc-pc/pull/848
Comment on attachment 8785441 [details]
Bug 1291715 - Add DTMF support to PeerConnection and AudioConduit;

https://reviewboard.mozilla.org/r/74644/#review81554

> Duration alone can be 8000 ms, which'll make this negative, causing a really long unsigned delay.

I captured some of Chrome's audio in Audacity and it appears that they are not adjusting for the intertone gap either (they delay between tones is greater than 2 seconds) so I'm going to remove this adjustment entirely.
I noticed some timing problems during testing this morning. The delay for a ',' was about twice as long as it should have been, and the time after the first character in a sequence was longer than the time after the remaining characters in the sequence. I verified this with an Audacity capture of the audio output.

Both problems go away with if I replace recurring timers with one shot timers. It seems like calling SetDelay on a recurring timer is less accurate than setting up a new one shot timer. :jib, unless you have strong objections, I'd like to land with the one shot timers.
Flags: needinfo?(jib)
Sounds good.
Flags: needinfo?(jib)
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/903fa45a9d6c
Add RTCDTMFSender and RTCDTMFToneChangeEvent to webidl; r=smaug
https://hg.mozilla.org/integration/autoland/rev/50497f2cdd28
Add RTCDTMFSender to PeerConnection.js; r=jib
https://hg.mozilla.org/integration/autoland/rev/5505acbab659
Add DTMF support to PeerConnection and AudioConduit; r=jib
https://hg.mozilla.org/integration/autoland/rev/91c0b78bb4dd
Add support for 44100 and 48000 Hz sample rates to DtmfInband; r=jesup
https://hg.mozilla.org/integration/autoland/rev/044172cb2dd8
Add mochitest for DTMF; r=jib
https://hg.mozilla.org/integration/autoland/rev/078248a9fdbc
Add pref for DTMF; r=jib,smaug
Backed out for bustage:

https://hg.mozilla.org/integration/autoland/rev/84cb9e48869cfa8126e44df7f5053e2d81e4f796
https://hg.mozilla.org/integration/autoland/rev/d4b05bd42e20322b36ce413071f07f07dcd083f8
https://hg.mozilla.org/integration/autoland/rev/6242662fb3243870ad1d11ab9d92f9493daf0b0d
https://hg.mozilla.org/integration/autoland/rev/71d8f186a5c1182c96cc2151e71e4a2f667609ff
https://hg.mozilla.org/integration/autoland/rev/23abc8c21f75778c224e44133a05e79970b92e32
https://hg.mozilla.org/integration/autoland/rev/31d703eb520875a72e955b8375352f507c575d3e

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=078248a9fdbc3c28e09c57b65d4024f5b5c39c57
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=4535675&repo=autoland

[task 2016-10-05T14:44:48.128480Z] 14:44:48     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:706:34: error: Cannot instantiate 'nsTArray_CopyChooser<mozilla::PeerConnectionImpl::DTMFState>' with non-memmovable template argument 'mozilla::PeerConnectionImpl::DTMFState'
[task 2016-10-05T14:44:48.128539Z] 14:44:48     INFO -  struct MOZ_NEEDS_MEMMOVABLE_TYPE nsTArray_CopyChooser
[task 2016-10-05T14:44:48.128579Z] 14:44:48     INFO -                                   ^
Flags: needinfo?(dminor)
:jib, please have a look at: https://reviewboard.mozilla.org/r/74644/diff/9-10/

Build-only try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3968835ef5d7110841f2efb228527e888130946
Flags: needinfo?(dminor) → needinfo?(jib)
Comment on attachment 8785441 [details]
Bug 1291715 - Add DTMF support to PeerConnection and AudioConduit;

https://reviewboard.mozilla.org/r/74644/#review82264

This is a good example of why APIs like PeerConnectionImpl::GetTrackId() are a bad idea.

I feel both sides of the string-wards, but let's avoid converting back and forth needlessly. Less is more.

I stopped commenting on every line, but this applies throughout.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2501
(Diff revisions 9 - 10)
>    std::string trackId = PeerConnectionImpl::GetTrackId(aTrack);
>  
> +  nsString sTrackId;
> +  sTrackId.AssignASCII(trackId.c_str());

Just call:

    nsString trackId;
    aTrack.GetId(trackId);

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2506
(Diff revisions 9 - 10)
>    std::string trackId = PeerConnectionImpl::GetTrackId(aTrack);
>  
> +  nsString sTrackId;
> +  sTrackId.AssignASCII(trackId.c_str());
>    for (size_t i = 0; i < mDTMFStates.Length(); ++i) {
> -    if (mDTMFStates[i].mTrackId == trackId) {
> +    if (mDTMFStates[i].mTrackId == sTrackId) {

Not a fan of this naming convention.
Attachment #8785441 - Flags: review+ → review-
Comment on attachment 8785441 [details]
Bug 1291715 - Add DTMF support to PeerConnection and AudioConduit;

https://reviewboard.mozilla.org/r/74644/#review82264

string-wars (odd, mozReview committed an earlier draft on me)
Flags: needinfo?(jib)
Comment on attachment 8785441 [details]
Bug 1291715 - Add DTMF support to PeerConnection and AudioConduit;

https://reviewboard.mozilla.org/r/74644/#review82438

lgtm.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2597
(Diff revisions 9 - 11)
> -    if (trackPair.mSending->GetTrackId() == state->mTrackId) {
> +    nsString sendingTrackId;
> +    sendingTrackId.AssignASCII(trackPair.mSending->GetTrackId().c_str());
> +    if (sendingTrackId == state->mTrackId) {

How about:

    state->mTrackId.EqualsASCII(trackPair.mSending->GetTrackId().c_str());

?
Attachment #8785441 - Flags: review?(jib) → review+
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/492a39e1ca7c
Add RTCDTMFSender and RTCDTMFToneChangeEvent to webidl; r=smaug
https://hg.mozilla.org/integration/autoland/rev/a55903b46ff5
Add RTCDTMFSender to PeerConnection.js; r=jib
https://hg.mozilla.org/integration/autoland/rev/f73858f36eeb
Add DTMF support to PeerConnection and AudioConduit; r=jib
https://hg.mozilla.org/integration/autoland/rev/5f4460d092e5
Add support for 44100 and 48000 Hz sample rates to DtmfInband; r=jesup
https://hg.mozilla.org/integration/autoland/rev/e79ca8b92bb2
Add mochitest for DTMF; r=jib
https://hg.mozilla.org/integration/autoland/rev/3e9a2031152f
Add pref for DTMF; r=jib,smaug
gcc 6 points out that the following tests in DtmfInband::DtmfFix_generate
can never be true, because |fs| is a signed 16-bit value:

    } else if (fs==44100) {
        a_times2Tbl=Dtmf_a_times2Tab44_1Khz;
        y2_Table=Dtmf_ym2Tab44_1Khz;
    } else if (fs==48000) {
        a_times2Tbl=Dtmf_a_times2Tab48Khz;
        y2_Table=Dtmf_ym2Tab48Khz;
Same applies in DtmfInband::SetSampleRate(uint16_t frequency).

_audioFrame.sample_rate_hz_ also needs to be checked.

w/ apologies in advance for the noise, if, either, I have
misinterpreted something, or this has already been reported.
Thanks Julian! Would you mind filing a new bug on it?

https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=WebRTC%3A%20Audio%2FVideo
Flags: needinfo?(jseward)
dminor filed already -- bug 1312431.

Also, what I said about DtmfInband::SetSampleRate is wrong, because
that is an unsigned value.  But the DtmfInband::DtmfFix_generate comment
stands.
Flags: needinfo?(jseward)
This webidl file should have a link to the relevant spec, if it's based on a spec.  Is it?

If it's not based on a spec, it needs documentation (which it can have even if based on a spec, but worst-case  the spec is the documentation).
Flags: needinfo?(dminor)
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #108)
> This webidl file should have a link to the relevant spec, if it's based on a
> spec.  Is it?
> 
> If it's not based on a spec, it needs documentation (which it can have even
> if based on a spec, but worst-case  the spec is the documentation).

Oops, it looks like I missed the comment block completely on RTCDTMFToneChangeEvent.webidl, I assume that is the file you mean. It is based on https://w3c.github.io/webrtc-pc/#rtcdtmftonechangeevent. I've filed Bug 1491128 to get this fixed. Thanks for pointing this out!
Flags: needinfo?(dminor)
Yes, I meant RTCDTMFToneChangeEvent.webidl.  Sorry for not making that clearer!
You need to log in before you can comment on or make changes to this bug.