1017888 - Signalling portion for renegotiation and negotiation needed events in the Peer Connection handshake

Status: RESOLVED FIXED

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

Description

[:shell escalante]

2 part breakout of bug 857115 (other part is 1017886).  This covers the signalling part (once 1017886 done) that pulls the pins to stop flowing through path.  cleaning up code path and working with #1 changes

Comment 1

[Maire Reavy [:mreavy]]

This should have said mozilla33

Comment 2

[Maire Reavy [:mreavy]]

I believe this isn't needed for switching cameras.

Comment 3

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support

Some initial work, mostly on creating reoffer.

Comment 4

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support

Unrot against latest multistream work, enable renegotiation mochitest, and fix some SetLocal checking.

Comment 5

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support.

Fix some bad assertions, start working on renegotiation logic for MediaPipelines.

Comment 6

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support

Unrot.

Comment 7

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support

Get our renegotiation mochitest working. Needs much more testing.

Comment 8

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support

Fix up some racy stuff in ice_unittest in preparation for testing AddStream during/after ICE.

Comment 9

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support.

Add some test cases for AddStream during/after ICE, and fix bugs in the same.

Comment 10

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support.

Decruft, and store Conduits by stream/track id instead of m-line index.

Comment 11

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support.

Begin work on allowing the transports for a MediaPipeline to be updated.

Comment 12

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support.

Start work on handling removal of remote tracks gracefully. Needs more work.

Comment 13

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support.

Remove some obsolete tests, and fix a bug in the MediaPipeline transport update logic.

Comment 14

[Nils Ohlmeier [:drno]]

Attachment: second_stream_test_fix.patch

This fixes the problem that the tests are expecting ICE connections to go through new -> checking -> connected even for renegotiated connections.
Note: making the checking step "optional" is not spec compliant right now, but make sense.

Comment 15

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support

Fix some bugs in track/stream id handling, add some logging, and disable the conduit pairing logic.

Comment 16

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support

Unrot

Comment 17

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support

Unrot

Comment 18

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support

Unrot.

Comment 19

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support

Unrot again.

Comment 20

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support

Better selection of numeric track ids

Comment 21

[Byron Campen [:bwc]]

Attachment: (WIP) Renegotiation support

Get the new trackid assignment logic working

Comment 22

[Byron Campen [:bwc]]

Attachment: Part 1: (WIP) Renegotiation support

Small fix to offerToReceive logic when renegotiating.

Comment 23

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Comment 24

[Byron Campen [:bwc]]

Attachment: Part 1: (WIP) Renegotiation support

Unrot

Comment 25

[Byron Campen [:bwc]]

Attachment: Part 1: (WIP) Renegotiation support

Move a couple of test cases to the test patch.

Comment 26

[Byron Campen [:bwc]]

Attachment: Part 1: (WIP) Renegotiation support

Move some stuff from the test patch. Also, a bugfix.

Comment 27

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Move some tests to this patch.

Comment 28

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Get rid of some log spew.

Comment 29

[Byron Campen [:bwc]]

Attachment: Part 1: (WIP) Renegotiation support

Some cleanup/simplification

Comment 30

[Byron Campen [:bwc]]

Attachment: Part 1: (WIP) Renegotiation support

Simplify and fix more corner cases.

Comment 31

[Byron Campen [:bwc]]

Attachment: Part 1: (WIP) Renegotiation support

More simplification/cleanup

Comment 32

[Byron Campen [:bwc]]

Attachment: Part 1: (WIP) Renegotiation support

Unrot, and fix bug where we would set different ports on m-sections we knew were bundled.

Comment 33

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Unrot.

Comment 34

[Byron Campen [:bwc]]

Attachment: Part 1: (WIP) Renegotiation support

Refactor to avoid nr_ice_ctx being initted more than once.

Comment 35

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

More tests.

Comment 36

[Byron Campen [:bwc]]

Attachment: Part 1: (WIP) Renegotiation support

Add some sanity checks for renegotiation.

Comment 37

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Lots more tests.

Comment 38

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Another test-case

Comment 39

[Byron Campen [:bwc]]

Attachment: Part 1: (WIP) Renegotiation support

Unrot

Comment 40

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Unrot

Comment 41

[Byron Campen [:bwc]]

Comment on attachment 8536736 [details] [diff] [review]
second_stream_test_fix.patch

It seems that the mochitest refactor somehow made this patch unnecessary. Does that sound right to you Nils?

Comment 42

[Nils Ohlmeier [:drno]]

Comment on attachment 8536736 [details] [diff] [review]
second_stream_test_fix.patch

Bits and pieces of this have landed already. But not everything. I'll update this part...

Comment 43

[Byron Campen [:bwc]]

Attachment: Part 1: (WIP) Renegotiation support

Comment 44

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Disable new mochitests on b2g emulator

Comment 45

[Nils Ohlmeier [:drno]]

Comment on attachment 8536736 [details] [diff] [review]
second_stream_test_fix.patch

You were right that most of this code is not needed any more.
I'll attach another patch for a small addition for the new tests.

Comment 46

[Nils Ohlmeier [:drno]]

Attachment: Part 3: add ICE checks to new test cases

Comment 47

[Byron Campen [:bwc]]

Attachment: Part 1: Renegotiation support

Comment 48

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Make renegotiation mochitests check everything the normal tests do, and work with steeplechase (hopefully). Also, add some NoBundle variants. Some bugs need fixing.

Comment 49

[Byron Campen [:bwc]]

Attachment: Part 1: Renegotiation support

Fix some bugs uncovered by testing.

Comment 50

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Fix track checking to pay attention to track ids/type, instead of just counters, and to be based on the tracks we actually add/remove, instead of constraints. Also, allow ICE to go back to checking on a couple of tests.

Comment 51

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Refactor and simplify the renegotiation mochitests. Add tests for datachannel renegotiation.

Comment 52

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Remove some steps from the datachannel tests that weren't there before, and that seem to be unreliable for some reason.

Comment 53

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Get the basicDataOnly test working again.

Comment 54

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Remove an invalid test.

Comment 55

[Byron Campen [:bwc]]

Attachment: MozReview Request: bz://1017888/bwc

/r/3657 - Bug 1017888 - Part 1: Renegotiation support.
/r/3659 - Bug 1017888 - Part 2: Testing for renegotiation.

Pull down these commits:

hg pull review -r 0003bb32034132b0e49136718217252bad3e8798

Comment 56

[Byron Campen [:bwc]]

Comment on attachment 8562213 [details]
MozReview Request: bz://1017888/bwc

Part 1 is for mt primarily, part 2 is for drno primarily, but it couldn't hurt to have both on both if there's time.

Note that we're dependent on bug 1130534, which is still in progress, but it looks close.

Comment 57

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Remove unused function

Comment 58

[Byron Campen [:bwc]]

Attachment: Part 1: Renegotiation support

Unrot

Comment 59

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Fix compile

Comment 60

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/3655/#review2849

::: dom/media/tests/mochitest/head.js
(Diff revision 1)
> +  getCommandsRangeInclusive: function(firstFunctionOrName,

This is unused, will remove.

Comment 61

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

Restore a fix.

Comment 62

[Byron Campen [:bwc]]

Attachment: Part 2: Testing for renegotiation

For tests that remove then add a track on one side, do not insist that the RTP packet count on the receiver pipeline is leq the count on the new sender pipeline, since it won't be.

Comment 63

[Martin Thomson [:mt:]]

Comment on attachment 8562213 [details]
MozReview Request: bz://1017888/bwc

https://reviewboard.mozilla.org/r/3655/#review2855

This is big enough that I'd like to take another swing at it later.  Fow now, I think that I have enough here for you to work on.

Big items:

- I'd like to see a little more testing.  I can't see a single test where the original answerer generates a renegotiation offer.
- There are several places where the addition of code has reached the point of breaking.  The complexity of (for example) setRemoteDescription is now beyond mortal ken.  I also think that JsepSessionImpl needs to be broken down a little more, the obvious split being a helper class that does SDP generation functions, or a small class that does all those various introspection tasks (like finding the bungle group).

::: dom/media/tests/mochitest/dataChannel.js
(Diff revision 1)
> +  // TODO: Figure out why the steps that follow this don't work.
> +  chain.removeAfter('PC_REMOTE_CHECK_ICE_CONNECTIONS');
> +  chain.append(commandsCheckDataChannel);

That sounds scary.  I couldn't r+ something with a comment like that in it.

::: dom/media/tests/mochitest/head.js
(Diff revision 1)
> +    if (!start) {
> +      start = 0;
> +    }

start = start || 0;

is axiomatic if you'd prefer it.

::: dom/media/tests/mochitest/head.js
(Diff revision 1)
> -  _insertHelper: function(functionOrName, commands, delta) {
> +  _insertHelper: function(functionOrName, commands, delta, all) {
>      var index = this.indexOf(functionOrName);
> -
> -    if (index >= 0) {
> +    for (; index !== -1; index = this.indexOf(functionOrName, index)) {
> +      info("index = " + index);

debugging statement?

::: dom/media/tests/mochitest/head.js
(Diff revision 1)
> +    var lastIndex = -1;
> +
> +    if (lastFunctionOrName) {
> +      lastIndex = this.indexOf(lastFunctionOrName);
> +    } else {
> +      lastIndex = this.commands.length - 1;
> -  }
> +    }

No need to initialize lastIndex on declaration.

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> +  removeSender : function(index) {
> +    var sender = this._pc.getSenders()[index];
> +    delete this.expectedLocalTrackTypesById[sender.track.id];
> +    this._pc.removeTrack(sender);
> +    return Promise.resolve();

No need to return a resolved promise here.

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -  /*
> -   * Counts the amount of tracks of the given type in a set of streams.
> -   *
> -   * @param type audio|video
> -   * @param streams
> -   *        An array of streams (as returned by getLocalStreams()) to be
> +  /**
> +   * Checks whether a given track is expected, has not been observed yet, and
> +   * is of the correct type. Then, moves the track from
> +   * |expectedTrackTypesById| to |observedTrackTypesById|.
> +   */
> +  checkTrack : function(track,

checkTrackIsExpected() ?

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -    return streams.reduce((count, st) => {
> -      return count + st[f]().length;
> +    Object.keys(expectedLocalTrackTypesById).forEach(id => {
> +      ok(false, this + " local id " + id + " was observed");

" was not observed" ?

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> +    if (Object.keys(expectedRemoteTrackTypesById).length === 0) {
> +      info(this + " checkMediaTracks() got called after all expected addstream events");
> +      this.addStreamEvents = [];
> -        return Promise.resolve();
> +      return Promise.resolve();
> -      }
> +    }

Factor this into checkAddStreamEvent and you will find that you can compress this a lot more.

::: dom/media/tests/mochitest/templates.js
(Diff revision 1)
> +        JSON.parse(JSON.stringify((test.pcLocal.expectedLocalTrackTypesById)));

nit: extra parens

::: dom/media/tests/mochitest/templates.js
(Diff revision 1)
> +  function PC_LOCAL_SETUP_NEGOTIATION_CALLBACK(test) {
> +    test.pcLocal.onNegotiationneededFired = false;
> +    test.pcLocal._pc.onnegotiationneeded = anEvent => {
> +      info("pcLocal.onnegotiationneeded fired");
> +      test.pcLocal.onNegotiationneededFired = true;
> +    };
> +  },

This isn't actually testing that this is generated at the right moment.  Can we instead have an expectOnNegotiationNeeded function on the pcWrapper?  That can be called as needed.

::: dom/media/tests/mochitest/test_peerConnection_removeAudioTrack.html
(Diff revision 1)
> +          test.setOfferOptions({ mandatory: { OfferToReceiveAudio: true } });

Is this the right syntax: https://w3c.github.io/webrtc-pc/#offer-answer-options

::: media/mtransport/nricectx.cpp
(Diff revision 1)
> -  MOZ_ASSERT(ctx_->state == ICE_CTX_INIT);
> -  if (ctx_->state != ICE_CTX_INIT) {
> -    MOZ_MTLOG(ML_ERROR, "ICE ctx in the wrong state for gathering: '"
> -              << name_ << "'");
> -    SetConnectionState(ICE_CTX_FAILED);
> -    return NS_ERROR_FAILURE;
> -  }

Hah, I just fixed that code in bug 1129791.

::: media/mtransport/test/ice_unittest.cpp
(Diff revision 1)
> -  void AddStream(int components) {
> +  void AddStream_s(int components) {

That's disturbing...

::: media/mtransport/test/ice_unittest.cpp
(Diff revision 1)
> +      remote_->UnsetRemote();

Why not Shutdown()

::: media/mtransport/test/ice_unittest.cpp
(Diff revision 1)
> -  void DisableComponent(size_t stream, int component_id) {
> +  // Allow us to parse candidates directly on the current thread.

Not sure about the comment now...

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h
(Diff revision 1)
> -  virtual size_t
> -  GetTransportCount() const MOZ_OVERRIDE
> +  virtual std::vector<RefPtr<JsepTransport>>
> +  GetTransports() const MOZ_OVERRIDE
>    {
> -    return mTransports.size();
> +    return mTransports;

A copy?  Why not a reference and a gentleman's agreement not to touch.  A modern language would allow you to create a read-only view.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h
(Diff revision 1)
>  class JsepSessionImpl : public JsepSession

As I predicted, this class is officially too big.

Might want to look for opportunities to refactor soon.  (I'm almost inclined to ask you to do that here.)

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
>  JsepSessionImpl::AddOfferMSectionsByType(SdpMediaSection::MediaType mediatype,
> -                                         Maybe<size_t> offerToReceive,
> +                                         Maybe<size_t> offerToReceiveCount,
>                                           Sdp* sdp)

There *has* to be a simpler way to construct this function.  As it stands, it's way, way too long and complex.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +    if (!oldAnswer.GetMediaSection(i).GetPort()) {
> +        // This m-section is disabled

MsectionIsDisabled() ?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +    // TODO: Spec requires us to not emit codecs that weren't in the answer,
> +    // but this runs counter to usual practice. Do we want to do this?
> +    // Same for stuff like rtcp-fb and extmap

We should generate a new offer with all that we are willing to do.  The answerer is free to do what they did last time.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +                       mIsOfferer ?
> +                         *mCurrentRemoteDescription :
> +                         *mCurrentRemoteDescription,

Take another look at this line.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +    // Now we copy over attributes that won't be added by the usual logic
> +    if (transportAttrs.HasAttribute(SdpAttribute::kCandidateAttribute)) {

If you are copying rtcpmux based on the answer, then you should probably not copy over candidates for the second component when it is set.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +  // Unassign local tracks whose m-sections don't have the recv bit set anymore
> +  for (auto i = mLocalTracks.begin(); i != mLocalTracks.end(); ++i) {

That comment didn't work for me, perhaps:

// Disable send for local tracks if the offer no longer allows it
// (i.e., the m-section is recvonly, inactive or disabled)

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +  for (auto i = mLocalTracks.begin(); i != mLocalTracks.end(); ++i) {

Also, new functions ensure better documentation that comments.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
>  nsresult
>  JsepSessionImpl::HandleNegotiatedSession(const UniquePtr<Sdp>& local,
>                                           const UniquePtr<Sdp>& remote)

OK, here's another function that needs an axe taken to it.  I couldn't follow it before.  Now...

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
>      nsresult rv = DetermineSendingDirection(
>          offerMsection.GetDirectionAttribute().mValue,
>          answerMsection.GetDirectionAttribute().mValue, &sending, &receiving);
>      NS_ENSURE_SUCCESS(rv, rv);

Can you use your IsSending and IsReceiving functions here now instead?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> -    // TODO(bug 1017888): Suppress new track creation on renegotiation
> -    // of existing tracks.
> -    if (direction & SdpDirectionAttribute::kSendFlag) {
> +    if (MsectionIsDisabled(msection) || !msection.IsSending()) {
> +      continue;
> +    }

Why not msection.IsDisabled() ?

And maybe msection.IsSending() could return false if msection.IsDisabled() is true.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
>  nsresult
> -JsepSessionImpl::CreateReceivingTrack(size_t mline,
> +JsepSessionImpl::ValidateRemoteDescription(const Sdp& description)

Might also want to check that the new description has as least as many msections as the old.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +                                      RefPtr<JsepTrack>* trackOut)

Calling a receiving track "trackOut" seems inadvisable.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +      } else {
> +        // Slave bundle m-section. Skip.
> +        return NS_OK;
> +      }

Prefer return-early.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +                               std::set<std::string>* bundleMids,

If this isn't creating and returning bundleMids, pass it by reference.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +    if (!(*bundleMsection)->GetPort()) {

MsectionIsDisabled()

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +  if (level >= localSdp.GetMediaSectionCount()) {
> +    return false;
> +  }

How can this actually happen?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +      mid != bundleMsection->GetAttributeList().GetMid()) {

maybe just compare level with the level of the bundle section.  Less code.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> -JsepSessionImpl::DisableMsection(Sdp& sdp, SdpMediaSection& msection) const
> +JsepSessionImpl::DisableMsection(Sdp& sdp, SdpMediaSection* msection) const

I'm not sure that I understand the reasoning behind this change.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +bool
> +JsepSessionImpl::MsectionIsDisabled(const SdpMediaSection& msection) const

Definitely move this to SdpMediaSection.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
(Diff revision 1)
> +    // TODO: There is no onaddtrack event in the spec. Is anyone actually using
> +    // this?

There is, but it's on the stream.  The expectation is that you generate a stream and fire onaddstream on the PC or you add the track to an existing stream and fire onaddtrack on that stream.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
(Diff revision 1)
> -    if (NS_SUCCEEDED(res) &&
> -        trackPair->mSending && // Assumes we don't do recvonly datachannel
> +    if (trackPair.mSending && // Assumes we don't do recvonly datachannel
> +        trackPair.mSending->GetMediaType() == SdpMediaSection::kApplication) {

Someone should test that assumption.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
(Diff revision 1)
>  PeerConnectionImpl::SetRemoteDescription(int32_t action, const char* aSDP)

Here is another one that needs work.  All the stream manipulation stuff could be factored out into another method at least.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
(Diff revision 1)
> +  if (!mShouldCallNegotiationNeeded) {

I see that this is false on construction, but I don't see it being set to false when the state changes.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
(Diff revision 1)
> +    if (!mJsepSession->AllLocalTracksAreAssigned()) {
> +      CSFLogWarn(logTag, "Not all local tracks were assigned to an "
> +                         "m-section, probably because the offerer did not offer"
> +                         " to receive enough tracks. This requires "
> +                         "renegotiation.");
> +      fireNegotiationNeeded = true;
> +    }

What about the removed tracks?  What about the data channels (for the first one, at least)?

And the warning isn't really necessary.  It's always possible to add tracks in non-stable states.

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  RefPtr<JsepTrack> GetTrack(JsepSessionImpl* side,
> +                             size_t index,
> +                             SdpMediaSection::MediaType type) {

Nit: type first, since that counts before index does.

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 1)
> +      if (video ==
> +          (currentMsection->GetMediaType() == SdpMediaSection::kVideo)) {

tricky

Comment 64

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/3655/#review2969

> That sounds scary.  I couldn't r+ something with a comment like that in it.

This is just how the test used to be structured.

> No need to return a resolved promise here.

Ok, should we also just do Promise.resolve(); return; in getAllUserMedia()?

> checkTrackIsExpected() ?

Maybe that's close enough to not be misleading? That function does a fair bit of stuff.

> " was not observed" ?

The convention here is to put what you were expecting to happen. Yes, it reads extremely poorly. :(

> Factor this into checkAddStreamEvent and you will find that you can compress this a lot more.

We cannot put the logging there, since it will be incorrect if it fires in our eventListener below. We cannot clear out addStreamEvents there, since that would cause us to nuke the array after we check the first event. And we cannot resolve there, or we'll bail out too early. I suppose we could write a single function that iterates over addStreamEvents, and have that be our eventHandler too, but I worry that we'll be prone to ordering problems (ie; we process addStreamEvents before the event ends up in addStreamEvents).

> Why not Shutdown()

Propagating the call to Shutdown() violates the principle of least surprise for me.

> If you are copying rtcpmux based on the answer, then you should probably not copy over candidates for the second component when it is set.

Fixing this would require re-parsing the candidates, and the parse code lives down in nICEr. I'm pretty sure the other end is required to ignore these.

> A copy?  Why not a reference and a gentleman's agreement not to touch.  A modern language would allow you to create a read-only view.

This function is declared in the base class, and returning a reference would force implementations to keep its JsepTransports in a vector. Plus, copying a RefPtr is dirt cheap.

> As I predicted, this class is officially too big.
> 
> Might want to look for opportunities to refactor soon.  (I'm almost inclined to ask you to do that here.)

Let's file a separate bug.

> There *has* to be a simpler way to construct this function.  As it stands, it's way, way too long and complex.

I've already spent a while trying to trim down this function. Each one of the 4 major steps need to be there, in the same order (make sure local tracks have m-sections, make sure we don't unset the recv bit on preexisting m-sections, setting the recv bit on preexisting m-sections if we need more, and finally adding recvonly m-sections if we still need more). Any modification of the order, or omission of steps, leads to weird crap like having more recv m-sections than were asked for, less recv m-sections than the other side was using previously, unsetting recv on an m-section that was being used by a remote stream and setting recv on an earlier m-section to compensate, adding recvonly m-sections when we could have just set the recv bit on a preexisting one, etc. I fear that the best we can do is simply factor out the 4 steps.

> How can this actually happen?

I think this was here because at one point I was using this function to filter remote candidates too. Will remove.

> Definitely move this to SdpMediaSection.

I thought about this, and decided against it because the result depends on whether the caller understands bundle or not, which the SdpMediaSection cannot know unless we make it an explicit param.

> Here is another one that needs work.  All the stream manipulation stuff could be factored out into another method at least.

Let's file a separate bug for this; I don't want to have to spend a day refactoring this and then figuring out why it broke when uplift is so close.

> What about the removed tracks?  What about the data channels (for the first one, at least)?
> 
> And the warning isn't really necessary.  It's always possible to add tracks in non-stable states.

Ok, if we're going to handle _removing_ tracks when not in stable, let's do that in a separate bug, and forbid it unless in stable for now. I am not even remotely confident that we'd do something sane right now, and I am also pretty sure making the behavior sane will not be trivial. Adding DataChannel will fall into the same codepath as adding media tracks.

> MsectionIsDisabled() ?

I guess we could use that, since putting bundle-only in an answer is undefined behavior.

> Might also want to check that the new description has as least as many msections as the old.

I did.

> If this isn't creating and returning bundleMids, pass it by reference.

This is a point of contention. Ask around and you'll get answers ranging from "Yes it should be a pointer." (ekr) to "Using a reference is ok." (abr) to "You should use a reference." (you). I'm kinda stuck in the middle. The code we have so far leans toward doing this, so that's what I'm going with.

Comment 65

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/3655/#review2995

> Ok, should we also just do Promise.resolve(); return; in getAllUserMedia()?

Just return here.  The call site can deal.  In this case, the test functions can return a non-promise, which will be interpreted by the promise chain as an immediate resolve.  Change all the call sites to remove the return statement and that will work.

> We cannot put the logging there, since it will be incorrect if it fires in our eventListener below. We cannot clear out addStreamEvents there, since that would cause us to nuke the array after we check the first event. And we cannot resolve there, or we'll bail out too early. I suppose we could write a single function that iterates over addStreamEvents, and have that be our eventHandler too, but I worry that we'll be prone to ordering problems (ie; we process addStreamEvents before the event ends up in addStreamEvents).

OK, I needed to think about this more.  Is there any case where receiving an onaddstream event after setRemoteDescription is not an error?  I think that removing the addEventListener code is actually better.  Then my only suggestion would be to inline checkAddStreamEvent.

> I've already spent a while trying to trim down this function. Each one of the 4 major steps need to be there, in the same order (make sure local tracks have m-sections, make sure we don't unset the recv bit on preexisting m-sections, setting the recv bit on preexisting m-sections if we need more, and finally adding recvonly m-sections if we still need more). Any modification of the order, or omission of steps, leads to weird crap like having more recv m-sections than were asked for, less recv m-sections than the other side was using previously, unsetting recv on an m-section that was being used by a remote stream and setting recv on an earlier m-section to compensate, adding recvonly m-sections when we could have just set the recv bit on a preexisting one, etc. I fear that the best we can do is simply factor out the 4 steps.

Four steps would help greatly.  The concern with a single function is always that state from one step is leaking across into the state of subsequent steps in non-obvious ways.

> Fixing this would require re-parsing the candidates, and the parse code lives down in nICEr. I'm pretty sure the other end is required to ignore these.

Ahh, I knew that I'd regret that.

> This is a point of contention. Ask around and you'll get answers ranging from "Yes it should be a pointer." (ekr) to "Using a reference is ok." (abr) to "You should use a reference." (you). I'm kinda stuck in the middle. The code we have so far leans toward doing this, so that's what I'm going with.

I know.  The point here being that Sdp is being passed by reference and it is being treated identically (i.e., it's being mucked with).

Ultimately, I don't care.

> Ok, if we're going to handle _removing_ tracks when not in stable, let's do that in a separate bug, and forbid it unless in stable for now. I am not even remotely confident that we'd do something sane right now, and I am also pretty sure making the behavior sane will not be trivial. Adding DataChannel will fall into the same codepath as adding media tracks.

Yep, that sounds reasonable.  It almost looked like you had things sorted out, but removal does require immediate action, so maybe not.

Comment 66

[Byron Campen [:bwc]]

Comment on attachment 8562213 [details]
MozReview Request: bz://1017888/bwc

/r/3657 - Bug 1017888 - Part 1: Renegotiation support.
/r/3659 - Bug 1017888 - Part 2: Testing for renegotiation.

Pull down these commits:

hg pull review -r f37968b1df0d31fc5a66e807323f4133715e8f91

Comment 67

[Byron Campen [:bwc]]

Comment on attachment 8562213 [details]
MozReview Request: bz://1017888/bwc

Sadly, the interdiff for part 1 is screwed up. Let me see what can be done about that.

Comment 68

[Byron Campen [:bwc]]

Less messed up interdiff for part 1 here: https://reviewboard.mozilla.org/r/3657/diff/1-2/

Still lots of spurious "whitespace only changes" for some reason.

Comment 69

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/3659/#review2977

::: dom/media/tests/mochitest/dataChannel.js
(Diff revision 1)
> +  // TODO: Figure out why the steps that follow this don't work.

Is this comment still true?

::: dom/media/tests/mochitest/head.js
(Diff revision 1)
>     * Returns the index of the specified command in the chain.

Can you please update the comment regarding the optional start parameter.

::: dom/media/tests/mochitest/head.js
(Diff revision 1)
> +  insertAfterAll: function(functionOrName, commands) {

Just from the function name I assumed this would append at the end of function list. Only the comment clarified it for me. Can we rename this into 'insertAfterEach'?
I think that should avoid confusion if someone reads just the function name somewhere.

::: dom/media/tests/mochitest/head.js
(Diff revision 1)
> +    if (firstIndex < 0 || lastIndex < 0 || lastIndex < firstIndex) {

The error should also be thrown for lastIndex <= firstIndex

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> +  info("Commands: " + JSON.stringify(options.commands));

Debug statement which can/should be removed?

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> +          // TODO (bug 1089798): Store both the stream and track id

Do we really want to add that in bug 1089798 or remoave this comment?

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> +          this.iceCheckingRestartExpected = undefined;

undefined? Why not simply 'false'?

I think we should also specify and set this variable in the PeerConnectionWrapper constructor. Otherwise this prone for someone trying to access this variable when it did not exist yet.

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> +    var observedLocalTrackTypesById = {};
> +    // We do not want to empty out this.expectedLocalTrackTypesById, so make a
> +    // copy.
> +    var expectedLocalTrackTypesById =
> +      JSON.parse(JSON.stringify((this.expectedLocalTrackTypesById)));
> +    info(this + " Checking local tracks " +
> +         JSON.stringify(expectedLocalTrackTypesById));
> +    this._pc.getLocalStreams().forEach(stream => {
> +      stream.getTracks().forEach(track => {
> +        this.checkTrack(track,
> +                        expectedLocalTrackTypesById,
> +                        observedLocalTrackTypesById);

What is observedLocalTrackTypesById good for? Seems to get filled by checkTrack() but then never used again?!

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> +        this.expectedLocalTrackTypesById[stream.getVideoTracks()[0].id] = "video";

Can we do a forEch(stream.getVideoTracks()) here so we are prepared for more then one stream?

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> +  this.addStreamEvents = [];

Wouldn't it be easier/cleaner to have this.expectedRemoteTrackTypeById and not store the addStreamEvents at all?
Or is the reason here that you are concerned about the expected streams messages between the two PeerConnection (potentially over the signaling server) arriving after the addStreamEvent?

::: dom/media/tests/mochitest/templates.js
(Diff revision 1)
> +  if (!pc.iceCheckingRestartExpected) {
> -  if (pc.isIceConnected()) {
> +    if (pc.isIceConnected()) {
> -    info(pc + ": ICE connection state log: " + pc.iceConnectionLog);
> +      info(pc + ": ICE connection state log: " + pc.iceConnectionLog);
> -    ok(true, pc + ": ICE is in connected state");
> +      ok(true, pc + ": ICE is in connected state");
> -    return Promise.resolve();
> +      return Promise.resolve();
> -  }
> +    }
>  
> -  if (!pc.isIceConnectionPending()) {
> +    if (!pc.isIceConnectionPending()) {
> -    dumpSdp(test);
> +      dumpSdp(test);
> -    var details = pc + ": ICE is already in bad state: " + pc.iceConnectionState;
> +      var details = pc + ": ICE is already in bad state: " + pc.iceConnectionState;
> -    ok(false, details);
> +      ok(false, details);
> -    return Promise.reject(new Error(details));
> +      return Promise.reject(new Error(details));
> -  }
> +    }
> +  }
>  
>    return pc.waitForIceConnected()
>      .then(() => {
>        info(pc + ": ICE connection state log: " + pc.iceConnectionLog);
>        ok(pc.isIceConnected(), pc + ": ICE switched to 'connected' state");
>      });

Which state are we in here that we are 100% sure we are not connected yet, but also not pending, but we still need to wait for connected again?

Is it posibble that by jumping over this if the pc.waitForIceConnected promise always resolves immediately, because was once connected already?

::: dom/media/tests/mochitest/templates.js
(Diff revision 1)
> +  function PC_LOCAL_STEEPLECHASE_SIGNAL_EXPECTED_LOCAL_TRACKS(test) {
> +    if (test.steeplechase) {
> +      send_message({"type": "local_expected_tracks",
> +                    "expected_tracks": test.pcLocal.expectedLocalTrackTypesById});
> +    } else {
> +      // Deep copy, as similar to steeplechase as possible
> +      test._localExpectedLocalTrackTypesById =
> +        JSON.parse(JSON.stringify((test.pcLocal.expectedLocalTrackTypesById)));
> +    }
> +  },

So why do we send the expected track IDs over to the other side only now?
Should we be able to do this right after the gUM call, or at latest when we generated a SDP?

::: dom/media/tests/mochitest/templates.js
(Diff revision 1)
> -    return test.pcLocal.checkMediaTracks(test._answer_constraints);
> +    return test.pcLocal.checkMediaTracks(test._remoteExpectedLocalTrackTypesById);

For the steeplechase cases you will have to use test.getSignalingMessage("remote_expected_tracks") somewhere. Otherwise pcLocal might just call checkMediaTracks(undefined) here.

::: dom/media/tests/mochitest/templates.js
(Diff revision 1)
> -    return test.pcRemote.checkMediaTracks(test._offer_constraints);
> +    return test.pcRemote.checkMediaTracks(test._localExpectedLocalTrackTypesById);

Same problem here.

::: dom/media/tests/mochitest/test_peerConnection_addSecondAudioStream.html
(Diff revision 1)
> +          test.setMediaConstraints([{audio: true}, {audio: true}],
> +                                   [{audio: true}]);

Do we gain something here from doing a send-/recv-only stream oposed to symmetric sendrecv with a second audio stream here?

::: dom/media/tests/mochitest/test_peerConnection_addSecondAudioStream.html
(Diff revision 1)
> -      // TODO(bug 1093835): figure out how to verify if media flows through the new stream

I think this comment is still applicable and should be preserved.

::: dom/media/tests/mochitest/test_peerConnection_removeAudioTrack.html
(Diff revision 1)
> +    // TODO(bug 1093835): figure out how to verify if media flows through the new stream

This should probably be either removed or updated to "verify that media only flows through the one remaining uni-directional audio stream".

::: dom/media/tests/mochitest/test_peerConnection_removeThenAddVideoTrackNoBundle.html
(Diff revision 1)
> +  runNetworkTest(function (options) {
> +    test = new PeerConnectionTest(options);
> +    addRenegotiation(test.chain,
> +      [
> +        function PC_LOCAL_REMOVE_AUDIO_TRACK(test) {
> +          return test.pcLocal.removeSender(0);
> +        },
> +        function PC_LOCAL_ADD_AUDIO_TRACK(test) {
> +          return test.pcLocal.getAllUserMedia([{video: true}]);
> +        },
> +      ]
> +    );
> +
> +    // TODO(bug 1093835): figure out how to verify if media flows through the new stream
> +    test.setMediaConstraints([{video: true}], [{video: true}]);
> +    test.run();

This is actually testing the bundle version and is therefore the identical test as test_peerConnection_removedThenAddVideoTrack.html.

::: dom/media/tests/mochitest/test_peerConnection_removeVideoTrack.html
(Diff revision 1)
> +    // TODO(bug 1093835): figure out how to verify if media flows through the new stream

Update comment.

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  RefPtr<JsepTrack> GetTrack(JsepSessionImpl* side,

Shouldn't a reference for the side be enough here?

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  std::vector<JsepTrackPair> GetTrackPairsByLevel(JsepSessionImpl* side) {

Reference?

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  int GetLevelForSendTrack(JsepSessionImpl* side,
> +                           const std::string& streamId,
> +                           const std::string& trackId) {

Reference?

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  int GetLevelForRecvTrack(JsepSessionImpl* side,
> +                           const std::string& streamId,
> +                           const std::string& trackId) {

Reference?

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  size_t GetTrackCount(JsepSessionImpl* side,
> +                       SdpMediaSection::MediaType type) {

Reference?

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  bool HasType(SdpMediaSection::MediaType type) {
> +    if (types.empty()) {
> +      types = BuildTypes(GetParam());
> +    }
> +
> +    return types.end() != std::find(types.begin(), types.end(), type);
> +  }

Unused code?

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  UniquePtr<Sdp> GetParsedLocalDescription(JsepSessionImpl* side) {

Reference?

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  SdpMediaSection* GetMsection(Sdp* sdp,

Reference?

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +    ASSERT_LT(level, parsed->GetMediaSectionCount()); 

Whitespace at EOL

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  if (types.front() == SdpMediaSection::kApplication) {

Shouldn't this get checked even before the AddTracks() calls?

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  auto added = mSessionAns.GetRemoteTracksAdded();
> +  auto removed = mSessionAns.GetRemoteTracksRemoved();
> +  ASSERT_EQ(types.size(), added.size());
> +  ASSERT_EQ(0U, removed.size());

With the amount of the same checks being done over and over again maybe factor this out into a small verification function?

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  auto added = mSessionAns.GetRemoteTracksAdded();
> +  auto removed = mSessionAns.GetRemoteTracksRemoved();
> +  ASSERT_EQ(0U, added.size());
> +  ASSERT_EQ(1U, removed.size());
> +
> +  ASSERT_EQ(removedTrack->GetMediaType(), removed[0]->GetMediaType());
> +  ASSERT_EQ(removedTrack->GetStreamId(), removed[0]->GetStreamId());
> +  ASSERT_EQ(removedTrack->GetTrackId(), removed[0]->GetTrackId());
> +
> +  added = mSessionOff.GetRemoteTracksAdded();
> +  removed = mSessionOff.GetRemoteTracksRemoved();
> +  ASSERT_EQ(0U, added.size());
> +  ASSERT_EQ(0U, removed.size());

The answerer sees one track missing, but the offerer not?

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  if (types.front() == SdpMediaSection::kApplication) {

Move up

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  if (types.front() == SdpMediaSection::kApplication) {

Move up

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +TEST_P(JsepSessionTest, RenegotiationBothRemoveTrack)

In this case both sides agree on removing the same track. How about a test where they remove different tracks?

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  if (types.front() == SdpMediaSection::kApplication) {

Move up

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  DisableMsid(&answer);
> +
> +  SetRemoteAnswer(answer, CHECK_SUCCESS);
> +
> +  auto newOffererPairs = mSessionOff.GetNegotiatedTrackPairs();
> +
> +  ASSERT_EQ(offererPairs.size(), newOffererPairs.size());
> +  for (size_t i = 0; i < offererPairs.size(); ++i) {
> +    ASSERT_TRUE(Equals(offererPairs[i], newOffererPairs[i]));
> +  }

Shouldn't you also compare track IDs here to see if they are in fact stable as the comment above the test suggests?

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +TEST_P(JsepSessionTest, RenegotiationOffererDisablesBundleTransport)
> +{
> +  AddTracks(&mSessionOff);
> +  AddTracks(&mSessionAns);
> +
> +  if (types.size() < 2) {
> +    return;
> +  }
> +
> +  OfferAnswer();
> +
> +  auto offererPairs = GetTrackPairsByLevel(&mSessionOff);
> +  auto answererPairs = GetTrackPairsByLevel(&mSessionAns);
> +
> +  std::string reoffer = CreateOffer();
> +
> +  DisableMsection(&reoffer, 0);

The test title suggests disabled bundle, but then the first m-section seems to get disabled instead.

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> +  DisableMsection(&reanswer, 0);

See above

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revision 1)
> -                                          "datachannel,video,audio"));
> +                      "datachannel,video,audio",

I think we should throw into the mix:
  audio, datachannel, video
just to be safe.

Comment 70

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/3659/#review3011

> Is this comment still true?

It is. There's something racy going on here, but it was a pre-existing problem (the tests did this before).

> The error should also be thrown for lastIndex <= firstIndex

I've removed this whole function, since I ended up refactoring to not use it.

> Wouldn't it be easier/cleaner to have this.expectedRemoteTrackTypeById and not store the addStreamEvents at all?
> Or is the reason here that you are concerned about the expected streams messages between the two PeerConnection (potentially over the signaling server) arriving after the addStreamEvent?

I'm thinking maybe what I should do instead is set up a Promise that handles these events and resolves once all the expected tracks are in place, and then wait on the promise later on in the test?

> What is observedLocalTrackTypesById good for? Seems to get filled by checkTrack() but then never used again?!

It is used by checkTracks on any subsequent call. I think I will refactor this, though, since I think I have an idea to make it better.

> Which state are we in here that we are 100% sure we are not connected yet, but also not pending, but we still need to wait for connected again?
> 
> Is it posibble that by jumping over this if the pc.waitForIceConnected promise always resolves immediately, because was once connected already?

The idea here is we are already in connected, and have done some sort of renegotiation that will transition us back to checking, and then to connected. We want to avoid resolving early if we have not done this back-and-forth yet, since that will cause races later on (particularly wrt ICE stats).

> With the amount of the same checks being done over and over again maybe factor this out into a small verification function?

I'm of mixed feelings on this. Factoring gtest assertions out into their own function means that when they are violated, the test does not halt immediately, which is bad. Factoring them out into a macro is better in that regard, but then again you're using a macro.

> Shouldn't this get checked even before the AddTracks() calls?

AddTracks() is what populates |types|

> The answerer sees one track missing, but the offerer not?

Yes, the offerer removes its track, but the answerer does not.

> Shouldn't you also compare track IDs here to see if they are in fact stable as the comment above the test suggests?

We check that the pointers to the tracks match, so unless the id on the track object itself changes (which we never do), we should be fine here.

> The test title suggests disabled bundle, but then the first m-section seems to get disabled instead.

So the "bundle transport" is the m-section that comes first in the group attribute, which is the first m-section in this case.

> See above

Same answer as above.

> I think we should throw into the mix:
>   audio, datachannel, video
> just to be safe.

All of these just get sorted anyway, I think. I'll double-check.

> In this case both sides agree on removing the same track. How about a test where they remove different tracks?

This should be doable.

> Unused code?

Looks like it. Will remove.

> This is actually testing the bundle version and is therefore the identical test as test_peerConnection_removedThenAddVideoTrack.html.

Whoops! Will fix.

> Do we gain something here from doing a send-/recv-only stream oposed to symmetric sendrecv with a second audio stream here?

The test is about one side adding a track only. We could write a separate test where the answerer also adds a track, but it is getting late to be adding more tests. We have tested this scenario in the jsep and signaling unit-tests, so I think there's limited utility in doing it at the mochitest level too; the mochitests are mostly there to do things like verify that the stuff JsepSessionImpl exposes is properly surfaced to content (including sensible stats), as well as some extra sanity-checking on video since the signaling_unittests don't check video.

> Do we really want to add that in bug 1089798 or remoave this comment?

Hmm. I think just looking at the track id should be sufficient. Will remove this comment.

> Debug statement which can/should be removed?

Yes.

> Just from the function name I assumed this would append at the end of function list. Only the comment clarified it for me. Can we rename this into 'insertAfterEach'?
> I think that should avoid confusion if someone reads just the function name somewhere.

Yeah, that's a better name.

> Can you please update the comment regarding the optional start parameter.

Will do.

> Can we do a forEch(stream.getVideoTracks()) here so we are prepared for more then one stream?

I can look into it.

> undefined? Why not simply 'false'?
> 
> I think we should also specify and set this variable in the PeerConnectionWrapper constructor. Otherwise this prone for someone trying to access this variable when it did not exist yet.

Fair enough.

> So why do we send the expected track IDs over to the other side only now?
> Should we be able to do this right after the gUM call, or at latest when we generated a SDP?

We could do that too.

> For the steeplechase cases you will have to use test.getSignalingMessage("remote_expected_tracks") somewhere. Otherwise pcLocal might just call checkMediaTracks(undefined) here.

Where does this need to be called? Before any messages of that type could be sent?

> I think this comment is still applicable and should be preserved.

Right.

> Shouldn't a reference for the side be enough here?

Yes, probably on the rest of these too. Also need to fix the const-correctness on most of these.

Comment 71

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/3659/#review3015

::: dom/media/tests/mochitest/templates.js
(Diff revisions 1 - 2)
> +var addRenegotiationAnswerer = (chain, commands, checks) => {
> +  chain.append(function SWAP_PC_LOCAL_PC_REMOTE(test) {
> +    var temp = test.pcLocal;
> +    test.pcLocal = test.pcRemote;
> +    test.pcRemote = temp;
> +  });
> +  addRenegotiation(chain, commands, checks);
> +};

I'm a little concerned that this will completely screw up the logging.
The function names are still PC_LOCAL and PC_REMOTE in templates.js, but they are referring to the opposite now. And also any function in pc.js which logs "this" probably still logs the original "PeerConnectionWrapper (pcLocal)".

I think this screems for changing the naming of pcLocal and pcRemote as in bug 1128046. This might actually be a strong argument for naming them "Alice" and "Bob" to avoid confusion with numbers in the names

::: dom/media/tests/mochitest/dataChannel.js
(Diff revisions 1 - 2)
> -  // TODO: Figure out why the steps that follow this don't work.
> +  chain.insertBefore('PC_LOCAL_CHECK_MEDIA_TRACKS', commandsWaitForDataChannel);
>    chain.removeAfter('PC_REMOTE_CHECK_ICE_CONNECTIONS');

I would prefer for cleaniness if we would first remove everything after ICE_CONNECTIONS and the append twice.

::: dom/media/tests/mochitest/pc.js
(Diff revisions 1 - 2)
> +                    ok(rem.packetsReceived <= res.packetsSent, "No more than sent");

Can we please update the messsage here to "No more than sent packets"?

::: dom/media/tests/mochitest/dataChannel.js
(Diff revisions 1 - 2)
> -var commandsCheckDataChannel = [
> +var commandsWaitForDataChannel = [
>    function PC_LOCAL_VERIFY_DATA_CHANNEL_STATE(test) {
>      return test.pcLocal.dataChannels[0].opened;
>    },
>  
>    function PC_REMOTE_VERIFY_DATA_CHANNEL_STATE(test) {
>      return test.pcRemote.nextDataChannel.then(channel => channel.opened);
>    },
> +];
>  
> +var commandsCheckDataChannel = [

Thank you!

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
(Diff revisions 1 - 2)
> +TEST_P(JsepSessionTest, RenegotiationFromAnswererNoChange)
> +{
> +  AddTracks(&mSessionOff);
> +  std::string offer = CreateOffer();
> +  SetLocalOffer(offer);
> +  SetRemoteOffer(offer);
> +
> +  auto added = mSessionAns.GetRemoteTracksAdded();
> +  auto removed = mSessionAns.GetRemoteTracksRemoved();
> +  ASSERT_EQ(types.size(), added.size());
> +  ASSERT_EQ(0U, removed.size());
> +
> +  AddTracks(&mSessionAns);
> +  std::string answer = CreateAnswer();
> +  SetLocalAnswer(answer);
> +  SetRemoteAnswer(answer);
> +
> +  added = mSessionOff.GetRemoteTracksAdded();
> +  removed = mSessionOff.GetRemoteTracksRemoved();
> +  ASSERT_EQ(types.size(), added.size());
> +  ASSERT_EQ(0U, removed.size());
> +
> +  auto offererPairs = GetTrackPairsByLevel(&mSessionOff);
> +  auto answererPairs = GetTrackPairsByLevel(&mSessionAns);
> +
> +  std::string reoffer = CreateOffer();
> +  SetLocalOffer(reoffer);
> +  SetRemoteOffer(reoffer);

From the title I assume this was suppose to create an offer from the initial answerer side. But I don't see any role change in here. So it looks like a plain re-negotiation without any changes, like we have already.

Comment 72

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/3659/#review3019

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 2)
> -  // Removes a stream from the PeerConnection. If the stream
> -  // parameter is absent, removes the stream that was most
> -  // recently added to the PeerConnection.
> +  void RemoveTrack(size_t streamIndex, bool videoTrack = false)
> +  {
> +    ASSERT_LT(streamIndex, domMediaStreams_.size());
> -  void RemoveLastStreamAdded() {
>      nsTArray<nsRefPtr<MediaStreamTrack>> tracks;
> -    domMediaStream_->GetTracks(tracks);
> +    domMediaStreams_[streamIndex]->GetTracks(tracks);
> +    for (size_t i = 0; i < tracks.Length(); ++i) {
> +      if (!!tracks[i]->AsVideoStreamTrack() == videoTrack) {
> +        ASSERT_EQ(pc->RemoveTrack(tracks[i].get()), NS_OK);
> +      }
> +    }
> +  }

I assume in the long run we want something here which allows not only to remove either the video or the audio track from a stream, but remove a video or audio track from a stream which has multiple audio and/or video tracks.

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 2)
> +        ASSERT_EQ(pc->RemoveTrack(tracks[i].get()), NS_OK);
> +      }
> +    }
> +  }
> +
> +  void RemoveStream(size_t index) {
> +    nsTArray<nsRefPtr<MediaStreamTrack>> tracks;
> +    domMediaStreams_[index]->GetTracks(tracks);
>      for (uint32_t i = 0; i < tracks.Length(); i++) {
>        ASSERT_EQ(pc->RemoveTrack(tracks[i]), NS_OK);

Why is line 1098 using tracks[i].get() and 1107 only tracks[i]?

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 2)
> +  // Removes a stream from the PeerConnection. If the stream
> +  // parameter is absent, removes the stream that was most
> +  // recently added to the PeerConnection.
> +  void RemoveLastStreamAdded() {

Please update the comment. I don't see a stream parameter here (any more).

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 2)
> +    std::cout << "OFFER:" << offer_;

Debugging statement or intended?

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 2)
>    // At present, we use the hints field in a stream to find and
>    // remove it. This only works if the specified hints flags are
>    // unique among all streams in the PeerConnection. This is not
>    // generally true, and will need significant revision once
>    // multiple streams are supported.
>    void CreateOfferRemoveStream(OfferOptions& options,
> -                               uint32_t hints, uint32_t sdpCheck) {
> +                               size_t index, uint32_t sdpCheck) {
> -
> -    domMediaStream_->SetHintContents(hints);
>  
> -    // This currently "removes" a stream that has the same audio/video
> -    // hints as were passed in.
> -    // When complete RemoveStream will remove and entire stream and its tracks
> -    // not just disable a track as this is currently doing
> -    RemoveLastStreamAdded();
>  
>      // Now call CreateOffer as JS would
>      pObserver->state = TestObserver::stateNoResponse;
>      ASSERT_EQ(pc->CreateOffer(options), NS_OK);
>      ASSERT_TRUE(pObserver->state == TestObserver::stateSuccess);
>      SDPSanityCheck(pObserver->lastString, sdpCheck, true);
>      offer_ = pObserver->lastString;
>      if (!mBundleEnabled) {
>        offer_ = RemoveBundle(offer_);
>      }
>    }

- the comment is outdated
- the index paramter is not used
- the function name suggests it removes something which I can't see it doing

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 2)
> -  int GetPacketsReceived(int stream) {
> +  int GetPacketsReceived(size_t stream) {
>      std::vector<DOMMediaStream *> streams = pObserver->GetStreams();
>  
> -    if ((int) streams.size() <= stream) {
> +    if (streams.size() <= stream) {
> +      EXPECT_TRUE(false);
>        return 0;
>      }
>  
>      return streams[stream]->GetStream()->AsSourceStream()->GetSegmentsAdded();
>    }
>  
> -  int GetPacketsSent(int stream) {
> +  int GetPacketsSent(size_t stream) {
> +    if (stream >= domMediaStreams_.size()) {
> +      EXPECT_TRUE(false);
> +      return 0;
> +    }
> +
>      return static_cast<Fake_MediaStreamBase *>(
> -        domMediaStream_->GetStream())->GetSegmentsAdded();
> +        domMediaStreams_[stream]->GetStream())->GetSegmentsAdded();
>    }

Is this inconsistency between these two functions intended (one calls GetStreams() and the other access the streams directly via domMediaStreams)?

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 2)
> +  std::string SwapMsids(const std::string& sdp, bool swapVideo) const
> +  {
> +    SipccSdpParser parser;
> +    UniquePtr<Sdp> parsed = parser.Parse(sdp);
> +
> +    SdpMediaSection* previousMsection = nullptr;
> +    for (size_t i = 0; i < parsed->GetMediaSectionCount(); ++i) {
> +      SdpMediaSection* currentMsection = &parsed->GetMediaSection(i);
> +      bool isVideo = currentMsection->GetMediaType() == SdpMediaSection::kVideo;
> +      if (swapVideo == isVideo) {
> +        if (previousMsection) {
> +          UniquePtr<SdpMsidAttributeList> prevMsid(
> +            new SdpMsidAttributeList(
> +                previousMsection->GetAttributeList().GetMsid()));
> +          UniquePtr<SdpMsidAttributeList> currMsid(
> +            new SdpMsidAttributeList(
> +                currentMsection->GetAttributeList().GetMsid()));
> +          previousMsection->GetAttributeList().SetAttribute(currMsid.release());
> +          currentMsection->GetAttributeList().SetAttribute(prevMsid.release());
> +        }
> +        previousMsection = currentMsection;
> +      }
> +    }
> +
> +    return parsed->ToString();
> +  }
> +

An error if swapping did not happen could be helpful here.

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 2)
> +  ASSERT_TRUE_WAIT(a1_->GetPacketsReceived(0) >= 80, kDefaultTimeout * 2);
> +  ASSERT_TRUE_WAIT(a2_->GetPacketsSent(0) >= 80, kDefaultTimeout * 2);

Wouldn't it be safer to query the packets received and sent and use them as threshold for no longer receiving data?

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 2)
> +  ASSERT_TRUE_WAIT(a2_->GetPacketsReceived(0) >= 80, kDefaultTimeout * 2);
> +  ASSERT_TRUE_WAIT(a1_->GetPacketsSent(0) >= 80, kDefaultTimeout * 2);

Same here, guessing that we should have received 80 packets by now seems not very reliable to me.

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 2)
> +TEST_P(SignalingTest, RenegotiationAnswererReplacesTrack)
> +{
> +  OfferOptions options;
> +  OfferAnswer(options, OFFER_AV | ANSWER_AV,
> +              SHOULD_SENDRECV_AV, SHOULD_SENDRECV_AV);
> +
> +  // Wait for some data to get received
> +  ASSERT_TRUE_WAIT(a2_->GetPacketsReceived(0) >= 40, kDefaultTimeout * 2);
> +  // Not really packets, but audio segments, happens later
> +  ASSERT_TRUE_WAIT(a1_->GetPacketsSent(0) >= 40, kDefaultTimeout * 2);
> +
> +  a1_->RemoveTrack(0, false);
> +
> +  OfferAnswer(options, OFFER_AUDIO,
> +              SHOULD_SENDRECV_AV, SHOULD_SENDRECV_AV);

Why is a1 removing a track if the Answerer is suppose to replace a track here?

Comment 73

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/3659/#review3033

> I'm thinking maybe what I should do instead is set up a Promise that handles these events and resolves once all the expected tracks are in place, and then wait on the promise later on in the test?

Yes, as we talked about. I think your best bet is a single promise and one function for the addstream event, which then resolves or rejects the promise.

> The idea here is we are already in connected, and have done some sort of renegotiation that will transition us back to checking, and then to connected. We want to avoid resolving early if we have not done this back-and-forth yet, since that will cause races later on (particularly wrt ICE stats).

My concern is in the other direction: you might call this too late and things might switch to checking and then to connected already. I'm afraid the pc.waitForIceConnected() will in this case never return, as it will only resolve its promise in case an ICE connection state change still happens after waitForIceConnected() got called.

I think this ICE connection checking code is due for refactoring with a proper promise.

> Where does this need to be called? Before any messages of that type could be sent?

Forget about my original comment. test.getSignalingMessage() is kind of blocking request. But that only works if you don't have a function registered for that message type, 'remote_expected_tracks' in this case.
It might be easier to remove your message handler at the top of templates.js and simply use the blocking getSignalingMessage() instead. Because with a message handler I think you would have to setup yet another promise for the incoming message to allow you to proceed here. But either way should probably work.

> The test is about one side adding a track only. We could write a separate test where the answerer also adds a track, but it is getting late to be adding more tests. We have tested this scenario in the jsep and signaling unit-tests, so I think there's limited utility in doing it at the mochitest level too; the mochitests are mostly there to do things like verify that the stuff JsepSessionImpl exposes is properly surfaced to content (including sensible stats), as well as some extra sanity-checking on video since the signaling_unittests don't check video.

Fair enough.

> I'm of mixed feelings on this. Factoring gtest assertions out into their own function means that when they are violated, the test does not halt immediately, which is bad. Factoring them out into a macro is better in that regard, but then again you're using a macro.

Alright. I have no big opinion on code re-use in tests :-)

> AddTracks() is what populates |types|

I see

Comment 74

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/3659/#review3029

> I assume in the long run we want something here which allows not only to remove either the video or the audio track from a stream, but remove a video or audio track from a stream which has multiple audio and/or video tracks.

In the long run, yes.

> Why is line 1098 using tracks[i].get() and 1107 only tracks[i]?

Probably no reason. Will fix.

> Debugging statement or intended?

Debugging, will remove.

> - the comment is outdated
> - the index paramter is not used
> - the function name suggests it removes something which I can't see it doing

Yeah, this is FUBAR. Will fix up. Only reason nothing broke was because all the tests that use it are disabled (although we might be able to re-enable them now, will try)

> Why is a1 removing a track if the Answerer is suppose to replace a track here?

Oops, will fix.

> An error if swapping did not happen could be helpful here.

Sure.

> Is this inconsistency between these two functions intended (one calls GetStreams() and the other access the streams directly via domMediaStreams)?

I'm going to guess this was deliberate. I haven't modified this, and I'm running low on time. Will investigate later.

Comment 75

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/3659/#review3027

> I would prefer for cleaniness if we would first remove everything after ICE_CONNECTIONS and the append twice.

We have to put commandsWaitForDataChannel earlier because we might end up arriving at later steps that need to count the number of datachannels we have, resulting in intermittent failures.

Comment 76

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/3659/#review3025

> All of these just get sorted anyway, I think. I'll double-check.

Ok, they don't get sorted, I'll add these.

Comment 77

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/3659/#review3035

> My concern is in the other direction: you might call this too late and things might switch to checking and then to connected already. I'm afraid the pc.waitForIceConnected() will in this case never return, as it will only resolve its promise in case an ICE connection state change still happens after waitForIceConnected() got called.
> 
> I think this ICE connection checking code is due for refactoring with a proper promise.

We set the flag before we start renegotiating, so we're ok here.

Comment 78

[Byron Campen [:bwc]]

Comment on attachment 8562213 [details]
MozReview Request: bz://1017888/bwc

/r/3657 - Bug 1017888 - Part 1: Renegotiation support.
/r/3659 - Bug 1017888 - Part 2: Testing for renegotiation.

Pull down these commits:

hg pull review -r 9b291b57d655eb3217caf58bb1f6eab6947390fd

Comment 79

[Byron Campen [:bwc]]

Comment on attachment 8562213 [details]
MozReview Request: bz://1017888/bwc

/r/3657 - Bug 1017888 - Part 1: Renegotiation support.
/r/3659 - Bug 1017888 - Part 2: Testing for renegotiation.

Pull down these commits:

hg pull review -r 95a5e934b464d0f3de1245e7c3a7a1c97ac8e23c

Comment 80

[Byron Campen [:bwc]]

Comment on attachment 8562213 [details]
MozReview Request: bz://1017888/bwc

/r/3657 - Bug 1017888 - Part 1: Renegotiation support.
/r/3659 - Bug 1017888 - Part 2: Testing for renegotiation.

Pull down these commits:

hg pull review -r 0c62c5c0e834190a5cc93d47bd0e7b558bd5cbc7

Comment 81

[Byron Campen [:bwc]]

Have incorporated feedback, and rebased onto latest m-c.

Comment 82

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/3659/#review3045

> We set the flag before we start renegotiating, so we're ok here.

I'm not talking about your new iceCheckingRestartExpected flag. My concern here is the code in pc.waitForIceConnected() is not handling all the possible scenarios properly if you switch over the pc.isIceConnected() check.

Comment 83

[Byron Campen [:bwc]]

Try looks great:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c1f6d733751

Comment 84

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/3659/#review3051

> I'm not talking about your new iceCheckingRestartExpected flag. My concern here is the code in pc.waitForIceConnected() is not handling all the possible scenarios properly if you switch over the pc.isIceConnected() check.

I'm not coming up with any scenarios that are a problem here, unless maybe we're already in failed? If we're in checking, this change has no effect. If we're in connected, we don't resolve, and start waiting for another state change. If the next state change goes to checking, pc.waitForIceConnected() does nothing. If failed or connected, it resolves (and logIceConnectionState triggers a failure since it wants to see checking next).

Comment 85

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #84)
> I'm not coming up with any scenarios that are a problem here, unless maybe
> we're already in failed? If we're in checking, this change has no effect. If
> we're in connected, we don't resolve, and start waiting for another state
> change. If the next state change goes to checking, pc.waitForIceConnected()
> does nothing. If failed or connected, it resolves (and logIceConnectionState
> triggers a failure since it wants to see checking next).

Byron pointed out to me a code part which I had overlooked. No concerns any more from my side.

Comment 86

[Martin Thomson [:mt:]]

Comment on attachment 8562213 [details]
MozReview Request: bz://1017888/bwc

https://reviewboard.mozilla.org/r/3655/#review3085

I couldn't see it, but is there a unit test that tests disabling (i.e., `m=audio 0`) media sections?  I want to make sure that we are covering the case where media sections are disabled entirely and then consequently restored.  In particular, the case where the first media section is disabled and re-enabled.  Any such test needs ICE so that we can be sure that nICEr handles a change in the first stream properly (it should, because this isn't new, but better to be safe).

::: dom/media/tests/mochitest/pc.js
(Diff revisions 1 - 5)
> -    return Promise.resolve();
> +    Promise.resolve();

Just delete the line now.  Promise.resolve() is a noop.

::: dom/media/tests/mochitest/pc.js
(Diff revisions 1 - 5)
> -        this.expectedLocalTrackTypesById[stream.getVideoTracks()[0].id] = "video";
> +        stream.getVideoTracks().forEach(track => {
> +          this.expectedLocalTrackTypesById[track.id] = "video";
> +        });

You can put this after the if statement and only have the code once.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
(Diff revisions 1 - 5)
> +    (void)sendDataChannel;

Shouldn't need to void this since it is used below.

I

Comment 87

[Byron Campen [:bwc]]

(In reply to Martin Thomson [:mt] from comment #86)
> Comment on attachment 8562213 [details]
> MozReview Request: bz://1017888/bwc
> 
> https://reviewboard.mozilla.org/r/3655/#review3085
> 
> I couldn't see it, but is there a unit test that tests disabling (i.e.,
> `m=audio 0`) media sections?  I want to make sure that we are covering the
> case where media sections are disabled entirely and then consequently
> restored.  In particular, the case where the first media section is disabled
> and re-enabled.  Any such test needs ICE so that we can be sure that nICEr
> handles a change in the first stream properly (it should, because this isn't
> new, but better to be safe).

   We have tests that disable an m-section, but not a test that re-enables it. We don't currently support ICE restart, which would be required to do this.

Comment 88

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/3659/#review3103

Ship It!

Comment 89

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=abd367878ac2

Comment 90

[Byron Campen [:bwc]]

Comment on attachment 8562213 [details]
MozReview Request: bz://1017888/bwc

DOM review needed for dom/webidl/PeerConnectionObserver.webidl

Comment 91

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e888d63dbd33
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf5ec9f850cd

Comment 92

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e888d63dbd33
https://hg.mozilla.org/mozilla-central/rev/cf5ec9f850cd

Comment 94

[Lukas Blakk [:lsblakk] use ?needinfo]

Added to release notes for 38 Aurora/DevEd as:

WebRTC now has multistream and renegotiation support

Comment 97

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1017888 - Part 1: Renegotiation support.

Comment 98

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1017888 - Part 2: Testing for renegotiation.