Closed Bug 1291714 Opened 8 years ago Closed 8 years ago

Add signaling support for DTMF in WebRTC

Categories

(Core :: WebRTC: Signaling, defect, P1)

Product:
Core
Component:
WebRTC: Signaling
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: dminor, Assigned: mjf)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1291714 - sdp changes to support DTMF signaling.
58 bytes, text/x-review-board-request
bwc
: review+
Details
Bug 1291714 - bool to enable dtmf tones in AudioConduit and setup dtmf payload type.
58 bytes, text/x-review-board-request
bwc
: review+
Details 
This bug covers signaling changes required to enable DTMF in WebRTC.
Rank: 15
Assignee: nobody → mfroman
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 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.
(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 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.
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.
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.
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.
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 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 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 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+
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 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 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 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+
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.
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
(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)
Attachment #8791048 - Attachment is obsolete: true
(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
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
https://hg.mozilla.org/mozilla-central/rev/d61cef181657
https://hg.mozilla.org/mozilla-central/rev/3df9873b4a0d
Status: NEW → RESOLVED
Closed: 8 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.

Attachment

General

Creator:
Created:
Updated:
Size: