1438290 - Implement RsdparsaSdpMediaSection::AddDataChannel

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, enhancement, P3)

Comment 1

[Johannes Willbold]

Attachment: Bug 1438290: Part 1: Implemented RsdparsaSdpMediaSection::AddDataChannel,

Implemented RsdparsaSdpMediaSection::AddDataChannel
Added C++/Rust glue code for the "sctp port" attribute
Added C++/Rust glue code for the "max message size" attribute

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

Comment 2

[Johannes Willbold]

Attachment: Bug 1438290: Part 2: Added testcases for AddDataChannel,

Added C++ unit test CheckAddDataChannel
Added C++ unit test CheckAddDataChannel_Draft05

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

Comment 3

[Dan Minor [:dminor]]

Comment on attachment 8986945 [details]
Bug 1438290: Part 1: Implemented RsdparsaSdpMediaSection::AddDataChannel,

https://reviewboard.mozilla.org/r/252172/#review258870

lgtm

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:1080
(Diff revision 1)
>  
>  void
> +RsdparsaSdpAttributeList::LoadSctpPort(RustAttributeList* attributeList)
> +{
> +  int64_t port = sdp_get_sctp_port(attributeList);
> +  if (port >= 0) {

Might as well check that port is <= 65535.

Comment 4

[Johannes Willbold]

Comment on attachment 8986945 [details]
Bug 1438290: Part 1: Implemented RsdparsaSdpMediaSection::AddDataChannel,

https://reviewboard.mozilla.org/r/252172/#review258870

> Might as well check that port is <= 65535.

Actually this is not a validity check for the port but a check whether the rust call was successfull. In case the rust lookup fails, it will return -1. 
In addition, the rust parser is checking that already.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8986945 [details]
Bug 1438290: Part 1: Implemented RsdparsaSdpMediaSection::AddDataChannel,

https://reviewboard.mozilla.org/r/252172/#review259274

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:817
(Diff revision 1)
>  }
>  
>  void
> +RsdparsaSdpAttributeList::LoadMaxMessageSize(RustAttributeList* attributeList)
> +{
> +  int64_t max_msg_size = sdp_get_max_msg_sizse(attributeList);

s/sizse/size/

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8986946 [details]
Bug 1438290: Part 2: Added testcases for AddDataChannel,

https://reviewboard.mozilla.org/r/252174/#review259304

::: media/webrtc/signaling/gtest/sdp_unittests.cpp:3988
(Diff revision 1)
> +  ParseSdp("v=0" CRLF
> +           "o=- 4294967296 2 IN IP4 127.0.0.1" CRLF
> +           "s=SIP Call" CRLF
> +           "c=IN IP4 198.51.100.7" CRLF
> +           "b=CT:5000" CRLF
> +           "t=0 0" CRLF
> +           "m=video 56436 RTP/SAVPF 120 110" CRLF
> +           "a=rtpmap:120 VP8/90000" CRLF
> +           "a=sendonly" CRLF
> +           "a=rtpmap:110 opus/48000/2" CRLF);
> +
> +  ASSERT_TRUE(!!mSdp) << "Parse failed: " << GetParseErrors();
> +  ASSERT_EQ(1U, mSdp->GetMediaSectionCount());

Hmm. I would have expected this test case to involve building the m-section with AddMediaSection instead of parsing it, but ok.

In this case, let's make this SDP identical to the one below, except substitute "DTLS/SCTP" for "UDP/DTLS/SCTP". Calling AddDataChannel on anything other than m=application probably should MOZ_CRASH, honestly.

Comment 7

[Johannes Willbold]

Comment on attachment 8986945 [details]
Bug 1438290: Part 1: Implemented RsdparsaSdpMediaSection::AddDataChannel,

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

Comment 8

[Johannes Willbold]

Comment on attachment 8986946 [details]
Bug 1438290: Part 2: Added testcases for AddDataChannel,

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

Comment 9

[Johannes Willbold]

Comment on attachment 8986946 [details]
Bug 1438290: Part 2: Added testcases for AddDataChannel,

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

Comment 10

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 0dae344aee446d53b4866a035db72094fee6157e -d 02a46b307edd: rebasing 469965:0dae344aee44 "Bug 1438290: Part 1: Implemented RsdparsaSdpMediaSection::AddDataChannel, r=dminor"
merging media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp
merging media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.h
merging media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h
merging media/webrtc/signaling/src/sdp/rsdparsa/src/media_type.rs
merging media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs
warning: conflicts while merging media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.h! (edit, then use 'hg resolve --mark')
warning: conflicts while merging media/webrtc/signaling/src/sdp/rsdparsa/src/media_type.rs! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 11

[Johannes Willbold]

Comment on attachment 8986945 [details]
Bug 1438290: Part 1: Implemented RsdparsaSdpMediaSection::AddDataChannel,

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

Comment 12

[Johannes Willbold]

Comment on attachment 8986946 [details]
Bug 1438290: Part 2: Added testcases for AddDataChannel,

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d41ad8a48d45
Part 1: Implemented RsdparsaSdpMediaSection::AddDataChannel, r=dminor
https://hg.mozilla.org/integration/autoland/rev/afd967c185fe
Part 2: Added testcases for AddDataChannel, r=bwc

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d41ad8a48d45
https://hg.mozilla.org/mozilla-central/rev/afd967c185fe