1160558 - Update SDP for datachannels to match draft 21

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P2)

Description

[Eric Rescorla (:ekr)]

The specification -14 (https://tools.ietf.org/html/draft-ietf-mmusic-sctp-sdp-14#section-4.5) requires the following syntax:

  m=application 12345 UDP/DTLS/SCTP webrtc-datachannel
  a=max-message-size: 100000


But this is not what we generate:

v=0 o=mozilla...THIS_IS_SDPARTA-40.0a1 1869052075196006211 0 IN IP4 0.0.0.0 s=- t=0 0 a=fingerprint:sha-256 BE:EB:6C:2F:07:72:A0:B8:CC:A3:7F:A8:34:16:97:E6:74:38:21:12:1B:69:75:FC:DA:02:A7:F6:ED:29:1B:07 a=group:BUNDLE sdparta_0 a=ice-options:trickle a=msid-semantic:WMS * m=application 9 DTLS/SCTP 5000 c=IN IP4 0.0.0.0 a=sendrecv a=ice-pwd:cfb8c4bd4d62b72c0006b132fecb13a1 a=ice-ufrag:859aaed6 a=mid:sdparta_0 a=sctpmap:5000 webrtc-datachannel 256 a=setup:actpass

Comment 1

[Eric Shepherd [:sheppy]]

Docs need updating if any compatibility issues arise, or if this affects Web apps that may fiddle with the SDP directly.

Comment 2

[Nils Ohlmeier [:drno]]

Lets use this as the reference to be implemented: https://datatracker.ietf.org/doc/draft-ietf-mmusic-sctp-sdp/

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: Bug 1160558: add support to parse and respond to sctp-sdp-21.

Review commit: https://reviewboard.mozilla.org/r/106348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106348/

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 8829198 [details]
Bug 1160558: add support to parse and respond to sctp-sdp-21.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106348/diff/1-2/

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8829198 [details]
Bug 1160558: add support to parse and respond to sctp-sdp-21.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106348/diff/2-3/

Comment 6

[Nils Ohlmeier [:drno]]

Comment on attachment 8829198 [details]
Bug 1160558: add support to parse and respond to sctp-sdp-21.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106348/diff/3-4/

Comment 7

[Nils Ohlmeier [:drno]]

Comment on attachment 8829198 [details]
Bug 1160558: add support to parse and respond to sctp-sdp-21.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106348/diff/4-5/

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8829198 [details]
Bug 1160558: add support to parse and respond to sctp-sdp-21.

https://reviewboard.mozilla.org/r/106348/#review109308

Some nits here and there, otherwise looks good.

::: media/webrtc/signaling/gtest/sdp_unittests.cpp:3236
(Diff revision 5)
> -const std::string kNewSctpmapOfferDraft07 =
> +const std::string kNewSctpmapOfferDraft21 =
>  "v=0" CRLF
>  "o=Mozilla-SIPUA-35.0a1 27987 0 IN IP4 0.0.0.0" CRLF
>  "s=SIP Call" CRLF
>  "t=0 0" CRLF
>  "a=ice-ufrag:8a39d2ae" CRLF
>  "a=ice-pwd:601d53aba51a318351b3ecf5ee00048f" CRLF
>  "a=fingerprint:sha-256 30:FF:8E:2B:AC:9D:ED:70:18:10:67:C8:AE:9E:68:F3:86:53:51:B0:AC:31:B7:BE:6D:CF:A4:2E:D3:6E:B4:28" CRLF
> -"m=application 9 DTLS/SCTP webrtc-datachannel" CRLF
> +"m=application 9 UDP/DTLS/SCTP webrtc-datachannel" CRLF
>  "c=IN IP4 0.0.0.0" CRLF
> -"a=fmtp:webrtc-datachannel max-message-size=100000" CRLF
> -"a=sctp-port 5000" CRLF
> +"a=sctp-port:5000" CRLF
> +"a=max-message-size:10000" CRLF
>  "a=setup:actpass" CRLF;
>  
>  TEST_P(NewSdpTest, NewSctpmapSdpParse) {
> -  ParseSdp(kNewSctpmapOfferDraft07, false);
> +  ParseSdp(kNewSctpmapOfferDraft21, false);
>  }

Probably should not mention sctpmap in here.

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:789
(Diff revision 5)
> +  virtual void
> +  AddToMediaSection(SdpMediaSection& msection) const override
> +  {
> +    if (mEnabled && msection.GetMediaType() == mType) {
> +      // Both send and recv codec will have the same pt, so don't add twice
> +      if (!msection.HasFormat(mDefaultPt)) {

It might be clearer if we do msection.GetFormats().empty() here; mDefaultPt isn't set right?

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:790
(Diff revision 5)
> +  AddToMediaSection(SdpMediaSection& msection) const override
> +  {
> +    if (mEnabled && msection.GetMediaType() == mType) {
> +      // Both send and recv codec will have the same pt, so don't add twice
> +      if (!msection.HasFormat(mDefaultPt)) {
> +        if (mType == SdpMediaSection::kApplication) {

This should always be true, right?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1746
(Diff revision 5)
> +        if (!SdpHelper::GetPtAsInt(fmt, &payloadType)) {
> +          JSEP_SET_ERROR("Payload type \"" << fmt <<
> +                         "\" is not a 16-bit unsigned int at level " << i);
> +          return NS_ERROR_INVALID_ARG;
> +        }
>          // TODO (bug 1204099): Make this check for reserved ranges.

/me puts on juberti hat.

We've handled this TODO, go ahead and delete it.

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:56
(Diff revision 5)
>  
>    for (JsepCodecDescription* codec : *codecs) {
>      // We assume there are no dupes in negotiated codecs; unnegotiated codecs
>      // need to change if there is a clash.
> -    if (!codec->mEnabled) {
> +    if (!codec->mEnabled ||
> +        !codec->mName.compare("webrtc-datachannel")) {

Might want a comment here about assuming that there is never a duplicate 'webrtc-datachannel'.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:714
(Diff revision 5)
>    void SendLocalIceCandidateToContent(uint16_t level,
>                                        const std::string& mid,
>                                        const std::string& candidate);
>  
>    nsresult GetDatachannelParameters(
> -      const mozilla::JsepApplicationCodecDescription** codec,
> +      //const mozilla::JsepApplicationCodecDescriptionOld** codec,

?

::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:704
(Diff revision 5)
>  SdpMediaSection::Protocol
>  SdpHelper::GetProtocolForMediaType(SdpMediaSection::MediaType type)
>  {
>    if (type == SdpMediaSection::kApplication) {
>      return SdpMediaSection::kDtlsSctp;
> +    // TODO switch to offer the new SCTP SDP (Bug XXX)

Let's file this bug.

Comment 9

[Nils Ohlmeier [:drno]]

dev-doc-needed moved over to bug 1335206, since we only need to document this once we start sending the new format.

Comment 10

[Nils Ohlmeier [:drno]]

Comment on attachment 8829198 [details]
Bug 1160558: add support to parse and respond to sctp-sdp-21.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106348/diff/5-6/

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8829198 [details]
Bug 1160558: add support to parse and respond to sctp-sdp-21.

https://reviewboard.mozilla.org/r/106348/#review109666

r+ with nits.

::: media/webrtc/signaling/gtest/jsep_track_unittest.cpp:1107
(Diff revisions 5 - 6)
> +  ASSERT_NE(mOffer->ToString().find("a=sctpmap:5999 webrtc-datachannel 256"),
> +      std::string::npos);
> +  ASSERT_NE(mAnswer->ToString().find("a=sctpmap:5999 webrtc-datachannel 256"),
> +      std::string::npos);
> +  ASSERT_EQ(mOffer->ToString().find("a=sctp-port"), std::string::npos);
> +  ASSERT_EQ(mAnswer->ToString().find("a=sctp-port"), std::string::npos);

The logging for this stuff is easier to read if the expected value is the first argument, and the observed is the second.

::: media/webrtc/signaling/gtest/jsep_track_unittest.cpp:1135
(Diff revisions 5 - 6)
> +  ASSERT_NE(mOffer->ToString().find("a=sctpmap:4555 webrtc-datachannel 256"),
> +      std::string::npos);
> +  ASSERT_NE(mAnswer->ToString().find("a=sctpmap:5999 webrtc-datachannel 256"),
> +      std::string::npos);
> +  ASSERT_EQ(mOffer->ToString().find("a=sctp-port"), std::string::npos);
> +  ASSERT_EQ(mAnswer->ToString().find("a=sctp-port"), std::string::npos);

Same thing here.

::: media/webrtc/signaling/gtest/jsep_track_unittest.cpp:1170
(Diff revisions 5 - 6)
> +  ASSERT_NE(mOffer->ToString().find("a=sctp-port:5999"), std::string::npos);
> +  ASSERT_NE(mAnswer->ToString().find("a=sctp-port:5999"), std::string::npos);
> +  ASSERT_EQ(mOffer->ToString().find("a=sctpmap"), std::string::npos);
> +  ASSERT_EQ(mAnswer->ToString().find("a=sctpmap"), std::string::npos);

And here.

Comment 12

[Nils Ohlmeier [:drno]]

Comment on attachment 8829198 [details]
Bug 1160558: add support to parse and respond to sctp-sdp-21.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106348/diff/6-7/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7f2dab158b9
add support to parse and respond to sctp-sdp-21. r=bwc

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d7f2dab158b9