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

RESOLVED FIXED in Firefox 38

Status

()

Core
WebRTC: Networking
P2
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: shell, Assigned: bwc)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {dev-doc-needed})

unspecified
mozilla38
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed, relnote-firefox 38+)

Details

(Whiteboard: p=3)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 55 obsolete attachments)

39 bytes, text/x-review-board-request
drno
: review+
smaug
: review+
Details | Review
39 bytes, text/x-review-board-request
drno
: review+
smaug
: review+
Details | Review
(Reporter)

Description

4 years ago
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
This should have said mozilla33
Target Milestone: mozilla32 → mozilla33
I believe this isn't needed for switching cameras.
Priority: P1 → P2
Target Milestone: mozilla33 → mozilla35
(Assignee)

Updated

4 years ago
Depends on: 1099318
(Assignee)

Updated

4 years ago
Depends on: 1091242
(Assignee)

Comment 3

4 years ago
Created attachment 8532330 [details] [diff] [review]
(WIP) Renegotiation support

Some initial work, mostly on creating reoffer.
(Assignee)

Updated

4 years ago
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
(Assignee)

Comment 4

4 years ago
Created attachment 8534669 [details] [diff] [review]
(WIP) Renegotiation support

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

Updated

4 years ago
Attachment #8532330 - Attachment is obsolete: true
(Assignee)

Comment 5

4 years ago
Created attachment 8534735 [details] [diff] [review]
(WIP) Renegotiation support.

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

Updated

4 years ago
Attachment #8534669 - Attachment is obsolete: true
(Assignee)

Comment 6

4 years ago
Created attachment 8535073 [details] [diff] [review]
(WIP) Renegotiation support

Unrot.
(Assignee)

Updated

4 years ago
Attachment #8534735 - Attachment is obsolete: true
(Assignee)

Comment 7

4 years ago
Created attachment 8535328 [details] [diff] [review]
(WIP) Renegotiation support

Get our renegotiation mochitest working. Needs much more testing.
(Assignee)

Updated

4 years ago
Attachment #8535073 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Created attachment 8535749 [details] [diff] [review]
(WIP) Renegotiation support

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

Updated

4 years ago
Attachment #8535328 - Attachment is obsolete: true
(Assignee)

Comment 9

4 years ago
Created attachment 8535836 [details] [diff] [review]
(WIP) Renegotiation support.

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

Updated

4 years ago
Attachment #8535749 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
Created attachment 8535867 [details] [diff] [review]
(WIP) Renegotiation support.

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

Updated

4 years ago
Attachment #8535836 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
Created attachment 8535919 [details] [diff] [review]
(WIP) Renegotiation support.

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

Updated

4 years ago
Attachment #8535867 - Attachment is obsolete: true
(Assignee)

Comment 12

4 years ago
Created attachment 8535944 [details] [diff] [review]
(WIP) Renegotiation support.

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

Updated

4 years ago
Attachment #8535919 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Created attachment 8535967 [details] [diff] [review]
(WIP) Renegotiation support.

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

Updated

4 years ago
Attachment #8535944 - Attachment is obsolete: true
Created attachment 8536736 [details] [diff] [review]
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.
(Assignee)

Comment 15

4 years ago
Created attachment 8537494 [details] [diff] [review]
(WIP) Renegotiation support

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

Updated

4 years ago
Attachment #8535967 - Attachment is obsolete: true
(Assignee)

Comment 16

4 years ago
Created attachment 8538842 [details] [diff] [review]
(WIP) Renegotiation support

Unrot
(Assignee)

Updated

4 years ago
Attachment #8537494 - Attachment is obsolete: true
(Assignee)

Comment 17

4 years ago
Created attachment 8539580 [details] [diff] [review]
(WIP) Renegotiation support

Unrot
(Assignee)

Updated

4 years ago
Attachment #8538842 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Depends on: 1108248

Updated

4 years ago
Blocks: 1115823
(Assignee)

Comment 18

3 years ago
Created attachment 8545442 [details] [diff] [review]
(WIP) Renegotiation support

Unrot.
(Assignee)

Updated

3 years ago
Attachment #8539580 - Attachment is obsolete: true
(Assignee)

Comment 19

3 years ago
Created attachment 8545509 [details] [diff] [review]
(WIP) Renegotiation support

Unrot again.
(Assignee)

Updated

3 years ago
Attachment #8545442 - Attachment is obsolete: true
(Assignee)

Comment 20

3 years ago
Created attachment 8546039 [details] [diff] [review]
(WIP) Renegotiation support

Better selection of numeric track ids
(Assignee)

Updated

3 years ago
Attachment #8545509 - Attachment is obsolete: true
(Assignee)

Comment 21

3 years ago
Created attachment 8546188 [details] [diff] [review]
(WIP) Renegotiation support

Get the new trackid assignment logic working
(Assignee)

Updated

3 years ago
Attachment #8546039 - Attachment is obsolete: true
(Assignee)

Comment 22

3 years ago
Created attachment 8546864 [details] [diff] [review]
Part 1: (WIP) Renegotiation support

Small fix to offerToReceive logic when renegotiating.
(Assignee)

Updated

3 years ago
Attachment #8546188 - Attachment is obsolete: true
(Assignee)

Comment 23

3 years ago
Created attachment 8546865 [details] [diff] [review]
Part 2: Testing for renegotiation
(Assignee)

Comment 24

3 years ago
Created attachment 8546930 [details] [diff] [review]
Part 1: (WIP) Renegotiation support

Unrot
(Assignee)

Updated

3 years ago
Attachment #8546864 - Attachment is obsolete: true
(Assignee)

Comment 25

3 years ago
Created attachment 8548280 [details] [diff] [review]
Part 1: (WIP) Renegotiation support

Move a couple of test cases to the test patch.
(Assignee)

Updated

3 years ago
Attachment #8546930 - Attachment is obsolete: true
(Assignee)

Comment 26

3 years ago
Created attachment 8548285 [details] [diff] [review]
Part 1: (WIP) Renegotiation support

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

Updated

3 years ago
Attachment #8548280 - Attachment is obsolete: true
(Assignee)

Comment 27

3 years ago
Created attachment 8548286 [details] [diff] [review]
Part 2: Testing for renegotiation

Move some tests to this patch.
(Assignee)

Updated

3 years ago
Attachment #8546865 - Attachment is obsolete: true
(Assignee)

Comment 28

3 years ago
Created attachment 8548431 [details] [diff] [review]
Part 2: Testing for renegotiation

Get rid of some log spew.
(Assignee)

Updated

3 years ago
Attachment #8548286 - Attachment is obsolete: true
(Assignee)

Comment 29

3 years ago
Created attachment 8549254 [details] [diff] [review]
Part 1: (WIP) Renegotiation support

Some cleanup/simplification
(Assignee)

Updated

3 years ago
Attachment #8548285 - Attachment is obsolete: true
(Assignee)

Comment 30

3 years ago
Created attachment 8549852 [details] [diff] [review]
Part 1: (WIP) Renegotiation support

Simplify and fix more corner cases.
(Assignee)

Updated

3 years ago
Attachment #8549254 - Attachment is obsolete: true
(Assignee)

Comment 31

3 years ago
Created attachment 8549953 [details] [diff] [review]
Part 1: (WIP) Renegotiation support

More simplification/cleanup
(Assignee)

Updated

3 years ago
Attachment #8549852 - Attachment is obsolete: true
(Assignee)

Comment 32

3 years ago
Created attachment 8556698 [details] [diff] [review]
Part 1: (WIP) Renegotiation support

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

Updated

3 years ago
Attachment #8549953 - Attachment is obsolete: true
(Assignee)

Comment 33

3 years ago
Created attachment 8556700 [details] [diff] [review]
Part 2: Testing for renegotiation

Unrot.
(Assignee)

Updated

3 years ago
Attachment #8548431 - Attachment is obsolete: true
(Assignee)

Comment 34

3 years ago
Created attachment 8557195 [details] [diff] [review]
Part 1: (WIP) Renegotiation support

Refactor to avoid nr_ice_ctx being initted more than once.
(Assignee)

Updated

3 years ago
Attachment #8556698 - Attachment is obsolete: true
(Assignee)

Comment 35

3 years ago
Created attachment 8557196 [details] [diff] [review]
Part 2: Testing for renegotiation

More tests.
(Assignee)

Updated

3 years ago
Attachment #8556700 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 840728
(Assignee)

Comment 36

3 years ago
Created attachment 8557418 [details] [diff] [review]
Part 1: (WIP) Renegotiation support

Add some sanity checks for renegotiation.
(Assignee)

Updated

3 years ago
Attachment #8557195 - Attachment is obsolete: true
(Assignee)

Comment 37

3 years ago
Created attachment 8557419 [details] [diff] [review]
Part 2: Testing for renegotiation

Lots more tests.
(Assignee)

Updated

3 years ago
Attachment #8557196 - Attachment is obsolete: true
(Assignee)

Comment 38

3 years ago
Created attachment 8558105 [details] [diff] [review]
Part 2: Testing for renegotiation

Another test-case
(Assignee)

Updated

3 years ago
Attachment #8557419 - Attachment is obsolete: true
(Assignee)

Comment 39

3 years ago
Created attachment 8558233 [details] [diff] [review]
Part 1: (WIP) Renegotiation support

Unrot
(Assignee)

Updated

3 years ago
Attachment #8557418 - Attachment is obsolete: true
(Assignee)

Comment 40

3 years ago
Created attachment 8558236 [details] [diff] [review]
Part 2: Testing for renegotiation

Unrot
(Assignee)

Updated

3 years ago
Attachment #8558105 - Attachment is obsolete: true
(Assignee)

Comment 41

3 years ago
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?
Attachment #8536736 - Attachment is obsolete: true
Flags: needinfo?(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...
Attachment #8536736 - Attachment is obsolete: false
Flags: needinfo?(drno)
(Assignee)

Comment 43

3 years ago
Created attachment 8558318 [details] [diff] [review]
Part 1: (WIP) Renegotiation support
(Assignee)

Updated

3 years ago
Attachment #8558233 - Attachment is obsolete: true
(Assignee)

Comment 44

3 years ago
Created attachment 8558603 [details] [diff] [review]
Part 2: Testing for renegotiation

Disable new mochitests on b2g emulator
(Assignee)

Updated

3 years ago
Attachment #8558236 - Attachment is obsolete: true
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.
Attachment #8536736 - Attachment is obsolete: true
Created attachment 8558762 [details] [diff] [review]
Part 3: add ICE checks to new test cases
(Assignee)

Comment 47

3 years ago
Created attachment 8558850 [details] [diff] [review]
Part 1: Renegotiation support
(Assignee)

Updated

3 years ago
Attachment #8558318 - Attachment is obsolete: true
(Assignee)

Comment 48

3 years ago
Created attachment 8558853 [details] [diff] [review]
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.
(Assignee)

Updated

3 years ago
Attachment #8558603 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 1130185
(Assignee)

Comment 49

3 years ago
Created attachment 8560185 [details] [diff] [review]
Part 1: Renegotiation support

Fix some bugs uncovered by testing.
(Assignee)

Updated

3 years ago
Attachment #8558850 - Attachment is obsolete: true
(Assignee)

Comment 50

3 years ago
Created attachment 8560186 [details] [diff] [review]
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.
(Assignee)

Updated

3 years ago
Attachment #8558853 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Depends on: 1130534
(Assignee)

Comment 51

3 years ago
Created attachment 8560712 [details] [diff] [review]
Part 2: Testing for renegotiation

Refactor and simplify the renegotiation mochitests. Add tests for datachannel renegotiation.
(Assignee)

Updated

3 years ago
Attachment #8560186 - Attachment is obsolete: true

Updated

3 years ago
Blocks: 1089798
(Assignee)

Comment 52

3 years ago
Created attachment 8561479 [details] [diff] [review]
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.
(Assignee)

Updated

3 years ago
Attachment #8560712 - Attachment is obsolete: true
(Assignee)

Comment 53

3 years ago
Created attachment 8561700 [details] [diff] [review]
Part 2: Testing for renegotiation

Get the basicDataOnly test working again.
(Assignee)

Updated

3 years ago
Attachment #8561479 - Attachment is obsolete: true
(Assignee)

Comment 54

3 years ago
Created attachment 8562211 [details] [diff] [review]
Part 2: Testing for renegotiation

Remove an invalid test.
(Assignee)

Updated

3 years ago
Attachment #8561700 - Attachment is obsolete: true
(Assignee)

Comment 55

3 years ago
Created 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 0003bb32034132b0e49136718217252bad3e8798
(Assignee)

Comment 56

3 years ago
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.
Attachment #8562213 - Flags: review?(martin.thomson)
Attachment #8562213 - Flags: review?(drno)
(Assignee)

Comment 57

3 years ago
Created attachment 8562224 [details] [diff] [review]
Part 2: Testing for renegotiation

Remove unused function
(Assignee)

Updated

3 years ago
Attachment #8562211 - Attachment is obsolete: true
(Assignee)

Comment 58

3 years ago
Created attachment 8562226 [details] [diff] [review]
Part 1: Renegotiation support

Unrot
(Assignee)

Updated

3 years ago
Attachment #8560185 - Attachment is obsolete: true
(Assignee)

Comment 59

3 years ago
Created attachment 8562228 [details] [diff] [review]
Part 2: Testing for renegotiation

Fix compile
(Assignee)

Updated

3 years ago
Attachment #8562224 - Attachment is obsolete: true
(Assignee)

Comment 60

3 years ago
https://reviewboard.mozilla.org/r/3655/#review2849

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

This is unused, will remove.
(Assignee)

Comment 61

3 years ago
Created attachment 8562318 [details] [diff] [review]
Part 2: Testing for renegotiation

Restore a fix.
(Assignee)

Updated

3 years ago
Attachment #8562228 - Attachment is obsolete: true

Updated

3 years ago
Attachment #8558762 - Attachment is obsolete: true
(Assignee)

Comment 62

3 years ago
Created attachment 8562422 [details] [diff] [review]
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.
(Assignee)

Updated

3 years ago
Attachment #8562318 - Attachment is obsolete: true
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
Attachment #8562213 - Flags: review?(martin.thomson)
(Assignee)

Comment 64

3 years ago
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.
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.
(Assignee)

Comment 66

3 years ago
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
Attachment #8562213 - Flags: review?(drno)
(Assignee)

Comment 67

3 years ago
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.
Attachment #8562213 - Flags: review?(martin.thomson)
Attachment #8562213 - Flags: review?(drno)
(Assignee)

Comment 68

3 years ago
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.
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.
(Assignee)

Comment 70

3 years ago
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.
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.
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?
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
(Assignee)

Comment 74

3 years ago
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.
(Assignee)

Comment 75

3 years ago
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.
(Assignee)

Comment 76

3 years ago
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.
(Assignee)

Comment 77

3 years ago
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.
(Assignee)

Comment 78

3 years ago
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
Attachment #8562213 - Flags: review?(martin.thomson)
Attachment #8562213 - Flags: review?(drno)
(Assignee)

Comment 79

3 years ago
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
(Assignee)

Comment 80

3 years ago
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
(Assignee)

Updated

3 years ago
Attachment #8562213 - Flags: review?(martin.thomson)
Attachment #8562213 - Flags: review?(drno)
(Assignee)

Updated

3 years ago
Attachment #8562226 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8562422 - Attachment is obsolete: true
(Assignee)

Comment 81

3 years ago
Have incorporated feedback, and rebased onto latest m-c.
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.
Blocks: 1129263
(Assignee)

Comment 84

3 years ago
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).
(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 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
Attachment #8562213 - Flags: review?(martin.thomson) → review+
(Assignee)

Comment 87

3 years ago
(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.

Updated

3 years ago
Attachment #8562213 - Flags: review?(drno) → review+
(Assignee)

Comment 90

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

DOM review needed for dom/webidl/PeerConnectionObserver.webidl
Attachment #8562213 - Flags: review?(bugs)

Updated

3 years ago
Attachment #8562213 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/e888d63dbd33
https://hg.mozilla.org/mozilla-central/rev/cf5ec9f850cd
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla35 → mozilla38
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1115823
Depends on: 1134384
Added to release notes for 38 Aurora/DevEd as:

WebRTC now has multistream and renegotiation support
relnote-firefox: --- → 38+
Duplicate of this bug: 857115
(Assignee)

Comment 96

3 years ago
Comment on attachment 8562213 [details]
MozReview Request: bz://1017888/bwc
Attachment #8562213 - Attachment is obsolete: true
Attachment #8618165 - Flags: review+
Attachment #8618166 - Flags: review+
(Assignee)

Comment 97

3 years ago
Created attachment 8618165 [details]
MozReview Request: Bug 1017888 - Part 1: Renegotiation support.
(Assignee)

Comment 98

3 years ago
Created attachment 8618166 [details]
MozReview Request: Bug 1017888 - Part 2: Testing for renegotiation.
Keywords: dev-doc-needed
Blocks: 1050930
You need to log in before you can comment on or make changes to this bug.