We need a rewrite of the SDP/JSEP handling code

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

(Blocks 2 bugs)

Trunk
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 37+)

Details

Attachments

(10 attachments, 62 obsolete attachments)

469.18 KB, patch
bwc
: review+
Details | Diff | Splinter Review
568.64 KB, patch
bwc
: review+
Details | Diff | Splinter Review
33.12 KB, patch
bwc
: review+
Details | Diff | Splinter Review
132.04 KB, patch
bwc
: review+
Details | Diff | Splinter Review
7.59 MB, patch
bwc
: review+
Details | Diff | Splinter Review
565.92 KB, patch
bwc
: review+
Details | Diff | Splinter Review
145.92 KB, patch
bwc
: review+
Details | Diff | Splinter Review
41.16 KB, patch
bwc
: review+
Details | Diff | Splinter Review
5.94 KB, patch
bwc
: review+
Details | Diff | Splinter Review
209.11 KB, patch
Details | Diff | Splinter Review
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.
Posted patch Part 3: Mochitest work. (obsolete) — Splinter Review
Attachment #8513866 - Attachment is obsolete: true
Blocks: sdparta
%z is not supported on MSVC
Attachment #8513874 - Attachment is obsolete: true
%z is not supported on MSVC
Attachment #8513876 - Attachment is obsolete: true
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.
Attachment #8514338 - Attachment is obsolete: true
Re-enable the old SDP test-cases.
Attachment #8513868 - Attachment is obsolete: true
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/
Attachment #8514439 - Flags: review?(rjesup)
Attachment #8514439 - Flags: review?(ethanhugg)
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/
Attachment #8513869 - Flags: review?(rjesup)
Attachment #8513869 - Flags: review?(ethanhugg)
Comment on attachment 8513871 [details] [diff] [review]
Part 3: Mochitest work.

Review of attachment 8513871 [details] [diff] [review]:
-----------------------------------------------------------------

https://reviewboard.allizom.org/r/88/
Attachment #8513871 - Flags: review?(jib)
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/
Attachment #8513873 - Flags: review?(ethanhugg)
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/
Attachment #8514428 - Flags: review?(ethanhugg)
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/
Attachment #8514429 - Flags: review?(pkerr)
Attachment #8514429 - Flags: review?(ethanhugg)
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/
Attachment #8514431 - Flags: review?(ethanhugg)
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/
Attachment #8514339 - Flags: review?(rjesup)
Attachment #8514339 - Flags: review?(padenot)
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/
Attachment #8513877 - Flags: review?(rjesup)
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/
Attachment #8514340 - Flags: review?(drno)
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/
Attachment #8514428 - Flags: review?(ethanhugg) → review+
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/
Attachment #8513873 - Flags: review?(ethanhugg) → review+
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/
Attachment #8514431 - Flags: review?(ethanhugg) → review+
Attachment #8514340 - Attachment is obsolete: true
Attachment #8514340 - Flags: review?(drno)
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
Attachment #8515299 - Flags: review+
Attachment #8514429 - Flags: review?(pkerr) → review+
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
Attachment #8513869 - Flags: review?(ethanhugg) → review+
Incorporate feedback.
Attachment #8513869 - Attachment is obsolete: true
Attachment #8513869 - Flags: review?(rjesup)
Comment on attachment 8516372 [details] [diff] [review]
Part 2: New JSEP handling code.

Review of attachment 8516372 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=ehugg
Attachment #8516372 - Flags: review+
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/
Attachment #8513871 - Flags: review?(jib) → review+
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/
Attachment #8514429 - Flags: review?(ethanhugg) → review+
Blocks: 1093835
Posted patch Part 3: Mochitest work. (obsolete) — Splinter Review
Incorporate feedback.
Attachment #8513871 - Attachment is obsolete: true
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.
Attachment #8516997 - Flags: review?(jib)
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/
Attachment #8514439 - Flags: review?(ethanhugg) → review+
Incorporate feedback.
Attachment #8514429 - Attachment is obsolete: true
Attachment #8514431 - Attachment is obsolete: true
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
Attachment #8517018 - Flags: review+
Update commit log
Attachment #8517018 - Attachment is obsolete: true
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
Attachment #8517022 - Flags: review+
Update commit log
Attachment #8517021 - Attachment is obsolete: true
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
Attachment #8517023 - Flags: review+
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.
Attachment #8514339 - Flags: review?(padenot)
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
Attachment #8514439 - Flags: review?(rjesup) → review+
Lots of fixes. See reviewboard for summary.
Attachment #8514439 - Attachment is obsolete: true
Lots of fixes, see reviewboard for more detail.
Attachment #8516372 - Attachment is obsolete: true
Move a macro required by an update to part 1.
Attachment #8517022 - Attachment is obsolete: true
Adjust for API movement in parts 1 and 2.
Attachment #8514339 - Attachment is obsolete: true
Attachment #8514339 - Flags: review?(rjesup)
Compensate for files removed in latest part 2.
Attachment #8513877 - Attachment is obsolete: true
Attachment #8513877 - Flags: review?(rjesup)
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.
Attachment #8518362 - Flags: review?(rjesup)
Attachment #8518375 - Flags: review?(rjesup)
Comment on attachment 8518377 [details] [diff] [review]
Part 5.2: Functionality changes to sipcc sdp code.

Carry forward r=ehugg, r=pkerr
Attachment #8518377 - Flags: review+
Attachment #8518380 - Flags: review?(rjesup)
Attachment #8518383 - Flags: review?(rjesup)
Blocks: 1095218
Blocks: 1095226
Comment on attachment 8516997 [details] [diff] [review]
Part 3: Mochitest work.

r=me with nit fixed.
Attachment #8516997 - Flags: review?(jib) → review+
Blocks: 1095743
Blocks: 1095780
Blocks: 1095793
Posted patch Part 3: Mochitest work. (obsolete) — Splinter Review
Fixup a couple of things from my own review pass, and incorporate more feedback.
Attachment #8516997 - Attachment is obsolete: true
Comment on attachment 8519480 [details] [diff] [review]
Part 3: Mochitest work.

Review of attachment 8519480 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=jib
Attachment #8519480 - Flags: review+
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?
Attachment #8518362 - Flags: review?(rjesup) → review+
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
Attachment #8518375 - Flags: review?(rjesup) → review+
(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.
Attachment #8518375 - Attachment is obsolete: true
Changes from my own review pass. More details on reviewboard.
Attachment #8520310 - Attachment is obsolete: true
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/
Attachment #8520323 - Flags: review?(rjesup)
Compensate for API movement.
Attachment #8518380 - Attachment is obsolete: true
Attachment #8518380 - Flags: review?(rjesup)
Attachment #8520332 - Flags: review?(rjesup)
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.
Attachment #8520332 - Attachment is obsolete: true
Attachment #8520332 - Flags: review?(rjesup)
Fix memory management of the sdp config pointer.
Attachment #8518362 - Attachment is obsolete: true
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
Attachment #8520956 - Flags: review+
Update commit log.
Attachment #8520956 - Attachment is obsolete: true
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
Attachment #8520957 - Flags: review+
Fixes from my review pass, see reviewboard for more details.
Attachment #8518377 - Attachment is obsolete: true
More fixup.
Attachment #8520965 - Attachment is obsolete: true
Yet more fixup.
Attachment #8520988 - Attachment is obsolete: true
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
Attachment #8520992 - Flags: review?(ethanhugg)
Remove a bunch of commented-out code.
Attachment #8518383 - Attachment is obsolete: true
Attachment #8518383 - Flags: review?(rjesup)
Attachment #8520996 - Flags: review?(rjesup)
Check max number of SCTP streams, since the SDP parser doesn't do the checking for us anymore.
Attachment #8520333 - Attachment is obsolete: true
Attachment #8520999 - Flags: review?(rjesup)
Attachment #8517023 - Attachment is obsolete: true
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
Attachment #8521002 - Flags: review+
Attachment #8520992 - Flags: review?(ethanhugg) → review+
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)?
Flags: needinfo?(docfaraday)
Fix compile warning on android.
Attachment #8520957 - Attachment is obsolete: true
I'm fixing some bustage from the review changes, hopefully should have something next week.
Flags: needinfo?(docfaraday)
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.
Attachment #8523029 - Attachment is obsolete: true
Unrot.
Attachment #8520323 - Attachment is obsolete: true
Attachment #8520323 - Flags: review?(rjesup)
Posted patch Part 3: Mochitest work. (obsolete) — Splinter Review
Unrot.
Attachment #8519480 - Attachment is obsolete: true
Attachment #8513873 - Attachment is obsolete: true
Unrot and fix bustage.
Attachment #8514428 - Attachment is obsolete: true
Attachment #8520992 - Attachment is obsolete: true
Attachment #8521002 - Attachment is obsolete: true
Unrot.
Attachment #8520996 - Attachment is obsolete: true
Attachment #8520996 - Flags: review?(rjesup)
Unrot, and lots of fixes. See reviewboard.
Attachment #8520999 - Attachment is obsolete: true
Attachment #8520999 - Flags: review?(rjesup)
Attachment #8523329 - Flags: review?(rjesup)
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.
Blocks: 1099351
Posted patch Part 3: Mochitest work. (obsolete) — Splinter Review
Get the cake test working.
Attachment #8523320 - Attachment is obsolete: true
Remove some mochitest changes that crept in.
Attachment #8523329 - Attachment is obsolete: true
Attachment #8523329 - Flags: review?(rjesup)
Attachment #8524110 - Flags: review?(rjesup)
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
Attachment #8523318 - Flags: review+
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
Attachment #8523319 - Flags: review+
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
Attachment #8523322 - Flags: review+
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
Attachment #8523324 - Flags: review+
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
Attachment #8523325 - Flags: review+
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
Attachment #8523326 - Flags: review+
Attachment #8523327 - Flags: review?(rjesup)
Comment on attachment 8524106 [details] [diff] [review]
Part 3: Mochitest work.

Review of attachment 8524106 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=jib
Attachment #8524106 - Flags: review+
Remove some more unused stuff.
Attachment #8524208 - Flags: review?(rjesup)
Attachment #8523327 - Attachment is obsolete: true
Attachment #8523327 - Flags: review?(rjesup)
Comment on attachment 8523329 [details] [diff] [review]
Part 6: Wiring the new JSEP handler code in.

Review of attachment 8523329 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Bindings.conf
@@ -861,5 @@
>      'headerFile': 'CanvasRenderingContext2D.h'
>  },
>  
>  'PeerConnectionImpl': {
> -    'nativeType': 'sipcc::PeerConnectionImpl',

though trivial, this might need a DOM reviewer

::: dom/webidl/PeerConnectionImpl.webidl
@@ -73,5 @@
>  
>    readonly attribute PCImplIceConnectionState iceConnectionState;
>    readonly attribute PCImplIceGatheringState iceGatheringState;
>    readonly attribute PCImplSignalingState signalingState;
> -  readonly attribute PCImplSipccState sipccState;

DOM review definitely needed

::: media/mtransport/logging.h
@@ +43,3 @@
>  #endif // defined(PR_LOGGING)
>  
> +#define MOZ_MTLOG_ENSURE_SUCCESS(res, msg)                                \

We're trying to slowly remove the idiom of ENSURE_SUCCESS() causing a return from inside a macro, so let's not add more

::: media/mtransport/nricectx.h
@@ +210,5 @@
> +  RefPtr<NrIceMediaStream> GetStream(size_t index) {
> +    if (index < streams_.size()) {
> +      return streams_[index];
> +    }
> +    return nullptr;

If this were an nsTArray I'd replace with with SafeElementAt(index), but std::vector::at() throws

::: media/mtransport/nricemediastream.cpp
@@ +342,5 @@
> +    NrIceCandidate* candidate) const {
> +
> +  nr_ice_candidate *cand;
> +
> +  int r = nr_ice_media_stream_get_default_candidate(stream_, 1, &cand);

I presume we don't own *cand, so there's no need to clean up

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ +862,5 @@
>    cinst.pacsize  =  codecInfo->mPacSize;
>    cinst.plfreq   =  codecInfo->mFreq;
> +  if (codecInfo->mName == "G722") {
> +    // Compensate for G.722 spec error in RFC 1890
> +    cinst.plfreq = 16000;

I'm guessing this is because code to handle this was removed elsewhere.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +288,5 @@
>      if (branch)
>      {
>        int32_t temp;
> +      (void)NS_WARN_IF(NS_FAILED(branch->GetBoolPref("media.video.test_latency", &mVideoLatencyTestEnable)));
> +      (void)NS_WARN_IF(NS_FAILED(branch->GetIntPref("media.peerconnection.video.min_bitrate", &temp)));

minor not: space after (void) (and repeat)

@@ +1277,5 @@
>    // Note: this assumes cinst is initialized to a base state either by
>    // hand or from a config fetched with GetConfig(); this modifies the config
>    // to match parameters from VideoCodecConfig
>    cinst.plType  = codecInfo->mType;
> +  if (codecInfo->mName == "H264") {

Be very careful around getting all the P0/P1 stuff right

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
@@ +16,5 @@
> +#include "OMXCodecWrapper.h"
> +#endif
> +#include "PeerConnectionImpl.h"
> +#include "PeerConnectionMedia.h"
> +#include "MediaPipelineFactory.h"

Shouldn't this be at/near the top?  (And if some others here are needed for it to be there, included from there?)

@@ +44,5 @@
> +  std::vector<T*> values;
> +};
> +
> +static nsresult JsepCodecDescToCodecConfig(const
> +                                           JsepCodecDescription& d,

'd'....  could be more descriptive. Also, while I'm not in love with aFoo naming, it is the default in our tree, even if the files in this directory have a weird mismash of aFoo and foo.  As this is a new file, it probably should stick to the style guide.

@@ +150,5 @@
> +nsresult MediaPipelineFactory::CreateOrGetTransportFlow(
> +    size_t level,
> +    bool rtcp,
> +    const mozilla::JsepTransport& transport,
> +    RefPtr<mozilla::TransportFlow>* out) {

style: { on left

@@ +269,5 @@
> +      return rv;
> +  } else if (track.GetMediaType() == mozilla::SdpMediaSection::kVideo) {
> +    rv = CreateVideoConduit(trackPair, track, &conduit);
> +    if (NS_FAILED(rv))
> +      return rv;

brace around these return rv's all over.

@@ +377,5 @@
> +  nsRefPtr<LocalSourceStreamInfo> stream =
> +      mPCMedia->GetLocalStreamById(track.GetStreamId());
> +  MOZ_ASSERT(stream);
> +  if (!stream) {
> +    // This should never happen

How badly?  NS_ASSERTION to fail mochitests?

@@ +578,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  if (receiving) {
> +    PtrVector<mozilla::VideoCodecConfig> configs;

Raw configs pushed here will leak if we error out

@@ +591,5 @@
> +                            << static_cast<uint32_t>(rv));
> +        return rv;
> +      }
> +
> +      mozilla::VideoCodecConfig *configRaw;

Does it need to be raw?

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.h
@@ +64,5 @@
> +    mozilla::RefPtr<mozilla::TransportFlow>* out);
> +
> + private:
> +  PeerConnectionMedia* mPCMedia;
> +  PeerConnectionImpl* mPC;

Please comment as to lifetimes/ownership

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ -375,5 @@
> -  //codecMask |= VCM_CODEC_RESOURCE_LINEAR;
> -  codecMask |= VCM_CODEC_RESOURCE_G722;
> -  //codecMask |= VCM_CODEC_RESOURCE_iLBC;
> -  //codecMask |= VCM_CODEC_RESOURCE_iSAC;
> -  mCCM->setAudioCodecs(codecMask);

Where does the equivalent to this code live now, another patch I presume?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +751,5 @@
> +class CompareCodecPriority {
> +  public:
> +    explicit CompareCodecPriority(int32_t preferredCodec) {
> +      // This pref really ought to be a string, preferably something like
> +      // "H264" or "VP8" instead of a payload type.

Agreed, file a bug and link here.  This is largely for testing and unusual cases so it's not high priority

@@ +988,5 @@
>  }
>  
>  nsresult
> +PeerConnectionImpl::GetDatachannelCodec(
> +    const mozilla::JsepApplicationCodecDescription** datachannelCodec,

"Codec" is a very wrong term here, and kinda confusing.
The value(s) at the end of the m= line are media-format-identifiers (and doesn't even have to be numeric, note, for non-RTP/(S)AVP(F) data).

@@ +997,5 @@
> +    nsresult res = mJsepSession->GetNegotiatedTrackPair(j, &trackPair);
> +
> +    if (NS_SUCCEEDED(res) &&
> +        trackPair->mSending && // Assumes we don't do recvonly datachannel
> +        trackPair->mSending->GetMediaType() == SdpMediaSection::kApplication) {

Datachannels don't have a=<direction> at all (sendrecv/etc)

@@ +1010,5 @@
> +      for (size_t i = 0;
> +           i < trackPair->mSending->GetNegotiatedDetails()->GetCodecCount();
> +           ++i) {
> +        const JsepCodecDescription* codec;
> +        res = trackPair->mSending->GetNegotiatedDetails()->GetCodec(i, &codec);

does this fetch from the a=sctpmap info?

@@ +1056,5 @@
> +  CSFLogDebug(logTag, "%s", __FUNCTION__);
> +
> +  const JsepApplicationCodecDescription* codec;
> +  uint16_t level;
> +  nsresult rv = GetDatachannelCodec(&codec, &level);

GetApplicationDatachannel()?  (To be really generic, GetApplicationFormat() == "webrtc-datachannel" perhaps, but we're not likely to add other types anytime soon.)

@@ +1339,5 @@
> +  if (NS_FAILED(nrv)) {
> +    Error error;
> +    switch (nrv) {
> +      case NS_ERROR_UNEXPECTED: error = kInvalidState; break;
> +      default: error = kInternalError;

reformat

@@ +1377,5 @@
> +  if (NS_FAILED(nrv)) {
> +    Error error;
> +    switch (nrv) {
> +      case NS_ERROR_UNEXPECTED: error = kInvalidState; break;
> +      default: error = kInternalError;

reformat

@@ +1441,5 @@
> +    Error error;
> +    switch (nrv) {
> +      case NS_ERROR_INVALID_ARG: error = kInvalidSessionDescription; break;
> +      case NS_ERROR_UNEXPECTED: error = kInvalidState; break;
> +      default: error = kInternalError;

reformat

@@ +1532,5 @@
> +    Error error;
> +    switch (nrv) {
> +      case NS_ERROR_INVALID_ARG: error = kInvalidSessionDescription; break;
> +      case NS_ERROR_UNEXPECTED: error = kInvalidState; break;
> +      default: error = kInternalError;

reformat

@@ +1544,5 @@
> +    // Add the tracks. This code is pretty complicated because the tracks
> +    // come in arbitrary orders and we want to group them by streamId.
> +    // We go through all the tracks and then for each track that represents
> +    // a new stream id, go through the rest of the tracks and deal with
> +    // them at once.

why do they need to be grouped by streamid?  not saying they don't, just wondering why

@@ +1752,5 @@
> +    Error error;
> +    switch (res) {
> +      case NS_ERROR_UNEXPECTED: error = kInvalidState; break;
> +      case NS_ERROR_INVALID_ARG: error = kInvalidCandidateType; break;
> +      default: error = kInternalError;

reformat

@@ +2033,5 @@
> +  std::string fpStr = os.str();
> +
> +  char* tmp = new char[fpStr.size() + 1];
> +  std::copy(fpStr.begin(), fpStr.end(), tmp);
> +  tmp[fpStr.size()] = '\0';

why not:
  char *fingerprint = strdup(fpStr.c_str())?
or if you must
  char *fingerprint = new char[fpStr.size() + 1];
  strcpy(*fingerprint, fpStr.c_str());

c++03 guarantees c_str() is nul-terminated (ASCIIZ).

Ditto for other instances here.  I realize you're just copying the existing code.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +325,5 @@
> +    }
> +
> +    // TODO(bug 1099318): We are forced to do receive then transmit, because of
> +    // a bug in the VideoConduit code. This will need to be fixed for
> +    // renegotiation.

/me holds my tongue over the conduit design...... Bug 864654, sigh

@@ +723,5 @@
>      return nullptr;
>    }
>  
>    MOZ_ASSERT(mRemoteSourceStreams[aIndex]);
>    return mRemoteSourceStreams[aIndex];

ASSERT_ON_THREAD(mMainThread);
MOZ_ASSERT(mRemoteSourceStreams.SafeElementAt(aIndex)); // if this is really needed
return mRemoteStreams.SafeElementAt(aIndex);

::: modules/libpref/init/all.js
@@ +336,1 @@
>  pref("media.navigator.video.h264.level", 31); // 0x42E01f - level 3.1

constrained baseline level 3.1
Attachment #8523329 - Attachment is obsolete: false
(In reply to Randell Jesup [:jesup] from comment #120)
> Comment on attachment 8523329 [details] [diff] [review]
> Part 6: Wiring the new JSEP handler code in.
> 
> Review of attachment 8523329 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bindings/Bindings.conf
> @@ -861,5 @@
> >      'headerFile': 'CanvasRenderingContext2D.h'
> >  },
> >  
> >  'PeerConnectionImpl': {
> > -    'nativeType': 'sipcc::PeerConnectionImpl',
> 
> though trivial, this might need a DOM reviewer
> 
> ::: dom/webidl/PeerConnectionImpl.webidl
> @@ -73,5 @@
> >  
> >    readonly attribute PCImplIceConnectionState iceConnectionState;
> >    readonly attribute PCImplIceGatheringState iceGatheringState;
> >    readonly attribute PCImplSignalingState signalingState;
> > -  readonly attribute PCImplSipccState sipccState;
> 
> DOM review definitely needed

   Ok, I'll find a DOM reviewer.

> 
> ::: media/mtransport/logging.h
> @@ +43,3 @@
> >  #endif // defined(PR_LOGGING)
> >  
> > +#define MOZ_MTLOG_ENSURE_SUCCESS(res, msg)                                \
> 
> We're trying to slowly remove the idiom of ENSURE_SUCCESS() causing a return
> from inside a macro, so let's not add more
> 

   Oh? This is news to me.

> ::: media/mtransport/nricemediastream.cpp
> @@ +342,5 @@
> > +    NrIceCandidate* candidate) const {
> > +
> > +  nr_ice_candidate *cand;
> > +
> > +  int r = nr_ice_media_stream_get_default_candidate(stream_, 1, &cand);
> 
> I presume we don't own *cand, so there's no need to clean up
> 

   Correct.

> ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
> @@ +862,5 @@
> >    cinst.pacsize  =  codecInfo->mPacSize;
> >    cinst.plfreq   =  codecInfo->mFreq;
> > +  if (codecInfo->mName == "G722") {
> > +    // Compensate for G.722 spec error in RFC 1890
> > +    cinst.plfreq = 16000;
> 
> I'm guessing this is because code to handle this was removed elsewhere.
> 

   Let me talk to ekr about that.

> @@ +44,5 @@
> > +  std::vector<T*> values;
> > +};
> > +
> > +static nsresult JsepCodecDescToCodecConfig(const
> > +                                           JsepCodecDescription& d,
> 
> 'd'....  could be more descriptive. Also, while I'm not in love with aFoo
> naming, it is the default in our tree, even if the files in this directory
> have a weird mismash of aFoo and foo.  As this is a new file, it probably
> should stick to the style guide.
> 

   Sure, why not.

> @@ +150,5 @@
> > +nsresult MediaPipelineFactory::CreateOrGetTransportFlow(
> > +    size_t level,
> > +    bool rtcp,
> > +    const mozilla::JsepTransport& transport,
> > +    RefPtr<mozilla::TransportFlow>* out) {
> 
> style: { on left

   I'll be running this file through clang-format, since it won't rot anyone.

> @@ +377,5 @@
> > +  nsRefPtr<LocalSourceStreamInfo> stream =
> > +      mPCMedia->GetLocalStreamById(track.GetStreamId());
> > +  MOZ_ASSERT(stream);
> > +  if (!stream) {
> > +    // This should never happen
> 
> How badly?  NS_ASSERTION to fail mochitests?
> 

   Well, if this happened in a mochitest I would expect the test to fail anyway. It might be worthwhile tossing in a MOZ_ASSERT().

> @@ +578,5 @@
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +
> > +  if (receiving) {
> > +    PtrVector<mozilla::VideoCodecConfig> configs;
> 
> Raw configs pushed here will leak if we error out
> 

   PtrVector takes ownership.

> @@ +591,5 @@
> > +                            << static_cast<uint32_t>(rv));
> > +        return rv;
> > +      }
> > +
> > +      mozilla::VideoCodecConfig *configRaw;
> 
> Does it need to be raw?
> 

   Well, at least initially, but we can put it in an auto ptr of some sort.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
> @@ -375,5 @@
> > -  //codecMask |= VCM_CODEC_RESOURCE_LINEAR;
> > -  codecMask |= VCM_CODEC_RESOURCE_G722;
> > -  //codecMask |= VCM_CODEC_RESOURCE_iLBC;
> > -  //codecMask |= VCM_CODEC_RESOURCE_iSAC;
> > -  mCCM->setAudioCodecs(codecMask);
> 
> Where does the equivalent to this code live now, another patch I presume?
>

   This is in part 2, and a little bit in PeerConnectionImpl::ConfigureJsepSessionCodecs.

> @@ +988,5 @@
> >  }
> >  
> >  nsresult
> > +PeerConnectionImpl::GetDatachannelCodec(
> > +    const mozilla::JsepApplicationCodecDescription** datachannelCodec,
> 
> "Codec" is a very wrong term here, and kinda confusing.
> The value(s) at the end of the m= line are media-format-identifiers (and
> doesn't even have to be numeric, note, for non-RTP/(S)AVP(F) data).
> 
> @@ +997,5 @@
> > +    nsresult res = mJsepSession->GetNegotiatedTrackPair(j, &trackPair);
> > +
> > +    if (NS_SUCCEEDED(res) &&
> > +        trackPair->mSending && // Assumes we don't do recvonly datachannel
> > +        trackPair->mSending->GetMediaType() == SdpMediaSection::kApplication) {
> 
> Datachannels don't have a=<direction> at all (sendrecv/etc)
> 

   Right, but JsepSessionImpl models this stuff the same way for simplicity.

> @@ +1010,5 @@
> > +      for (size_t i = 0;
> > +           i < trackPair->mSending->GetNegotiatedDetails()->GetCodecCount();
> > +           ++i) {
> > +        const JsepCodecDescription* codec;
> > +        res = trackPair->mSending->GetNegotiatedDetails()->GetCodec(i, &codec);
> 
> does this fetch from the a=sctpmap info?
> 

   The construction of the JsepApplicationCodecDescription is based in part on sctpmap, yes.

> @@ +1056,5 @@
> > +  CSFLogDebug(logTag, "%s", __FUNCTION__);
> > +
> > +  const JsepApplicationCodecDescription* codec;
> > +  uint16_t level;
> > +  nsresult rv = GetDatachannelCodec(&codec, &level);
> 
> GetApplicationDatachannel()?  (To be really generic, GetApplicationFormat()
> == "webrtc-datachannel" perhaps, but we're not likely to add other types
> anytime soon.)
> 

   How about GetDatachannelParameters?

> @@ +1544,5 @@
> > +    // Add the tracks. This code is pretty complicated because the tracks
> > +    // come in arbitrary orders and we want to group them by streamId.
> > +    // We go through all the tracks and then for each track that represents
> > +    // a new stream id, go through the rest of the tracks and deal with
> > +    // them at once.
> 
> why do they need to be grouped by streamid?  not saying they don't, just
> wondering why
> 

   I think so we issue exactly one call to OnTracksAvailable for each stream, with all of the tracks in that stream taken into account. I suppose an alternative would be to loop through the stream ids we recorded at the very end. It would be less efficient, but easier to read.

> @@ +2033,5 @@
> > +  std::string fpStr = os.str();
> > +
> > +  char* tmp = new char[fpStr.size() + 1];
> > +  std::copy(fpStr.begin(), fpStr.end(), tmp);
> > +  tmp[fpStr.size()] = '\0';
> 
> why not:
>   char *fingerprint = strdup(fpStr.c_str())?
> or if you must
>   char *fingerprint = new char[fpStr.size() + 1];
>   strcpy(*fingerprint, fpStr.c_str());
> 
> c++03 guarantees c_str() is nul-terminated (ASCIIZ).
> 
> Ditto for other instances here.  I realize you're just copying the existing
> code.
> 

   I read that strdup is deprecated on windows, so wanted to avoid. I suspect that the author of the code was just sick to death of people pestering to use some safe strncpy. It doesn't seem like much of a difference to me.

> @@ +723,5 @@
> >      return nullptr;
> >    }
> >  
> >    MOZ_ASSERT(mRemoteSourceStreams[aIndex]);
> >    return mRemoteSourceStreams[aIndex];
> 
> ASSERT_ON_THREAD(mMainThread);
> MOZ_ASSERT(mRemoteSourceStreams.SafeElementAt(aIndex)); // if this is really
> needed
> return mRemoteStreams.SafeElementAt(aIndex);
> 

   This function should be taking a size_t too. I'll fix.

> ::: modules/libpref/init/all.js
> @@ +336,1 @@
> >  pref("media.navigator.video.h264.level", 31); // 0x42E01f - level 3.1
> 
> constrained baseline level 3.1

   Not my code, but can fix.
Incorporate feedback
Attachment #8523329 - Attachment is obsolete: true
Attachment #8524110 - Attachment is obsolete: true
Attachment #8524110 - Flags: review?(rjesup)
Comment on attachment 8525009 [details] [diff] [review]
Part 6: Wiring the new JSEP handler code in.

Review of attachment 8525009 [details] [diff] [review]:
-----------------------------------------------------------------

smaug to review webidl changes
Attachment #8525009 - Flags: review?(rjesup)
Attachment #8525009 - Flags: review?(bugs)
Attachment #8525009 - Flags: review?(bugs) → review+
> > ::: media/mtransport/logging.h
> > @@ +43,3 @@
> > >  #endif // defined(PR_LOGGING)
> > >  
> > > +#define MOZ_MTLOG_ENSURE_SUCCESS(res, msg)                                \
> > 
> > We're trying to slowly remove the idiom of ENSURE_SUCCESS() causing a return
> > from inside a macro, so let's not add more
> > 
> 
>    Oh? This is news to me.

Yeah, see "if (NS_WARN_IF(NS_FAILED(rv)) { return rv; }" type of things.  Note that NS_WARN_IF() is designed to be used in an if(), and doesn't disappear in opt builds like the much older NS_WARN_IF_FALSE(test, "string").  Generally not going back to older code, but new/modified code should avoid "hidden" returns in macros.
 
> > ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
> > @@ +862,5 @@
> > >    cinst.pacsize  =  codecInfo->mPacSize;
> > >    cinst.plfreq   =  codecInfo->mFreq;
> > > +  if (codecInfo->mName == "G722") {
> > > +    // Compensate for G.722 spec error in RFC 1890
> > > +    cinst.plfreq = 16000;
> > 
> > I'm guessing this is because code to handle this was removed elsewhere.
> > 
> 
>    Let me talk to ekr about that.

This sort of thing is normal for G.722, something somewhere has to deal with the rate-in-SDP != rate-codec-works-at.  This certainly already existed.


> > @@ +377,5 @@
> > > +  nsRefPtr<LocalSourceStreamInfo> stream =
> > > +      mPCMedia->GetLocalStreamById(track.GetStreamId());
> > > +  MOZ_ASSERT(stream);
> > > +  if (!stream) {
> > > +    // This should never happen
> > 
> > How badly?  NS_ASSERTION to fail mochitests?
> > 
> 
>    Well, if this happened in a mochitest I would expect the test to fail
> anyway. It might be worthwhile tossing in a MOZ_ASSERT().

Well, there is one there already, so likely ignore this.

> > @@ +578,5 @@
> > > +    return NS_ERROR_FAILURE;
> > > +  }
> > > +
> > > +  if (receiving) {
> > > +    PtrVector<mozilla::VideoCodecConfig> configs;
> > 
> > Raw configs pushed here will leak if we error out
> > 
> 
>    PtrVector takes ownership.

Good; that wasn't clear (and I didn't go back and check).  


> > @@ +1010,5 @@
> > > +      for (size_t i = 0;
> > > +           i < trackPair->mSending->GetNegotiatedDetails()->GetCodecCount();
> > > +           ++i) {
> > > +        const JsepCodecDescription* codec;
> > > +        res = trackPair->mSending->GetNegotiatedDetails()->GetCodec(i, &codec);
> > 
> > does this fetch from the a=sctpmap info?
> > 
> 
>    The construction of the JsepApplicationCodecDescription is based in part
> on sctpmap, yes.

Ok.  Add a comment that "codec" here means media format and it gets it from sctpmap.
 
> > @@ +1056,5 @@
> > > +  CSFLogDebug(logTag, "%s", __FUNCTION__);
> > > +
> > > +  const JsepApplicationCodecDescription* codec;
> > > +  uint16_t level;
> > > +  nsresult rv = GetDatachannelCodec(&codec, &level);
> > 
> > GetApplicationDatachannel()?  (To be really generic, GetApplicationFormat()
> > == "webrtc-datachannel" perhaps, but we're not likely to add other types
> > anytime soon.)
> > 
> 
>    How about GetDatachannelParameters?

Sure

> 
> > @@ +1544,5 @@
> > > +    // Add the tracks. This code is pretty complicated because the tracks
> > > +    // come in arbitrary orders and we want to group them by streamId.
> > > +    // We go through all the tracks and then for each track that represents
> > > +    // a new stream id, go through the rest of the tracks and deal with
> > > +    // them at once.
> > 
> > why do they need to be grouped by streamid?  not saying they don't, just
> > wondering why
> > 
> 
>    I think so we issue exactly one call to OnTracksAvailable for each
> stream, with all of the tracks in that stream taken into account. I suppose
> an alternative would be to loop through the stream ids we recorded at the
> very end. It would be less efficient, but easier to read.

I doubt the perf difference here is measurable, so I'd go with whatever is clearer and more maintainable.

> 
> > @@ +2033,5 @@
> > > +  std::string fpStr = os.str();
> > > +
> > > +  char* tmp = new char[fpStr.size() + 1];
> > > +  std::copy(fpStr.begin(), fpStr.end(), tmp);
> > > +  tmp[fpStr.size()] = '\0';
> > 
> > why not:
> >   char *fingerprint = strdup(fpStr.c_str())?
> > or if you must
> >   char *fingerprint = new char[fpStr.size() + 1];
> >   strcpy(*fingerprint, fpStr.c_str());
> > 
> > c++03 guarantees c_str() is nul-terminated (ASCIIZ).
> > 
> > Ditto for other instances here.  I realize you're just copying the existing
> > code.
> > 
> 
>    I read that strdup is deprecated on windows, so wanted to avoid. I
> suspect that the author of the code was just sick to death of people
> pestering to use some safe strncpy. It doesn't seem like much of a
> difference to me.

ok.  The second form would work or *fingerprint = strcpy(new charfpStr.size() + 1], fpStr.c_str());  But I'm ok with leaving this too, even if I prefer to avoid explicit separate terminations (since sometimes people get them wrong).

> > ::: modules/libpref/init/all.js
> > @@ +336,1 @@
> > >  pref("media.navigator.video.h264.level", 31); // 0x42E01f - level 3.1
> > 
> > constrained baseline level 3.1
> 
>    Not my code, but can fix.

That was me.  :-)  and looking at this, it's *only* the level, so it's ok to leave the comment as-is
Comment on attachment 8524208 [details] [diff] [review]
Part 7: Wiring the build system together.

Review of attachment 8524208 [details] [diff] [review]:
-----------------------------------------------------------------

Needs build peer review more than mine
Attachment #8524208 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #124)
> > > ::: media/mtransport/logging.h
> > > @@ +43,3 @@
> > > >  #endif // defined(PR_LOGGING)
> > > >  
> > > > +#define MOZ_MTLOG_ENSURE_SUCCESS(res, msg)                                \
> > > 
> > > We're trying to slowly remove the idiom of ENSURE_SUCCESS() causing a return
> > > from inside a macro, so let's not add more
> > > 
> > 
> >    Oh? This is news to me.
> 
> Yeah, see "if (NS_WARN_IF(NS_FAILED(rv)) { return rv; }" type of things. 
> Note that NS_WARN_IF() is designed to be used in an if(), and doesn't
> disappear in opt builds like the much older NS_WARN_IF_FALSE(test,
> "string").  Generally not going back to older code, but new/modified code
> should avoid "hidden" returns in macros.

What's the reasoning here? It requires a lot of extra typing
when each of these also has a log.


> > > ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
> > > @@ +862,5 @@
> > > >    cinst.pacsize  =  codecInfo->mPacSize;
> > > >    cinst.plfreq   =  codecInfo->mFreq;
> > > > +  if (codecInfo->mName == "G722") {
> > > > +    // Compensate for G.722 spec error in RFC 1890
> > > > +    cinst.plfreq = 16000;
> > > 
> > > I'm guessing this is because code to handle this was removed elsewhere.
> > > 
> > 
> >    Let me talk to ekr about that.
> 
> This sort of thing is normal for G.722, something somewhere has to deal with
> the rate-in-SDP != rate-codec-works-at.  This certainly already existed.

Yes, it was in SIPCC code which was now removed.

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c#3498

When I wrote this code, I decided it was better to have it
nearer to VoiceEngine because the problem is in the RTP
profile.


>  
> > 
> > > @@ +1544,5 @@
> > > > +    // Add the tracks. This code is pretty complicated because the tracks
> > > > +    // come in arbitrary orders and we want to group them by streamId.
> > > > +    // We go through all the tracks and then for each track that represents
> > > > +    // a new stream id, go through the rest of the tracks and deal with
> > > > +    // them at once.
> > > 
> > > why do they need to be grouped by streamid?  not saying they don't, just
> > > wondering why
> > > 
> > 
> >    I think so we issue exactly one call to OnTracksAvailable for each
> > stream, with all of the tracks in that stream taken into account. 

Yes, that's correct.
(In reply to Eric Rescorla (:ekr) from comment #126)
> (In reply to Randell Jesup [:jesup] from comment #124)
> > > > ::: media/mtransport/logging.h
> > > > @@ +43,3 @@
> > > > >  #endif // defined(PR_LOGGING)
> > > > >  
> > > > > +#define MOZ_MTLOG_ENSURE_SUCCESS(res, msg)                                \
> > > > 
> > > > We're trying to slowly remove the idiom of ENSURE_SUCCESS() causing a return
> > > > from inside a macro, so let's not add more
> > > > 
> > > 
> > >    Oh? This is news to me.
> > 
> > Yeah, see "if (NS_WARN_IF(NS_FAILED(rv)) { return rv; }" type of things. 
> > Note that NS_WARN_IF() is designed to be used in an if(), and doesn't
> > disappear in opt builds like the much older NS_WARN_IF_FALSE(test,
> > "string").  Generally not going back to older code, but new/modified code
> > should avoid "hidden" returns in macros.
> 
> What's the reasoning here? It requires a lot of extra typing
> when each of these also has a log.

   Honestly, there's only two places we use this, and in one of those places we really ought to be calling JSEP_SET_ERROR to set the last error string and spit out a log, and in the other we've already logged at a lower level.
(In reply to Byron Campen [:bwc] from comment #127)
> (In reply to Eric Rescorla (:ekr) from comment #126)
> > (In reply to Randell Jesup [:jesup] from comment #124)
> > > > > ::: media/mtransport/logging.h
> > > > > @@ +43,3 @@
> > > > > >  #endif // defined(PR_LOGGING)
> > > > > >  
> > > > > > +#define MOZ_MTLOG_ENSURE_SUCCESS(res, msg)                                \
> > > > > 
> > > > > We're trying to slowly remove the idiom of ENSURE_SUCCESS() causing a return
> > > > > from inside a macro, so let's not add more
> > > > > 
> > > > 
> > > >    Oh? This is news to me.
> > > 
> > > Yeah, see "if (NS_WARN_IF(NS_FAILED(rv)) { return rv; }" type of things. 
> > > Note that NS_WARN_IF() is designed to be used in an if(), and doesn't
> > > disappear in opt builds like the much older NS_WARN_IF_FALSE(test,
> > > "string").  Generally not going back to older code, but new/modified code
> > > should avoid "hidden" returns in macros.
> > 
> > What's the reasoning here? It requires a lot of extra typing
> > when each of these also has a log.
> 
>    Honestly, there's only two places we use this, and in one of those places
> we really ought to be calling JSEP_SET_ERROR to set the last error string
> and spit out a log, and in the other we've already logged at a lower level.

Sure. My plan was to replace all of these.
Call JSEP_SET_ERROR in a couple of places, and make sure we return on error in another.
Attachment #8525009 - Attachment is obsolete: true
Attachment #8525009 - Flags: review?(rjesup)
(In reply to Eric Rescorla (:ekr) from comment #126)
> (In reply to Randell Jesup [:jesup] from comment #124)
> > > > ::: media/mtransport/logging.h
> > > > @@ +43,3 @@
> > > > >  #endif // defined(PR_LOGGING)
> > > > >  
> > > > > +#define MOZ_MTLOG_ENSURE_SUCCESS(res, msg)                                \
> > > > 
> > > > We're trying to slowly remove the idiom of ENSURE_SUCCESS() causing a return
> > > > from inside a macro, so let's not add more
> > > > 
> > > 
> > >    Oh? This is news to me.
> > 
> > Yeah, see "if (NS_WARN_IF(NS_FAILED(rv)) { return rv; }" type of things. 
> > Note that NS_WARN_IF() is designed to be used in an if(), and doesn't
> > disappear in opt builds like the much older NS_WARN_IF_FALSE(test,
> > "string").  Generally not going back to older code, but new/modified code
> > should avoid "hidden" returns in macros.
> 
> What's the reasoning here? It requires a lot of extra typing
> when each of these also has a log.

You don't have to use NS_WARN_IF(); that was just an example of something that can be used to replace the older ENSURE_FOO() macros.  This was a ever-so-often-erupting disagreement on m.d.platform, and a year or so ago(thread starts 11/22/2013, see bug 672843) it was decided to start moving away from 'hidden' returns in macros as a confusing idiom especially to people new to the codebase (and just a bad idea style/maintenance-wise).  Saves a few characters/lines, but obscures execution flow.  Also many people didn't understand that they issued warnings, and so used the macros for normal return, leading to warning spam on debug.
Attachment #8525523 - Flags: review?(rjesup)
(In reply to Randell Jesup [:jesup] from comment #125)
> Comment on attachment 8524208 [details] [diff] [review]
> Part 7: Wiring the build system together.
> 
> Review of attachment 8524208 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Needs build peer review more than mine

   It is mostly just removing files/include directories, probably not something we need to get a build peer for, right?
Comment on attachment 8525523 [details] [diff] [review]
Part 6: Wiring the new JSEP handler code in.

r+'d with one nit on reviewboard
Attachment #8525523 - Flags: review?(rjesup) → review+
(In reply to Byron Campen [:bwc] from comment #131)
> > Needs build peer review more than mine
> 
>    It is mostly just removing files/include directories, probably not
> something we need to get a build peer for, right?

A while back khuey et al made a large deal over "all buildfile changes need build peer review, even simple stuff".  A single added or removed file may not and I suspect the edict has weakened, and these changes while large are mostly adds/removed and so would be a slam-dunk review, but like with webidl changes, anything non-truly-trivial I poke a build peer on.

You could also ask if this stricture has loosened any since then.
Attachment #8524208 - Flags: review?(ted)
Run through clang-format, update commit log to point code archaeologists to the github repo.
Attachment #8523318 - Attachment is obsolete: true
Comment on attachment 8525581 [details] [diff] [review]
Part 1: SDP wrapper code around sipcc sdp impl. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history.

Review of attachment 8525581 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=jesup, and asking feedback to double-check that clang-format is configured properly (-style=Mozilla and the .clang-format file in the repo don't seem to have the linebreak brace style quite right, so I needed to tweak a little).
Attachment #8525581 - Flags: review+
Attachment #8525581 - Flags: feedback?(rjesup)
Get clang-format to put return types on their own line. Required pulling down a new version, which seems to have changed some other things.
Attachment #8525581 - Attachment is obsolete: true
Attachment #8525581 - Flags: feedback?(rjesup)
Attachment #8525632 - Attachment is obsolete: true
Attachment #8523319 - Attachment is obsolete: true
Run MediaPipelineFactory through clang-format. Left pre-existing stuff alone.
Attachment #8525523 - Attachment is obsolete: true
Attachment #8524106 - Attachment is obsolete: true
Comment on attachment 8525668 [details] [diff] [review]
Part 1: SDP wrapper code around sipcc sdp impl. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history.

Review of attachment 8525668 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=jesup
Attachment #8525668 - Flags: review+
Comment on attachment 8525669 [details] [diff] [review]
Part 2: New JSEP handling code. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history.

Review of attachment 8525669 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=jesup, r=ehugg
Attachment #8525669 - Flags: review+
Comment on attachment 8525675 [details] [diff] [review]
Part 6: Wiring the new JSEP handler code in. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history.

Review of attachment 8525675 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=jib
Attachment #8525675 - Flags: review+
Comment on attachment 8525678 [details] [diff] [review]
Part 3: Mochitest work. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history.

Review of attachment 8525678 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=jib
Attachment #8525678 - Flags: review+
Blocks: 784491
Comment on attachment 8524208 [details] [diff] [review]
Part 7: Wiring the build system together.

Review of attachment 8524208 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/mtransport/test/moz.build
@@ -19,5 @@
>  
> -    # Bug 1037618 - Cross-tree (network related?) failures on OSX
> -    if CONFIG['OS_TARGET'] != 'Darwin':
> -        GeckoCppUnitTests([
> -            'ice_unittest',

I assume your patch fixes these failures?

::: media/webrtc/signaling/test/moz.build
@@ +10,4 @@
>          'mediaconduit_unittests',
>          'mediapipeline_unittest',
> +       'sdp_unittests',
> +       'signaling_unittests',

nit: indentation here looks busted
Attachment #8524208 - Flags: review?(ted) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #146)
> Comment on attachment 8524208 [details] [diff] [review]
> Part 7: Wiring the build system together.
> 
> Review of attachment 8524208 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/mtransport/test/moz.build
> @@ -19,5 @@
> >  
> > -    # Bug 1037618 - Cross-tree (network related?) failures on OSX
> > -    if CONFIG['OS_TARGET'] != 'Darwin':
> > -        GeckoCppUnitTests([
> > -            'ice_unittest',
> 
> I assume your patch fixes these failures?
> 

   Part 8 does, yeah.
Fix bugs in extmap handling where we would 1) Answer with extensions that the other side did not indicate support for and 2) Not reflect the extmap entries used by the offerer
Attachment #8525669 - Attachment is obsolete: true
Comment on attachment 8527999 [details] [diff] [review]
Part 2: New JSEP handling code. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history

Review of attachment 8527999 [details] [diff] [review]:
-----------------------------------------------------------------

Fixed the extmap handling some, and moved some hunks that had crept into part 6. Enough change that the interdiff needs looking at. Interdiff here:

https://bugzilla.mozilla.org/attachment.cgi?oldid=8525669&action=interdiff&newid=8527999&headers=1
Attachment #8527999 - Flags: review?(rjesup)
Attachment #8525675 - Attachment is obsolete: true
Comment on attachment 8528006 [details] [diff] [review]
Part 6: Wiring the new JSEP handler code in. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history.

Review of attachment 8528006 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=jesup, r=smaug
Attachment #8528006 - Flags: review+
Blocks: 1107307
Attachment #8528006 - Attachment is obsolete: true
Comment on attachment 8532065 [details] [diff] [review]
Part 6: Wiring the new JSEP handler code in. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history

Review of attachment 8532065 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=jesup, r=smaug
Attachment #8532065 - Flags: review+
Blocks: 1017888
Comment on attachment 8527999 [details] [diff] [review]
Part 2: New JSEP handling code. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history

Review of attachment 8527999 [details] [diff] [review]:
-----------------------------------------------------------------

add some NS_ASSERTION()s to cases with "this should not happen"
Attachment #8527999 - Flags: review?(rjesup) → review+
Attachment #8527999 - Attachment is obsolete: true
Comment on attachment 8532730 [details] [diff] [review]
Part 2: New JSEP handling code. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history

Review of attachment 8532730 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=jesup, r=ehugg
Attachment #8532730 - Flags: review+
Attachment #8523322 - Attachment is obsolete: true
Attachment #8532065 - Attachment is obsolete: true
Comment on attachment 8533299 [details] [diff] [review]
Part 4: Remove most of sipcc, and move just the sdp stuff into a new location

Review of attachment 8533299 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=ehugg
Attachment #8533299 - Flags: review+
Comment on attachment 8533320 [details] [diff] [review]
Part 6: Wiring the new JSEP handler code in. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history

Review of attachment 8533320 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=jesup
Attachment #8533320 - Flags: review+
Attachment #8533320 - Attachment is obsolete: true
Comment on attachment 8533344 [details] [diff] [review]
Part 6: Wiring the new JSEP handler code in. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history

Review of attachment 8533344 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=jesup, r=smaug
Attachment #8533344 - Flags: review+
Attachment #8523324 - Attachment is obsolete: true
Update commit log.
Attachment #8524208 - Attachment is obsolete: true
Attachment #8515299 - Attachment is obsolete: true
Comment on attachment 8533354 [details] [diff] [review]
Part 5.1: Whitespace-only modifications to sipcc sdp code

Review of attachment 8533354 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=ehugg
Attachment #8533354 - Flags: review+
Comment on attachment 8533356 [details] [diff] [review]
Part 7: Wiring the build system together

Review of attachment 8533356 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=jesup, r=ted
Attachment #8533356 - Flags: review+
Comment on attachment 8533357 [details] [diff] [review]
Part 8: When running on tbpl, disable parts of ice_unittest that rely on external network

Review of attachment 8533357 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=drno
Attachment #8533357 - Flags: review+
Attachment #8525668 - Attachment is obsolete: true
Depends on: 1109130
Depends on: 1113229
Blocks: 840728
relnoted as "New SDP/JSEP implementation in WebRTC".
Duplicate of this bug: 1072384
Blocks: 1247547
(In reply to Daniel Holbert [:dholbert])
> Blocks: 1273965

What?
(In reply to Adam Roach [:abr] from comment #176)
> (In reply to Daniel Holbert [:dholbert])
> > Blocks: 1273965
> 
> What?

I guess this is one way of referencing which bug introduce the issue/code :-)
You need to log in before you can comment on or make changes to this bug.