1091242 - We need a rewrite of the SDP/JSEP handling code

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Byron Campen [:bwc]]

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.

Comment 1

[Byron Campen [:bwc]]

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

Comment 2

[Byron Campen [:bwc]]

Attachment: Part 7: Wiring the build system together.

Decomposed patch.

Comment 3

[Byron Campen [:bwc]]

Attachment: Part 1: SDP wrapper code around sipcc sdp impl.

Comment 4

[Byron Campen [:bwc]]

Attachment: Part 2: New JSEP handling code.

Comment 5

[Byron Campen [:bwc]]

Attachment: Part 3: Mochitest work.

Comment 6

[Byron Campen [:bwc]]

Attachment: Part 4: Remove most of sipcc, and move just the sdp stuff into a new location.

Comment 7

[Byron Campen [:bwc]]

Attachment: Part 5: Modifications to sipcc sdp code.

Comment 8

[Byron Campen [:bwc]]

Attachment: Part 6: Wiring the new JSEP handler code in.

Comment 9

[Byron Campen [:bwc]]

Attachment: Part 7: Wiring the build system together.

Comment 10

[Byron Campen [:bwc]]

Attachment: Part 5: Modifications to sipcc sdp code.

%z is not supported on MSVC

Comment 11

[Byron Campen [:bwc]]

Attachment: Part 6: Wiring the new JSEP handler code in.

%z is not supported on MSVC

Comment 12

[Byron Campen [:bwc]]

Attachment: Part 8: When running on tbpl, disable parts of ice_unittest that rely on external network.

Comment 13

[Byron Campen [:bwc]]

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

Comment 14

[Eric Rescorla (:ekr)]

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

Comment 15

[Byron Campen [:bwc]]

I'll try that.

Comment 16

[Byron Campen [:bwc]]

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).

Comment 17

[Byron Campen [:bwc]]

Attachment: Part 5.1: Whitespace-only modifications to sipcc sdp code.

Splitting off whitespace-only changes.

Comment 18

[Byron Campen [:bwc]]

Attachment: Part 5.2: Functionality changes to sipcc sdp code.

Actual functionality changes to sipcc sdp code.

Comment 19

[Byron Campen [:bwc]]

Attachment: Part 5.3: Use modern integral types in sipcc code.

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

Comment 20

[Byron Campen [:bwc]]

Attachment: Part 1: SDP wrapper code around sipcc sdp impl.

Re-enable the old SDP test-cases.

Comment 21

[Byron Campen [:bwc]]

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 22

[Byron Campen [:bwc]]

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 23

[Byron Campen [:bwc]]

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 24

[Byron Campen [:bwc]]

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 25

[Byron Campen [:bwc]]

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 26

[Byron Campen [:bwc]]

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 27

[Byron Campen [:bwc]]

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 28

[Byron Campen [:bwc]]

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 29

[Byron Campen [:bwc]]

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 30

[Byron Campen [:bwc]]

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/

Comment 31

[Byron Campen [:bwc]]

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.

Comment 32

[Ethan Hugg [:ehugg]]

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.

Comment 33

[Byron Campen [:bwc]]

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

Comment 34

[Ethan Hugg [:ehugg]]

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?

Comment 35

[Byron Campen [:bwc]]

(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...

Comment 36

[Byron Campen [:bwc]]

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

Comment 37

[Ethan Hugg [:ehugg]]

(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 38

[Ethan Hugg [:ehugg]]

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 39

[Ethan Hugg [:ehugg]]

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 40

[Ethan Hugg [:ehugg]]

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 41

[Byron Campen [:bwc]]

Attachment: Part 8: When running on tbpl, disable parts of ice_unittest that rely on external network.

Incorporate feedback.

Comment 42

[Nils Ohlmeier [: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

Comment 43

[Ethan Hugg [:ehugg]]

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

Comment 44

[Byron Campen [:bwc]]

Attachment: Part 2: New JSEP handling code.

Incorporate feedback.

Comment 45

[Byron Campen [:bwc]]

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 46

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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/

Comment 47

[Ethan Hugg [:ehugg]]

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.

Comment 48

[Eric Rescorla (:ekr)]

(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 49

[Ethan Hugg [:ehugg]]

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 50

[Byron Campen [:bwc]]

Attachment: Part 3: Mochitest work.

Incorporate feedback.

Comment 51

[Byron Campen [:bwc]]

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 52

[Ethan Hugg [:ehugg]]

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 53

[Byron Campen [:bwc]]

Attachment: Part 5.2: Functionality changes to sipcc sdp code.

Incorporate feedback.

Comment 54

[Byron Campen [:bwc]]

Attachment: Part 5.3: Use modern integral types in sipcc code.

Unrot.

Comment 55

[Byron Campen [:bwc]]

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

Comment 56

[Byron Campen [:bwc]]

Attachment: Part 5.2: Functionality changes to sipcc sdp code.

Update commit log

Comment 57

[Byron Campen [:bwc]]

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

Comment 58

[Byron Campen [:bwc]]

Attachment: Part 5.3: Use modern integral types in sipcc code.

Update commit log

Comment 59

[Byron Campen [:bwc]]

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 60

[Paul Adenot (:padenot)]

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 61

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 62

[Byron Campen [:bwc]]

Attachment: Part 1: SDP wrapper code around sipcc sdp impl.

Lots of fixes. See reviewboard for summary.

Comment 63

[Byron Campen [:bwc]]

Attachment: Part 2: New JSEP handling code.

Lots of fixes, see reviewboard for more detail.

Comment 64

[Byron Campen [:bwc]]

Attachment: Part 5.2: Functionality changes to sipcc sdp code.

Move a macro required by an update to part 1.

Comment 65

[Byron Campen [:bwc]]

Attachment: Part 6: Wiring the new JSEP handler code in.

Adjust for API movement in parts 1 and 2.

Comment 66

[Byron Campen [:bwc]]

Attachment: Part 7: Wiring the build system together.

Compensate for files removed in latest part 2.

Comment 67

[Byron Campen [:bwc]]

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 68

[Byron Campen [:bwc]]

Comment on attachment 8518377 [details] [diff] [review]
Part 5.2: Functionality changes to sipcc sdp code.

Carry forward r=ehugg, r=pkerr

Comment 69

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8516997 [details] [diff] [review]
Part 3: Mochitest work.

r=me with nit fixed.

Comment 70

[Byron Campen [:bwc]]

Attachment: Part 3: Mochitest work.

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

Comment 71

[Byron Campen [:bwc]]

Comment on attachment 8519480 [details] [diff] [review]
Part 3: Mochitest work.

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

Carry forward r=jib

Comment 72

[Randell Jesup [:jesup] (needinfo me)]

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?

Comment 73

[Byron Campen [:bwc]]

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

Comment 74

[Byron Campen [:bwc]]

(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 75

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 76

[Byron Campen [:bwc]]

(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.

Comment 77

[Byron Campen [:bwc]]

Attachment: Part 2: New JSEP handling code.

Incorporate feedback from jesup.

Comment 78

[Byron Campen [:bwc]]

Attachment: Part 2: New JSEP handling code.

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

Comment 79

[Byron Campen [:bwc]]

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/

Comment 80

[Byron Campen [:bwc]]

Attachment: Part 6: Wiring the new JSEP handler code in.

Compensate for API movement.

Comment 81

[Byron Campen [:bwc]]

Attachment: Part 6: Wiring the new JSEP handler code in.

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.

Comment 82

[Byron Campen [:bwc]]

Attachment: Part 1: SDP wrapper code around sipcc sdp impl.

Fix memory management of the sdp config pointer.

Comment 83

[Byron Campen [:bwc]]

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

Comment 84

[Byron Campen [:bwc]]

Attachment: Part 1: SDP wrapper code around sipcc sdp impl.

Update commit log.

Comment 85

[Byron Campen [:bwc]]

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

Comment 86

[Byron Campen [:bwc]]

Attachment: Part 5.2: Functionality changes to sipcc sdp code.

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

Comment 87

[Byron Campen [:bwc]]

Attachment: Part 5.2: Functionality changes to sipcc sdp code.

More fixup.

Comment 88

[Byron Campen [:bwc]]

Attachment: Part 5.2: Functionality changes to sipcc sdp code.

Yet more fixup.

Comment 89

[Byron Campen [:bwc]]

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

Comment 90

[Byron Campen [:bwc]]

Attachment: Part 7: Wiring the build system together.

Remove a bunch of commented-out code.

Comment 91

[Byron Campen [:bwc]]

Attachment: Part 6: Wiring the new JSEP handler code in.

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

Comment 92

[Byron Campen [:bwc]]

Attachment: Part 5.3: Use modern integral types in sipcc code.

Unrot.

Comment 93

[Byron Campen [:bwc]]

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

Comment 94

[Nils Ohlmeier [:drno]]

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)?

Comment 95

[Byron Campen [:bwc]]

Attachment: Part 1: SDP wrapper code around sipcc sdp impl.

Fix compile warning on android.

Comment 96

[Byron Campen [:bwc]]

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

Comment 97

[Byron Campen [:bwc]]

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

Comment 98

[Byron Campen [:bwc]]

Attachment: Part 1: SDP wrapper code around sipcc sdp impl.

Unrot.

Comment 99

[Byron Campen [:bwc]]

Attachment: Part 2: New JSEP handling code.

Unrot.

Comment 100

[Byron Campen [:bwc]]

Attachment: Part 3: Mochitest work.

Unrot.

Comment 101

[Byron Campen [:bwc]]

Attachment: Part 4: Remove most of sipcc, and move just the sdp stuff into a new location.

Unrot and fix bustage.

Comment 102

[Byron Campen [:bwc]]

Attachment: Part 5.1: Whitespace-only modifications to sipcc sdp code.

Unrot and fix bustage.

Comment 103

[Byron Campen [:bwc]]

Attachment: Part 5.2: Functionality changes to sipcc sdp code.

Unrot.

Comment 104

[Byron Campen [:bwc]]

Attachment: Part 5.3: Use modern integral types in sipcc code.

Unrot.

Comment 105

[Byron Campen [:bwc]]

Attachment: Part 7: Wiring the build system together.

Unrot.

Comment 106

[Byron Campen [:bwc]]

Attachment: Part 6: Wiring the new JSEP handler code in.

Unrot, and lots of fixes. See reviewboard.

Comment 107

[Byron Campen [:bwc]]

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

Comment 108

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 109

[Byron Campen [:bwc]]

Attachment: Part 3: Mochitest work.

Get the cake test working.

Comment 110

[Byron Campen [:bwc]]

Attachment: Part 6: Wiring the new JSEP handler code in.

Remove some mochitest changes that crept in.

Comment 111

[Byron Campen [:bwc]]

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 112

[Byron Campen [:bwc]]

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 113

[Byron Campen [:bwc]]

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 114

[Byron Campen [:bwc]]

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 115

[Byron Campen [:bwc]]

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 116

[Byron Campen [:bwc]]

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 117

[Byron Campen [:bwc]]

Comment on attachment 8524106 [details] [diff] [review]
Part 3: Mochitest work.

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

Carry forward r=jib

Comment 118

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=101bf8577315

Comment 119

[Byron Campen [:bwc]]

Attachment: Part 7: Wiring the build system together.

Remove some more unused stuff.

Comment 120

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 121

[Byron Campen [:bwc]]

(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.

Comment 122

[Byron Campen [:bwc]]

Attachment: Part 6: Wiring the new JSEP handler code in.

Incorporate feedback

Comment 123

[Byron Campen [:bwc]]

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

Comment 124

[Randell Jesup [:jesup] (needinfo me)]

> > ::: 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 125

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 126

[Eric Rescorla (:ekr)]

(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.

Comment 127

[Byron Campen [:bwc]]

(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.

Comment 128

[Eric Rescorla (:ekr)]

(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.

Comment 129

[Byron Campen [:bwc]]

Attachment: Part 6: Wiring the new JSEP handler code in.

Call JSEP_SET_ERROR in a couple of places, and make sure we return on error in another.

Comment 130

[Randell Jesup [:jesup] (needinfo me)]

(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.

Comment 131

[Byron Campen [:bwc]]

(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 132

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8525523 [details] [diff] [review]
Part 6: Wiring the new JSEP handler code in.

r+'d with one nit on reviewboard

Comment 133

[Randell Jesup [:jesup] (needinfo me)]

(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.

Comment 134

[Byron Campen [:bwc]]

Attachment: Part 1: SDP wrapper code around sipcc sdp impl. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history.

Run through clang-format, update commit log to point code archaeologists to the github repo.

Comment 135

[Byron Campen [:bwc]]

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).

Comment 136

[Byron Campen [:bwc]]

Attachment: Part 1: SDP wrapper code around sipcc sdp impl. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history.

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.

Comment 137

[Byron Campen [:bwc]]

Attachment: Part 1: SDP wrapper code around sipcc sdp impl. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history.

Move an unrot hunk that mistakenly ended up in part 2.

Comment 138

[Byron Campen [:bwc]]

Attachment: Part 2: New JSEP handling code. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history.

Run through clang-format.

Comment 139

[Byron Campen [:bwc]]

Attachment: Part 6: Wiring the new JSEP handler code in. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history.

Run MediaPipelineFactory through clang-format. Left pre-existing stuff alone.

Comment 140

[Byron Campen [:bwc]]

Attachment: Part 3: Mochitest work. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history.

Unrot.

Comment 141

[Byron Campen [:bwc]]

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

Comment 142

[Byron Campen [:bwc]]

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

Comment 143

[Byron Campen [:bwc]]

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

Comment 144

[Byron Campen [:bwc]]

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

Comment 145

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=39ffd1587b8b

Comment 146

[(not currently active) Ted Mielczarek]

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

Comment 147

[Byron Campen [:bwc]]

(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.

Comment 148

[Byron Campen [:bwc]]

Attachment: Part 2: New JSEP handling code. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history

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

Comment 149

[Byron Campen [:bwc]]

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

Comment 150

[Byron Campen [:bwc]]

Attachment: Part 6: Wiring the new JSEP handler code in. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history.

Remove hunks that belong in part 2.

Comment 151

[Byron Campen [:bwc]]

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

Comment 152

[Byron Campen [:bwc]]

Attachment: Part 6: Wiring the new JSEP handler code in. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history

Unrot.

Comment 153

[Byron Campen [:bwc]]

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

Comment 154

[Randell Jesup [:jesup] (needinfo me)]

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"

Comment 155

[Byron Campen [:bwc]]

Attachment: Part 2: New JSEP handling code. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history

Incorporate feedback.

Comment 156

[Byron Campen [:bwc]]

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

Comment 157

[Byron Campen [:bwc]]

One last try push

https://tbpl.mozilla.org/?tree=Try&rev=16007b74d0f2

Comment 158

[Byron Campen [:bwc]]

Attachment: Part 4: Remove most of sipcc, and move just the sdp stuff into a new location

Unrot

Comment 159

[Byron Campen [:bwc]]

Attachment: Part 6: Wiring the new JSEP handler code in. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history

Restore an error code removed in bug 1053407

Comment 160

[Byron Campen [:bwc]]

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

Comment 161

[Byron Campen [:bwc]]

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

Comment 162

[Byron Campen [:bwc]]

Attachment: Part 6: Wiring the new JSEP handler code in. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history

Update DOMError name from InvalidCandidate to InvalidCandidateError

Comment 163

[Byron Campen [:bwc]]

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

Comment 164

[Byron Campen [:bwc]]

Attachment: Part 5.1: Whitespace-only modifications to sipcc sdp code

Update commit log

Comment 165

[Byron Campen [:bwc]]

Attachment: Part 7: Wiring the build system together

Update commit log.

Comment 166

[Byron Campen [:bwc]]

Attachment: Part 8: When running on tbpl, disable parts of ice_unittest that rely on external network

Update commit log.

Comment 167

[Byron Campen [:bwc]]

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

Comment 168

[Byron Campen [:bwc]]

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

Comment 169

[Byron Campen [:bwc]]

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

Comment 170

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/960303b07c90 for b2g/Android non-unified bustage like http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-inbound-android-api-10-nonunified/1418095823/mozilla-inbound-android-api-10-nonunified-bm85-build1-build2.txt.gz

Comment 171

[Byron Campen [:bwc]]

Attachment: Part 1: SDP wrapper code around sipcc sdp impl. See https://github.com/unicorn-wg/gecko-dev/tree/multistream_rebase for more history.

Unbust non-unified stlport-based builds.

Comment 172

[Byron Campen [:bwc]]

That was the strangest presentation of a missing include I've ever seen, but have re-landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dc512e25a60
https://hg.mozilla.org/integration/mozilla-inbound/rev/0adf6b67d3fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/c615927bea71
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b4e3f6dadaf
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe944c0ff3af
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6848398dcee
https://hg.mozilla.org/integration/mozilla-inbound/rev/e495b40b1d5b
https://hg.mozilla.org/integration/mozilla-inbound/rev/890e7d2587f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5bdfd0b694e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2112c70ecc8b


Working try for non-unified:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=85e5828c9ebc

Non-working try with the same errors, but without the include:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=34c87f897759

Comment 173

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5dc512e25a60
https://hg.mozilla.org/mozilla-central/rev/0adf6b67d3fb
https://hg.mozilla.org/mozilla-central/rev/c615927bea71
https://hg.mozilla.org/mozilla-central/rev/5b4e3f6dadaf
https://hg.mozilla.org/mozilla-central/rev/fe944c0ff3af
https://hg.mozilla.org/mozilla-central/rev/b6848398dcee
https://hg.mozilla.org/mozilla-central/rev/e495b40b1d5b
https://hg.mozilla.org/mozilla-central/rev/890e7d2587f9
https://hg.mozilla.org/mozilla-central/rev/d5bdfd0b694e
https://hg.mozilla.org/mozilla-central/rev/2112c70ecc8b

Comment 174

[Lawrence Mandel [:lmandel] (use needinfo)]

relnoted as "New SDP/JSEP implementation in WebRTC".

Comment 176

[Adam Roach [:abr]]

(In reply to Daniel Holbert [:dholbert])
> Blocks: 1273965

What?

Comment 177

[Nils Ohlmeier [:drno]]

(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 :-)