In order to make progress on things like multistream and renegotition support, we need to rewrite most of the functionality currently supplied by sipcc, because of numerous baked-in limitations in the same.

The work at https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase is essentially at feature-parity with the code on m-c. I have translated the work over to a single hg patch and tossed it at try, and will be breaking up the monolithic patch later today and attaching to this bug. https://tbpl.mozilla.org/?tree=Try&rev=ef62d8bb26b6

Decomposed patch.

%z is not supported on MSVC

%z is not supported on MSVC

Not sure why those failures did not show up in my earlier try pushes, but they should be fixed now. https://tbpl.mozilla.org/?tree=Try&rev=e85d0528b374

Byron, I would suggest that these patches would be best reviewed on ReviewBoard if that's available now

I'll try that.

I'm working on chopping up part 5 into three parts that should be much easier to review (there's a large diff of ws-only changes, a huge diff that just replaces sipcc's integral types with modern integral types, and then a much smaller diff of actual functionality changes).

Splitting off whitespace-only changes.

Actual functionality changes to sipcc sdp code.

Use modern integral types in sipcc code; basically a big search/replace.

Re-enable the old SDP test-cases.

Comment on attachment 8514439 [details] [diff] [review] Part 1: SDP wrapper code around sipcc sdp impl. Review of attachment 8514439 [details] [diff] [review]: ----------------------------------------------------------------- https://reviewboard.allizom.org/r/85/

Comment on attachment 8513869 [details] [diff] [review] Part 2: New JSEP handling code. Review of attachment 8513869 [details] [diff] [review]: ----------------------------------------------------------------- https://reviewboard.allizom.org/r/87/

Comment on attachment 8513871 [details] [diff] [review] Part 3: Mochitest work. Review of attachment 8513871 [details] [diff] [review]: ----------------------------------------------------------------- https://reviewboard.allizom.org/r/88/

Comment on attachment 8513873 [details] [diff] [review] Part 4: Remove most of sipcc, and move just the sdp stuff into a new location. Review of attachment 8513873 [details] [diff] [review]: ----------------------------------------------------------------- https://reviewboard.allizom.org/r/89/diff/

Comment on attachment 8514428 [details] [diff] [review] Part 5.1: Whitespace-only modifications to sipcc sdp code. Review of attachment 8514428 [details] [diff] [review]: ----------------------------------------------------------------- https://reviewboard.allizom.org/r/90/

Comment on attachment 8514429 [details] [diff] [review] Part 5.2: Functionality changes to sipcc sdp code. Review of attachment 8514429 [details] [diff] [review]: ----------------------------------------------------------------- https://reviewboard.allizom.org/r/91/

Comment on attachment 8514431 [details] [diff] [review] Part 5.3: Use modern integral types in sipcc code. Review of attachment 8514431 [details] [diff] [review]: ----------------------------------------------------------------- https://reviewboard.allizom.org/r/92/

Comment on attachment 8514339 [details] [diff] [review] Part 6: Wiring the new JSEP handler code in. Review of attachment 8514339 [details] [diff] [review]: ----------------------------------------------------------------- https://reviewboard.allizom.org/r/93/

Comment on attachment 8513877 [details] [diff] [review] Part 7: Wiring the build system together. Review of attachment 8513877 [details] [diff] [review]: ----------------------------------------------------------------- https://reviewboard.allizom.org/r/94/

Comment on attachment 8514340 [details] [diff] [review] Part 8: When running on tbpl, disable parts of ice_unittest that rely on external network. Review of attachment 8514340 [details] [diff] [review]: ----------------------------------------------------------------- https://reviewboard.allizom.org/r/95/

Regarding formatting; the plan is to run all this stuff through clang-format once we're ready to land to iron out irregularities. There are naming-style inconsistencies where different authors worked on different parts of the code, I'll look into smoothing that out as well.

I just pulled down all thees patches and applied them to M-C on OSX. In my testing so far, talky.io worked connected to a FF33 and the landing page worked http://mozilla.github.io/webrtc-landing/pc_test.html with the exception of when you specify to use H264. There is one difference is the offer params for H264, but I'm not sure yet whether that is the problem. I will dig into it later unless you already know why this is the case.

I think I did some H264 testing a while ago, but some stuff has changed since then.

Comment on attachment 8514428 [details] [diff] [review] Part 5.1: Whitespace-only modifications to sipcc sdp code. Review of attachment 8514428 [details] [diff] [review]: ----------------------------------------------------------------- The diff in reviewboard here - https://reviewboard.allizom.org/r/90/diff/#index_header Only has five files changed, this one shows 10 files changed. Should I do my reviews here or there?

(In reply to Ethan Hugg [:ehugg] from comment #34) > Comment on attachment 8514428 [details] [diff] [review] > Part 5.1: Whitespace-only modifications to sipcc sdp code. > > Review of attachment 8514428 [details] [diff] [review]: > ----------------------------------------------------------------- > > The diff in reviewboard here - > https://reviewboard.allizom.org/r/90/diff/#index_header > Only has five files changed, this one shows 10 files changed. Should I do > my reviews here or there? I'm not sure what reviewboard is doing there; the patchfile it has definitely has all of the files in it (you can check by downloading the patch from reviewboard). For some reason it is choosing not to display some of the files...

It seems that reviewboard hides leading-whitespace changes, judging by which changes it is choosing to display.

(In reply to Byron Campen [:bwc] from comment #36) > It seems that reviewboard hides leading-whitespace changes, judging by which > changes it is choosing to display. OK, I'll assume the other patches won't do this then. Thanks.

Comment on attachment 8514428 [details] [diff] [review] Part 5.1: Whitespace-only modifications to sipcc sdp code. Review of attachment 8514428 [details] [diff] [review]: ----------------------------------------------------------------- Reviewed here: https://reviewboard.allizom.org/r/90/

Comment on attachment 8513873 [details] [diff] [review] Part 4: Remove most of sipcc, and move just the sdp stuff into a new location. Review of attachment 8513873 [details] [diff] [review]: ----------------------------------------------------------------- lgtm - https://reviewboard.allizom.org/r/89/

Comment on attachment 8514431 [details] [diff] [review] Part 5.3: Use modern integral types in sipcc code. Review of attachment 8514431 [details] [diff] [review]: ----------------------------------------------------------------- https://reviewboard.allizom.org/r/92/

Incorporate feedback.

Comment on attachment 8515299 [details] [diff] [review] Part 8: When running on tbpl, disable parts of ice_unittest that rely on external network. Review of attachment 8515299 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Comment on attachment 8514339 [details] [diff] [review] Part 6: Wiring the new JSEP handler code in. Review of attachment 8514339 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp @@ +647,5 @@ > + bool send) > +{ > + if (config->mName == "VP8") { > + return kMediaConduitNoError; > + } else if (config->mName == "H264") { I got H264 to work with the loopback page - http://mozilla.github.io/webrtc-landing/pc_test.html by hacking this line to: } else if (config->mName == "H264" && config->mType == 126) { Otherwise it goes through this again with an mType of 97 and sets the mExternalRecvCodec again. VideoConduit.cpp:674 has this: codecConfigList[i]->mType == mExternalRecvCodec->mType) { which then will not match 126 and all of the incoming RTP will be rejected by the webrtc code. Looking at M-C, this may be an existing bug and possibly the cause of this: https://bugzilla.mozilla.org/show_bug.cgi?id=1073475

Incorporate feedback.

Comment on attachment 8516372 [details] [diff] [review] Part 2: New JSEP handling code. Review of attachment 8516372 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r=ehugg

Comment on attachment 8513871 [details] [diff] [review] Part 3: Mochitest work. Lots of nits. Would like to see it again if you take me up on some of the bigger changes, but not essential. https://reviewboard.allizom.org/r/88/

Continuing the thread from comment 43 - The reason this problem shows up here is that the answer contains all of the video codecs, but the released FF code only returns one. We only have one mExternalRecvCodec data member but we try to create two of them because both H264 lines are present. So we probably need to do this: 1. Change CreateAnswer to be the same as release and only respond with one codec. I haven't tested with old versions of FF but I'm guessing that this will be needed in order to connect H264 with them in the case where this new version does the answer. 2. Change mExternalRecvCodec to a Vector type so it can handle an answer with multiple video codecs listed. That should also fix bug 1073475. Once that change is in place for a few versions we could revert #1 and return multiple codecs in the answer again if that is desired. Other thoughts on this? I think it's important for these patches to land being able to connect H264 at least to itself.

(In reply to Ethan Hugg [:ehugg] from comment #47) > Continuing the thread from comment 43 - The reason this problem shows up > here is that the answer contains all of the video codecs, but the released > FF code only returns one. We only have one mExternalRecvCodec data member > but we try to create two of them because both H264 lines are present. So we > probably need to do this: > > 1. Change CreateAnswer to be the same as release and only respond with one > codec. I haven't tested with old versions of FF but I'm guessing that this > will be needed in order to connect H264 with them in the case where this new > version does the answer. Sounds like we will have to do this. > 2. Change mExternalRecvCodec to a Vector type so it can handle an answer > with multiple video codecs listed. That should also fix bug 1073475. Once > that change is in place for a few versions we could revert #1 and return > multiple codecs in the answer again if that is desired. Right. > Other thoughts on this? I think it's important for these patches to land > being able to connect H264 at least to itself.

Comment on attachment 8514429 [details] [diff] [review] Part 5.2: Functionality changes to sipcc sdp code. Review of attachment 8514429 [details] [diff] [review]: ----------------------------------------------------------------- https://reviewboard.allizom.org/r/91/

Incorporate feedback.

Comment on attachment 8516997 [details] [diff] [review] Part 3: Mochitest work. Review of attachment 8516997 [details] [diff] [review]: ----------------------------------------------------------------- Made some of the larger changes, new patch up reviewboard too.

Comment on attachment 8514439 [details] [diff] [review] Part 1: SDP wrapper code around sipcc sdp impl. Review of attachment 8514439 [details] [diff] [review]: ----------------------------------------------------------------- https://reviewboard.allizom.org/r/85/

Incorporate feedback.

Unrot.

Comment on attachment 8517018 [details] [diff] [review] Part 5.2: Functionality changes to sipcc sdp code. Review of attachment 8517018 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r=ehugg

Update commit log

Comment on attachment 8517022 [details] [diff] [review] Part 5.2: Functionality changes to sipcc sdp code. Review of attachment 8517022 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r=ehugg, r=pkerr

Update commit log

Comment on attachment 8517023 [details] [diff] [review] Part 5.3: Use modern integral types in sipcc code. Review of attachment 8517023 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r=ehugg

Comment on attachment 8514339 [details] [diff] [review] Part 6: Wiring the new JSEP handler code in. Review of attachment 8514339 [details] [diff] [review]: ----------------------------------------------------------------- I talked with Randell, he's going to do this one.

Comment on attachment 8514439 [details] [diff] [review] Part 1: SDP wrapper code around sipcc sdp impl. r+ but there are some nits to resolve that may require quick interdiff review

Lots of fixes. See reviewboard for summary.

Lots of fixes, see reviewboard for more detail.

Move a macro required by an update to part 1.

Adjust for API movement in parts 1 and 2.

Compensate for files removed in latest part 2.

Comment on attachment 8518362 [details] [diff] [review] Part 1: SDP wrapper code around sipcc sdp impl. If you could review the rewritten SipccSdpAttributeList::LoadFingerprint, that would be good.

Comment on attachment 8518377 [details] [diff] [review] Part 5.2: Functionality changes to sipcc sdp code. Carry forward r=ehugg, r=pkerr

Comment on attachment 8516997 [details] [diff] [review] Part 3: Mochitest work. r=me with nit fixed.

Fixup a couple of things from my own review pass, and incorporate more feedback.

Comment on attachment 8519480 [details] [diff] [review] Part 3: Mochitest work. Review of attachment 8519480 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r=jib

Comment on attachment 8518362 [details] [diff] [review] Part 1: SDP wrapper code around sipcc sdp impl. Review of attachment 8518362 [details] [diff] [review]: ----------------------------------------------------------------- r+, but note per the request I only examined LoadFingerprint. For the rest of it if I need to review changes from the last version I reviewed *please* provide interdiffs. Right now I'm assuming there's nothing else that needed re-review ::: media/webrtc/signaling/src/sdp/SipccSdpAttributeList.cpp @@ +195,5 @@ > + } > +} > + > +bool > +SipccSdpAttributeList::LoadFingerprint(sdp_t* sdp, uint16_t level, SdpErrorHolder& errorHolder) { please comment here with the grammar for the fingerprint we're parsing @@ +216,5 @@ > + std::string fingerprintAttr(value); > + uint32_t lineNumber = sdp_attr_line_number(sdp, SDP_ATTR_DTLS_FINGERPRINT, level, 0, i); > + > + // sipcc does not expose parse code for this > + size_t start = fingerprintAttr.find_first_not_of(" \t"); Could it expose it? Would that help?

You can get interdiffs out of reviewboard, there should be a box you can click on the "View Diff" tab.

(In reply to Randell Jesup [:jesup] from comment #72) > Comment on attachment 8518362 [details] [diff] [review] > Part 1: SDP wrapper code around sipcc sdp impl. > > Review of attachment 8518362 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+, but note per the request I only examined LoadFingerprint. For the rest > of it if I need to review changes from the last version I reviewed *please* > provide interdiffs. Right now I'm assuming there's nothing else that needed > re-review > > ::: media/webrtc/signaling/src/sdp/SipccSdpAttributeList.cpp > @@ +195,5 @@ > > + } > > +} > > + > > +bool > > +SipccSdpAttributeList::LoadFingerprint(sdp_t* sdp, uint16_t level, SdpErrorHolder& errorHolder) { > > please comment here with the grammar for the fingerprint we're parsing > Good point, will do. > @@ +216,5 @@ > > + std::string fingerprintAttr(value); > > + uint32_t lineNumber = sdp_attr_line_number(sdp, SDP_ATTR_DTLS_FINGERPRINT, level, 0, i); > > + > > + // sipcc does not expose parse code for this > > + size_t start = fingerprintAttr.find_first_not_of(" \t"); > > Could it expose it? Would that help? This is poorly worded. Will s/expose/have/

Comment on attachment 8518375 [details] [diff] [review] Part 2: New JSEP handling code. Review of attachment 8518375 [details] [diff] [review]: ----------------------------------------------------------------- r+, with major style nits, and one serious question (likely for code not in this patch) triggered by looking at the unittest code ::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h @@ +99,5 @@ > + virtual JsepCodecDescription* MakeNegotiatedCodec( > + const mozilla::SdpMediaSection& remoteMsection, > + const std::string& pt, > + bool sending) const { > + JsepCodecDescription* negotiated = Clone(); I note you're having to do manual lifetimes for negotiated ("delete negotiated;" in error cases and similar vars in other routines here (fmtp, etc)). Why not use a UniquePtr or equivalent? @@ +280,5 @@ > + } > + } > + > + virtual void AddRtcpFbs(SdpRtcpFbAttributeList& rtcpfb) const MOZ_OVERRIDE { > + // Just hard code for now File a bug? @@ +339,5 @@ > + if (mName == "H264") { > + if (!fmtp) { > + // Debatable, but if we assume the default is to allow level > + // asymmetry (an assumption we've been making), and that the default > + // packetization mode is 0, we should match. level-asymmetry should not be the default per previous comments @@ +347,5 @@ > + auto* h264Params = > + static_cast<const SdpFmtpAttributeList::H264Parameters*>(fmtp); > + > + if (!h264Params->level_asymmetry_allowed && > + h264Params->profile_level_id != mProfileLevelId) { Responses for H.264 are allowed to change the ProfileLevelId (an in particular, reduce the level or add constraints). Allowing a lower level in a response is mandatory. A higher level in a response is allowed with level-asymmetry. The comparison for the profile & constraint section here is somewhat complex. Also note that it's a little unclear in rfc 6184 if responding with a "matching" profile & constraints that isn't identical is allowed, however I think it is NOT allowed (8.2.2, first bullet). That means if offered 4D80XX, and we support constrained baseline (42E0xx) normally, we must respond with 4D80xx. This makes life ... interesting. I suggest noting this fast in a comment (link to the RFC and this comment), and needinfo mo zanaty to check with Cisco's codec & SDP people), and if they say it's true, then file a followup bug to do this (and verify it works in the codecs we have!) RFC 6184 has a table (Table 5) that shows the equivalences for profile & constraints. The wording is dense, but effectively there are combinations of profile plus constraints that are equivalent. Constrained Baseline (what we're doing) has the most equivalents in other profiles that have the high bit in the constraints set (see the table). eg something like: if ((a & 0x00FF0000) == 0x00420000) { if ((a & 0x00004000) != 0) { // profile CB if ((b & 0x00FFC000) == 0x0058C000) { return true; } else { switch (b & 0xFF8000) { case 0x004D8000: case 0x00648000: case 0x006E8000: case 0x007A8000: case 0x00F48000: return true; } } } else // profile B { if ((b & 0x00FFC000) == 0x00588000) { return true; } if ((b & 0x00FFE000) == 0x00428000) { // profile CB return true; } } return false; } @@ +355,5 @@ > + if (h264Params->packetization_mode != mPacketizationMode) { > + return false; > + } > + > + // TODO: What else do we need to check here? two payloads for h264 match if their profiles match (see above), and their packetization modes match. (RFC 6184 8.2.2). Note that the payload numbers can be totally different. @@ +393,5 @@ > + > + // H264-specific stuff > + uint32_t mProfileLevelId; > + uint32_t mMaxFr; > + uint32_t mPacketizationMode; overkill for 0..2, but ok ::: media/webrtc/signaling/src/jsep/JsepSession.h @@ +16,5 @@ > +#include "signaling/src/sdp/Sdp.h" > + > + > +namespace mozilla { > +namespace jsep { per the guidelines, I believe we are supposed to avoid defining new sub-namespaces to mozilla ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp @@ +28,5 @@ > +namespace jsep { > + > +MOZ_MTLOG_MODULE("jsep") > + > +JsepSessionImpl::~JsepSessionImpl() { { on new line please ("Class and function definitions are not control structures; left brace goes by itself on the second line and without extra indentation, in general for C++.") apply across all new files @@ +41,5 @@ > + MOZ_ASSERT(!mSessionId, "Init called more than once"); > + > + SECStatus rv = PK11_GenerateRandom( > + reinterpret_cast<unsigned char *>(&mSessionId), sizeof(mSessionId)); > + mSessionId >>= 2; // Discard high order bits. Ummm.. a) indicate why you need to discard two bits, b) discard the correct bits. And why with a shift instead of mask? ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h @@ +18,5 @@ > + > +namespace mozilla { > +namespace jsep { > + > +class JsepUuidGenerator { See comment about bracing - new line for class' '{' please @@ +175,5 @@ > + > + virtual nsresult GetTransport(size_t index, RefPtr<JsepTransport>* transport) > + const MOZ_OVERRIDE { > + if (index >= mTransports.size()) > + return NS_ERROR_INVALID_ARG; brace @@ +191,5 @@ > + > + virtual nsresult GetNegotiatedTrackPair(size_t index, > + const JsepTrackPair** pair) const { > + if (index >= mNegotiatedTrackPairs.size()) > + return NS_ERROR_INVALID_ARG; brace ::: media/webrtc/signaling/src/jsep/JsepTrackImpl.h @@ +22,5 @@ > + public: > + virtual ~JsepTrackNegotiatedDetailsImpl() { > + for (auto c = mCodecs.begin(); c != mCodecs.end(); ++c) { > + delete *c; > + } Annoying we have something we have to manually release... Would std::vector<UniquePtr<JsepCodecDescription>> work? Not a huge deal ::: media/webrtc/signaling/test/jsep_session_unittest.cpp @@ +133,5 @@ > + AddTracks(side, types); > + > + // Now that we have added streams, we expect audio, then video, then > + // application in the SDP, regardless of the order in which the streams were > + // added. Ummm... If we're generating an SDP as a response, that is not in any way correct. It might be correct for this particular test, but we should be testing responding correctly to SDP with the media lines in any order. @@ +564,5 @@ > + "video,audio", > + "audio,datachannel", > + "video,datachannel", > + "video,audio,datachannel", > + "audio,video,datachannel")); Perhaps this may answer my question above, though I don't see datachannel, audio/video [, audio/video] However, it isn't clear to me what this is actually doing. @@ +589,5 @@ > + .GetMediaType()); > + ASSERT_EQ(SdpDirectionAttribute::kRecvonly, outputSdp->GetMediaSection(1) > + .GetAttributeList().GetDirection()); > + ASSERT_EQ(SdpMediaSection::kVideo, outputSdp->GetMediaSection(2) > + .GetMediaType()); And here, clearly, we're checking the ordering of m= lines

(In reply to Randell Jesup [:jesup] from comment #75) > Comment on attachment 8518375 [details] [diff] [review] > Part 2: New JSEP handling code. > > Review of attachment 8518375 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+, with major style nits, and one serious question (likely for code not in > this patch) triggered by looking at the unittest code > > ::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h > @@ +99,5 @@ > > + virtual JsepCodecDescription* MakeNegotiatedCodec( > > + const mozilla::SdpMediaSection& remoteMsection, > > + const std::string& pt, > > + bool sending) const { > > + JsepCodecDescription* negotiated = Clone(); > > I note you're having to do manual lifetimes for negotiated ("delete > negotiated;" in error cases and similar vars in other routines here (fmtp, > etc)). Why not use a UniquePtr or equivalent? > Can do. > @@ +280,5 @@ > > + } > > + } > > + > > + virtual void AddRtcpFbs(SdpRtcpFbAttributeList& rtcpfb) const MOZ_OVERRIDE { > > + // Just hard code for now > > File a bug? > I don't really have any plans to change this. > @@ +339,5 @@ > > + if (mName == "H264") { > > + if (!fmtp) { > > + // Debatable, but if we assume the default is to allow level > > + // asymmetry (an assumption we've been making), and that the default > > + // packetization mode is 0, we should match. > > level-asymmetry should not be the default per previous comments > Actually, that default we were discussing earlier was about what we assume when the other side doesn't specify; sipcc was assuming the absence of level-asymmetry-allowed meant that it was allowed, whereas the spec said the opposite. We've always included level-asymmetry-allowed=1. > @@ +347,5 @@ > > + auto* h264Params = > > + static_cast<const SdpFmtpAttributeList::H264Parameters*>(fmtp); > > + > > + if (!h264Params->level_asymmetry_allowed && > > + h264Params->profile_level_id != mProfileLevelId) { > > Responses for H.264 are allowed to change the ProfileLevelId (an in > particular, reduce the level or add constraints). Allowing a lower level in > a response is mandatory. A higher level in a response is allowed with > level-asymmetry. > > The comparison for the profile & constraint section here is somewhat > complex. Also note that it's a little unclear in rfc 6184 if responding > with a "matching" profile & constraints that isn't identical is allowed, > however I think it is NOT allowed (8.2.2, first bullet). That means if > offered 4D80XX, and we support constrained baseline (42E0xx) normally, we > must respond with 4D80xx. This makes life ... interesting. I suggest > noting this fast in a comment (link to the RFC and this comment), and > needinfo mo zanaty to check with Cisco's codec & SDP people), and if they > say it's true, then file a followup bug to do this (and verify it works in > the codecs we have!) > > RFC 6184 has a table (Table 5) that shows the equivalences for profile & > constraints. The wording is dense, but effectively there are combinations > of profile plus constraints that are equivalent. > Constrained Baseline (what we're doing) has the most equivalents in other > profiles that have the high bit in the constraints set (see the table). > eg something like: > if ((a & 0x00FF0000) == 0x00420000) { > if ((a & 0x00004000) != 0) { // profile CB > if ((b & 0x00FFC000) == 0x0058C000) { > return true; > } else { > switch (b & 0xFF8000) { > case 0x004D8000: > case 0x00648000: > case 0x006E8000: > case 0x007A8000: > case 0x00F48000: > return true; > } > } > } > else // profile B > { > if ((b & 0x00FFC000) == 0x00588000) { > return true; > } > if ((b & 0x00FFE000) == 0x00428000) { // profile CB > return true; > } > } > return false; > } > Ok, will fix this up. Thanks. > @@ +355,5 @@ > > + if (h264Params->packetization_mode != mPacketizationMode) { > > + return false; > > + } > > + > > + // TODO: What else do we need to check here? > > two payloads for h264 match if their profiles match (see above), and their > packetization modes match. (RFC 6184 8.2.2). Note that the payload numbers > can be totally different. > Ok, will remove the TODO once the profile stuff is squared away. > @@ +393,5 @@ > > + > > + // H264-specific stuff > > + uint32_t mProfileLevelId; > > + uint32_t mMaxFr; > > + uint32_t mPacketizationMode; > > overkill for 0..2, but ok > Just doing what everything else is doing here. > ::: media/webrtc/signaling/src/jsep/JsepSession.h > @@ +16,5 @@ > > +#include "signaling/src/sdp/Sdp.h" > > + > > + > > +namespace mozilla { > > +namespace jsep { > > per the guidelines, I believe we are supposed to avoid defining new > sub-namespaces to mozilla > I think we could get away with this, since all of the classes have a prefix of "Jsep". > ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp > @@ +28,5 @@ > > +namespace jsep { > > + > > +MOZ_MTLOG_MODULE("jsep") > > + > > +JsepSessionImpl::~JsepSessionImpl() { > > { on new line please ("Class and function definitions are not control > structures; left brace goes by itself on the second line and without extra > indentation, in general for C++.") > apply across all new files > I'm going to run all of this through clang-format when we're done. Don't worry about whitespace, it will all be sorted in the end. > @@ +41,5 @@ > > + MOZ_ASSERT(!mSessionId, "Init called more than once"); > > + > > + SECStatus rv = PK11_GenerateRandom( > > + reinterpret_cast<unsigned char *>(&mSessionId), sizeof(mSessionId)); > > + mSessionId >>= 2; // Discard high order bits. > > Ummm.. a) indicate why you need to discard two bits, b) discard the correct > bits. And why with a shift instead of mask? > I'll ask ekr. > ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h > ::: media/webrtc/signaling/src/jsep/JsepTrackImpl.h > @@ +22,5 @@ > > + public: > > + virtual ~JsepTrackNegotiatedDetailsImpl() { > > + for (auto c = mCodecs.begin(); c != mCodecs.end(); ++c) { > > + delete *c; > > + } > > Annoying we have something we have to manually release... Would > std::vector<UniquePtr<JsepCodecDescription>> work? Not a huge deal > Sadly, no. Our buildconfig is using stl headers from 2007 without move semantics. It is a perpetual thorn in my side. > ::: media/webrtc/signaling/test/jsep_session_unittest.cpp > @@ +133,5 @@ > > + AddTracks(side, types); > > + > > + // Now that we have added streams, we expect audio, then video, then > > + // application in the SDP, regardless of the order in which the streams were > > + // added. > > Ummm... If we're generating an SDP as a response, that is not in any way > correct. It might be correct for this particular test, but we should be > testing responding correctly to SDP with the media lines in any order. > This is only about our own offers; we need to emit in this order, or pre-rewrite Firefox will fail to cope. > @@ +564,5 @@ > > + "video,audio", > > + "audio,datachannel", > > + "video,datachannel", > > + "video,audio,datachannel", > > + "audio,video,datachannel")); > > Perhaps this may answer my question above, though I don't see datachannel, > audio/video [, audio/video] > However, it isn't clear to me what this is actually doing. > This specifies the types/order in which tracks are added. I can look again and see why we don't have a case where datachannel comes first, but I bet there was a reason. > @@ +589,5 @@ > > + .GetMediaType()); > > + ASSERT_EQ(SdpDirectionAttribute::kRecvonly, outputSdp->GetMediaSection(1) > > + .GetAttributeList().GetDirection()); > > + ASSERT_EQ(SdpMediaSection::kVideo, outputSdp->GetMediaSection(2) > > + .GetMediaType()); > > And here, clearly, we're checking the ordering of m= lines Yep; we have to emit in this order or we won't interop with pre-rewrite Firefox.

Incorporate feedback from jesup.

Changes from my own review pass. More details on reviewboard.

Comment on attachment 8520323 [details] [diff] [review] Part 2: New JSEP handling code. Review of attachment 8520323 [details] [diff] [review]: ----------------------------------------------------------------- The interdiff for your feedback is here: https://reviewboard.allizom.org/r/87/diff/3-4/ In particular, I'd like a sanity-check on the H264 profile matching stuff. You can also look at the subsequent interdiff for my own pass here, I've added issues for some new code to make them easy to find: https://reviewboard.allizom.org/r/87/diff/4-5/

Compensate for API movement.

Missed some changes for API movement, and also tweak a test that does insane things on SetLocal, which are now an error due to changes to part 2.

Fix memory management of the sdp config pointer.

Comment on attachment 8520956 [details] [diff] [review] Part 1: SDP wrapper code around sipcc sdp impl. Review of attachment 8520956 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r=jesup

Update commit log.

Comment on attachment 8520957 [details] [diff] [review] Part 1: SDP wrapper code around sipcc sdp impl. Review of attachment 8520957 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r=jesup

Fixes from my review pass, see reviewboard for more details.

More fixup.

Yet more fixup.

Comment on attachment 8520992 [details] [diff] [review] Part 5.2: Functionality changes to sipcc sdp code. Review of attachment 8520992 [details] [diff] [review]: ----------------------------------------------------------------- There's enough interdiff here that it merits a second look: https://reviewboard.allizom.org/r/91/diff/3-6/#index_header

Remove a bunch of commented-out code.

Check max number of SCTP streams, since the SDP parser doesn't do the checking for us anymore.

Unrot.

Comment on attachment 8521002 [details] [diff] [review] Part 5.3: Use modern integral types in sipcc code. Review of attachment 8521002 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r=ehugg

Is there a newer try build available (the last one is gone already, and when trying to build locally Part1 seems to be bit-rotten already)?

Fix compile warning on android.

I'm fixing some bustage from the review changes, hopefully should have something next week.

Ok, I think I have the bustage sorted out, doing a try run to make sure. If this works out, I'll update the patches. https://tbpl.mozilla.org/?tree=Try&rev=74f169a85839

Unrot.

Unrot.

Unrot.

Unrot and fix bustage.

Unrot and fix bustage.

Unrot.

Unrot.

Unrot.

Unrot, and lots of fixes. See reviewboard.

For some reason, the "cake" mochitest is failing with the unrotted patches. Need to figure out why.

The "cake" is the only sendonly test in the tree if that helps.

Get the cake test working.

Remove some mochitest changes that crept in.

Comment on attachment 8523318 [details] [diff] [review] Part 1: SDP wrapper code around sipcc sdp impl. Review of attachment 8523318 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r=jesup

Comment on attachment 8523319 [details] [diff] [review] Part 2: New JSEP handling code. Review of attachment 8523319 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r=jesup, r=ehugg

Comment on attachment 8523322 [details] [diff] [review] Part 4: Remove most of sipcc, and move just the sdp stuff into a new location. Review of attachment 8523322 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r=ehugg

Comment on attachment 8523324 [details] [diff] [review] Part 5.1: Whitespace-only modifications to sipcc sdp code. Review of attachment 8523324 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r=ehugg

Comment on attachment 8523325 [details] [diff] [review] Part 5.2: Functionality changes to sipcc sdp code. Review of attachment 8523325 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r=ehugg, r=pkerr

Comment on attachment 8523326 [details] [diff] [review] Part 5.3: Use modern integral types in sipcc code. Review of attachment 8523326 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r=ehugg

Comment on attachment 8524106 [details] [diff] [review] Part 3: Mochitest work. Review of attachment 8524106 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r=jib