Add signaling support for DTMF in WebRTC

RESOLVED FIXED in Firefox 52

Status

()

Core
WebRTC: Signaling
P1
normal
Rank:
15
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dminor, Assigned: mjf)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
This bug covers signaling changes required to enable DTMF in WebRTC.
(Reporter)

Updated

2 years ago
Rank: 15
(Assignee)

Updated

2 years ago
Assignee: nobody → mfroman
(Assignee)

Comment 1

2 years ago
Were you thinking we'd carry the RTP packets for DTMF in the normal audio stream?  If so, I believe we will be forced to support multiple codecs in answers (Bug 814227).  If not, we will need to add an additional audio stream to the SDP for DTMF.  I see Chrome is including it on the normal audio stream.
Flags: needinfo?(rjesup)
(In reply to Michael Froman [:mjf] from comment #1)
> Were you thinking we'd carry the RTP packets for DTMF in the normal audio
> stream?  If so, I believe we will be forced to support multiple codecs in
> answers (Bug 814227).  If not, we will need to add an additional audio
> stream to the SDP for DTMF.  I see Chrome is including it on the normal
> audio stream.

They *must* live in the same stream/m= line.
While it's a separate payload, it's not exactly the same as answering with multiple payloads at the media level (incoming DTMF payloads are handled separately from normal audio decode).  You do need to have two payloads in the m= line SDP and rtpmaps for them.
Flags: needinfo?(rjesup)
Comment hidden (mozreview-request)
(Reporter)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8791048 [details]
Bug 1291714 - sdp changes to support DTMF signaling.

https://reviewboard.mozilla.org/r/78608/#review77252

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:971
(Diff revision 1)
>        branch->GetBoolPref("media.navigator.audio.use_fec", &mUseAudioFec);
>  
>        branch->GetBoolPref("media.navigator.video.red_ulpfec_enabled",
>                            &mRedUlpfecEnabled);
> +
> +      branch->GetBoolPref("media.navigator.audio.dtmf_enabled", &mDtmfEnabled);

Rather than make a separate permission for this, you could use "media.peerconnection.dtmf.enabled" which is what I was planning to use to determine whether or not DTMF would be exposed to DOM.

Also, as I gave it some more thought, I'm not sure if we need to hide this behind a pref - unlike some of the other options here, whether or not DTMF is generated is more or less controlled by the user, rather than negotiated behind the scenes.
(Assignee)

Comment 5

2 years ago
(In reply to Dan Minor [:dminor] from comment #4)
> Rather than make a separate permission for this, you could use
> "media.peerconnection.dtmf.enabled" which is what I was planning to use to
> determine whether or not DTMF would be exposed to DOM.
I don't have a problem with one pref controlling both signaling and media.

>
> Also, as I gave it some more thought, I'm not sure if we need to hide this
> behind a pref - unlike some of the other options here, whether or not DTMF
> is generated is more or less controlled by the user, rather than negotiated
> behind the scenes.
True, but part of the reason I put the SDP generation behind a pref is because
this requires multiple codecs on the answer SDP which is, in the general case,
not supported yet (Bug 814227).  I wanted to have the ability to shut off SDP
generation in case the multiple answer codecs causes a problem elsewhere.  We
have the identical issue when red/ulpfec is on for video (also behind a pref).
After we've seen lots of offer/answer exchanges with red/ulpfec, thus causing
the full list of supported codecs to show in the answer SDP, we can take another
look at Bug 814227, and remove the special handling cases of red/ulpfec and dtmf.

Comment 6

2 years ago
mozreview-review
Comment on attachment 8791048 [details]
Bug 1291714 - sdp changes to support DTMF signaling.

https://reviewboard.mozilla.org/r/78608/#review77298

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:352
(Diff revision 1)
>      if (codec->mName == "red") {
>        red = static_cast<JsepVideoCodecDescription*>(codec);
>      }
>      else if (codec->mName == "ulpfec") {
>        ulpfec = static_cast<JsepVideoCodecDescription*>(codec);
>      }
> +    else if (codec->mName == "telephone-event") {
> +      dtmf = static_cast<JsepAudioCodecDescription*>(codec);
> +    }

Hmm. What if someone puts a telephone-event in a video m-section? (Or for that matter, a red or ulpfec in an audio m-section) Do we perform a bad cast?

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:400
(Diff revision 1)
> +  // rtcpfb attr). If we see the telephone-event codec, we enabled dtmf
> +  // support on all the other audio codecs.
> +  if (dtmf) {
> +    for (auto codec : *codecs) {
> +      if (codec->mType == SdpMediaSection::kAudio
> +          && codec->mName != "telephone-event") {

Probably doesn't matter if we set mDtmfEnabled on the telephone-event codec thingy.

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:403
(Diff revision 1)
> +    for (auto codec : *codecs) {
> +      if (codec->mType == SdpMediaSection::kAudio
> +          && codec->mName != "telephone-event") {
> +        JsepAudioCodecDescription* audioCodec =
> +            static_cast<JsepAudioCodecDescription*>(codec);
> +        audioCodec->mDtmfEnabled = true;

If renegotiation turns DTMF off, do we set this to false?

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:418
(Diff revision 1)
>    // answer.  For now, remove all but the first codec unless the red codec
>    // exists, and then we include the others per RFC 5109, section 14.2.
> +  // Note: now allows keeping the telephone-event codec, if it appears, as the
> +  // last codec in the list.
>    if (!codecs->empty() && !red) {
> +    int newSize = 1 + (dtmf ? 1 : 0);

int newSize = dtmf ? 2 : 1; is a little more readable.

::: media/webrtc/signaling/src/sdp/SdpAttribute.h:1090
(Diff revision 1)
>      kiLBC,
>      kiSAC,
>      kH264,
>      kRed,
>      kUlpfec,
> +    kAvt, // telephone-event

Let's not follow sipcc's (bad) example. This should probably be called kTelephoneEvent.

::: media/webrtc/signaling/src/sdp/SipccSdpAttributeList.cpp:761
(Diff revision 1)
>          opusParameters->maxplaybackrate = fmtp->maxplaybackrate;
>          opusParameters->stereo = fmtp->stereo;
>          opusParameters->useInBandFec = fmtp->useinbandfec;
>          parameters.reset(opusParameters);
>        } break;
> +      case RTP_AVT: { // telephone-event

Actually, from what I can tell, RTP_AVT isn't used anywhere in sipcc right now. Can we just rename it to RTP_TELEPHONE_EVENT?

::: media/webrtc/signaling/src/sdp/sipcc/sdp_access.c:34
(Diff revision 1)
>  #define SIPSDP_ATTR_ENCNAME_L16_256K  "L16"
>  #define SIPSDP_ATTR_ENCNAME_ISAC      "ISAC"
>  #define SIPSDP_ATTR_ENCNAME_OPUS      "opus"
>  #define SIPSDP_ATTR_ENCNAME_RED       "red"
>  #define SIPSDP_ATTR_ENCNAME_ULPFEC    "ulpfec"
> +#define SIPSDP_ATTR_ENCNAME_AVT       "telephone-event"

Let's use a better name here.

::: media/webrtc/signaling/src/sdp/sipcc/sdp_attr.c:1806
(Diff revision 1)
> +                   && (('0' <= tmp[1] && tmp[1] <= '9')
> +                       || tmp[1] == '-' || tmp[1] == ',')

Won't this always be true if the strspn == strlen?

(Provided strlen is at least 2, do we check?)

::: media/webrtc/signaling/test/jsep_track_unittest.cpp:253
(Diff revision 1)
> -    const JsepVideoCodecDescription*
> -    GetVideoCodec(const JsepTrack& track,
> -                  size_t expectedSize = 1,
> -                  size_t codecIndex = 0) const
> +    const JsepCodecDescription* GetCodec(const JsepTrack& track,
> +                                         SdpMediaSection::MediaType type,
> +                                         size_t expectedSize,
> +                                         size_t codecIndex) const

Convention here is to have the return on its own line.

::: media/webrtc/signaling/test/jsep_track_unittest.cpp:265
(Diff revision 1)
>          return nullptr;
>        }
>        const std::vector<JsepCodecDescription*>& codecs =
>          track.GetNegotiatedDetails()->GetEncoding(0).GetCodecs();
> -      if (codecs.size() != expectedSize ||
> -          codecs[codecIndex]->mType != SdpMediaSection::kVideo) {
> +      if (codecs.size() != expectedSize || codecIndex >= expectedSize ||
> +          codecs[codecIndex]->mType != type) {

Maybe check the type of the track in the first if block?

::: media/webrtc/signaling/test/jsep_track_unittest.cpp:397
(Diff revision 1)
> +    void RemoveFmtp(SdpMediaSection& mediaSection, const char* pt)
> +    {
> +      UniquePtr<SdpFmtpAttributeList> fmtps(new SdpFmtpAttributeList);
> +
> +      SdpAttributeList& attrList = mediaSection.GetAttributeList();
> +      if (attrList.HasAttribute(SdpAttribute::kFmtpAttribute)) {
> +        *fmtps = attrList.GetFmtp();
> +      }
> +      for (size_t i=0; i<fmtps->mFmtps.size(); ++i) {
> +        if (pt == fmtps->mFmtps[i].format) {
> +          fmtps->mFmtps.erase(fmtps->mFmtps.begin()+i);
> +          break;
> +        }
> +      }
> +      attrList.SetAttribute(fmtps.release());
> +    }

Let's make this a function in SdpMediaSection.

::: media/webrtc/signaling/test/sdp_unittests.cpp:1727
(Diff revision 1)
> +    static_cast<SdpFmtpAttributeList::TelephoneEventParameters*>(
> +        audio_format_params[1].parameters.get());
> +  ASSERT_NE(0U, te_parameters->dtmfTones.size());
> +  ASSERT_EQ("5,6,7", te_parameters->dtmfTones);
> +}
> +

a=fmtp:101 0 is valid right? We should have a test for that.
(Assignee)

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8791048 [details]
Bug 1291714 - sdp changes to support DTMF signaling.

https://reviewboard.mozilla.org/r/78608/#review77298

> Hmm. What if someone puts a telephone-event in a video m-section? (Or for that matter, a red or ulpfec in an audio m-section) Do we perform a bad cast?

JsepTrack has a MediaType and refuses to add Codecs that differ in type.
JsepTrack also won't add codecs to SdpMediaSection that differ in type.
In addition, during negotiation, JsepCodecDescription::Matches will not match if the codec description's MediaType is not equal to the SdpMediaSection's MediaSection.
I think we're safe here, but I'll write a couple of of tests in jsep_track_unittest to act as a canary in case this behavior changes.
(Assignee)

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8791048 [details]
Bug 1291714 - sdp changes to support DTMF signaling.

https://reviewboard.mozilla.org/r/78608/#review77298

> Won't this always be true if the strspn == strlen?
> 
> (Provided strlen is at least 2, do we check?)

Good catch on both.  I've added a new funtion that does a much more complete job making sure this string is legal.
(Assignee)

Comment 9

2 years ago
mozreview-review-reply
Comment on attachment 8791048 [details]
Bug 1291714 - sdp changes to support DTMF signaling.

https://reviewboard.mozilla.org/r/78608/#review77298

> If renegotiation turns DTMF off, do we set this to false?

I added a test in jsep_session_unittest.cpp (RenegotiationOffererDisablesTelephoneEvent) to verify.  Each time JsepTrack::NegotiateCodecs is called, it starts with a fresh clone of the prototype codecs, which all start with mDtmfEnabled = false.
(Assignee)

Comment 10

2 years ago
mozreview-review-reply
Comment on attachment 8791048 [details]
Bug 1291714 - sdp changes to support DTMF signaling.

https://reviewboard.mozilla.org/r/78608/#review77252

> Rather than make a separate permission for this, you could use "media.peerconnection.dtmf.enabled" which is what I was planning to use to determine whether or not DTMF would be exposed to DOM.
> 
> Also, as I gave it some more thought, I'm not sure if we need to hide this behind a pref - unlike some of the other options here, whether or not DTMF is generated is more or less controlled by the user, rather than negotiated behind the scenes.

Because of bug 814227 I want to make sure we have the ability to turn off the sdp generation.  I have modified the code to use your pref name.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8793454 [details]
Bug 1291714 - sdp changes to support DTMF signaling.

https://reviewboard.mozilla.org/r/80194/#review78856

::: media/webrtc/signaling/src/sdp/sipcc/sdp_attr.c:483
(Diff revision 2)
> +  PL_strncpyz(dtmf_tones, fmtpVal, sizeof(dtmf_tones));
> +
> +  char *strtok_state;
> +  char *temp = PL_strtok_r(dtmf_tones, ",", &strtok_state);
> +
> +  while (temp != NULL) {

It looks like this will accept stuff like ",,,,,,,"

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:1894
(Diff revision 2)
> +      ASSERT_EQ(1U, details->GetEncodingCount());
> +      for (size_t j = 0; j < details->GetEncodingCount(); ++j) {
> +        const JsepTrackEncoding& encoding = details->GetEncoding(j);

No need to loop here, because of the assert.

::: media/webrtc/signaling/test/sdp_unittests.cpp:1792
(Diff revision 2)
> +  ASSERT_TRUE(mSdp->GetMediaSection(0).GetAttributeList().HasAttribute(
> +              SdpAttribute::kFmtpAttribute));
> +  auto audio_format_params =
> +      mSdp->GetMediaSection(0).GetAttributeList().GetFmtp().mFmtps;
> +  ASSERT_EQ(2U, audio_format_params.size());
> +
> +  ASSERT_EQ("101", audio_format_params[1].format);
> +  ASSERT_TRUE(!!audio_format_params[1].parameters);
> +  const SdpFmtpAttributeList::TelephoneEventParameters* te_parameters =
> +    static_cast<SdpFmtpAttributeList::TelephoneEventParameters*>(
> +        audio_format_params[1].parameters.get());
> +  ASSERT_NE(0U, te_parameters->dtmfTones.size());
> +  ASSERT_EQ("1", te_parameters->dtmfTones);

Lots of copy-paste here, maybe could break out into a separate function that checks for a specific dtmf string.

Comment 15

2 years ago
mozreview-review
Comment on attachment 8793454 [details]
Bug 1291714 - sdp changes to support DTMF signaling.

https://reviewboard.mozilla.org/r/80194/#review79072
Attachment #8793454 - Flags: review?(docfaraday)

Comment 16

2 years ago
mozreview-review
Comment on attachment 8791048 [details]
Bug 1291714 - sdp changes to support DTMF signaling.

https://reviewboard.mozilla.org/r/78608/#review79074

Review will happen on subsequent patches.
Attachment #8791048 - Flags: review?(docfaraday) → review+
(Assignee)

Comment 17

2 years ago
mozreview-review-reply
Comment on attachment 8793454 [details]
Bug 1291714 - sdp changes to support DTMF signaling.

https://reviewboard.mozilla.org/r/80194/#review78856

> It looks like this will accept stuff like ",,,,,,,"

It would have caught the leading ",", but not the case where that happened in the middle of the string.  Thanks for catching that.  I added tests for leading and trailing hyphens as well.
Comment hidden (mozreview-request)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8793454 [details]
Bug 1291714 - sdp changes to support DTMF signaling.

https://reviewboard.mozilla.org/r/80194/#review79240
Attachment #8793454 - Flags: review?(docfaraday) → review+
Comment hidden (mozreview-request)

Comment 21

2 years ago
mozreview-review
Comment on attachment 8796632 [details]
Bug 1291714 - bool to enable dtmf tones in AudioConduit and setup dtmf payload type.

https://reviewboard.mozilla.org/r/82420/#review81018

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:225
(Diff revision 1)
> +  if (!mVoiceEngine) {
> +    return false;
> +  }

Hmm. Should this be possible if this object is being used correctly? Maybe we need an assert here? Maybe some logging too.

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:238
(Diff revision 1)
> +  if (!mPtrVoEDtmf) {
> +    CSFLogError(logTag, "%s Unable to initialize VoEDtmf", __FUNCTION__);
> +    return false;
> +  }
> +
> +  int result = mPtrVoEDtmf->SetSendTelephoneEventPayloadType(mChannel, type);

Let's log something if this doesn't work.

Comment 22

2 years ago
mozreview-review
Comment on attachment 8796632 [details]
Bug 1291714 - bool to enable dtmf tones in AudioConduit and setup dtmf payload type.

https://reviewboard.mozilla.org/r/82420/#review81026

With nits.
Attachment #8796632 - Flags: review?(docfaraday) → review+
(Assignee)

Comment 23

2 years ago
mozreview-review-reply
Comment on attachment 8796632 [details]
Bug 1291714 - bool to enable dtmf tones in AudioConduit and setup dtmf payload type.

https://reviewboard.mozilla.org/r/82420/#review81018

> Hmm. Should this be possible if this object is being used correctly? Maybe we need an assert here? Maybe some logging too.

You're right, just being paranoid.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Seems like folding the review fixes into the final commit instead of landing them separately would have been a good idea.
Patch needs rebasing anyway. Please go ahead and fold the review commit while you're at it.
Keywords: checkin-needed
(Assignee)

Comment 27

2 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #26)
> Patch needs rebasing anyway. Please go ahead and fold the review commit
> while you're at it.
I will fold the review fixes, but can you explain why it needs rebasing?  I purposely applied the 3 patches to a clean checkout of central late yesterday, and inbound late today, and they apply cleanly here.  I'd like to better understand what I'm missing so I make sure things go more smoothly next time.
Flags: needinfo?(ryanvm)
(In reply to Michael Froman [:mjf] from comment #27)
> can you explain why it needs rebasing?

patching file media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
Hunk #1 FAILED at 2134
1 out of 1 hunks FAILED -- saving rejects to file media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp.rej
patching file media/webrtc/signaling/test/jsep_session_unittest.cpp
Hunk #2 FAILED at 2840
1 out of 2 hunks FAILED -- saving rejects to file media/webrtc/signaling/test/jsep_session_unittest.cpp.rej
patch failed to apply

Looks like you got bitrotted by bug 1304165 and/or bug 1306873.
Flags: needinfo?(ryanvm)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8791048 - Attachment is obsolete: true
(Assignee)

Comment 31

2 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #26)
> Patch needs rebasing anyway. Please go ahead and fold the review commit
> while you're at it.

Folded and rebased.
Keywords: checkin-needed

Comment 32

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d61cef181657
sdp changes to support DTMF signaling. r=bwc
https://hg.mozilla.org/integration/autoland/rev/3df9873b4a0d
bool to enable dtmf tones in AudioConduit and setup dtmf payload type. r=bwc
Keywords: checkin-needed

Comment 33

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d61cef181657
https://hg.mozilla.org/mozilla-central/rev/3df9873b4a0d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.