Closed Bug 1290948 Opened 8 years ago Closed 7 years ago

Implement RTCRtpTransceiver and pc.addTransceiver

Categories

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

Product:
Core
Component:
WebRTC: Signaling
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: jib, Assigned: bwc)

References

(Blocks 3 open bugs,
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(12 files, 2 obsolete files)

Bug 1290948 - Part 1: Mochitest for transceivers. r+jib
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1290948 - Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
jib
: review+
Details
Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1290948 - Part 4: Transceivers JSEP/SDP work. r+drno
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1290948 - Part 5: TransceiverImpl and some major refactoring. r+drno
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1290948 - Part 6: Remove some unused code. r+drno
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1290948 - Part 7: Update existing mochitests to track the spec fixes in previous parts. r+drno
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1290948 - Part 8: Don't cause ICE to fail if there's no streams to establish. r+drno
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1290948 - Part 9: Add RTCRtpTransceiver to test_interfaces.js r+smaug
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1290948 - Part 10: Stop expecting failure on lots of webrtc web-platform-tests.
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1290948 - Part 11: Fix a nullptr crash that web-platform-tests is hitting.
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1290948 - Part 12: Disable most of the webrtc web-platform-tests until bug 1418367 is fixed. r+jib
59 bytes, text/x-review-board-request
jib
: review+
Details 
Implement this new layer of the spec. [1]

Note: The arrival of transceivers in the spec affects the behavior of our already implemented pc.removeTrack, which in the spec was subsequently changed [2] to no longer remove the sender from getSenders().

[1] https://w3c.github.io/webrtc-pc/#rtcrtptransceiver-interface

[2] https://github.com/w3c/webrtc-pc/commit/67f7c4108c40434fc4e9bfbf77b7d909a39b4db3#diff-25266486aad3fa9244c53420694e037eL2519
backlog: --- → webrtc/webaudio+
Rank: 28
Component: WebRTC → WebRTC: Signaling
Priority: -- → P2
I'm going to treat this as a separate task from finishing up the sender and receiver objects.  Specifically, have this NOT block on work like Bug 1307996 and Bug 1307994. We can always add a meta bug.
Assignee: nobody → docfaraday
See Also: → 1355486
See Also: → 1285009
Comment on attachment 8855978 [details]
Bug 1290948: WIP Transceivers

https://reviewboard.mozilla.org/r/127840/#review152864

Drive-by review: I added a few nits generated by clang-tidy.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:384
(Diff revision 14)
> +    return nullptr;
> +  }
> +
> +  JsepTransceiver*
> +  GetNegotiatedTransceiver(JsepSession& side, size_t index) {
> +    for (size_t i = 0; i < side.GetTransceivers().size(); ++i) {

Nit: You could use range-based for loop instead (and maybe drive-by fix `auto e = parser.GetParseErrors().begin()` at line 1152 and `auto i = mSessionAns.Codecs().begin()` at line 3450 as well) [clang-tidy: modernize-loop-convert]

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:3007
(Diff revision 14)
>    SetLocalAnswer(answer, CHECK_SUCCESS);
>    SetRemoteAnswer(answer, CHECK_SUCCESS);
>  
> -  std::vector<JsepTrackPair> trackPairs(mSessionOff.GetNegotiatedTrackPairs());
> -  ASSERT_EQ(2U, trackPairs.size());
> -  for (auto pair : trackPairs) {
> +  std::vector<JsepTransceiver> transceivers(mSessionOff.GetTransceivers());
> +  ASSERT_EQ(3U, transceivers.size());
> +  for (auto transceiver : transceivers) {

Note: loop variable is copied but only used as const reference; consider making it a const reference [clang-tidy: performance-for-range-copy]

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:656
(Diff revision 14)
> +   public:
> +    Message(MediaStream* aStream, nsAutoPtr<MediaSegment>&& aSegment)
> +      : ControlMessage(aStream),
> +        segment_(aSegment) {}
> +
> +    virtual void Run() override {

Nit: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
See Also: → 1373144
Depends on: 1208378
Comment on attachment 8855978 [details]
Bug 1290948: WIP Transceivers

https://reviewboard.mozilla.org/r/127840/#review165830

Publishing what I managed to review before my PTO

::: dom/media/PeerConnection.js:828
(Diff revision 23)
> +  // This ensures there are at least |count| |kind| transceivers that are
> +  // configured to receive. It will create transceivers if necessary.
> +  _applyOfferToReceive(kind, count) {
> +    this._transceivers.forEach(transceiver => {
> +      if (count && transceiver.getKind() == kind && !transceiver.stopped) {
> +        this._enableReceive(transceiver);

Only count-- if _enableReceive() returned true?

::: dom/media/PeerConnection.js:1168
(Diff revision 23)
> +    });
> +
> +    if (transceiver) {
> +      transceiver.sender.setTrack(track);
> +      transceiver.sender.streams = [stream];
> +      this._enableSend(transceiver);

Check return value?
Comment on attachment 8855978 [details]
Bug 1290948: WIP Transceivers

https://reviewboard.mozilla.org/r/127840/#review165830

> Only count-- if _enableReceive() returned true?

Nope; in our present code, offerToReceive ensures that at least that many m-sections are receiving for the type. So, calling |_enableReceive| here without checking the return is effectively an ensureReceive.

> Check return value?

Same answer here as above.
At present, there are many tests in https://github.com/w3c/web-platform-tests/tree/master/webrtc that rely on addTransceiver(...) as well as tests for the method itself: https://github.com/w3c/web-platform-tests/blob/master/webrtc/RTCPeerConnection-addTransceiver.html 

While you're implementing, any feedback on these will be greatly appreciated. Look forward to working together
Attachment #8855978 - Attachment is obsolete: true
(In reply to Rick Waldron [:rwaldron] from comment #37)
> At present, there are many tests in
> https://github.com/w3c/web-platform-tests/tree/master/webrtc that rely on
> addTransceiver(...) as well as tests for the method itself:
> https://github.com/w3c/web-platform-tests/blob/master/webrtc/
> RTCPeerConnection-addTransceiver.html 
> 
> While you're implementing, any feedback on these will be greatly
> appreciated. Look forward to working together

I'm beginning to look at these. I seem to have broken a single test-case, trying to figure out what happened. More feedback to come.
(In reply to Byron Campen [:bwc] from comment #56)
> I seem to have broken a single test-case,

Let me help if I can! Which test case is breaking for you?
(In reply to Rick Waldron [:rwaldron] from comment #57)
> (In reply to Byron Campen [:bwc] from comment #56)
> > I seem to have broken a single test-case,
> 
> Let me help if I can! Which test case is breaking for you?

Not sure why there is a new failure in this test, but it was pretty busted to begin with.

https://github.com/w3c/web-platform-tests/issues/7054
Attachment #8900490 - Flags: review?(jib)
Attachment #8900491 - Flags: review?(jib)
Attachment #8900491 - Flags: review?(ehsan)
Attachment #8900492 - Flags: review?(jib)
Attachment #8900493 - Flags: review?(drno)
Attachment #8900494 - Flags: review?(drno)
Attachment #8900495 - Flags: review?(drno)
Attachment #8900496 - Flags: review?(drno)
Comment on attachment 8900491 [details]
Bug 1290948 - Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan

https://reviewboard.mozilla.org/r/171862/#review180646

::: commit-message-4029f:1
(Diff revision 4)
> +Bug 1290948 - Part 2: webidl updates

Please include a proper commit message that describes the change.  It's impossible to understand from this commit message for example what these updates are for, which specs I should be consulting, etc.

::: dom/webidl/RTCPeerConnection.webidl:116
(Diff revision 4)
>                          MediaStream stream,
>                          MediaStream... moreStreams);
>    void removeTrack(RTCRtpSender sender);
>  
> +  // Gets the track id that was in the SDP for this track
> +  DOMString mozGetWebrtcTrackId(MediaStreamTrack track);

This interface is exposed to the Web, right?  Our API exposure guidelines prohibit exposure of new APIs with vendor prefixes.  If this is an experimental API, please consider exposing it without a prefix behind a pref.

::: dom/webidl/RTCRtpSender.webidl:79
(Diff revision 4)
>    RTCRtpParameters getParameters();
> -  Promise<void> replaceTrack(MediaStreamTrack track);
> +  Promise<void> replaceTrack(MediaStreamTrack? track);
>    Promise<RTCStatsReport> getStats();
>    [Pref="media.peerconnection.dtmf.enabled"]
>    readonly attribute RTCDTMFSender? dtmf;
> +  sequence<MediaStream> mozGetStreams();

Similar comment to the above.

::: dom/webidl/RTCRtpTransceiver.webidl:19
(Diff revision 4)
> +    "inactive"
> +};
> +
> +dictionary RTCRtpTransceiverInit {
> +    RTCRtpTransceiverDirection         direction = "sendrecv";
> +    sequence<MediaStream>              streams;

In the spec, this has a default value of [].

::: dom/webidl/RTCRtpTransceiver.webidl:20
(Diff revision 4)
> +};
> +
> +dictionary RTCRtpTransceiverInit {
> +    RTCRtpTransceiverDirection         direction = "sendrecv";
> +    sequence<MediaStream>              streams;
> +    // sequence<RTCRtpEncodingParameters> sendEncodings;

Please file a follow-up bug for enabling this and add a comment pointing to that bug here.

::: dom/webidl/RTCRtpTransceiver.webidl:34
(Diff revision 4)
> +    [SameObject]
> +    readonly attribute RTCRtpReceiver              receiver;
> +    readonly attribute boolean                     stopped;
> +    readonly attribute RTCRtpTransceiverDirection  direction;
> +    readonly attribute RTCRtpTransceiverDirection? currentDirection;
> +    readonly attribute DOMString?                  remoteTrackId;

I don't see this attribute in http://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver...

::: dom/webidl/RTCRtpTransceiver.webidl:38
(Diff revision 4)
> +    readonly attribute RTCRtpTransceiverDirection? currentDirection;
> +    readonly attribute DOMString?                  remoteTrackId;
> +
> +    void setDirection(RTCRtpTransceiverDirection direction);
> +    void stop();
> +    // void setCodecPreferences(sequence<RTCRtpCodecCapability> codecs);

Please file a follow-up bug for enabling this and add a comment pointing to that bug here.

::: dom/webidl/TransceiverImpl.webidl:16
(Diff revision 4)
> + *
> + * See media/webrtc/signaling/src/peerconnection/TransceiverImpl.h
> + *
> + */
> +
> +interface nsISupports;

Why this?
Attachment #8900491 - Flags: review?(ehsan) → review-
Comment on attachment 8900491 [details]
Bug 1290948 - Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan

https://reviewboard.mozilla.org/r/171862/#review180640

Looks good, except for a couple of non-spec moz-methods. We should not add any of those without a really really good reason, and I don't see one.

::: dom/webidl/RTCPeerConnection.webidl:115
(Diff revision 4)
> +  // Gets the track id that was in the SDP for this track
> +  DOMString mozGetWebrtcTrackId(MediaStreamTrack track);

We shouldn't ship non-spec moz-APIs. Is

    pc.getTransceivers().find(tc => tc.receiver.track == track).remoteTrackId

not sufficient?

::: dom/webidl/RTCRtpSender.webidl:75
(Diff revision 4)
>   JSImplementation="@mozilla.org/dom/rtpsender;1"]
>  interface RTCRtpSender {
> -  readonly attribute MediaStreamTrack track;
> +  readonly attribute MediaStreamTrack? track;
>    Promise<void> setParameters (optional RTCRtpParameters parameters);
>    RTCRtpParameters getParameters();
> -  Promise<void> replaceTrack(MediaStreamTrack track);
> +  Promise<void> replaceTrack(MediaStreamTrack? track);

Nit: Might as well s/track/withTrack/ to match spec verbatum.

::: dom/webidl/RTCRtpSender.webidl:79
(Diff revision 4)
>    RTCRtpParameters getParameters();
> -  Promise<void> replaceTrack(MediaStreamTrack track);
> +  Promise<void> replaceTrack(MediaStreamTrack? track);
>    Promise<RTCStatsReport> getStats();
>    [Pref="media.peerconnection.dtmf.enabled"]
>    readonly attribute RTCDTMFSender? dtmf;
> +  sequence<MediaStream> mozGetStreams();

We should not add non-spec moz-APIs. Why do we need this?

Can we make it [ChromeOnly]?

::: dom/webidl/RTCRtpTransceiver.webidl:19
(Diff revision 4)
> +    "inactive"
> +};
> +
> +dictionary RTCRtpTransceiverInit {
> +    RTCRtpTransceiverDirection         direction = "sendrecv";
> +    sequence<MediaStream>              streams;

streams = [];
Attachment #8900491 - Flags: review?(jib) → review-
Comment on attachment 8900491 [details]
Bug 1290948 - Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan

https://reviewboard.mozilla.org/r/171862/#review181398

::: dom/webidl/RTCRtpTransceiver.webidl:34
(Diff revision 4)
> +    [SameObject]
> +    readonly attribute RTCRtpReceiver              receiver;
> +    readonly attribute boolean                     stopped;
> +    readonly attribute RTCRtpTransceiverDirection  direction;
> +    readonly attribute RTCRtpTransceiverDirection? currentDirection;
> +    readonly attribute DOMString?                  remoteTrackId;

This is in the github repo, the editor's draft has not been generated since then.

https://github.com/w3c/webrtc-pc/pull/1567
Comment on attachment 8900491 [details]
Bug 1290948 - Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan

https://reviewboard.mozilla.org/r/171862/#review180646

> I don't see this attribute in http://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver...

This change was merged here, the draft just hasn't been updated yet:

https://github.com/w3c/webrtc-pc/pull/1567
Comment on attachment 8900491 [details]
Bug 1290948 - Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan

https://reviewboard.mozilla.org/r/171862/#review181830

Thanks a lot Byron for addressing the review comments.

Since the changes in this patch introduce Web-facing API changes that aren't hidden behind prefs, landing the patches effectively ships new APIs.  Therefore, as per our policy for exposing new Web-facing APIs <https://wiki.mozilla.org/WebAPI/ExposureGuidelines>, before landing the patches please send out an intent to implement and ship email to dev-platform.  r=me on the WebIDL changes with that.
Attachment #8900491 - Flags: review?(ehsan) → review+
Ok, mail sent.
Comment on attachment 8900491 [details]
Bug 1290948 - Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan

https://reviewboard.mozilla.org/r/171862/#review182050
Attachment #8900491 - Flags: review?(jib) → review+
Comment on attachment 8900490 [details]
Bug 1290948 - Part 1: Mochitest for transceivers. r+jib

https://reviewboard.mozilla.org/r/171860/#review182052

Awesome tests! Mostly lots of nits (there are almost a thousand lines of code here after all!) to preserve JS style consistency in mochitests. r- for a couple of differences from spec, some questions on behavior, and I'd like to see it again.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:14
(Diff revision 5)
> +  createHTML({
> +    bug: "1290948",
> +    title: "Transceivers API tests"
> +  });
> +
> +  var logExpected = (expected) => {

Nit: redundant parens around single arg.

We can also use template strings now instead of string math:

    let logExpected = expected => info(`(expected ${JSON.stringify(expected)})`);

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:18
(Diff revision 5)
> +
> +  var logExpected = (expected) => {
> +    info("(expected " + JSON.stringify(expected) + ")");
> +  };
> +
> +  var dictCompare = (observed, expected) => {

Maybe add a comment that this isn't a plain AB compare function (like e.g. [1]) but rather returns true if all props in expected are in observed (i.e. may return true when observed is a superset of expected)? This threw me initially.

I don't know of a better name. dictHas?

[1] https://dxr.mozilla.org/mozilla-central/rev/c959327c6b75cd4930a6ea087583c38b805e7524/dom/media/tests/mochitest/test_peerConnection_bug825703.html#35-39

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:67
(Diff revision 5)
> +  var checkAddTransceiverNoTrack = async function(options) {
> +    var pc = new RTCPeerConnection();

s/var/let/ throughout the entire file. We should avoid var.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:67
(Diff revision 5)
> +    ok(false, "Expected (" + JSON.stringify(expected) + ") did not match " +
> +              "observed (" + JSON.stringify(observed) + ")");
> +    return false;
> +  };
> +
> +  var checkAddTransceiverNoTrack = async function(options) {

`options` appears unused throughout. Maybe remove all traces of it?

Applies throughout.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:75
(Diff revision 5)
> +      [
> +        {
> +          receiver: {track: {kind: "audio"}},
> +          sender: {track: null},
> +          direction: "sendrecv",
> +          mid: null,
> +          currentDirection: null,
> +          stopped: false
> +        },
> +        {
> +          receiver: {track: {kind: "video"}},
> +          sender: {track: null},
> +          direction: "sendrecv",
> +          mid: null,
> +          currentDirection: null,
> +          stopped: false
> +        }
> +      ]);

This test and the ones that follow appear to rely on Firefox-specific behavior here, since the spec [1] says:

  "The conversion from the transceiver set to the sequence is user agent defined and the order does not have to be stable between calls."

This may matter if we want to promote any of these to web-platform tests in the future. We should add a comment if we don't change them to be order-agnostic.

Side-note: I'm a bit worried about the spec here actually, since the approach in this test seems like a reasonable way to deal with transceivers, but unfortunately is not web compatible! How are apps expected to keep track of and tell apart implicitly created transceivers? mid I suppose, but mids sometimes disappear!

[1] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dom-rtcpeerconnection-gettransceivers

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:97
(Diff revision 5)
> +      ]);
> +
> +    pc.close();
> +  };
> +
> +  var checkAddTransceiverWithTrack = async function(options) {

I tend to see these permutations in the tree:

    let checkAddTransceiverWithTrack = async options => {

or

    async function checkAddTransceiverWithTrack(options) {

Any reason not to pick one of those?

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:99
(Diff revision 5)
> +    pc.close();
> +  };
> +
> +  var checkAddTransceiverWithTrack = async function(options) {
> +    var pc = new RTCPeerConnection();
> +    dictCompare(pc.getTransceivers(), []);

We aready tested this in checkAddTransceiverNoTrack(), so we don't need it in every test. Applies throughout.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:128
(Diff revision 5)
> +          currentDirection: null,
> +          stopped: false
> +        }
> +      ]);
> +
> +    pc.close();

For the same reason we do pc.close() we should do:

    stream.getTracks().forEach(track => track.stop);

Applies throughout.

To avoid repetition, we could use a callback pattern to reduce repetition and avoid forgetting cleanup. I something similar in testVideoCall(callback) here FWIW:
https://reviewboard.mozilla.org/r/173324/diff/5#index_header
Just an idea. FYI. E.g. The callback could receive ({pc1, pc2, stream}) => {} // and ignore pc2 if unneeded.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:240
(Diff revision 5)
> +      pc.createOffer({offerToReceiveAudio: true});
> +    } else {
> +      pc.createOffer({offerToReceiveVideo: true});

Maybe a third time with both?

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:268
(Diff revision 5)
> +    var offer = await pc1.createOffer();
> +    await pc2.setRemoteDescription(offer);

Nit: offer is unused below. Why not:

    await pc2.setRemoteDescription(await pc1.createOffer());

? Applies throughout, in places.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:271
(Diff revision 5)
> +    dictCompare(pc2.getTransceivers(), 
> +      [

Nit: trailing space.

Looks like you were deciding between trailing bracket or next-line bracket. I'm kinda partial to trailing bracket:

    dictCompare(pc2.getTransceivers(), [
      {
        ...
      }
    ]);

because then the closing bracket lines up with the start of the function, and we indent less. This is also consistent with function args, e.g.

    runNetworkTest(async function (options) {
      ...
    });

Applies throughout.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:276
(Diff revision 5)
> +          // rtcweb-jsep says this is recvonly, w3c-webrtc does not...
> +          direction: "recvonly",

Can you file a spec issue for clarification if you haven't already, and insert a link in a comment?

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:278
(Diff revision 5)
> +        {
> +          receiver: {track: {kind: "audio"}},
> +          sender: {track: null},
> +          // rtcweb-jsep says this is recvonly, w3c-webrtc does not...
> +          direction: "recvonly",
> +          mid: "sdparta_0",

This is Firefox specific, right? Maybe a comment.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:300
(Diff revision 5)
> +    pc1.getTransceivers()[0].setDirection("recvonly");
> +
> +    var offer = await pc1.createOffer();
> +    await pc2.setRemoteDescription(offer);
> +
> +    dictCompare(pc2.getTransceivers(), 

trailing space

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:366
(Diff revision 5)
> +
> +    var offer = await pc1.createOffer();
> +    await pc2.setRemoteDescription(offer);
> +    dictCompare(pc2.getTransceivers(),
> +      [
> +        {mid: null, sender: {track: track}},

Tip: we can use object literal shorthand property names here (helps test ES2015):

    {mid: null, sender: {track}},

Applies a couple of places.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:462
(Diff revision 5)
> +    pc.getTransceivers()[0].setDirection("sendonly");
> +    dictCompare(pc.getTransceivers(),[{direction: "sendonly"}]);
> +    pc.getTransceivers()[0].setDirection("recvonly");
> +    dictCompare(pc.getTransceivers(),[{direction: "recvonly"}]);
> +    pc.getTransceivers()[0].setDirection("inactive");
> +    dictCompare(pc.getTransceivers(),[{direction: "inactive"}]);
> +    pc.getTransceivers()[0].setDirection("sendrecv");
> +    dictCompare(pc.getTransceivers(),[{direction: "sendrecv"}]);

Seems a bit verbose. How about:

    ["sendonly", "recvonly", "inactive", "sendrecv"].forEach(direction => {
      pc.getTransceivers()[0].setDirection(direction);
      dictCompare(pc.getTransceivers()[0], {direction});
    });

?

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:487
(Diff revision 5)
> +    await pc1.setLocalDescription(offer);
> +    // Debatable, spec is kinda vague
> +    dictCompare(pc1.getTransceivers(), [{currentDirection: "inactive"}]);
> +
> +    await pc2.setRemoteDescription(offer);
> +    // Debatable, spec is kinda vague
> +    dictCompare(pc2.getTransceivers(), [{currentDirection: "inactive"}]);
> +
> +    var answer = await pc2.createAnswer();
> +    dictCompare(pc2.getTransceivers(), [{currentDirection: "inactive"}]);

Can you link to support for your reasoning here?

I would expect all three of these to be null, since AFAICT [[CurrentDirection]] is only set when setting (pr)answers. [1]

[1] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#set-description

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:510
(Diff revision 5)
> +    // Debatable, spec is kinda vague
> +    dictCompare(pc2.getTransceivers(), [{currentDirection: "sendrecv"}]);
> +
> +    await pc1.setRemoteDescription(offer);
> +    // Debatable, spec is kinda vague
> +    dictCompare(pc1.getTransceivers(), [{currentDirection: "sendrecv"}]);

These seem right to me (remove comments).

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:523
(Diff revision 5)
> +    await pc2.setRemoteDescription(answer);
> +    dictCompare(pc2.getTransceivers(), [{currentDirection: "sendonly"}]);

Have you implemented firing of the removetrack event on the stream, and the muted event on the removed track? [1][3]

If so, we could test that they're fired here.

If not, we should do so, or file a bug to do so, as we'd want to do that in the same release, to follow the spec, and to give people a way to transition.

Lastly, do we want to complete the circle with one more round here, setting direction back with either:

    pc2.getTransceivers()[0].setDirection("sendrecv");

or maybe:

    pc1.getTransceivers()[0].setDirection("sendrecv");

and renegotiate again? (That would let us test for the addtrack event as well.)

[1] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#process-remote-track-removal
[2] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#process-remote-track-addition

[2] I guess I'm only asking about the add/removetrack events until Bug 1208378 comment 1 is implemented.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:542
(Diff revision 5)
> +    stream = await getUserMedia({audio: true});
> +    track = stream.getAudioTracks()[0];
> +    pc2.addTrack(track, stream);

Why not reuse the existing gum stream and track?

If not, maybe stream2 and track2 so we don't forget to stop them all at the end? We could also use stream.clone() here for brievity if we want (but we still should stop all at the end, to avoid non-deterministic gum bleed).

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:560
(Diff revision 5)
> +          sender: {track: {kind: "audio"}},
> +          receiver: {track: {kind: "audio"}},
> +          stopped: true,
> +          mid: "sdparta_0",
> +          currentDirection: null,
> +          direction: "sendrecv" // Debatable?

Seems right to me (remove comment), since [[Direction]] is "preferred direction", non-nullable, and only set from addTrack(), removeTrack() and setDirection().

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:573
(Diff revision 5)
> +          stopped: true,
> +          mid: null,
> +          currentDirection: null,
> +          direction: "sendrecv"
> +        }
> +      ]);

I find 7 hits in the spec for "If transceiver's [[Stopped]] slot is true," which we could test for here (8 with https://github.com/w3c/webrtc-pc/issues/1437).

I've also filed https://github.com/w3c/webrtc-pc/issues/1575 for clarification on whether this should end peer connection tracks.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:598
(Diff revision 5)
> +
> +    dictCompare(pc1.getTransceivers(),
> +      [
> +        {
> +          receiver: {track: {kind: "audio"}},
> +          sender: {track: track},

{track}

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:608
(Diff revision 5)
> +    stream = await getUserMedia({audio: true});
> +    track = stream.getAudioTracks()[0];

Maybe use stream2/track2 to make sure both gUM tracks are stopped at the end of this test?

(Might make the test a bit easier to follow as well, since it seems the point is to not trounce {track: track2})

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:612
(Diff revision 5)
> +    pc1.getTransceivers()[0].setDirection("sendonly");
> +    stream = await getUserMedia({audio: true});
> +    track = stream.getAudioTracks()[0];
> +    await pc1.getTransceivers()[0].sender.replaceTrack(track);
> +
> +    await pc1.setLocalDescription({sdp: "", type: "rollback"});

The sdp arg seems redundant here. Applies throughout.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:643
(Diff revision 5)
> +    // Transceiver should be _gone_
> +    dictCompare(pc2.getTransceivers(), []);

It's gone from the array, but if we held a reference to it, according to jsep [1], we should be able to observe that:

 - transceiver.stopped is true,
 - it's mid is null, and
 - depending on the outcome of this [2], its receiver.track has ended.

We probably want to test that.

[1] https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-20#section-4.1.8.2

[2] https://github.com/w3c/webrtc-pc/issues/1575

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:659
(Diff revision 5)
> +    stream = await getUserMedia({audio: true});
> +    track = stream.getAudioTracks()[0];

Maybe reuse the same stream and track, so we can stop them properly at the end of this test?

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:739
(Diff revision 5)
> +    await pc1.setLocalDescription({sdp: "", type: "rollback"});
> +    dictCompare(pc1.getTransceivers(),
> +      [
> +        {
> +          receiver: {track: {kind: "audio"}},
> +          sender: {track: {kind: "audio"}},

If we add track2 as mentioned earlier, we can use:

    sender: {track}

here

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:754
(Diff revision 5)
> +    await pc2.setRemoteDescription(offer);
> +    dictCompare(pc2.getTransceivers(),
> +      [
> +        {
> +          receiver: {track: {kind: "audio"}},
> +          sender: {track: {kind: "audio"}},

sender: {track: track2},

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:818
(Diff revision 5)
> +    stream = await getUserMedia({audio: true});
> +    track = stream.getAudioTracks()[0];

stream2, track2

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:824
(Diff revision 5)
> +    track = stream.getAudioTracks()[0];
> +
> +    pc1.addTrack(track, stream);
> +    offer = await pc1.createOffer();
> +    // Exactly one m= line
> +    is(1, offer.sdp.match(/m=/g).length);

I believe the intended arg order here is (value, expected_value), unless I'm wrong:

    is(offer.sdp.match(/m=/g).length, 1, "exactly one m= line");

for saner log messages.

Applies throughout.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:877
(Diff revision 5)
> +    stream = await getUserMedia({audio: true});
> +    track = stream.getAudioTracks()[0];

stream3, track3

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:900
(Diff revision 5)
> +        {
> +          stopped: true
> +        },
> +        {
> +          mid: "sdparta_1",
> +          sender: {track: null},



::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:977
(Diff revision 5)
> +    await checkStop(options);
> +    await checkRollback(options);
> +    await checkMsectionReuse(options);
> +    return SimpleTest.finish();
> +  });
> +</script>

Do we plan on flipping some existing mochitests to use the transceiver API? I'm not seeing an end-to-end test with media-flow otherwise.
Attachment #8900490 - Flags: review?(jib) → review-
Comment on attachment 8900490 [details]
Bug 1290948 - Part 1: Mochitest for transceivers. r+jib

https://reviewboard.mozilla.org/r/127838/#review184196

I noticed something while reviewing the other patch.

::: dom/webidl/RTCRtpTransceiver.webidl:61
(Diff revision 42)
> +    [ChromeOnly]
> +    boolean hasBeenUsedToSend();
> +    [ChromeOnly]
> +    void sync();
> +
> +    [ChromeOnly]
> +    void insertDTMF(DOMString tones,
> +                    optional unsigned long duration = 100,
> +                    optional unsigned long interToneGap = 70);

Are these three ChromeOnly methods called from c++?

If not, they don't need to be defined, I don't think.

PeerConnection.js can call xray chrome js functions directly without any binding code AFAIK. E.g. [1]

All the others appear used from c++, but I could have missed some.

[1] http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/dom/media/PeerConnection.js#1679
Comment on attachment 8900491 [details]
Bug 1290948 - Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan

https://reviewboard.mozilla.org/r/171862/#review184198

::: dom/webidl/RTCRtpSender.webidl:81
(Diff revision 5)
> +  [ChromeOnly]
> +  void setTrack(MediaStreamTrack? track);
> +  [ChromeOnly]
> +  void checkWasCreatedByPc(RTCPeerConnection pc);

These two appear unused from c++ as well.
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
Comment on attachment 8900492 [details]
Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver.

https://reviewboard.mozilla.org/r/171864/#review184150

Fantastic! Lots of code, so lots of commentss.

::: dom/media/PeerConnection.js:150
(Diff revision 5)
>      let cleanupPcRef = function(pcref) {
>        let pc = pcref.get();
>        if (pc) {
> -        pc._pc.close();
> +        pc.close();

cleanupPcRef() is called when the window is torn down.

pc.close() fires an "iceconnectionstatechange" event at the window.

We don't want to fire events during teardown.

::: dom/media/PeerConnection.js:594
(Diff revision 5)
>      }
>    }
>  
> +  // This implements the fairly common "Queue a task" logic
> +  async _queueTaskWithClosedCheck(func) {
> +    let pc = this;

Nit: redundant temporary when we have arrow functions.

::: dom/media/PeerConnection.js:703
(Diff revision 5)
> +  _getTransceiverWithSender(sender) {
> +    let transceiver = this._transceivers.find(t => t.sender == sender);
> +    if (!transceiver) {
> +      throw new this._win.DOMException("This isn't one of my senders!",
> +                                       "InvalidAccessError");
> +    }
> +    return transceiver;
> +  }

In the spec, I believe this function corresponds to the common:

    Let transceiver be the RTCRtpTransceiver object associated with sender.

This CANNOT FAIL. As such we should just return transceiver here I think, which will an undefined dereference (which should show in browser console only). I.e. we should not throw a content-visible "InvalidAccessError".

To reduce pilot error, we could move this function to the sender, e.g.:

    _getTransceiver() {
      return this._pc._transceivers.find(t => t.sender == this);
    }

::: dom/media/PeerConnection.js:782
(Diff revision 5)
> +    // Spec language implies that this needs to happen as if it were called
> +    // before createOffer, so we do this as early as possible.
> +    this._ensureTransceiversForOfferToReceive(optionsOrOnSucc);
> +
>      // This entry-point handles both new and legacy call sig. Decipher which one
>      if (typeof optionsOrOnSucc == "function") {
>        return this._legacy(optionsOrOnSucc, onErr, () => this._createOffer(options));

I think we need to call

    _ensureTransceiversForOfferToReceive(options);

here specifically in the legacy case, otherwise this lookss like it will break legacy callback users who use offerToReceive.

::: dom/media/PeerConnection.js:793
(Diff revision 5)
> +  _enableReceive(transceiver) {
> +    if (transceiver.direction == "sendonly") {
> +      transceiver.setDirection("sendrecv");
> +      return true;
> +    } else if (transceiver.direction == "inactive") {
> +      transceiver.setDirection("recvonly");
> +      return true;
> +    }
> +    return false;
> +  }
> +
> +  _enableSend(transceiver) {
> +    if (transceiver.direction == "recvonly") {
> +      transceiver.setDirection("sendrecv");
> +      return true;
> +    } else if (transceiver.direction == "inactive") {
> +      transceiver.setDirection("sendonly");
> +      return true;
> +    }
> +    return false;
> +  }

Unused return values. Also, redundant functions called from one place each. Maybe inline? Makes it easier to compare to spec algorithms steps too imho.

::: dom/media/PeerConnection.js:815
(Diff revision 5)
> +  _disableSend(transceiver) {
> +    if (transceiver.direction == "sendrecv") {
> +      transceiver.setDirection("recvonly");
> +      return true;
> +    } else if (transceiver.direction == "sendonly") {
> +      transceiver.setDirection("inactive");
> +      return true;
> +    }
> +    return false;
> +  }

Even though return values are used here, this function also seems redundant, called from one place. Maybe inline? Makes it easier to compare to spec algorithms steps.

Flat seems better here. In particular, I dislike updateNegotiationNeeded() being called both in here and again at lone call site.

::: dom/media/PeerConnection.js:826
(Diff revision 5)
> +  // This ensures there are at least |count| |kind| transceivers that are
> +  // configured to receive. It will create transceivers if necessary.
> +  _applyOfferToReceive(kind, count) {

count is options.offerToReceiveVideo and  options.offerToReceiveVideo) which are booleans thanks to webidl, not numbers. That's all the spec says to support.

::: dom/media/PeerConnection.js:829
(Diff revision 5)
> +    this._transceivers.forEach(transceiver => {
> +      if (count && transceiver.getKind() == kind && !transceiver.stopped) {
> +        this._enableReceive(transceiver);
> +        count--;
> +      }
> +    });

If we do this as

    for (let transceiver of this._transceivers) {

then we can break out of the loop.

(Imperative for( : ) loops are also back in fashion as the dominant pattern since they work well with async/await, where forEach is a bit of a deathtrap. YMMV).

::: dom/media/PeerConnection.js:830
(Diff revision 5)
> +
> +  // This ensures there are at least |count| |kind| transceivers that are
> +  // configured to receive. It will create transceivers if necessary.
> +  _applyOfferToReceive(kind, count) {
> +    this._transceivers.forEach(transceiver => {
> +      if (count && transceiver.getKind() == kind && !transceiver.stopped) {

Nit: s/.getKind()/._kind/

Since the transceivers pushed on this._transceivers appear to be WebIDL objects, .getKind() likely calls redundant c++ binding code here.

No need to call accessor functionss which exists for binding code only, at least for getters IMHO. False dependence. Better to call the xray ._kind directly.

Applies throughout.

::: dom/media/PeerConnection.js:842
(Diff revision 5)
> +      this._addTransceiverNoEvents(kind, {direction: "recvonly"});
> +      --count;
> +    }
> +  }
> +
> +  // Handles legacy offerToReceiveAudio/Video

Nit: I find "legacy" slightly confusing here, given where this function is called from. Specifically, it's called in both the this._legacy() vs. this._async() case.

s/legacy/outmoded/ ? pre-transceivers?

::: dom/media/PeerConnection.js:1156
(Diff revision 5)
> -      if (sender.track == track) {
> -        throw new this._win.DOMException("already added.",
> +    if (this._transceivers.some(
> +          transceiver => transceiver.sender.track === track)) {

In JavaScript, == is preferred to ===

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators

PS: I find s/transceiver/tc/ a reasonable abbreviation in one-liners, if you're looking to shorten things.

::: dom/media/PeerConnection.js:1163
(Diff revision 5)
> +      return transceiver.sender.track === null &&
> +             transceiver.getKind() === track.kind &&

== is fine for objects and strings.

::: dom/media/PeerConnection.js:1166
(Diff revision 5)
> +
> +    let transceiver = this._transceivers.find(transceiver => {
> +      return transceiver.sender.track === null &&
> +             transceiver.getKind() === track.kind &&
> +             !transceiver.stopped &&
> +             !transceiver.hasBeenUsedToSend();

prefer ._hasBeenUsedToSend to avoid binding code

(abstractions in JS are not free).

::: dom/media/PeerConnection.js:1171
(Diff revision 5)
> +             !transceiver.hasBeenUsedToSend();
>      });
> -    this._impl.addTrack(track, stream);
> -    let sender = this._win.RTCRtpSender._create(this._win,
> -                                                new RTCRtpSender(this, track,
> -                                                                 stream));
> +
> +    if (transceiver) {
> +      transceiver.sender.setTrack(track);
> +      transceiver.sender.streams = [stream];

Bug: s/streams/_streams/

::: dom/media/PeerConnection.js:1174
(Diff revision 5)
> +      transceiver = this._addTransceiverNoEvents(
> +          track,
> +          {
> +            streams: [stream],
> +            direction: "sendrecv"
> +          });

Nit: Odd indent. { at eol is common in this file:

    transceiver = this._addTransceiverNoEvents(track, {
      streams: [stream],
      direction: "sendrecv"
    });

::: dom/media/PeerConnection.js:1195
(Diff revision 5)
> +    try {
> +      transceiver = this._getTransceiverWithSender(sender);
> +    } catch (e) {
> +      return;
> +    }

Instead of try/catch use:

    if (!transceiver) return;

::: dom/media/PeerConnection.js:1201
(Diff revision 5)
> +      transceiver = this._getTransceiverWithSender(sender);
> +    } catch (e) {
> +      return;
> +    }
> +
> +    // TODO(bug XXXXX): Handle in TransceiverImpl::SyncWithJS?

bug #

::: dom/media/PeerConnection.js:1202
(Diff revision 5)
> +    } catch (e) {
> +      return;
> +    }
> +
> +    // TODO(bug XXXXX): Handle in TransceiverImpl::SyncWithJS?
> +    this._impl.removeTrack(sender.track);

Missing spec step ahead of this:

 "7. If sender.track is null, then abort these steps."

::: dom/media/PeerConnection.js:1205
(Diff revision 5)
> +    if (this._disableSend(transceiver)) {
> +      transceiver.sync();
> +      this.updateNegotiationNeeded();
>      }

Spec says to call updateNegotiationNeeded unconditionally here.

::: dom/media/PeerConnection.js:1212
(Diff revision 5)
> +      this.updateNegotiationNeeded();
>      }
>    }
>  
> -  _insertDTMF(sender, tones, duration, interToneGap) {
> -    return this._impl.insertDTMF(sender.__DOM_IMPL__, tones, duration, interToneGap);
> +  _addTransceiverNoEvents(sendTrackOrKind, init) {
> +    let kind = "";

unused initialization

::: dom/media/PeerConnection.js:1256
(Diff revision 5)
> +  clearNegotiationNeeded() {
> +    this._negotiationNeeded = false;
> +  }

wrapper seems redundant

::: dom/media/PeerConnection.js:1260
(Diff revision 5)
> +  updateNegotiationNeeded() {
>      this._checkClosed();

this._checkClosed() throws. Spec says:

"1. If connection's [[IsClosed]] slot is true, abort these steps.
"

https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dfn-update-the-negotiation-needed-flag

::: dom/media/PeerConnection.js:1285
(Diff revision 5)
> -        throw new this._win.DOMException("Duplicate rid", "TypeError");
> +  _getOrCreateStream(id) {
> +    if (!this._receiveStreams.has(id)) {
> +      let stream = new this._win.MediaStream();
> +      stream.assignId(id);
> +      // Legacy event, remove eventually
> +      let ev = new this._win.MediaStreamEvent("addstream", { stream });
> +      this.dispatchEvent(ev);

Inlining this function would make it easier to reason about why it's firing events I think, and whether we should queue a task or not (we should not).

::: dom/media/PeerConnection.js:1306
(Diff revision 5)
> +  async _replaceTrack(transceiverImpl, withTrack) {
> +    this._checkClosed();
> +
> +    return new Promise((resolve, reject) => {
> +      this._onReplaceTrackSuccess = resolve;
> +      this._onReplaceTrackFailure = reject;
> +      this._impl.replaceTrackNoRenegotiation(transceiverImpl, withTrack);
> +    });
>    }

This will hang if someone calls:

    let p = pc.replaceTrack(track);
    pc.replaceTrack(track);
    await p; // hang

...because the this.members storing the resolve and reject callbacks will get overwritten.

That's why we were chaining (enqueuing) replaceTrack before even though the spec says not to.

The proper fix would be to have replaceTrackNoRenegotiation() return a promise.

::: dom/media/PeerConnection.js:1509
(Diff revision 5)
> -    return this._impl.createDataChannel(label, protocol, type, ordered,
> +    let dataChannel =
> +      this._impl.createDataChannel(label, protocol, type, ordered,
> -                                        maxPacketLifeTime, maxRetransmits,
> +                                   maxPacketLifeTime, maxRetransmits,
> -                                        negotiated, id);
> +                                   negotiated, id);
> +
> +    // Spec says to do this in the background. Close enough.

I've filed https://github.com/w3c/webrtc-pc/issues/1597 to resolve this. See comments there. Should be uncontroversial.

::: dom/media/PeerConnection.js:1510
(Diff revision 5)
> +    this._queueTaskWithClosedCheck(() => {
> +      this.updateNegotiationNeeded();
> +    });

Actually, the spec says to do this for the "first channel created" only...

Even then it says to run the update-the-negotiation-needed-flag algorithm [1]
which in turn runs the check-if-negotiation-is-needed algorithm [2], and only then conditionally queues a task to fire an event.

Would it make sense to align our functions closer with the spec algorithms here?

By that I mean not just putting the queueing into the function, which would help, but perhaps even centralize the negotiation-needed logic the same way the spec does? A bit inefficient perhaps, but appealing on other levels (avoiding spec interpretation errors). Thoughts?

If we DON'T do this, we should rename updateNegotiationNeeded() to something else so it's not mistaken for the spec algorithm of the same name (which could lead to forgetting to queue a task, as well as maybe firing in situations we shouldn't).

[1] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dfn-update-the-negotiation-needed-flag
[2] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dfn-check-if-negotiation-is-needed

::: dom/media/PeerConnection.js:1579
(Diff revision 5)
>    onSetLocalDescriptionSuccess() {
> +    this._dompc._syncTransceivers();
> +    this._dompc.clearNegotiationNeeded();
> +    this._dompc.updateNegotiationNeeded();

Here's an example where that might be helpful.

This looks like it might fire the negotiatonneeded event before the SLD success callback, which seems wrong.

The spec says "If connection's signaling state is now "stable", update the negotiation-needed flag."

So A) we should never run this outside of stable state, and B) we should queue a task to fire an event.

::: dom/media/PeerConnection.js:1586
(Diff revision 5)
>    onSetRemoteDescriptionSuccess() {
> +    this._dompc._syncTransceivers();
> +    this._dompc.clearNegotiationNeeded();
> +    this._dompc.updateNegotiationNeeded();

Same here.

::: dom/media/PeerConnection.js:1762
(Diff revision 5)
> -  onAddTrack(track, streams) {
> +  _getTransceiverWithRecvTrack(webrtcTrackId) {
> +    return this._dompc.getTransceivers().find(
> +        transceiver => transceiver.remoteTrackId == webrtcTrackId);

remoteTrackId got junked from the spec this morning.

::: dom/media/PeerConnection.js:1770
(Diff revision 5)
> -                                                  new RTCRtpReceiver(pc,
> -                                                                     track));
> -    pc._receivers.push(receiver);
> -    let ev = new pc._win.RTCTrackEvent("track", { receiver, track, streams });
> +    if (!matchingTransceiver) {
> +      throw new pc._win.DOMException(
> +          "No transceiver with receive track " + webrtcTrackId,
> +          "InternalError");

Throwing errors in PeerConnectionObserver doesn't work (leaving aside broken cases where we call back into it synchronously, which we should treat as a bug). We're in a queued task here typically, so there's no callstack to throw up.

Also, we should only throw content-window-visible exceptions (the pc._win. prefix part) for spec errors.

If this is a "can't happen except through programming error", then my recommendation is to remove all assert-like throws in JS. There are no unsafe memory errors in JS, so let it throw on undefined access a few line below instead.

::: dom/media/PeerConnection.js:1779
(Diff revision 5)
> +    if (!streams.length) {
> +      throw new pc._win.DOMException(
> +          "No streams for receive track " + webrtcTrackId,
> +          "InternalError");
> +    }

https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-20#section-5.2.1 says:

"If no MediaStream is associated with the transceiver, a single
 "a=msid" line with the special value "-" in place of the
 MediaStream ID"
 
so streams.length == 0 should be allowed I think, and the remote track streamless.

::: dom/media/PeerConnection.js:1785
(Diff revision 5)
> +    streams.forEach(
> +        stream => {

Nit: I think consistent indent for this file would be:

    streams.forEach(stream => {
      ..
    });

Applies throughout.

::: dom/media/PeerConnection.js:1785
(Diff revision 5)
> +    streams.forEach(
> +        stream => {
> +          stream.addTrack(matchingTransceiver.receiver.track);
> +          // Adding tracks from JS does not result in the stream getting
> +          // onaddtrack, so we need to do that here. The mediacapture spec says
> +          // this needs to be queued, also.
> +          pc._queueTaskWithClosedCheck(() => {
> +            stream.dispatchEvent(
> +                new pc._win.MediaStreamTrackEvent(
> +                  "addtrack", { track: matchingTransceiver.receiver.track }));
> +          });

These events happen in the wrong order I think.

A recent PR [1] ensures event.streams are fully populated by the time the "track" event fires. But this necessitates adding tracks to streams in SRD, not onTrack:

 1. Let trackEvents be an empty list.
 2. Run the following steps for each media description in description:
    1. Add tracks to streams, etc.
 3. For each event in trackEvents, fire event

I also believe we shouldn't queue a task for the addtrack/remove events. I've filed an issue on the spec for that [2].

[1] https://github.com/w3c/webrtc-pc/pull/1519 
[2] https://github.com/w3c/webrtc-pc/issues/1599

::: dom/media/PeerConnection.js:1804
(Diff revision 5)
> +    // TODO(bug XXXXX): Queue a task for this, and fix the tests, unless the
> +    // spec changes.
>      this.dispatchEvent(ev);
>  
>      // Fire legacy event as well for a little bit.
> -    ev = new pc._win.MediaStreamTrackEvent("addtrack", { track });
> +    ev = new pc._win.MediaStreamTrackEvent("addtrack",
> +        { track: matchingTransceiver.receiver.track });
> +    // TODO(bug XXXXX): Queue a task for this, and fix the tests, unless the
> +    // spec changes.

We should not queue a task here, since event must fire before promise resolves (remove comment).

::: dom/media/PeerConnection.js:1934
(Diff revision 5)
> +    // TODO (bug XXXXX): Technically, if the transceiver is not yet associated,
> +    // we're supposed to synchronously set the track and return a resolved
> +    // promise. However, PeerConnectionImpl::ReplaceTrackNoNegotiation does
> +    // stuff like updating principal change observers. We might want to set
> +    // that stuff up later than CreateTransceiverInternal so we don't need to
> +    // do this for tracks that haven't sent anything yet.
> +
> +    await this._pc._replaceTrack(this._transceiverImpl, withTrack);

this._pc._replaceTrack and ultimately this._pc._impl.replaceTrackNoRenegotiation() are still called synchronously here, so if the latter were to call PeerConnectionObserver.onReplaceTrackSuccess() synchronously when the transceiver is not yet associated, then the promise returned synchronously from both should already be rejected, as the spec demands, and the await here will basically (in the syntactic sugar-world of async functions) "throw" here, which basically means this async function just returns the rejected promise at this point, abandoning the remainder of the function.

Alternatively if we fix this._pc._impl.replaceTrackNoRenegotiation() to return a promise, the c++ code could return a rejected promise directly.

::: dom/media/PeerConnection.js:1950
(Diff revision 5)
>    setParameters(parameters) {
> -    return this._pc._win.Promise.resolve()
> +    let copy = Object.create(parameters);

WebIDL bindings already copy dictionaries, so this should be unnecessary.

::: dom/media/PeerConnection.js:1986
(Diff revision 5)
> +    if (transceiver.stopped) {
> +      throw new this._pc._win.DOMException(
> +          "This sender's transceiver is stopped", "InvalidStateError");
> +    }
> +
> +    // TODO: transaction ids

Bug # or additional patch

::: dom/media/PeerConnection.js:1988
(Diff revision 5)
> +          "This sender's transceiver is stopped", "InvalidStateError");
> +    }
> +
> +    // TODO: transaction ids
> +
> +    // Spec says this stuff needs to be done async. May change.

Should be good once https://github.com/w3c/webrtc-pc/pull/1560 is merged.

::: dom/media/PeerConnection.js:2075
(Diff revision 5)
> +          // the receiver starts out without a track, so record this here
> +          _kind: kind,

I'm curious, does a transceiver always know its kind? If so, this might help fix bug 1376947.

::: dom/media/PeerConnection.js:2081
(Diff revision 5)
> +  setDirection(direction) {
> +    if (this.direction == direction) {

this._pc._checkClosed() first, and if (this.stopped) throw

::: dom/media/PeerConnection.js:2088
(Diff revision 5)
> +      return;
> +    }
> +
> +    this.direction = direction;
> +    this.sync();
> +    this._pc.updateNegotiationNeeded();

Again, looking at "check if negotiation is needed" in the spec [1] I'm not positive there aren't cases where we shouldn't fire an event here. I just can't tell.

In lieu of that refactor, a comment briefly explaining why setDirection here would basically always need to trigger ONN I think would be helpful.

[1] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dfn-check-if-negotiation-is-needed

::: dom/media/PeerConnection.js:2096
(Diff revision 5)
> +  stop() {
> +    if (this.stopped) {
> +      return;
> +    }
> +
> +    this.setStopped();

this._pc._checkClosed() first

::: dom/media/PeerConnection.js:2101
(Diff revision 5)
> +  setStopped() {
> +    this.stopped = true;
> +    this.currentDirection = null;

We should queue a task to fire "ended" on receiver.track here, as described in [1]

[1] https://w3c.github.io/mediacapture-main/getusermedia.html#track-ended

::: dom/media/PeerConnection.js:2130
(Diff revision 5)
> +    if (this._syncing) {
> +      throw new this._pc._win.DOMException(
> +          "Reentrant sync! This is a bug!",
> +          "InternalError");
> +    }

Maybe

    throw new DOMException(

since this is a programming error, not a spec error? (i.e. I think we want it to go to browser console, not web console)
Attachment #8900492 - Flags: review?(jib) → review-
Blocks: 1400363
Blocks: 1095780
No longer depends on: 1208378
Comment on attachment 8900492 [details]
Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver.

https://reviewboard.mozilla.org/r/171864/#review186650

::: dom/media/PeerConnection.js:150
(Diff revision 5)
>      let cleanupPcRef = function(pcref) {
>        let pc = pcref.get();
>        if (pc) {
> -        pc._pc.close();
> +        pc.close();

Hmm. Right now it looks like PeerConnectionImpl::Close is going to fire a signaling state change when this code is run. Is that a bug? Or is there some code that filters that event out later?

::: dom/media/PeerConnection.js:703
(Diff revision 5)
> +  _getTransceiverWithSender(sender) {
> +    let transceiver = this._transceivers.find(t => t.sender == sender);
> +    if (!transceiver) {
> +      throw new this._win.DOMException("This isn't one of my senders!",
> +                                       "InvalidAccessError");
> +    }
> +    return transceiver;
> +  }

So if we are asserting that this cannot fail, sender needs a backpointer to its transceiver, because there are cases where transceivers are removed from the PC.

::: dom/media/PeerConnection.js:782
(Diff revision 5)
> +    // Spec language implies that this needs to happen as if it were called
> +    // before createOffer, so we do this as early as possible.
> +    this._ensureTransceiversForOfferToReceive(optionsOrOnSucc);
> +
>      // This entry-point handles both new and legacy call sig. Decipher which one
>      if (typeof optionsOrOnSucc == "function") {
>        return this._legacy(optionsOrOnSucc, onErr, () => this._createOffer(options));

We call that above. Is there something wrong with that?

::: dom/media/PeerConnection.js:1510
(Diff revision 5)
> +    this._queueTaskWithClosedCheck(() => {
> +      this.updateNegotiationNeeded();
> +    });

This should be equivalent I think; let's say this is not the first datachannel created. If there is no m=application section negotiated yet, that implies we have already called updateNegotiationNeeded, so this call won't have any effect. If there is an m=application section negotiated, this check will not have any effect either.

I queued this because step 19 says to run the following steps (including update the negotiation-needed step) in the background. Is there another way I should be doing this?

::: dom/media/PeerConnection.js:1579
(Diff revision 5)
>    onSetLocalDescriptionSuccess() {
> +    this._dompc._syncTransceivers();
> +    this._dompc.clearNegotiationNeeded();
> +    this._dompc.updateNegotiationNeeded();

updateNegotiationNeeded never causes something to fire synchronously. It is exactly the same as the "update the negotiation-needed flag" algorithm in the spec, except it will throw if the pc is closed (which you noted elsewhere, and which I will fix).

The business about (duplicate) checking the signaling state before running tht algorithm, and then having a (duplicate) event dispatch, strikes me as a spec bug. Step 10 should probably just be:

"10. Update the negotiation-needed flag on connection."

::: dom/media/PeerConnection.js:1762
(Diff revision 5)
> -  onAddTrack(track, streams) {
> +  _getTransceiverWithRecvTrack(webrtcTrackId) {
> +    return this._dompc.getTransceivers().find(
> +        transceiver => transceiver.remoteTrackId == webrtcTrackId);

Booooo!

::: dom/media/PeerConnection.js:1779
(Diff revision 5)
> +    if (!streams.length) {
> +      throw new pc._win.DOMException(
> +          "No streams for receive track " + webrtcTrackId,
> +          "InternalError");
> +    }

I have not seen anything in the spec that indicates "-" as a stream id is interpreted by the other side as "no stream". I interpreted this as saying if the local side doesn't specify a stream, just pretend there is one with a placeholder. These ids are opaque unless there is spec language saying otherwise.

::: dom/media/PeerConnection.js:1785
(Diff revision 5)
> +    streams.forEach(
> +        stream => {
> +          stream.addTrack(matchingTransceiver.receiver.track);
> +          // Adding tracks from JS does not result in the stream getting
> +          // onaddtrack, so we need to do that here. The mediacapture spec says
> +          // this needs to be queued, also.
> +          pc._queueTaskWithClosedCheck(() => {
> +            stream.dispatchEvent(
> +                new pc._win.MediaStreamTrackEvent(
> +                  "addtrack", { track: matchingTransceiver.receiver.track }));
> +          });

So, the TLDR here is the tracks need to be added, and the track events need to fire, before SRD resolves?

::: dom/media/PeerConnection.js:1934
(Diff revision 5)
> +    // TODO (bug XXXXX): Technically, if the transceiver is not yet associated,
> +    // we're supposed to synchronously set the track and return a resolved
> +    // promise. However, PeerConnectionImpl::ReplaceTrackNoNegotiation does
> +    // stuff like updating principal change observers. We might want to set
> +    // that stuff up later than CreateTransceiverInternal so we don't need to
> +    // do this for tracks that haven't sent anything yet.
> +
> +    await this._pc._replaceTrack(this._transceiverImpl, withTrack);

I'm confused. Why would calling onReplaceTrackSuccess() lead to rejection of the promise?

I guess I could skip the task queuing at the bottom if we weren't associated yet, and just do it directly, and that would get the right effect...

::: dom/media/PeerConnection.js:2075
(Diff revision 5)
> +          // the receiver starts out without a track, so record this here
> +          _kind: kind,

Yeah, we always know this. The tricky bit is that the spec says the an RTCRtpTransceiver always has a receiver.track, and this is the source of truth for what kind of transceiver it is. However, since the receiver's track is created in c++ _after_ creation of the RTCRtpTransceiver (but before JS gets to see it), we need this _kind breadcrumb so the right kind of track is created.

::: dom/media/PeerConnection.js:2088
(Diff revision 5)
> +      return;
> +    }
> +
> +    this.direction = direction;
> +    this.sync();
> +    this._pc.updateNegotiationNeeded();

updateNegotiationNeeded doesn't unconditionally fire events, it does the checking in the spec.

::: dom/media/PeerConnection.js:2101
(Diff revision 5)
> +  setStopped() {
> +    this.stopped = true;
> +    this.currentDirection = null;

This is called from c++, which is going to cause the track to end too.
Comment on attachment 8900492 [details]
Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver.

https://reviewboard.mozilla.org/r/171864/#review184150

> cleanupPcRef() is called when the window is torn down.
> 
> pc.close() fires an "iceconnectionstatechange" event at the window.
> 
> We don't want to fire events during teardown.

Ugh, reviewboard submitted what I thought were replies as standalone comments. Ok, now to copy all of these over...

Hmm. Right now it looks like PeerConnectionImpl::Close is going to fire a signaling state change when this code is run. Is that a bug? Or is there some code that filters that event out later?

> In the spec, I believe this function corresponds to the common:
> 
>     Let transceiver be the RTCRtpTransceiver object associated with sender.
> 
> This CANNOT FAIL. As such we should just return transceiver here I think, which will an undefined dereference (which should show in browser console only). I.e. we should not throw a content-visible "InvalidAccessError".
> 
> To reduce pilot error, we could move this function to the sender, e.g.:
> 
>     _getTransceiver() {
>       return this._pc._transceivers.find(t => t.sender == this);
>     }

So if we are asserting that this cannot fail, sender needs a backpointer to its transceiver, because there are cases where transceivers are removed from the PC.

> I think we need to call
> 
>     _ensureTransceiversForOfferToReceive(options);
> 
> here specifically in the legacy case, otherwise this lookss like it will break legacy callback users who use offerToReceive.

We call that above. Is there something wrong with that?

> Actually, the spec says to do this for the "first channel created" only...
> 
> Even then it says to run the update-the-negotiation-needed-flag algorithm [1]
> which in turn runs the check-if-negotiation-is-needed algorithm [2], and only then conditionally queues a task to fire an event.
> 
> Would it make sense to align our functions closer with the spec algorithms here?
> 
> By that I mean not just putting the queueing into the function, which would help, but perhaps even centralize the negotiation-needed logic the same way the spec does? A bit inefficient perhaps, but appealing on other levels (avoiding spec interpretation errors). Thoughts?
> 
> If we DON'T do this, we should rename updateNegotiationNeeded() to something else so it's not mistaken for the spec algorithm of the same name (which could lead to forgetting to queue a task, as well as maybe firing in situations we shouldn't).
> 
> [1] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dfn-update-the-negotiation-needed-flag
> [2] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dfn-check-if-negotiation-is-needed

This should be equivalent I think; let's say this is not the first datachannel created. If there is no m=application section negotiated yet, that implies we have already called updateNegotiationNeeded, so this call won't have any effect. If there is an m=application section negotiated, this check will not have any effect either.

I queued this because step 19 says to run the following steps (including update the negotiation-needed step) in the background. Is there another way I should be doing this?

> Here's an example where that might be helpful.
> 
> This looks like it might fire the negotiatonneeded event before the SLD success callback, which seems wrong.
> 
> The spec says "If connection's signaling state is now "stable", update the negotiation-needed flag."
> 
> So A) we should never run this outside of stable state, and B) we should queue a task to fire an event.

updateNegotiationNeeded never causes something to fire synchronously. It is exactly the same as the "update the negotiation-needed flag" algorithm in the spec, except it will throw if the pc is closed (which you noted elsewhere, and which I will fix).

The business about (duplicate) checking the signaling state before running that algorithm, and then having a (duplicate) event dispatch, strikes me as a spec bug. Step 10 should probably just be:

"10. Update the negotiation-needed flag on connection."

> remoteTrackId got junked from the spec this morning.

Booooo!

> https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-20#section-5.2.1 says:
> 
> "If no MediaStream is associated with the transceiver, a single
>  "a=msid" line with the special value "-" in place of the
>  MediaStream ID"
>  
> so streams.length == 0 should be allowed I think, and the remote track streamless.

I have not seen anything in the spec that indicates "-" as a stream id is interpreted by the other side as "no stream". I interpreted this as saying if the local side doesn't specify a stream, just pretend there is one with a placeholder. These ids are opaque unless there is spec language saying otherwise.

Basically, if this happens, the c++ code has a bug.

> These events happen in the wrong order I think.
> 
> A recent PR [1] ensures event.streams are fully populated by the time the "track" event fires. But this necessitates adding tracks to streams in SRD, not onTrack:
> 
>  1. Let trackEvents be an empty list.
>  2. Run the following steps for each media description in description:
>     1. Add tracks to streams, etc.
>  3. For each event in trackEvents, fire event
> 
> I also believe we shouldn't queue a task for the addtrack/remove events. I've filed an issue on the spec for that [2].
> 
> [1] https://github.com/w3c/webrtc-pc/pull/1519 
> [2] https://github.com/w3c/webrtc-pc/issues/1599

So, the TLDR here is the tracks need to be added, then the track events need to fire, then SRD resolves?

> this._pc._replaceTrack and ultimately this._pc._impl.replaceTrackNoRenegotiation() are still called synchronously here, so if the latter were to call PeerConnectionObserver.onReplaceTrackSuccess() synchronously when the transceiver is not yet associated, then the promise returned synchronously from both should already be rejected, as the spec demands, and the await here will basically (in the syntactic sugar-world of async functions) "throw" here, which basically means this async function just returns the rejected promise at this point, abandoning the remainder of the function.
> 
> Alternatively if we fix this._pc._impl.replaceTrackNoRenegotiation() to return a promise, the c++ code could return a rejected promise directly.

I'm confused. Why would calling onReplaceTrackSuccess() lead to rejection of the promise?

I guess I could skip the task queuing at the bottom if we weren't associated yet, and just do it directly, and that would get the right effect...

> I'm curious, does a transceiver always know its kind? If so, this might help fix bug 1376947.

Yeah, we always know this. The tricky bit is that the spec says the an RTCRtpTransceiver always has a receiver.track, and this is the source of truth for what kind of transceiver it is. However, since the receiver's track is created in c++ _after_ creation of the RTCRtpTransceiver (but before JS gets to see it), we need this _kind breadcrumb so the right kind of track is created.

> Again, looking at "check if negotiation is needed" in the spec [1] I'm not positive there aren't cases where we shouldn't fire an event here. I just can't tell.
> 
> In lieu of that refactor, a comment briefly explaining why setDirection here would basically always need to trigger ONN I think would be helpful.
> 
> [1] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dfn-check-if-negotiation-is-needed

updateNegotiationNeeded doesn't unconditionally fire events, it does the checking in the spec.

> We should queue a task to fire "ended" on receiver.track here, as described in [1]
> 
> [1] https://w3c.github.io/mediacapture-main/getusermedia.html#track-ended

This is called from c++, which is going to cause the track to end too.
Comment on attachment 8900492 [details]
Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver.

https://reviewboard.mozilla.org/r/171864/#review184150

> Nit: redundant temporary when we have arrow functions.

Huh, even when there's another non-arrow function along the way...

> Even though return values are used here, this function also seems redundant, called from one place. Maybe inline? Makes it easier to compare to spec algorithms steps.
> 
> Flat seems better here. In particular, I dislike updateNegotiationNeeded() being called both in here and again at lone call site.

I'm confused about your comment about updateNegotiationNeeded. Where's the extra call?

> If we do this as
> 
>     for (let transceiver of this._transceivers) {
> 
> then we can break out of the loop.
> 
> (Imperative for( : ) loops are also back in fashion as the dominant pattern since they work well with async/await, where forEach is a bit of a deathtrap. YMMV).

Rewrote, so this is no longer a problem.

> Nit: s/.getKind()/._kind/
> 
> Since the transceivers pushed on this._transceivers appear to be WebIDL objects, .getKind() likely calls redundant c++ binding code here.
> 
> No need to call accessor functionss which exists for binding code only, at least for getters IMHO. False dependence. Better to call the xray ._kind directly.
> 
> Applies throughout.

This doesn't work; _kind is undefined here.
Comment on attachment 8900492 [details]
Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver.

https://reviewboard.mozilla.org/r/171864/#review184150

> We call that above. Is there something wrong with that?

Nevermind, I see the problem. Have rewritten this function to be clearer.

> prefer ._hasBeenUsedToSend to avoid binding code
> 
> (abstractions in JS are not free).

Doesn't work, this is undefined here.

> Bug: s/streams/_streams/

I bet this doesn't work, since we cannot access variables like this on RTCRtpTransceiver. Let me see what I can do instead.

> Inlining this function would make it easier to reason about why it's firing events I think, and whether we should queue a task or not (we should not).

Too much access into private members of _dompc for my taste.

> This will hang if someone calls:
> 
>     let p = pc.replaceTrack(track);
>     pc.replaceTrack(track);
>     await p; // hang
> 
> ...because the this.members storing the resolve and reject callbacks will get overwritten.
> 
> That's why we were chaining (enqueuing) replaceTrack before even though the spec says not to.
> 
> The proper fix would be to have replaceTrackNoRenegotiation() return a promise.

If replaceTrackNoRenegotiation is synchronous, we could simply do away with the success/failure callbacks, right?

> Same here.

Same question as above.

> I'm confused. Why would calling onReplaceTrackSuccess() lead to rejection of the promise?
> 
> I guess I could skip the task queuing at the bottom if we weren't associated yet, and just do it directly, and that would get the right effect...

Fixed I think. Please check.

> Should be good once https://github.com/w3c/webrtc-pc/pull/1560 is merged.

We can fix this in bug 1401592, since the test expects this async step to be here.

> This is called from c++, which is going to cause the track to end too.

To be more specific, when we sync with c++, the track will be ended.
Comment on attachment 8900493 [details]
Bug 1290948 - Part 4: Transceivers JSEP/SDP work. r+drno

https://reviewboard.mozilla.org/r/171866/#review182950

Wow. Maybe I should have started with reading the tests first.
Lets do one more round...

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:914
(Diff revision 7)
> +    info("Offer is: " + offer.sdp);
> +    info("Answer is: " + answer.sdp);

Are these left over debug message, or do you want them to stay?

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:363
(Diff revision 7)
>          return 1;
>        }
>      }
>      return 0;

Can we change these |1| and |0| to true and false please?

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:850
(Diff revision 7)
>      if (checkFlags & CHECK_SUCCESS) {
>        ASSERT_EQ(NS_OK, rv);
>      }
>  
>      if (checkFlags & CHECK_TRACKS) {
> -      auto tracks = mSessionAns->GetRemoteTracks();
> +      ASSERT_EQ(types.size(), mSessionAns->GetTransceivers().size());

I guess the same limitation regarding recvonly and inactive applies here as well. Lets put that comment then here as well.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:897
(Diff revision 7)
>          // These might have been in the SDP, or might have been randomly
>          // chosen by JsepSessionImpl

I think this comment no longer applies, or?

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:947
(Diff revision 7)
>          // These might have been in the SDP, or might have been randomly
>          // chosen by JsepSessionImpl

Same here. No longer applies, I think.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:1614
(Diff revision 7)
> -  auto offererPairs = GetTrackPairsByLevel(*mSessionOff);
> -  auto answererPairs = GetTrackPairsByLevel(*mSessionAns);
> +  std::vector<RefPtr<JsepTransceiver>> origOffererTransceivers
> +    = DeepCopy(mSessionOff->GetTransceivers());
> +  std::vector<RefPtr<JsepTransceiver>> origAnswererTransceivers
> +    = DeepCopy(mSessionAns->GetTransceivers());

auto doesn't work here for the type?

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:1711
(Diff revision 7)
> -  ASSERT_EQ(offererPairs.size(), newAnswererPairs.size());
> -  for (size_t i = 0; i < offererPairs.size(); ++i) {
> -    ASSERT_TRUE(Equals(offererPairs[i], newAnswererPairs[i]));
> +  ASSERT_EQ(offererTransceivers.size(), newAnswererTransceivers.size());
> +  for (size_t i = 0; i < offererTransceivers.size(); ++i) {
> +    ASSERT_TRUE(Equals(*offererTransceivers[i], *newAnswererTransceivers[i]));
>    }
>  
> -  ASSERT_EQ(answererPairs.size(), newOffererPairs.size());
> -  for (size_t i = 0; i < answererPairs.size(); ++i) {
> -    ASSERT_TRUE(Equals(answererPairs[i], newOffererPairs[i]));
> +  ASSERT_EQ(answererTransceivers.size(), newOffererTransceivers.size());
> +  for (size_t i = 0; i < answererTransceivers.size(); ++i) {
> +    ASSERT_TRUE(Equals(*answererTransceivers[i], *newOffererTransceivers[i]));

Having seen this loop the third time: how about a little helper function for doing this?

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:1733
(Diff revision 7)
> -  auto offererPairs = GetTrackPairsByLevel(*mSessionOff);
> -  auto answererPairs = GetTrackPairsByLevel(*mSessionAns);
> +  std::vector<RefPtr<JsepTransceiver>> origOffererTransceivers
> +    = DeepCopy(mSessionOff->GetTransceivers());
> +  std::vector<RefPtr<JsepTransceiver>> origAnswererTransceivers
> +    = DeepCopy(mSessionAns->GetTransceivers());

auto?

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:1788
(Diff revision 7)
> -  auto offererPairs = GetTrackPairsByLevel(*mSessionOff);
> -  auto answererPairs = GetTrackPairsByLevel(*mSessionAns);
> +  std::vector<RefPtr<JsepTransceiver>> origOffererTransceivers
> +    = DeepCopy(mSessionOff->GetTransceivers());
> +  std::vector<RefPtr<JsepTransceiver>> origAnswererTransceivers
> +    = DeepCopy(mSessionAns->GetTransceivers());

auto

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:1855
(Diff revision 7)
> -  auto offererPairs = GetTrackPairsByLevel(*mSessionOff);
> -  auto answererPairs = GetTrackPairsByLevel(*mSessionAns);
> +  std::vector<RefPtr<JsepTransceiver>> origOffererTransceivers
> +    = DeepCopy(mSessionOff->GetTransceivers());
> +  std::vector<RefPtr<JsepTransceiver>> origAnswererTransceivers
> +    = DeepCopy(mSessionAns->GetTransceivers());

auto

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:1988
(Diff revision 7)
>  
> -  RefPtr<JsepTrack> removedTrack = GetTrackOff(0, types.front());
> -  ASSERT_TRUE(removedTrack);
> -  ASSERT_EQ(NS_OK, mSessionOff->RemoveTrack(removedTrack->GetStreamId(),
> -                                           removedTrack->GetTrackId()));
> +  SetRemoteOffer(offer);
> +
> +  std::vector<JsepTrack> removedTracks = mSessionAns->GetRemoteTracksRemoved();
> +  std::vector<JsepTrack> addedTracks = mSessionAns->GetRemoteTracksAdded();
> +

Maybe add a comment here, that point of this test is to accept a change of a MSID, but the internal APIs should still report the original IDs, if I'm not mistaken.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:2132
(Diff revision 7)
> -  auto offererPairs = GetTrackPairsByLevel(*mSessionOff);
> -  auto answererPairs = GetTrackPairsByLevel(*mSessionAns);
> -
> -  RefPtr<JsepTrack> removedTrack = GetTrackAns(0, types.front());
> +  std::vector<RefPtr<JsepTransceiver>> origOffererTransceivers
> +    = DeepCopy(mSessionOff->GetTransceivers());
> +  std::vector<RefPtr<JsepTransceiver>> origAnswererTransceivers
> +    = DeepCopy(mSessionAns->GetTransceivers());

auto

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:2205
(Diff revision 7)
> -  auto offererPairs = GetTrackPairsByLevel(*mSessionOff);
> -  auto answererPairs = GetTrackPairsByLevel(*mSessionAns);
> -
> -  RefPtr<JsepTrack> removedTrackAnswer = GetTrackAns(0, types.front());
> +  std::vector<RefPtr<JsepTransceiver>> origOffererTransceivers
> +    = DeepCopy(mSessionOff->GetTransceivers());
> +  std::vector<RefPtr<JsepTransceiver>> origAnswererTransceivers
> +    = DeepCopy(mSessionAns->GetTransceivers());

auto

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:5782
(Diff revision 7)
> +// 1. AddTrack either caused a transceiver to be created, or set the send
> +// track on a preexisting transceiver.
> +// 2. The transceiver was not created as a side-effect of AddTrack, and the
> +// send track was put in place by some other means than AddTrack.

Yikes

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:5823
(Diff revision 7)
> +  // TODO: Once nils' code for switching offer/answer roles lands, switch and
> +  // have the other side reoffer to negotiate the new transceivers.

This has landed.

::: media/webrtc/signaling/gtest/jsep_track_unittest.cpp:123
(Diff revision 7)
> -      mSendAns->PopulateCodecs(mAnsCodecs.values);
> -      mRecvAns->PopulateCodecs(mAnsCodecs.values);
> +      mRecvOff.PopulateCodecs(mOffCodecs.values);
> +
> +      mSendAns = JsepTrack(type, sdp::kSend);
> +      if (type != SdpMediaSection::MediaType::kApplication) {
> +        mSendAns.UpdateTrack(
> +            std::vector<std::string>(1, "stream_id"), "track_id");

If stream and track ID's don't have to match up between offer and answer in the Trasceiver world, lets please use different values for the answerer.

::: media/webrtc/signaling/gtest/jsep_track_unittest.cpp:197
(Diff revision 7)
> -      if (mRecvAns && GetAnswer().IsReceiving()) {
> -        mRecvAns->Negotiate(GetAnswer(), GetOffer());
> +      if (GetAnswer().IsReceiving()) {
> +        mRecvAns.Negotiate(GetAnswer(), GetOffer());
>        }
>  
> -      if (mSendOff && GetAnswer().IsReceiving()) {
> -        mSendOff->Negotiate(GetAnswer(), GetAnswer());
> +      if (GetAnswer().IsReceiving()) {
> +        mSendOff.Negotiate(GetAnswer(), GetAnswer());

Looks like these two can be merged into the same if block now.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h:197
(Diff revision 7)
> -  nsresult CreateReceivingTrack(size_t mline,
> -                                const Sdp& sdp,
> +  JsepTransceiver* GetTransceiverForLevel(size_t level);
> +  JsepTransceiver* GetTransceiverForLocal(size_t level);

Sigh... wouldn't it be nice if you could use mid's instead of level index some day?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h:230
(Diff revision 7)
> -                                  SdpMediaSection** msection);
> -  nsresult CreateAnswerMSection(const JsepAnswerOptions& options,
> -                                size_t mlineIndex,
>                                  const SdpMediaSection& remoteMsection,
>                                  Sdp* sdp);
> -  nsresult SetRecvonlySsrc(SdpMediaSection* msection);
> +  nsresult CreateReceivingTrack(const Sdp& sdp,

Is this the only remaining track based function? Can we remove it, rename it?
Not sure how complicated it would be.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:76
(Diff revision 7)
> -  track->PopulateCodecs(mSupportedCodecs.values);
> -
> -  JsepSendingTrack strack;
> -  strack.mTrack = track;
>  
> -  mLocalTracks.push_back(strack);
> +  if (transceiver->mSending.GetMediaType() != SdpMediaSection::kApplication) {

As I'm assuming that a transceiver can't support different media types for sending and receiving it might be worth adding a GetMediaType() convenience function directly to the Transceiver class.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:93
(Diff revision 7)
> -  if (mState != kJsepStateStable) {
> -    JSEP_SET_ERROR("Removing tracks outside of stable is unsupported.");
> -    return NS_ERROR_UNEXPECTED;
> -  }
> +    }
> -
> -  auto track = FindTrackByIds(mLocalTracks, streamId, trackId);
> +  } else {
> +    transceiver->mJsDirection = SdpDirectionAttribute::Direction::kSendrecv;

Why is |mJsDirection| only set for data channels?
I get why SSRC, trackID etc don't apply to data channels. But this looks like it would/could leave transceivers in inconsistent state depending on which media type they are.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:212
(Diff revision 7)
> +  if (lastAnswerMsection) {
> +    // Use the protocol the answer used, even if it is not what we would have
> +    // used.
> +    protocol = lastAnswerMsection->GetProtocol();
> +  }

I think you can safely move this into the previous if {} block.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:226
(Diff revision 7)
> +  // Some of this stuff (eg; mid) sticks around even if disabled
> +  if (lastAnswerMsection) {
> +    nsresult rv = mSdpHelper.CopyStickyParams(*lastAnswerMsection, msection);

Does this mean that in case of a re-Offer the mid from the last answers m-section N is going to copied to the re-Offer's m-section N+1?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:266
(Diff revision 7)
> -// This function creates a skeleton SDP based on the old descriptions
> -// (ie; all m-sections are inactive).
> +  msection->GetAttributeList().SetAttribute(
> +      new SdpStringAttribute(SdpAttribute::kMidAttribute, mid));

This logic here makes sense to set the mid based on the transceiver. But this stomps over the mid which got potentially set by mSdpHelper.CopyStickyParams(). It looks to me like mid's stick to the transceiver (instead of the indexed m-section), and therefore mSdpHelper.CopyStickyParams() probably should not attempt to copy it. Some maybe we need to split the SdpHelper's into smaller pieces here.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:365
(Diff revision 7)
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> -  if (mCurrentLocalDescription) {
> -    rv = AddReofferMsections(*mCurrentLocalDescription,
> -                             *GetAnswer(),
> -                             sdp.get());
> +  for (size_t level = 0;
> +       JsepTransceiver* transceiver = GetTransceiverForLocal(level);
> +       ++level) {
> +    rv = CreateOfferMsection(options, *transceiver, sdp.get());

I think you still want the NS_ENSURE_SUCCESS() check here, or?
At least when generating the answer you still call that for each CreateAnswerMsection() call.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:368
(Diff revision 7)
> -  // Ensure that we have all the m-sections we need, and disable extras
> -  rv = SetupOfferMSections(options, sdp.get());
> -  NS_ENSURE_SUCCESS(rv, rv);
> +  if (!sdp->GetMediaSectionCount()) {
> +    JSEP_SET_ERROR("Cannot create offer when there are no valid transceivers.");
> +    return NS_ERROR_UNEXPECTED;
> +  }

Actually according to latest spec discussions (https://github.com/rtcweb-wg/jsep/issues/832) we need to be able to generate an empty SDP. But let's handle that in a new bug.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:559
(Diff revision 7)
> -        mPendingRemoteDescription->GetMediaSection(msection->GetLevel()),
> -        msection);
> -  }
>  
> -  return NS_OK;
> -}
> +  // Add extmap attributes. This logic will probably be moved to the track,
> +  // since it can be specified on a per-sender basis in JS.

Might be worth adding here that we also need an extmap registry either per bundled transport or on the session level to ensure the extension IDs are identical within the same bundled transport.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:668
(Diff revision 7)
>    // Check that content hasn't done anything unsupported with the SDP
>    rv = ValidateLocalDescription(*parsed);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> -  // Create transport objects.
> -  mOldTransports = mTransports; // Save in case we need to rollback
> +  if (type == kJsepSdpOffer) {
> +    // For rollback

I found the new comment more confusing then the old one, because I remembered that in case of a rollback we had returned from this function earlier already. Can you change the comment to indicate that this is in preparation for a potential future rollback?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:820
(Diff revision 7)
>    if (parsed->GetAttributeList().HasAttribute(
>            SdpAttribute::kIceOptionsAttribute)) {
>      iceOptions = parsed->GetAttributeList().GetIceOptions().mValues;
>    }
>  
> +  // For rollback.

"Prepared for future rollback"?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:884
(Diff revision 7)
> -
> -  // Now walk through the m-sections, make sure they match, and create
> -  // track pairs that describe the media to be set up.
> -  for (size_t i = 0; i < local->GetMediaSectionCount(); ++i) {
>      // Skip disabled m-sections.
>      if (answer.GetMediaSection(i).GetPort() == 0) {

Any good reason to use GetPort() instead of IsMsectionDisabled() here?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1007
(Diff revision 7)
>  
> -  trackPairOut->mRtpTransport = transport;
> +  if (transceiver->mTransport->mComponents == 2) {
> -
> -  if (transport->mComponents == 2) {
>      // RTCP MUX or not.
>      // TODO(bug 1095743): verify that the PTs are consistent with mux.

I just commented in that bug. I don't see how this would ever become a problem for us.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1337
(Diff revision 7)
> +      transceiver->Disassociate();
> +      JsepTransceiver* newTransceiver = FindUnassociatedTransceiver(
> +          transceiver->mSending.GetMediaType(), false);
> +      if (newTransceiver) {
> +        newTransceiver->SetLevel(level);
> +        transceiver->ClearLevel();
> +        return newTransceiver;
> +      }
> -      }
> +    }
>  
> -      std::vector<JsepReceivingTrack>::iterator track;
> +    return transceiver;

So if we get in here the transceiver gets disassociated. If then the FindUnassociatedTransceiver() call returns a nullptr, this appears to return the matching transceiver for the level, but disassociated. That looks like it's probably not intended.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1384
(Diff revision 7)
> +  if (transceiver) {
> +    transceiver->SetLevel(level);
> +    return transceiver;
> +  }
> +
> +  // Make a new transceiver

Is it intentional that GetTransceiverForLocal() can return a nullptr, but GetTransceiverForRemote() never returns a nullptr because it creates a Transceiver if none is found?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1438
(Diff revision 7)
> +      transceiver->mReceiving.UpdateTrack(streamIds, trackId);
> +    }
> +
> +    transceiver->mReceiving.UpdateRecvTrack(remote, msection);
>  
> -        track = FindTrackByIds(mRemoteTracks, streamId, trackId);
> +    if (!transceiver->mReceiving.GetTrackId().empty()) {

Is the else branch here when the remote msection is recvonly, or is that some error scenario for which we should have a log message?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1476
(Diff revision 7)
> +{
> +  const Sdp* answer(GetAnswer());
> +
> +  if (answer &&
> +      (level < answer->GetMediaSectionCount()) &&
> +      mSdpHelper.MsectionIsDisabled(answer->GetMediaSection(level))) {

Why not simply return mSdpHelper.MsectionIsDisabled() here directly?

::: media/webrtc/signaling/src/jsep/JsepTrack.h:109
(Diff revision 7)
>          mDirection(direction),
>          mActive(false)
> -  {}
> +  {
> +  }
>  
> -  virtual mozilla::SdpMediaSection::MediaType
> +  void UpdateTrack(const std::vector<std::string>& streamIds,

How about UpdateTrackIds()?

::: media/webrtc/signaling/src/jsep/JsepTrack.h:116
(Diff revision 7)
>    {
> -    return mType;
> +    mStreamIds = streamIds;
> +    mTrackId = trackId;
>    }
>  
> -  virtual const std::string&
> +  void ClearTrack()

Same here. How about ClearTrackIds()?

::: media/webrtc/signaling/src/jsep/JsepTrack.h:357
(Diff revision 7)
>    UniquePtr<JsepTrackNegotiatedDetails> mNegotiatedDetails;
>    std::vector<uint32_t> mSsrcs;
>    bool mActive;
>  };
>  
> -// Need a better name for this.
> +class JsepTransceiver {

I would not expect to find JsepTransceivers to be defined in JsepTrack.h. How about moving this into it's own JsepTranceivers.h file?

::: media/webrtc/signaling/src/jsep/JsepTrack.h:366
(Diff revision 7)
> -  RefPtr<JsepTransport> mRtcpTransport;
> +      mSending(type, sdp::kSend),
> +      mReceiving(type, sdp::kRecv),

I though |mSending| and |mReceiving| were booleans. How about renaming them to something like |mSendingTrack| and |mReceivingTrack|?

::: media/webrtc/signaling/src/jsep/JsepTrack.h:399
(Diff revision 7)
> +    void Rollback(JsepTransceiver& oldTransceiver)
> +    {
> +      *mTransport = *oldTransceiver.mTransport;
> +      mLevel = oldTransceiver.mLevel;
> +      mBundleLevel = oldTransceiver.mBundleLevel;
> +      mReceiving = oldTransceiver.mReceiving;

The sending track never changes through an O/A and therefore does not need to be rolled back?

::: media/webrtc/signaling/src/jsep/JsepTrack.h:401
(Diff revision 7)
> +      // stop() caused by a disabled m-section in a remote offer cannot be
> +      // rolled back.

Maybe open a bug report for this and reference it here?
Even if we can't fix it right now this might help people stumbling about this API limitation.

::: media/webrtc/signaling/src/jsep/JsepTrack.h:437
(Diff revision 7)
> +      MOZ_ASSERT(!HasLevel());
> +      MOZ_ASSERT(!IsStopped());

As SIZE_MAX is used as a special value we should probably assert here that |level != SIZE_MAX| (see SetBundleLevel() below) to avoid problems with future mega SDPs.

::: media/webrtc/signaling/src/jsep/JsepTrack.h:458
(Diff revision 7)
> +      return mLevel;
> +    }
> +
> +    void Stop()
> +    {
> +      mStopped = true;

Hmm is the actual logic of stopping handled some where else?

::: media/webrtc/signaling/src/jsep/JsepTrack.h:486
(Diff revision 7)
> -    MOZ_ASSERT(aBundleLevel != SIZE_MAX);
> +      MOZ_ASSERT(aBundleLevel != SIZE_MAX);
> -    mBundleLevel = aBundleLevel;
> +      mBundleLevel = aBundleLevel;

Should this be guarded against an already set |mBundleLevel| and !IsStopped() like you did for SetLevel() above?

::: media/webrtc/signaling/src/jsep/JsepTrack.h:534
(Diff revision 7)
> +    OwningNonNull<JsepTransport> mTransport;
> +
> +  private:
> +    // Stuff that is not negotiated
> +    std::string mMid;
> +    size_t mLevel;

For consistency add the same // SIZE_MAX comment as below for |mBundleLevel|

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:127
(Diff revision 7)
>    if (mDirection == sdp::kSend) {
> -    AddToMsection(mJsEncodeConstraints, sdp::kSend, offer);
> +    std::vector<JsConstraints> constraints;
> +    if (offer->IsSending()) {
> +      constraints = mJsEncodeConstraints;
> +    }
> +    AddToMsection(mJsEncodeConstraints, sdp::kSend, ssrcGenerator, offer);

I'm guessing you wanted |constraints| here as the parameter for the AddToMsection() call.
Otherwise you could delete the 4 lines above, because |constraints| never gets used anywhere.

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:178
(Diff revision 7)
>    for (const JsepCodecDescription* codec : codecs) {
>      codec->AddToMediaSection(*msection);
>    }
>  
> -  if (mDirection == sdp::kSend) {
> -    if (msection->GetMediaType() != SdpMediaSection::kApplication) {
> +  if (mDirection == sdp::kSend && mType != SdpMediaSection::kApplication) {
> +    if (msection->IsSending()) {

If there really is no else branch to |msection->IsSending()| then roll this condition into the if condition above.

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:185
(Diff revision 7)
> -    msection->SetSending(true);
> -  } else {
> +      } else {
> -    msection->SetReceiving(true);
> +        for (const std::string& streamId : mStreamIds) {
> +          msection->AddMsid(streamId, mTrackId);
> +          // TODO() Interop hack; older Firefox barfs if there is more than one
> +          // msid. Remove when safe.

Let's put concrete information here: at earliest this when ESR 52 is no longer supported, so after the release of ESR 59.2 or Fx 61.

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:259
(Diff revision 7)
>      }
>    }
>  
> -  if (!rids->mRids.empty()) {
> +  if (rids->mRids.size() > 1) {
>      msection->GetAttributeList().SetAttribute(simulcast.release());
> +    if (!rids->mRids.empty()) {

What is this new check for?
How can this be ever false if we can get here only if |mRids.size() > 1|?

::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:505
(Diff revision 7)
> +      streamIds->clear();
> +      streamIds->push_back(i->identifier);

Assuming you are going to find multiple streams which contain the same track isn't this just going to return always "only" the streamIDs of the last one found?
Attachment #8900493 - Flags: review?(drno)
Blocks: 1401983
Just wanted to follow up: are the web-platform-tests/webrtc test issues resolved? Are those tests currently being used to check spec conformance?
Blocks: 1402912
Comment on attachment 8900493 [details]
Bug 1290948 - Part 4: Transceivers JSEP/SDP work. r+drno

https://reviewboard.mozilla.org/r/171866/#review182950

> I think this comment no longer applies, or?

So at the JSEP level, the track id for remote tracks matches what was signaled (if it was signaled). And, we do propogate this id down to JS, but not as the MediaStreamTrack.id; we instead put it in (pref-guarded) RTCRtpTransceiver.remoteTrackId. Does this answer your question?

> Same here. No longer applies, I think.

Same question as above.

> auto doesn't work here for the type?

It does, I'm just becoming less of a fan of auto as I read more code with it.

> Having seen this loop the third time: how about a little helper function for doing this?

Sure.

> auto?

For the deep copies, I decided having the full type would make the code easier to understand.

> Maybe add a comment here, that point of this test is to accept a change of a MSID, but the internal APIs should still report the original IDs, if I'm not mistaken.

Correct.

> Yikes

Just remember; while you are _watching_ sausage being made, I am the one stuck _making_ the sausage. :)

> This has landed.

Ok, have finished writing this test.

> If stream and track ID's don't have to match up between offer and answer in the Trasceiver world, lets please use different values for the answerer.

Well, they'll line up in JSEP, but not in JS.

> Sigh... wouldn't it be nice if you could use mid's instead of level index some day?

Perhaps. But that day will never come. The time at which a transceiver is associated with an m-line, and the time at which a transceiver is associated with a mid, is _completely_ different.

> Is this the only remaining track based function? Can we remove it, rename it?
> Not sure how complicated it would be.

Was not implemented! Good catch.

> Why is |mJsDirection| only set for data channels?
> I get why SSRC, trackID etc don't apply to data channels. But this looks like it would/could leave transceivers in inconsistent state depending on which media type they are.

RTP transceivers use the JS direction the caller specified. The logic that creates transceivers due to seeing a new m-section in a remote description uses recvonly, which is wrong for m=application. This just makes sure nothing makes that mistake. I could move this into the caller, but I would end up wanting to leave an assert here, which is why I didn't do that.

> Does this mean that in case of a re-Offer the mid from the last answers m-section N is going to copied to the re-Offer's m-section N+1?

The m-line index for |lastAnswerMsection| and |msection| is the same (ie; local->GetMediaSectionCount() before |msection| is added).

> This logic here makes sense to set the mid based on the transceiver. But this stomps over the mid which got potentially set by mSdpHelper.CopyStickyParams(). It looks to me like mid's stick to the transceiver (instead of the indexed m-section), and therefore mSdpHelper.CopyStickyParams() probably should not attempt to copy it. Some maybe we need to split the SdpHelper's into smaller pieces here.

Hmm. So we need to keep the mid the same if the transceiver is stopped/disassociated though. Maybe what we ought to do here is:

1. If there was a previous offer/answer, use the mid from the previous m-section, and assert it matches the transceiver (if it has one).
2. If there was not a previous offer/answer, use the mid from the transceiver if it has one, and make one up otherwise.

Does that sound sane to you?

> Actually according to latest spec discussions (https://github.com/rtcweb-wg/jsep/issues/832) we need to be able to generate an empty SDP. But let's handle that in a new bug.

Yeah, this will probably take a little work to make sure nothing else in the system gets sad.

> Any good reason to use GetPort() instead of IsMsectionDisabled() here?

We're supposed to ignore a=bundle-only in answers, and just look at the port.

> So if we get in here the transceiver gets disassociated. If then the FindUnassociatedTransceiver() call returns a nullptr, this appears to return the matching transceiver for the level, but disassociated. That looks like it's probably not intended.

It is the intent, actually. :( Disassociate just un-sets the mid on the transceiver, it does not disassociate it with its m-section. This is one case where the time at which we unset the mid, and the time at which we un-set the level, is completely different.

> Is it intentional that GetTransceiverForLocal() can return a nullptr, but GetTransceiverForRemote() never returns a nullptr because it creates a Transceiver if none is found?

Yep. New remote m-sections cause transceivers to be created locally, if necessary (which of course depends on addtrack magic *barf*).

> Is the else branch here when the remote msection is recvonly, or is that some error scenario for which we should have a log message?

This is changing due to jib's review; shortly we will be checking the direction only. (Same higher up where we look for old tracks)

> Why not simply return mSdpHelper.MsectionIsDisabled() here directly?

Sure, why not.

> How about UpdateTrackIds()?

Sure.

> Same here. How about ClearTrackIds()?

Sure.

> The sending track never changes through an O/A and therefore does not need to be rolled back?

Not from the application of an offer, no.

> Maybe open a bug report for this and reference it here?
> Even if we can't fix it right now this might help people stumbling about this API limitation.

The spec was written this way deliberately, I'm pretty sure.

> Hmm is the actual logic of stopping handled some where else?

Yep. That's in TransceiverImpl::SyncWithJS, since either JS or JSEP can cause transceivers to stop.

> Should this be guarded against an already set |mBundleLevel| and !IsStopped() like you did for SetLevel() above?

The bundle level on a transceiver can change freely, whereas the level cannot. We can also end up with weird cases where JS calls stop() on a transceiver in the middle of negotiation, when it is too late to disable transports or unbundle things, so I can envision cases where we set the bundle level on a stopped transceiver.

> Let's put concrete information here: at earliest this when ESR 52 is no longer supported, so after the release of ESR 59.2 or Fx 61.

I've put a bug number here, and the details in the bug.

> Assuming you are going to find multiple streams which contain the same track isn't this just going to return always "only" the streamIDs of the last one found?

Yeah, this is wrong. Must be a holdover from the "only one msid" days.
Comment on attachment 8900494 [details]
Bug 1290948 - Part 5: TransceiverImpl and some major refactoring. r+drno

https://reviewboard.mozilla.org/r/171868/#review187400

I like the resulting code in the end a lot better. Look less complex to me.
But I guess with the amount of comments we need another round.

::: media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:328
(Diff revision 9)
>    // TODO(bcampen@mozilla.com): Right now this does not let us test RTCP in
>    // both directions; only the sender's RTCP is sent, but the receiver should
>    // be sending it too.

Looks like this comment no longer applies?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:370
(Diff revision 9)
>  // A specialization of pipeline for reading from the network and
>  // rendering video.
>  class MediaPipelineReceive : public MediaPipeline {

Lets fix this comment. This generic PipelineReceiver is not rendering video.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2133
(Diff revision 9)
>    }
>  }
>  
> -nsresult MediaPipelineReceiveAudio::Init()
> +void MediaPipelineReceiveAudio::SetPrincipalHandle_m(const PrincipalHandle& principal_handle)
>  {
> -  ASSERT_ON_THREAD(main_thread_);
> +  listener_->SetPrincipalHandle_m(principal_handle);

Since |listener_| can now be a nullptr when constructing, is this guaranteed to be called only when |listener_| is set?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2314
(Diff revision 9)
>    }
>  }
>  
> -nsresult MediaPipelineReceiveVideo::Init() {
> -  ASSERT_ON_THREAD(main_thread_);
> -  MOZ_MTLOG(ML_DEBUG, __FUNCTION__);
> +void MediaPipelineReceiveVideo::SetPrincipalHandle_m(const PrincipalHandle& principal_handle)
> +{
> +  listener_->SetPrincipalHandle_m(principal_handle);

Same here: can |listener_| here be null?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2321
(Diff revision 9)
>  
> +void
> +MediaPipelineReceiveVideo::Start()
> +{
> +  conduit_->StartReceiving();
>    listener_->AddSelf();

In MediaPipelineReceiveAudio::Start() this line is guarded with an if(listener_). Why not here?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2327
(Diff revision 9)
>  }
>  
> -void MediaPipelineReceiveVideo::SetPrincipalHandle_m(const PrincipalHandle& principal_handle)
> +void
> +MediaPipelineReceiveVideo::Stop()
>  {
> -  listener_->SetPrincipalHandle_m(principal_handle);
> +  listener_->RemoveSelf();

Same question here: guard with an if?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1223
(Diff revision 9)
>  
>    if (NS_FAILED(res)) {
>      std::string errorString = mJsepSession->GetLastError();
>      CSFLogError(logTag, "%s (%s) : pc = %s, error = %s",
>                  __FUNCTION__,
> -                type == SdpMediaSection::kAudio ? "audio" : "video",
> +                transceiver->mSending.GetMediaType() == SdpMediaSection::kAudio ?

Could the media type here also be kApplication?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1907
(Diff revision 9)
>      default:
>        MOZ_ASSERT(false);
>        return NS_ERROR_FAILURE;
>    }
>  
> +  size_t transceiverCount = mJsepSession->GetTransceivers().size();

I got confused why the count is taken before calling SetRemoteDescription(). As this appears to be the counter before SRD(), we should try to find a better name. How about originalTransceiverCount?

And then used that as the initial value in a for loop below?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1964
(Diff revision 9)
> +      }
> +
> +      if (jrv.Failed()) {
> +        nsresult rv = jrv.StealNSResult();
> +        CSFLogError(logTag, "%s: pc = %s, OnTransceiverNeeded failed. "
> +                    "This should never happen. rv = %d",

Famous last words :)

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2193
(Diff revision 9)
>    CSFLogError(logTag, "Encountered media error! %s", aError.c_str());
>    // TODO: Let content know about this somehow.
>  }
>  
>  nsresult
>  PeerConnectionImpl::AddTrack(MediaStreamTrack& aTrack,

This appears to be a void operation now. Can we remove it?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2225
(Diff revision 9)
>  
> -  nsString wideTrackId;
> -  aTrack.GetId(wideTrackId);
> +  nsresult rv = NS_ERROR_INVALID_ARG;
> +
> +  for (RefPtr<TransceiverImpl>& transceiver : transceivers) {
> +    if (transceiver->HasSendTrack(&aTrack)) {
> +      // TODO(bug XXXXX): Move DTMF stuff to TransceiverImpl

Can we fill this bug number?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2368
(Diff revision 9)
>        break;
>      }
>    }
>  
>    return NS_OK;

Not really related to this patch set, but this function return NS_OK and does not even touch |outToneBuffer| when it fails to locate the track? That looks like a bug to me.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2394
(Diff revision 9)
> -    CSFLogError(logTag, "Could not find stream from trackId");
> +    return rv;
> -    return NS_ERROR_UNEXPECTED;
>    }
>  
> -  std::string origStreamId = info->GetId();
> -  std::string newStreamId =
> +  for (size_t i = 0; i < mDTMFStates.Length(); ++i) {
> +    if (mDTMFStates[i].mTransceiver.get() == &aTransceiver) {

Does this DTMF stuff also need to get moved over to the Transceivers? If so, put the same comment here as above with the bug number.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h:44
(Diff revision 9)
>  class PCUuidGenerator;
> -
> -class SourceStreamInfo {
> -public:
> -  SourceStreamInfo(DOMMediaStream* aMediaStream,
> -                   PeerConnectionMedia *aParent,
> +class MediaPipeline;
> +class MediaPipelineFilter;
> +class JsepSession;
> +
> +// TODO(bug XXXXX): If we move the TransceiverImpl stuff out of here, this will

That sounds like a good idea. Fill in a bug number.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:359
(Diff revision 9)
> +    bool levelHasTransport =
> +      transport->mComponents &&
> +      transport->mIce &&
> +      (!transceiver->HasBundleLevel() || (transceiver->BundleLevel() == level));
> +
> +    if (levelHasTransport) {

levelHasTransport is only the condition for the if() block, or? Let move it there.

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:33
(Diff revision 9)
> +#include "mozilla/dom/RTCRtpTransceiverBinding.h"
> +#include "mozilla/dom/TransceiverImplBinding.h"
> +
> +namespace mozilla {
> +
> +MOZ_MTLOG_MODULE("mediapipeline")

I would say lets give this it's own logging module.

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:109
(Diff revision 9)
> +  mTransmitPipeline->UpdateSinkIdentity_m(aTrack, aPrincipal, aSinkIdentity);
> +  return NS_OK;
> +}
> +
> +void
> +TransceiverImpl::Shutdown_m()

We don't need to do anything with mReceiveStream here?

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:154
(Diff revision 9)
> +    for (unsigned int ssrc : mJsepTransceiver->mReceiving.GetSsrcs()) {
> +      filter->AddRemoteSSRC(ssrc);
> +    }

I'm starting to wonder if it's a good thing to restrict the filter to SSRC's from signaling only, or if we should carry forward SSRC's we (think we) learned at run time from the media side. But that is probably for another bug.

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:177
(Diff revision 9)
> +
> +nsresult
> +TransceiverImpl::UpdateConduit()
> +{
> +  MOZ_MTLOG(ML_DEBUG, mPCHandle << "[" << mMid << "]: "
> +                      "In UpdateConduit");

s/UpdateConduit/__FUNCTION__

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:189
(Diff revision 9)
> +
> +  mReceivePipeline->Stop();
> +  mTransmitPipeline->Stop();
> +
> +  if (mJsepTransceiver->IsStopped()) {
> +    MOZ_MTLOG(ML_DEBUG, this << " " << mPCHandle << " [" << mMid << "]: "

The other log message don't start with |this|. Is there a special reason this one is different?
I would prefer to have them consistent. Either all logging |this| or without, no strong preference from my side.

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:224
(Diff revision 9)
> +  } else {
> +    rv = UpdateAudioConduit();
> +  }
> +
> +  if (NS_FAILED(rv)) {
> +    return rv;

Lets add MOZ_MTLOG() here before returning, just in case.

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:235
(Diff revision 9)
> +      MOZ_MTLOG(ML_WARNING, mPCHandle << "[" << mMid << "]: "
> +                            "Starting transmit conduit without send track!");

Is there a valid use case for this, or does this indicate a bug of some kind where we should abort?

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:301
(Diff revision 9)
> +        WebrtcAudioConduit *audio_conduit =
> +          static_cast<WebrtcAudioConduit*>(mConduit.get());
> +        WebrtcVideoConduit *video_conduit =
> +          static_cast<WebrtcVideoConduit*>(transceiver->mConduit.get());
> +
> +        video_conduit->SyncTo(audio_conduit);

Is this intentionally not exiting after the first match?
Or in other words: does it work if an audio conduit gets synced to multiple video conduits?

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:325
(Diff revision 9)
> +  if (!mSendTrack) {
> +    return false;
> +  }
> +
> +  if (!aSendTrack) {
> +    return true;

I would have expected to see false here.
Or is that an intentional feature to say claim that HasSendTrack(nullptr) returns that the transceiver has AN (as in any) sending track?

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:335
(Diff revision 9)
> +
> +void
> +TransceiverImpl::SyncWithJS(dom::RTCRtpTransceiver& aJsTransceiver,
> +                            ErrorResult& aRv)
> +{
> +  MOZ_MTLOG(ML_DEBUG, "Syncing with JS transceiver");

Lets have this log the same information as the other initial function log message (mPCHandle, mMid).

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:534
(Diff revision 9)
> +  if (!mHaveStartedReceiving) {
> +    return false;
> +  }
> +
> +  if (!aRecvTrack) {
> +    return true;

Same question here: is this a special if queried with null it tells you if any receive track is set feature?

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:568
(Diff revision 9)
> +  MOZ_MTLOG(ML_WARNING, mPCHandle << "[" << mMid << "]: " << __FUNCTION__ <<
> +            " returning " << mReceivePipeline.get());

Why is this at warning level?
If at all, then debug I would say. But since GetSendPipeline() doesn't log anything I would suggest to remove this to make it consistent.

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:690
(Diff revision 9)
> +                          "AudioCodecConfigs (send).");
> +      return rv;
> +    }
> +
> +    if (configs.values.size() > 1
> +        && configs.values.back()->mName == "telephone-event") {

This will break if you/we turn on RED support, because then we will have three config values.
I fear we need to iterate over the config values here.
Can be done in new bug since RED support is not on by default yet.

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:722
(Diff revision 9)
> +
> +  return NS_OK;
> +}
> +
> +static nsresult
> +JsepCodecDescToCodecConfig(const JsepCodecDescription& aCodec,

Can we please rename this to JsepCodecDescToVideoCodecConfig() and the other one to Audio?

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:752
(Diff revision 9)
> +            desc.mSpropParameterSets.c_str(),
> +            spropSize);
> +    h264Config->sprop_parameter_sets[spropSize - 1] = '\0';
> +    h264Config->packetization_mode = desc.mPacketizationMode;
> +    h264Config->profile_level_id = desc.mProfileLevelId;
> +    h264Config->tias_bw = 0; // TODO. Issue 165.

I think soon nobody will be able to link "Issue 165" to this https://github.com/unicorn-wg/gecko-dev/issues/165 any more. Let's either remove the comment, or replace it with a proper reference to a bugzilla entry.

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:927
(Diff revision 9)
> +  std::vector<webrtc::RtpExtension> extmaps;
> +  // @@NG read extmap from track
> +  aDetails.ForEachRTPHeaderExtension(
> +    [&extmaps](const SdpExtmapAttributeList::Extmap& extmap)
> +  {
> +    extmaps.emplace_back(extmap.extensionname,extmap.entry);

Missing space :)
Attachment #8900494 - Flags: review?(drno)
Comment on attachment 8900495 [details]
Bug 1290948 - Part 6: Remove some unused code. r+drno

https://reviewboard.mozilla.org/r/171870/#review188460

LGTM
Attachment #8900495 - Flags: review?(drno) → review+
Blocks: 1402997
Comment on attachment 8900496 [details]
Bug 1290948 - Part 7: Update existing mochitests to track the spec fixes in previous parts. r+drno

https://reviewboard.mozilla.org/r/171872/#review188500

LGTM with some smaller tweaks.

::: dom/media/tests/mochitest/pc.js:902
(Diff revision 9)
>      this._pc.setIdentityProvider(provider, protocol, identity);
>    },
>  
> -  ensureMediaElement : function(track, direction) {
> +  getMediaElementForTrack : (track, direction) =>
> +  {
> +    const idPrefix = [this.label, direction].join('_');

How about putting this into a separate small function createPrefix() or something like that, instead of duplicating it?

::: dom/media/tests/mochitest/pc.js:1081
(Diff revision 9)
>      }
>  
>      info("Get " + constraintsList.length + " local streams");
> -    return Promise.all(constraintsList.map(constraints => {
> -      return getUserMedia(constraints).then(stream => {
> -        if (constraints.audio) {
> +
> +    return Promise.all(
> +      constraintsList.map(constraints => this.getUserMedia(constraints)));

I think the closing ) bracket from the Promise should go on a new line here for better readability.

::: dom/media/tests/mochitest/pc.js:1089
(Diff revision 9)
> -              " with audio track " + track.id);
> -          });
> +  getAllUserMediaAndAddTracks : async function(constraintsList) {
> +    var streams = await this.getAllUserMedia(constraintsList);
> +    if (!streams) {
> +      return;
> -        }
> +    }
> -        if (constraints.video) {
> +    return Promise.all(streams.map(stream => this.attachLocalStream(stream)));

Is this on purpose calling attachLocalStream()?
From the name of the function I would have expected that this calls attachLocalTrack() instead. Plus this function appears to be identical to getAllUserMediaAndAddTransceivers() below.

::: dom/media/tests/mochitest/pc.js:1285
(Diff revision 9)
>      this._pc.addEventListener('track', event => {
> -      info(this + ": 'ontrack' event fired for " + JSON.stringify(event.track));
> -
> -      this.checkTrackIsExpected(event.track,
> -                                this.expectedRemoteTrackInfoById,
> -                                this.observedRemoteTrackInfoById);
> +      info(this + ": 'ontrack' event fired for " + event.track.id +
> +                  "(SDP msid is " + this.getWebrtcTrackId(event.track) +
> +                  ")");
> +
> +      // TODO(bug XXXXX): Checking for remote tracks needs to be completely

Fill in the bug number.

::: dom/media/tests/mochitest/pc.js:1604
(Diff revision 9)
>    waitForMediaFlow : function() {
>      return Promise.all([].concat(
>        this.localMediaElements.map(element => this.waitForMediaElementFlow(element)),
> -      Object.keys(this.expectedRemoteTrackInfoById)
> -          .map(id => this.remoteMediaElements
> -              .find(e => e.srcObject.getTracks().some(t => t.id == id)))
> +      this.remoteMediaElements
> +        .filter(elem =>
> +                this.getExpectedActiveReceiveTracks().some(track => elem.srcObject.getTracks().some(t => t == track)))

Add some line break(s) please.

::: dom/media/tests/mochitest/pc.js:1606
(Diff revision 9)
> -      this._pc.getSenders().map(sender => this.waitForRtpFlow(sender.track)),
> -      this._pc.getReceivers().map(receiver => this.waitForRtpFlow(receiver.track))));
> +      this.getExpectedActiveReceiveTracks().map(track => this.waitForRtpFlow(track)),
> +      this.getExpectedSendTracks().map(track => this.waitForRtpFlow(track))));

Did you on purpose switch the order of checking senders and then receivers?
I'm concerned that checking receivers first could slow down things (assuming the verification for sending can succeed earlier/faster).

::: dom/media/tests/mochitest/pc.js:1680
(Diff revision 9)
> +      if (localTransceivers[i].receiver.track.kind != "audio") {
> +        continue;
> +      }

Wouldn't it make sense to have the filtering for "audio" already have part of initial gathering of the |localTransceivers| and |remoteTransceivers|?

::: dom/media/tests/mochitest/templates.js:108
(Diff revision 9)
> -    pc._pc.getSenders().map(sender => checkTrackStats(pc, sender, true)),
> -    pc._pc.getReceivers().map(receiver => checkTrackStats(pc, receiver, false))));
> +    pc.getExpectedActiveReceiveTracks().map(track => checkTrackStats(pc, track, false)),
> +    pc.getExpectedSendTracks().map(track => checkTrackStats(pc, track, true))));

Again: any reason to switch the receiver vs sender order here?

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:165
(Diff revision 9)
> -        var track = stream.getVideoTracks()[0];
> +          .filter(transceiver => {
> +            return !transceiver.stopped &&
> +                   transceiver.receiver.track.kind == "video" &&
> +                   transceiver.sender.track;
> +          })
> +          .forEach(transceiver => {

We probably want some form of verification that the filter at least returns one transceiver here.

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:176
(Diff revision 9)
> -        } catch (e) {
> +            } catch (e) {
> -          is(e.name, "InvalidParameterError",
> +              is(e.name, "InvalidAccessError",
> -             "addTrack existing track should fail");
> +                 "addTrack existing track should fail");
> -        }
> +            }
> -        try {
> +            try {
> -          test.pcLocal._pc.addTrack(track, stream);
> +              test.pcLocal._pc.addTrack(track, stream);

Not sure why we try to add this twice... but what ever :)

::: dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:75
(Diff revision 9)
> +    await v2loadedmetadata;
> +
> +    await waitUntil(() => v2.currentTime > 0 && v2.srcObject.currentTime > 0);
> +    ok(v2.currentTime > 0, "v2.currentTime is moving (" + v2.currentTime + ")");
> +
> +    await wait(3000); // TODO: Bug 1248154

Bug 1248154 landed in Firefox 48.

::: dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:79
(Diff revision 9)
> +
> +    await wait(3000); // TODO: Bug 1248154
> +
> -        ok(v1.videoWidth > 0, "source width is positive");
> +    ok(v1.videoWidth > 0, "source width is positive");
> -        ok(v1.videoHeight > 0, "source height is positive");
> +    ok(v1.videoHeight > 0, "source height is positive");
> -        if (v2.videoWidth == 640 && v2.videoHeight == 480) { // TODO: Bug 1248154
> +    if (v2.videoWidth == 640 && v2.videoHeight == 480) { // TODO: Bug 1248154

Same here: bug 1248154 has landed.
Attachment #8900496 - Flags: review?(drno) → review+
Comment on attachment 8900493 [details]
Bug 1290948 - Part 4: Transceivers JSEP/SDP work. r+drno

https://reviewboard.mozilla.org/r/171866/#review182950

> So at the JSEP level, the track id for remote tracks matches what was signaled (if it was signaled). And, we do propogate this id down to JS, but not as the MediaStreamTrack.id; we instead put it in (pref-guarded) RTCRtpTransceiver.remoteTrackId. Does this answer your question?

I think I now better understand (compared to when reviewing the patch) when this gets assinged.

> RTP transceivers use the JS direction the caller specified. The logic that creates transceivers due to seeing a new m-section in a remote description uses recvonly, which is wrong for m=application. This just makes sure nothing makes that mistake. I could move this into the caller, but I would end up wanting to leave an assert here, which is why I didn't do that.

Let's add a comment then here explaining why this happens here to asymmetrically.

> Hmm. So we need to keep the mid the same if the transceiver is stopped/disassociated though. Maybe what we ought to do here is:
> 
> 1. If there was a previous offer/answer, use the mid from the previous m-section, and assert it matches the transceiver (if it has one).
> 2. If there was not a previous offer/answer, use the mid from the transceiver if it has one, and make one up otherwise.
> 
> Does that sound sane to you?

Yep, that sounds like what we need to do here.
(In reply to Rick Waldron [:rwaldron] from comment #112)
> Just wanted to follow up: are the web-platform-tests/webrtc test issues
> resolved?

Can you please be more specific? What issues are you talking about?

> Are those tests currently being used to check spec conformance?

Not really, because quite frankly the WebRTC web platform tests are in pretty bad shape. So the success or failure of the WebRTC web platform tests currently does not correctly tell someone if a given browser is spec compliant or not.
Comment on attachment 8900494 [details]
Bug 1290948 - Part 5: TransceiverImpl and some major refactoring. r+drno

https://reviewboard.mozilla.org/r/171868/#review187400

> Looks like this comment no longer applies?

It still applies, although your question caused me to find a bug. Once this is all done, _only_ MediaPipelineReceive will handle incoming packets, including receiver reports. In this patchset, both are handling incoming RTCP, which was not the intent. I also needed to modify MediaPipelineFilter::FilterSenderReport to _only_ filter out sender reports (it was always dropping receiver reports, which meant that when bundle was being used, only the MediaPipelineTransmit was handling them, but when bundle was _not_ in use, the MediaPipelineReceive would handle them _also_).

> Since |listener_| can now be a nullptr when constructing, is this guaranteed to be called only when |listener_| is set?

The unit-tests allow this to be null, and DetachMedia also nulls it out. I'm just going to check everywhere for this.

> Could the media type here also be kApplication?

No. Will rename function to be more clear.

> Not really related to this patch set, but this function return NS_OK and does not even touch |outToneBuffer| when it fails to locate the track? That looks like a bug to me.

This is what underpins the |toneBuffer| attribute on RTCDTMFSender, which is expected to simply be empty if no DTMF stuff is being sent. So, we don't want to throw in that case.

> Does this DTMF stuff also need to get moved over to the Transceivers? If so, put the same comment here as above with the bug number.

Yes. I will put this everywhere we use mDTMFStates.

> We don't need to do anything with mReceiveStream here?

It couldn't hurt to null it out I guess.

> s/UpdateConduit/__FUNCTION__

I'm just going to remove this. It isn't super useful.

> The other log message don't start with |this|. Is there a special reason this one is different?
> I would prefer to have them consistent. Either all logging |this| or without, no strong preference from my side.

I've removed this anyhow.

> Lets add MOZ_MTLOG() here before returning, just in case.

I'm removing the entry logging, so we don't really need this.

> Is there a valid use case for this, or does this indicate a bug of some kind where we should abort?

The spec explicitly allows using a=sendxxxx when you don't actually have a local track. I'm not sure whether it is appropriate to send RTCP SR in this case though, so I've plumbed this weird case all the way down.

> Is this intentionally not exiting after the first match?
> Or in other words: does it work if an audio conduit gets synced to multiple video conduits?

This was implemented to match our previous behavior, so in that respect it was intentional, but I am not sure the previous code will do something sane.

> I would have expected to see false here.
> Or is that an intentional feature to say claim that HasSendTrack(nullptr) returns that the transceiver has AN (as in any) sending track?

Yes, that's the expected behavior.

> Same question here: is this a special if queried with null it tells you if any receive track is set feature?

Yes.

> This will break if you/we turn on RED support, because then we will have three config values.
> I fear we need to iterate over the config values here.
> Can be done in new bug since RED support is not on by default yet.

Bug filed.

> I think soon nobody will be able to link "Issue 165" to this https://github.com/unicorn-wg/gecko-dev/issues/165 any more. Let's either remove the comment, or replace it with a proper reference to a bugzilla entry.

Filed and updated comment.
Comment on attachment 8900491 [details]
Bug 1290948 - Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan

https://reviewboard.mozilla.org/r/171862/#review184198

> These two appear unused from c++ as well.

JS needs these, unless I slap an _innerObject on the webidl objects, but that's bad for encapsulation.
Comment on attachment 8900491 [details]
Bug 1290948 - Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan

https://reviewboard.mozilla.org/r/171862/#review180646

> This change was merged here, the draft just hasn't been updated yet:
> 
> https://github.com/w3c/webrtc-pc/pull/1567

Aaaand that change was dropped from the spec.
Comment on attachment 8900490 [details]
Bug 1290948 - Part 1: Mochitest for transceivers. r+jib

https://reviewboard.mozilla.org/r/171860/#review182052

> Have you implemented firing of the removetrack event on the stream, and the muted event on the removed track? [1][3]
> 
> If so, we could test that they're fired here.
> 
> If not, we should do so, or file a bug to do so, as we'd want to do that in the same release, to follow the spec, and to give people a way to transition.
> 
> Lastly, do we want to complete the circle with one more round here, setting direction back with either:
> 
>     pc2.getTransceivers()[0].setDirection("sendrecv");
> 
> or maybe:
> 
>     pc1.getTransceivers()[0].setDirection("sendrecv");
> 
> and renegotiate again? (That would let us test for the addtrack event as well.)
> 
> [1] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#process-remote-track-removal
> [2] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#process-remote-track-addition
> 
> [2] I guess I'm only asking about the add/removetrack events until Bug 1208378 comment 1 is implemented.

I've added some addtrack testing, but not any removetrack/muted stuff since that wasn't implemented to begin with. Let's do that separately.
Blocks: 1403238
Comment on attachment 8900496 [details]
Bug 1290948 - Part 7: Update existing mochitests to track the spec fixes in previous parts. r+drno

https://reviewboard.mozilla.org/r/171872/#review188500

> Is this on purpose calling attachLocalStream()?
> From the name of the function I would have expected that this calls attachLocalTrack() instead. Plus this function appears to be identical to getAllUserMediaAndAddTransceivers() below.

It is calling attachLocalStream on purpose, yes. I have fixed the transceivers version though.

> Did you on purpose switch the order of checking senders and then receivers?
> I'm concerned that checking receivers first could slow down things (assuming the verification for sending can succeed earlier/faster).

This is just elements in a Promise.all, so they run in parallel. Can't recall why the order switched, but it should not matter.

> Wouldn't it make sense to have the filtering for "audio" already have part of initial gathering of the |localTransceivers| and |remoteTransceivers|?

Yeah, and I need to be checking that the mid matches too.

> Again: any reason to switch the receiver vs sender order here?

It is a Promise.all, so shouldn't matter.

> Not sure why we try to add this twice... but what ever :)

Merge badness I guess? I've removed the dupe.

> Bug 1248154 landed in Firefox 48.

I'll see what happens if I remove this, but I am not optimistic...

> Same here: bug 1248154 has landed.

Same response here.
First pass is done. Every patch has at least a little work in it, please look at interdiff and review replies. I will be rebasing in the meantime, and pushing that separately.
Flags: needinfo?(jib)
Flags: needinfo?(drno)
Comment on attachment 8900494 [details]
Bug 1290948 - Part 5: TransceiverImpl and some major refactoring. r+drno

https://reviewboard.mozilla.org/r/171868/#review190602


C/C++ Static analysis found 23 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:661
(Diff revision 11)
>                      filter),
>                  NS_DISPATCH_NORMAL);
>  }
>  
>  void
> -MediaPipeline::UpdateTransport_s(int level,
> +MediaPipeline::UpdateTransport_s(RefPtr<TransportFlow> rtp_transport,

Warning: The parameter 'rtp_transport' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1399
(Diff revision 11)
>      const std::string& pc,
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> +    bool is_video,
>      dom::MediaStreamTrack* domtrack,
> -    const std::string& track_id,
> +    RefPtr<MediaSessionConduit> conduit) :

Warning: The parameter 'conduit' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1400
(Diff revision 11)
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> +    bool is_video,
>      dom::MediaStreamTrack* domtrack,
> -    const std::string& track_id,
> -    int level,
> +    RefPtr<MediaSessionConduit> conduit) :
> +  MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit),

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit),
                              ^
                              std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1400
(Diff revision 11)
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> +    bool is_video,
>      dom::MediaStreamTrack* domtrack,
> -    const std::string& track_id,
> -    int level,
> +    RefPtr<MediaSessionConduit> conduit) :
> +  MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit),

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit),
                                           ^
                                           std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1983
(Diff revision 11)
>  MediaPipelineReceive::MediaPipelineReceive(
>      const std::string& pc,
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> -    SourceMediaStream *stream,
> -    const std::string& track_id,
> +    RefPtr<MediaSessionConduit> conduit) :
> +  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
                             ^
                             std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1983
(Diff revision 11)
>  MediaPipelineReceive::MediaPipelineReceive(
>      const std::string& pc,
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> -    SourceMediaStream *stream,
> -    const std::string& track_id,
> +    RefPtr<MediaSessionConduit> conduit) :
> +  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
                                          ^
                                          std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1983
(Diff revision 11)
>  MediaPipelineReceive::MediaPipelineReceive(
>      const std::string& pc,
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> -    SourceMediaStream *stream,
> -    const std::string& track_id,
> +    RefPtr<MediaSessionConduit> conduit) :
> +  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),

Warning: Parameter 'conduit' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
                                                      ^
                                                      std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2117
(Diff revision 11)
> -    const std::string& media_stream_track_id,
> -    TrackID numeric_track_id,
> -    int level,
>      RefPtr<AudioSessionConduit> conduit,
> -    RefPtr<TransportFlow> rtp_transport,
> -    RefPtr<TransportFlow> rtcp_transport,
> +    SourceMediaStream* aStream) :
> +  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
                           ^
                           std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2117
(Diff revision 11)
> -    const std::string& media_stream_track_id,
> -    TrackID numeric_track_id,
> -    int level,
>      RefPtr<AudioSessionConduit> conduit,
> -    RefPtr<TransportFlow> rtp_transport,
> -    RefPtr<TransportFlow> rtcp_transport,
> +    SourceMediaStream* aStream) :
> +  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
                                        ^
                                        std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2292
(Diff revision 11)
> -    const std::string& media_stream_track_id,
> -    TrackID numeric_track_id,
> -    int level,
>      RefPtr<VideoSessionConduit> conduit,
> -    RefPtr<TransportFlow> rtp_transport,
> -    RefPtr<TransportFlow> rtcp_transport,
> +    SourceMediaStream* aStream) :
> +  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
                           ^
                           std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2292
(Diff revision 11)
> -    const std::string& media_stream_track_id,
> -    TrackID numeric_track_id,
> -    int level,
>      RefPtr<VideoSessionConduit> conduit,
> -    RefPtr<TransportFlow> rtp_transport,
> -    RefPtr<TransportFlow> rtcp_transport,
> +    SourceMediaStream* aStream) :
> +  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
                                        ^
                                        std::move()


::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1273
(Diff revision 11)
> +  }
> +
> +  RefPtr<JsepTransceiver> jsepTransceiver = new JsepTransceiver(type);
> +
> +  RefPtr<TransceiverImpl> transceiverImpl =
> +    CreateTransceiverImpl(jsepTransceiver, aSendTrack, jrv);

Warning: Parameter 'asendtrack' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3691
(Diff revision 11)
>      NS_WARNING("Failed to dispatch the RTCDTMFToneChange event!");
>      return;
>    }
>  }
>  
> +PeerConnectionImpl::DTMFState::DTMFState()

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3694
(Diff revision 11)
>  }
>  
> +PeerConnectionImpl::DTMFState::DTMFState()
> +{}
> +
> +PeerConnectionImpl::DTMFState::~DTMFState()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:530
(Diff revision 11)
> +
> +// Accessing the PCMedia should be safe here because we shouldn't
> +// have enqueued this function unless it was still active and
> +// the ICE data is destroyed on the STS.
> +static void
> +FinalizeTransportFlow_s(RefPtr<PeerConnectionMedia> aPCMedia,

Warning: The parameter 'apcmedia' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

FinalizeTransportFlow_s(RefPtr<PeerConnectionMedia> aPCMedia,
                                                    ^
                        const                      &

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:531
(Diff revision 11)
> +// Accessing the PCMedia should be safe here because we shouldn't
> +// have enqueued this function unless it was still active and
> +// the ICE data is destroyed on the STS.
> +static void
> +FinalizeTransportFlow_s(RefPtr<PeerConnectionMedia> aPCMedia,
> +                        RefPtr<TransportFlow> aFlow, size_t aLevel,

Warning: The parameter 'aflow' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                        RefPtr<TransportFlow> aFlow, size_t aLevel,
                                              ^
                        const                &

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:533
(Diff revision 11)
> +// the ICE data is destroyed on the STS.
> +static void
> +FinalizeTransportFlow_s(RefPtr<PeerConnectionMedia> aPCMedia,
> +                        RefPtr<TransportFlow> aFlow, size_t aLevel,
> +                        bool aIsRtcp,
> +                        nsAutoPtr<PtrVector<TransportLayer> > aLayerList)

Warning: The parameter 'alayerlist' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                        nsAutoPtr<PtrVector<TransportLayer> > aLayerList)
                                                              ^
                        const                                &

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:549
(Diff revision 11)
> +  aLayerList->values.clear();
> +  (void)aFlow->PushLayers(layerQueue); // TODO(bug 854518): Process errors.
> +}
> +
> +static void
> +AddNewIceStreamForRestart_s(RefPtr<PeerConnectionMedia> aPCMedia,

Warning: The parameter 'apcmedia' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

AddNewIceStreamForRestart_s(RefPtr<PeerConnectionMedia> aPCMedia,
                                                        ^
                            const                      &

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:550
(Diff revision 11)
> +  (void)aFlow->PushLayers(layerQueue); // TODO(bug 854518): Process errors.
> +}
> +
> +static void
> +AddNewIceStreamForRestart_s(RefPtr<PeerConnectionMedia> aPCMedia,
> +                            RefPtr<TransportFlow> aFlow,

Warning: The parameter 'aflow' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                            RefPtr<TransportFlow> aFlow,
                                                  ^
                            const                &

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:1054
(Diff revision 11)
>    if (mProxyRequest) {
>      mProxyRequest->Cancel(NS_ERROR_ABORT);
>      mProxyRequest = nullptr;
>    }
>  
> +  for (auto transceiver : mTransceivers) {

Warning: Loop variable is copied but only used as const reference; consider making it a const reference [clang-tidy: performance-for-range-copy]

  for (auto transceiver : mTransceivers) {
            ^
       const  &

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:1127
(Diff revision 11)
> -  if(aIndex < 0 || aIndex >= (int) mLocalSourceStreams.Length()) {
> -    return nullptr;
> +    RefPtr<dom::MediaStreamTrack>& aSendTrack,
> +    RefPtr<TransceiverImpl>* aTransceiverImpl)
> +{
> +  RefPtr<TransceiverImpl> transceiver = new TransceiverImpl(
> +      mParent->GetHandle(),
> +      aJsepTransceiver,

Warning: Parameter 'ajseptransceiver' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

      aJsepTransceiver,
      ^
      std::move(      )

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:44
(Diff revision 11)
> +    nsIEventTarget* aStsThread,
> +    OwningNonNull<DOMMediaStream>& aReceiveStream,
> +    RefPtr<dom::MediaStreamTrack>& aSendTrack,
> +    RefPtr<WebRtcCallWrapper>& aCallWrapper) :
> +  mPCHandle(aPCHandle),
> +  mJsepTransceiver(aJsepTransceiver),

Warning: Parameter 'ajseptransceiver' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:125
(Diff revision 11)
> +  mConduit = nullptr;
> +  RUN_ON_THREAD(mStsThread, WrapRelease(mRtpFlow.forget()), NS_DISPATCH_NORMAL);
> +  RUN_ON_THREAD(mStsThread, WrapRelease(mRtcpFlow.forget()), NS_DISPATCH_NORMAL);
> +}
> +
> +TransceiverImpl::~TransceiverImpl()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]
Comment on attachment 8900494 [details]
Bug 1290948 - Part 5: TransceiverImpl and some major refactoring. r+drno

https://reviewboard.mozilla.org/r/171868/#review190602

I am dropping all of these that were pre-existing, but I've fixed the new stuff.
Blocks: 1397881
Comment on attachment 8913686 [details]
Bug 1290948 - Part 8: Don't cause ICE to fail if there's no streams to establish. r+drno

https://reviewboard.mozilla.org/r/185086/#review191734
Attachment #8913686 - Flags: review?(drno) → review+
Comment on attachment 8900494 [details]
Bug 1290948 - Part 5: TransceiverImpl and some major refactoring. r+drno

https://reviewboard.mozilla.org/r/171868/#review191736

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1967
(Diff revisions 9 - 11)
> -    // transceivers with ours.
> +    pco->SyncTransceivers(jrv);
> -    pco->OnSetRemoteDescriptionSuccess(jrv);
>  
> -    nrv = FireOnTrackEvents(pco);
> -    if (NS_FAILED(nrv)) {
> -      // aPco was already notified, just return early.
> +    FireOnTrackEvents(pco);
> +
> +    pco->OnSetRemoteDescriptionSuccess(jrv);

Is it really okay to return NS_OK below if any of these two call fail?
I would assume we would want to check jrv after each of these.

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:349
(Diff revisions 9 - 11)
> -  MOZ_MTLOG(ML_DEBUG, "Syncing with JS transceiver");
> +  MOZ_MTLOG(ML_DEBUG, mPCHandle << "[" << mMid << "]: " << __FUNCTION__
> +                      << " Syncing with JS transceiver");
>  
>    // Update stopped, both ways, since either JSEP or JS can stop these
>    if (mJsepTransceiver->IsStopped()) {
>      // We don't call Stop(), because that causes another sync

This comment appears to be outdated now.

::: media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp:106
(Diff revision 11)
>    }
>  
>    uint8_t payload_type = data[PT_OFFSET];
>  
>    if (payload_type != SENDER_REPORT_T) {
> -    return false;
> +    // Not a sender report, let it through

Uhh... good catch.
Lets see how that changes things.
Attachment #8900494 - Flags: review?(drno) → review+
Comment on attachment 8900493 [details]
Bug 1290948 - Part 4: Transceivers JSEP/SDP work. r+drno

https://reviewboard.mozilla.org/r/171866/#review192410

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:576
(Diff revisions 7 - 9)
> -  transceiver.mReceiving.AddToAnswer(remoteMsection, mSsrcGenerator, &msection);
> +  transceiver.mRecvTrack.AddToAnswer(remoteMsection, mSsrcGenerator, &msection);
>  
>    // Add extmap attributes. This logic will probably be moved to the track,
>    // since it can be specified on a per-sender basis in JS.
> +  // We will need some validation to ensure that the ids are identical for
> +  // RTP streams that are bundled together, though.

Lets put bug 1406529 here.
Attachment #8900493 - Flags: review?(drno) → review+
Blocks: 1404686
Comment on attachment 8900496 [details]
Bug 1290948 - Part 7: Update existing mochitests to track the spec fixes in previous parts. r+drno

https://reviewboard.mozilla.org/r/171872/#review188500

> I'll see what happens if I remove this, but I am not optimistic...

Ok, removing it seems to work fine.
Comment on attachment 8900494 [details]
Bug 1290948 - Part 5: TransceiverImpl and some major refactoring. r+drno

https://reviewboard.mozilla.org/r/171868/#review191736

> Is it really okay to return NS_OK below if any of these two call fail?
> I would assume we would want to check jrv after each of these.

We were already returning NS_OK in these cases. The only places either of these fail is if a callback to JS fails for some reason, and I don't think it makes sense to throw an internal error if content JS throws in the callback or something.

> This comment appears to be outdated now.

This comment refers to RTCRtpTransceiver::Stop(), have updated the comment to be more clear.
Comment on attachment 8900490 [details]
Bug 1290948 - Part 1: Mochitest for transceivers. r+jib

https://reviewboard.mozilla.org/r/171860/#review194566

Fab. We should nominate this test for web-platform-tests.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:23
(Diff revisions 5 - 7)
> +    } catch (e) {
> +      is(e.name, exceptionName, description + " throws " + exceptionName);
> +    }
> +  };
> +
> +  let stopTracks = streams => {

Nit: we could write:

    let stopTracks = (...streams) => {

...which would let us call like this:

    stopTracks(audioStream, videoStream);

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:29
(Diff revisions 5 - 7)
> +    let oldOnTrack = pc.ontrack;
> +    pc.ontrack = e => trackEvents.push(e);

Seems cleaner to avoid ontrack here than worry about oldOnTrack. e.g.:

    let listener = e => trackEvents.push(e);

    pc.addEventListener("track", listener);
    await pc.setRemoteDescription(desc);
    pc.removeEventListener("track", listener);

Then readers don't have to wonder whether temporarily blocking firing on oldOnTrack is a feature.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:35
(Diff revisions 5 - 7)
> +    pc.ontrack = e => trackEvents.push(e);
> +    await pc.setRemoteDescription(desc);
> +    pc.ontrack = oldOnTrack;
> +
> +    // basic sanity-check, simplifies testing elsewhere
> +    trackEvents.forEach(e => {

Nit: I recommend avoiding forEach inside async functions in general, due to the maintenance risk of someone later adding an await statement inside the loop, and expecting it to work right.

The following is only two characters more:

    for (let e of trackEvents) {

Applies throughout.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:47
(Diff revisions 5 - 7)
> +    });
> +
> +    return trackEvents;
> +  };
> +
> +  let trickle = (pc1, pc2, stopBuffering) => {

stopBuffering is unused. Remove.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:49
(Diff revisions 5 - 7)
> +    return trackEvents;
> +  };
> +
> +  let trickle = (pc1, pc2, stopBuffering) => {
> +    pc1.onicecandidate = async e => {
> +      await stopBuffering;

stopBuffering will always be `undefined` here AFAICT.

Still, this `await` statement causes the rest of this function to happen on the microtask queue (actually on the next task until Firefox fixes its microtask queues).

Specifically, this means that

    pc2.addIceCandidate(e.candidate);

...will be called *after* this function returns. This seems uncommon. Was the intention here to test something uncommon?

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:51
(Diff revisions 5 - 7)
> +
> +  let trickle = (pc1, pc2, stopBuffering) => {
> +    pc1.onicecandidate = async e => {
> +      await stopBuffering;
> +      info("Adding ICE candidate: " + JSON.stringify(e.candidate));
> +      pc2.addIceCandidate(e.candidate);

Technically, we should use `await` here to catch any errors from `addIceCandidate()`, and

    try{
      ...
    } catch(e){
      is(e, null, "got error"); 
    }

...around all this code since `pc1.onicecandidate` ignores the return values and thus any promise errors from this async function.

That said, I confess to skimping when ICE isn't the main thing being tested, like e.g.

    pc1.onicecandidate = e => pc2.addIceCandidate(e.candidate);

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:836
(Diff revisions 5 - 7)
>  
> -    dictCompare(pc2.getTransceivers(),
> +    hasProps(pc2.getTransceivers(),
>        [
>          {
>            sender: {track: {kind: "audio"}},
>            receiver: {track: {kind: "audio"}},

Maybe also check track's `readyState: "ended"` and that track.onended fired?

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:844
(Diff revisions 5 - 7)
>            currentDirection: null,
>            direction: "sendrecv"
>          }
>        ]);
>  
>      pc1.close();

Maybe check that calling .stop() again doesn't throw, neither before nor after close?

And finally, maybe check that transceiver.stop() throws InvalidStateError on a closed pc3 whose transceiver has not been stopped? (odd but that's what the spec says).

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:1377
(Diff revisions 5 - 7)
> +      ]);
> +    hasProps(pc1.getTransceivers(),
> +      [
> +        {
> +          receiver: {track: {kind: "audio"}},
> +          sender: {track: track},

{track}

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:1632
(Diff revisions 5 - 7)
> -    await checkCurrentDirection(options);
> -    await checkAddTransceiverNoTrackDoesntPair(options);
> -    await checkAddTransceiverWithTrackDoesntPair(options);
> +    await checkSetDirection();
> +    await checkCurrentDirection();
> +    await checkSendrecvWithNoSendTrack();

Nit: these three are out of order.
Attachment #8900490 - Flags: review?(jib) → review+
Comment on attachment 8900490 [details]
Bug 1290948 - Part 1: Mochitest for transceivers. r+jib

https://reviewboard.mozilla.org/r/171860/#review194566

> Maybe also check track's `readyState: "ended"` and that track.onended fired?

So the stopped transceiver's (ie; pc1's first transceiver) receive track doesn't have a readyState here. Is that a bug? Seems weird to me...

> Nit: these three are out of order.

I don't understand what you mean here. checkCurrentDirection and checkSendrecvWithNoSendTrack use setDirection, which means it would be nice if checkSetDirection came first.
Comment on attachment 8900490 [details]
Bug 1290948 - Part 1: Mochitest for transceivers. r+jib

https://reviewboard.mozilla.org/r/171860/#review194566

> So the stopped transceiver's (ie; pc1's first transceiver) receive track doesn't have a readyState here. Is that a bug? Seems weird to me...

Huh, weird. Maybe I typoed it, because now it works.
Comment on attachment 8900492 [details]
Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver.

https://reviewboard.mozilla.org/r/171864/#review184150

> This should be equivalent I think; let's say this is not the first datachannel created. If there is no m=application section negotiated yet, that implies we have already called updateNegotiationNeeded, so this call won't have any effect. If there is an m=application section negotiated, this check will not have any effect either.
> 
> I queued this because step 19 says to run the following steps (including update the negotiation-needed step) in the background. Is there another way I should be doing this?

On the second part, with https://github.com/w3c/webrtc-pc/pull/1614 the spec no longer says to do this in parallel, so we should remove the queuing.

On the first part, are you saying the prose "If channel is the first RTCDataChannel created" in the spec is effectively redundant?

If yes, then good. If no, then please add a comment about how our implementation deviates from the spec in a way that upholds this.

> updateNegotiationNeeded never causes something to fire synchronously. It is exactly the same as the "update the negotiation-needed flag" algorithm in the spec, except it will throw if the pc is closed (which you noted elsewhere, and which I will fix).
> 
> The business about (duplicate) checking the signaling state before running that algorithm, and then having a (duplicate) event dispatch, strikes me as a spec bug. Step 10 should probably just be:
> 
> "10. Update the negotiation-needed flag on connection."

Ah, I missed that the inner algorithm queues also.

The spec isn't trying to fire a duplicate event here, rather it's circumventing the following line inside update-the-negotiation-needed-flag:

    4. If connection's [[NegotiationNeeded]] slot is already true, abort these steps.

We have have the same check:

    if (this._negotiationNeeded) {
      return;
    }

but I see we circumvent it using this._dompc.clearNegotiationNeeded(); instead. I suppose the spec could be simplified the same way.

But (A) is still a problem. This patch looks like it's firing negotiationneeded events indiscriminately in response to SLD and SRD regardless of state, when step 10 says specifically to only fire if in "stable" state.

> I have not seen anything in the spec that indicates "-" as a stream id is interpreted by the other side as "no stream". I interpreted this as saying if the local side doesn't specify a stream, just pretend there is one with a placeholder. These ids are opaque unless there is spec language saying otherwise.
> 
> Basically, if this happens, the c++ code has a bug.

Sorry, not following what you mean with "opaque".

Are you saying the remote side should make up a stream with a random id in this case? Do you have support for this?

From my reading, "-" is JSEP's encoding for "no MediaStream". I naively read the intent here to be to mirror this remotely.

I find support in http://w3c.github.io/webrtc-pc/#process-remote-track-addition which says:

"Set the associated remote streams given receiver and a list of the MSIDs that the media description indicates track is to be *associated with*."

The key word here being "associated with", which matches the JSEP terminology when it effectively defines "-" to mean "no MediaStream is associated with the transceiver". To me, that means "-" means no mediastream is associated with this received track.

That's the only way I make sense of http://w3c.github.io/webrtc-pc/#set-associated-remote-streams which says:

"For each MSID in msids, unless a MediaStream object has previously been created with that id for this connection, create a MediaStream object with that id."

Basically it says MSID and stream.id are 1-1. I see only two readings of this:

    A) "list of MSIDs" excludes "-"
    B) spec says to create a stream where stream.id == "-".

B seems undesirable.
Clearing NI (as I think I'm done with my reviews)
Flags: needinfo?(drno)
Comment on attachment 8900492 [details]
Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver.

https://reviewboard.mozilla.org/r/171864/#review184150

> Ah, I missed that the inner algorithm queues also.
> 
> The spec isn't trying to fire a duplicate event here, rather it's circumventing the following line inside update-the-negotiation-needed-flag:
> 
>     4. If connection's [[NegotiationNeeded]] slot is already true, abort these steps.
> 
> We have have the same check:
> 
>     if (this._negotiationNeeded) {
>       return;
>     }
> 
> but I see we circumvent it using this._dompc.clearNegotiationNeeded(); instead. I suppose the spec could be simplified the same way.
> 
> But (A) is still a problem. This patch looks like it's firing negotiationneeded events indiscriminately in response to SLD and SRD regardless of state, when step 10 says specifically to only fire if in "stable" state.

Ah, nevermind. I missed that the update-negotiation-needed algorithm itself checks for "stable", so this is good. I agree we should simplify step 10 in the spec!
Comment on attachment 8900492 [details]
Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver.

https://reviewboard.mozilla.org/r/171864/#review196126

Would like to see it one more time.

::: dom/media/PeerConnection.js:785
(Diff revisions 5 - 7)
> -    // This entry-point handles both new and legacy call sig. Decipher which one
> +      // This entry-point handles both new and legacy call sig. Decipher which one
> -    if (typeof optionsOrOnSucc == "function") {
> +      return this._legacy(onSuccess, onErr, () => this._createOffer(options));

fix comment

::: dom/media/PeerConnection.js:798
(Diff revisions 5 - 7)
> +    if (transceiver) {
> +      if (transceiver.direction == "sendonly") {
> -      transceiver.setDirection("sendrecv");
> +        transceiver.setDirection("sendrecv");
> -      return true;
> -    } else if (transceiver.direction == "inactive") {
> +      } else if (transceiver.direction == "inactive") {

This goes beyond the spec-requirement [1]. Is this needed for Firefox legacy?

Also, calling setDirection() inside createOffer potentially does a lot of things that seem wrong, like firing negotiationneeded, which the spec says explicitly not to do, or throwing InvalidStateError if transceiver is stopped.

[1] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dom-rtcofferoptions-offertoreceiveaudio

::: dom/media/PeerConnection.js:807
(Diff revisions 5 - 7)
> -        count--;
> -      }
> +    }
> -    });
>  
> -    while (count) {
> -      this._addTransceiverNoEvents(kind, {direction: "recvonly"});
> +    this._addTransceiverNoEvents(kind, {direction: "recvonly"});

"recvonly" here seems right based on current behavior [1] but isn't in spec. Should we file an issue?

[1] https://jsfiddle.net/c48q11jd/

::: dom/media/PeerConnection.js:812
(Diff revisions 5 - 7)
> -    }
>    }
>  
> -  // Handles legacy offerToReceiveAudio/Video
> +  // Handles offerToReceiveAudio/Video
>    _ensureTransceiversForOfferToReceive(options) {
>      if (options.offerToReceiveVideo) {

Speaking of Firefox legacy, today users can use {offerToReceiveVideo: false} to set sendonly or even inactive [1]. This breaks that. Are we ok with this? Should we warn people who do that?

Also, we've been fairly aggressive with deprecation warnings so far. Should we perhaps warn on offerToReceive?

[1] https://jsfiddle.net/c48q11jd/

::: dom/media/PeerConnection.js:1141
(Diff revisions 5 - 7)
> +        transceiver.setDirection("sendrecv");
> +      } else if (transceiver.direction == "inactive") {
> +        transceiver.setDirection("sendonly");

The spec says to set the [[Direction]] internal slot here, not call the setDirection algorithm.

Since we know the transceiver isn't stopped, this would appear to not have any side-effects, but seems inefficient, since it means

    transceiver.sync();
    this.updateNegotiationNeeded();

effectively get called twice, and before

    transceiver.setAddTrackMagic();

which I can't tell whether matters or not.

::: dom/media/PeerConnection.js:1146
(Diff revisions 5 - 7)
> -      transceiver = this._addTransceiverNoEvents(
> +      transceiver = this._addTransceiverNoEvents(track, {
> -          track,
> -          {
>              streams: [stream],
>              direction: "sendrecv"
> -          });
> +      });

Nit: deep indent

::: dom/media/PeerConnection.js:1176
(Diff revisions 5 - 7)
> +      transceiver.setDirection("recvonly");
> +    } else if (transceiver.direction == "sendonly") {
> +      transceiver.setDirection("inactive");
> +    }
> +
> -      transceiver.sync();
> +    transceiver.sync();
> -      this.updateNegotiationNeeded();
> +    this.updateNegotiationNeeded();

same thing here

::: dom/media/PeerConnection.js:1875
(Diff revisions 5 - 7)
> -    // TODO (bug XXXXX): Technically, if the transceiver is not yet associated,
> -    // we're supposed to synchronously set the track and return a resolved
> -    // promise. However, PeerConnectionImpl::ReplaceTrackNoNegotiation does
> -    // stuff like updating principal change observers. We might want to set
> +    // Updates the track on the MediaPipeline; this is needed whether or not
> +    // we've associated this transceiver, the spec language notwithstanding.
> +    // Synchronous, and will throw on failure.
> +    this._pc._replaceTrack(this._transceiverImpl, withTrack);

I agree the spec is a little weird here. Does this synchronous call do anything that is JS-observable? If so, then that might be a problem in the case where the transceiver is associated.
Attachment #8900492 - Flags: review?(jib) → review-
Comment on attachment 8900492 [details]
Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver.

https://reviewboard.mozilla.org/r/171864/#review196126

> This goes beyond the spec-requirement [1]. Is this needed for Firefox legacy?
> 
> Also, calling setDirection() inside createOffer potentially does a lot of things that seem wrong, like firing negotiationneeded, which the spec says explicitly not to do, or throwing InvalidStateError if transceiver is stopped.
> 
> [1] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dom-rtcofferoptions-offertoreceiveaudio

This is basically what we did before (ie; we would set the recv bit on pre-existing m-sections), but yeah we probably should change this since the spec language says something completely different.

> "recvonly" here seems right based on current behavior [1] but isn't in spec. Should we file an issue?
> 
> [1] https://jsfiddle.net/c48q11jd/

Yeah, it would be kinda weird to create a sendrecv transceiver in this case.

> Speaking of Firefox legacy, today users can use {offerToReceiveVideo: false} to set sendonly or even inactive [1]. This breaks that. Are we ok with this? Should we warn people who do that?
> 
> Also, we've been fairly aggressive with deprecation warnings so far. Should we perhaps warn on offerToReceive?
> 
> [1] https://jsfiddle.net/c48q11jd/

Hmm. I wonder how many people use {offerToReceiveX: false}. We should probably fire warnings when someone does that.

> The spec says to set the [[Direction]] internal slot here, not call the setDirection algorithm.
> 
> Since we know the transceiver isn't stopped, this would appear to not have any side-effects, but seems inefficient, since it means
> 
>     transceiver.sync();
>     this.updateNegotiationNeeded();
> 
> effectively get called twice, and before
> 
>     transceiver.setAddTrackMagic();
> 
> which I can't tell whether matters or not.

Fair enough, I can set directly.

> I agree the spec is a little weird here. Does this synchronous call do anything that is JS-observable? If so, then that might be a problem in the case where the transceiver is associated.

I don't think it does anything that will get back to JS. There are no callbacks, and the modification to the JS objects only occurs when the sync happens.
Comment on attachment 8900492 [details]
Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver.

https://reviewboard.mozilla.org/r/171864/#review184150

> On the second part, with https://github.com/w3c/webrtc-pc/pull/1614 the spec no longer says to do this in parallel, so we should remove the queuing.
> 
> On the first part, are you saying the prose "If channel is the first RTCDataChannel created" in the spec is effectively redundant?
> 
> If yes, then good. If no, then please add a comment about how our implementation deviates from the spec in a way that upholds this.

The prose is fine, our underlying implementation yields the same result though. I can add a comment.

> Ah, nevermind. I missed that the update-negotiation-needed algorithm itself checks for "stable", so this is good. I agree we should simplify step 10 in the spec!

Cool.

> Sorry, not following what you mean with "opaque".
> 
> Are you saying the remote side should make up a stream with a random id in this case? Do you have support for this?
> 
> From my reading, "-" is JSEP's encoding for "no MediaStream". I naively read the intent here to be to mirror this remotely.
> 
> I find support in http://w3c.github.io/webrtc-pc/#process-remote-track-addition which says:
> 
> "Set the associated remote streams given receiver and a list of the MSIDs that the media description indicates track is to be *associated with*."
> 
> The key word here being "associated with", which matches the JSEP terminology when it effectively defines "-" to mean "no MediaStream is associated with the transceiver". To me, that means "-" means no mediastream is associated with this received track.
> 
> That's the only way I make sense of http://w3c.github.io/webrtc-pc/#set-associated-remote-streams which says:
> 
> "For each MSID in msids, unless a MediaStream object has previously been created with that id for this connection, create a MediaStream object with that id."
> 
> Basically it says MSID and stream.id are 1-1. I see only two readings of this:
> 
>     A) "list of MSIDs" excludes "-"
>     B) spec says to create a stream where stream.id == "-".
> 
> B seems undesirable.

"Opaque" here means that the other end doesn't assign any special meaning to the bits in the ID; "-" is fundamentally no different from "{some-uuid}", it's just bits. If we want it to mean something special on the receiving end, we have to spell that out explicitly in the spec. I could ask deadbeef what the intent was here.
Just to make sure we are all on the same page: the consensus today is that we will wait with landing this for 59 to get a full Nightly cycle (rather then just the remainder of the 58 Nightly cycle).
Priority: P3 → P2
^ That was a rebase only, please ignore.
Comment on attachment 8900492 [details]
Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver.

https://reviewboard.mozilla.org/r/171864/#review184150

> "Opaque" here means that the other end doesn't assign any special meaning to the bits in the ID; "-" is fundamentally no different from "{some-uuid}", it's just bits. If we want it to mean something special on the receiving end, we have to spell that out explicitly in the spec. I could ask deadbeef what the intent was here.

Ok, so the relevant spec is in draft-ietf-mmusic-msid, I'll fix this up.
Comment on attachment 8900494 [details]
Bug 1290948 - Part 5: TransceiverImpl and some major refactoring. r+drno

https://reviewboard.mozilla.org/r/171868/#review196926


C/C++ static analysis found 25 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:349
(Diff revision 12)
>      EXPECT_EQ(mozilla::kMediaConduitNoError, err);
>  
>      audio_stream_track_ = new FakeAudioStreamTrack();
>    }
>  
> -  virtual void CreatePipelines_s(bool aIsRtcpMux) {
> +  virtual void CreatePipeline(bool aIsRtcpMux) {

Warning: Prefer using 'override' or (rarely) 'final' instead of 'virtual' [clang-tidy: modernize-use-override]

  virtual void CreatePipeline(bool aIsRtcpMux) {
               ^
                                               override

::: media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:397
(Diff revision 12)
>          static_cast<mozilla::AudioSessionConduit *>(audio_conduit_.get())->
>          ConfigureRecvMediaCodecs(codecs);
>      EXPECT_EQ(mozilla::kMediaConduitNoError, err);
>    }
>  
> -  virtual void CreatePipelines_s(bool aIsRtcpMux) {
> +  virtual void CreatePipeline(bool aIsRtcpMux) {

Warning: Prefer using 'override' or (rarely) 'final' instead of 'virtual' [clang-tidy: modernize-use-override]

  virtual void CreatePipeline(bool aIsRtcpMux) {
               ^
                                               override

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:672
(Diff revision 12)
>                      filter),
>                  NS_DISPATCH_NORMAL);
>  }
>  
>  void
> -MediaPipeline::UpdateTransport_s(int level,
> +MediaPipeline::UpdateTransport_s(RefPtr<TransportFlow> rtp_transport,

Warning: The parameter 'rtp_transport' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1417
(Diff revision 12)
>      const std::string& pc,
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> +    bool is_video,
>      dom::MediaStreamTrack* domtrack,
> -    const std::string& track_id,
> +    RefPtr<MediaSessionConduit> conduit) :

Warning: The parameter 'conduit' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1418
(Diff revision 12)
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> +    bool is_video,
>      dom::MediaStreamTrack* domtrack,
> -    const std::string& track_id,
> -    int level,
> +    RefPtr<MediaSessionConduit> conduit) :
> +  MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit),

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit),
                              ^
                              std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1418
(Diff revision 12)
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> +    bool is_video,
>      dom::MediaStreamTrack* domtrack,
> -    const std::string& track_id,
> -    int level,
> +    RefPtr<MediaSessionConduit> conduit) :
> +  MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit),

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit),
                                           ^
                                           std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2010
(Diff revision 12)
>  MediaPipelineReceive::MediaPipelineReceive(
>      const std::string& pc,
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> -    SourceMediaStream *stream,
> -    const std::string& track_id,
> +    RefPtr<MediaSessionConduit> conduit) :
> +  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
                             ^
                             std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2010
(Diff revision 12)
>  MediaPipelineReceive::MediaPipelineReceive(
>      const std::string& pc,
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> -    SourceMediaStream *stream,
> -    const std::string& track_id,
> +    RefPtr<MediaSessionConduit> conduit) :
> +  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
                                          ^
                                          std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2010
(Diff revision 12)
>  MediaPipelineReceive::MediaPipelineReceive(
>      const std::string& pc,
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> -    SourceMediaStream *stream,
> -    const std::string& track_id,
> +    RefPtr<MediaSessionConduit> conduit) :
> +  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),

Warning: Parameter 'conduit' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
                                                      ^
                                                      std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2143
(Diff revision 12)
> -    const std::string& media_stream_track_id,
> -    TrackID numeric_track_id,
> -    int level,
>      RefPtr<AudioSessionConduit> conduit,
> -    RefPtr<TransportFlow> rtp_transport,
> -    RefPtr<TransportFlow> rtcp_transport,
> +    SourceMediaStream* aStream) :
> +  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
                           ^
                           std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2143
(Diff revision 12)
> -    const std::string& media_stream_track_id,
> -    TrackID numeric_track_id,
> -    int level,
>      RefPtr<AudioSessionConduit> conduit,
> -    RefPtr<TransportFlow> rtp_transport,
> -    RefPtr<TransportFlow> rtcp_transport,
> +    SourceMediaStream* aStream) :
> +  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
                                        ^
                                        std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2318
(Diff revision 12)
> -    const std::string& media_stream_track_id,
> -    TrackID numeric_track_id,
> -    int level,
>      RefPtr<VideoSessionConduit> conduit,
> -    RefPtr<TransportFlow> rtp_transport,
> -    RefPtr<TransportFlow> rtcp_transport,
> +    SourceMediaStream* aStream) :
> +  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
                           ^
                           std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2318
(Diff revision 12)
> -    const std::string& media_stream_track_id,
> -    TrackID numeric_track_id,
> -    int level,
>      RefPtr<VideoSessionConduit> conduit,
> -    RefPtr<TransportFlow> rtp_transport,
> -    RefPtr<TransportFlow> rtcp_transport,
> +    SourceMediaStream* aStream) :
> +  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
                                        ^
                                        std::move()


::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1282
(Diff revision 12)
> +  }
> +
> +  RefPtr<JsepTransceiver> jsepTransceiver = new JsepTransceiver(type);
> +
> +  RefPtr<TransceiverImpl> transceiverImpl =
> +    CreateTransceiverImpl(jsepTransceiver, aSendTrack, jrv);

Warning: Parameter 'asendtrack' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3812
(Diff revision 12)
>      NS_WARNING("Failed to dispatch the RTCDTMFToneChange event!");
>      return;
>    }
>  }
>  
> +PeerConnectionImpl::DTMFState::DTMFState()

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3815
(Diff revision 12)
>  }
>  
> +PeerConnectionImpl::DTMFState::DTMFState()
> +{}
> +
> +PeerConnectionImpl::DTMFState::~DTMFState()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:534
(Diff revision 12)
> +
> +// Accessing the PCMedia should be safe here because we shouldn't
> +// have enqueued this function unless it was still active and
> +// the ICE data is destroyed on the STS.
> +static void
> +FinalizeTransportFlow_s(RefPtr<PeerConnectionMedia> aPCMedia,

Warning: The parameter 'apcmedia' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

FinalizeTransportFlow_s(RefPtr<PeerConnectionMedia> aPCMedia,
                                                    ^
                        const                      &

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:535
(Diff revision 12)
> +// Accessing the PCMedia should be safe here because we shouldn't
> +// have enqueued this function unless it was still active and
> +// the ICE data is destroyed on the STS.
> +static void
> +FinalizeTransportFlow_s(RefPtr<PeerConnectionMedia> aPCMedia,
> +                        RefPtr<TransportFlow> aFlow, size_t aLevel,

Warning: The parameter 'aflow' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                        RefPtr<TransportFlow> aFlow, size_t aLevel,
                                              ^
                        const                &

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:537
(Diff revision 12)
> +// the ICE data is destroyed on the STS.
> +static void
> +FinalizeTransportFlow_s(RefPtr<PeerConnectionMedia> aPCMedia,
> +                        RefPtr<TransportFlow> aFlow, size_t aLevel,
> +                        bool aIsRtcp,
> +                        nsAutoPtr<PtrVector<TransportLayer> > aLayerList)

Warning: The parameter 'alayerlist' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                        nsAutoPtr<PtrVector<TransportLayer> > aLayerList)
                                                              ^
                        const                                &

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:553
(Diff revision 12)
> +  aLayerList->values.clear();
> +  (void)aFlow->PushLayers(layerQueue); // TODO(bug 854518): Process errors.
> +}
> +
> +static void
> +AddNewIceStreamForRestart_s(RefPtr<PeerConnectionMedia> aPCMedia,

Warning: The parameter 'apcmedia' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

AddNewIceStreamForRestart_s(RefPtr<PeerConnectionMedia> aPCMedia,
                                                        ^
                            const                      &

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:554
(Diff revision 12)
> +  (void)aFlow->PushLayers(layerQueue); // TODO(bug 854518): Process errors.
> +}
> +
> +static void
> +AddNewIceStreamForRestart_s(RefPtr<PeerConnectionMedia> aPCMedia,
> +                            RefPtr<TransportFlow> aFlow,

Warning: The parameter 'aflow' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                            RefPtr<TransportFlow> aFlow,
                                                  ^
                            const                &

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:1058
(Diff revision 12)
>    if (mProxyRequest) {
>      mProxyRequest->Cancel(NS_ERROR_ABORT);
>      mProxyRequest = nullptr;
>    }
>  
> +  for (auto transceiver : mTransceivers) {

Warning: Loop variable is copied but only used as const reference; consider making it a const reference [clang-tidy: performance-for-range-copy]

  for (auto transceiver : mTransceivers) {
            ^
       const  &

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:1131
(Diff revision 12)
> -  if(aIndex < 0 || aIndex >= (int) mLocalSourceStreams.Length()) {
> -    return nullptr;
> +    RefPtr<dom::MediaStreamTrack>& aSendTrack,
> +    RefPtr<TransceiverImpl>* aTransceiverImpl)
> +{
> +  RefPtr<TransceiverImpl> transceiver = new TransceiverImpl(
> +      mParent->GetHandle(),
> +      aJsepTransceiver,

Warning: Parameter 'ajseptransceiver' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

      aJsepTransceiver,
      ^
      std::move(      )

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:44
(Diff revision 12)
> +    nsIEventTarget* aStsThread,
> +    OwningNonNull<DOMMediaStream>& aReceiveStream,
> +    RefPtr<dom::MediaStreamTrack>& aSendTrack,
> +    RefPtr<WebRtcCallWrapper>& aCallWrapper) :
> +  mPCHandle(aPCHandle),
> +  mJsepTransceiver(aJsepTransceiver),

Warning: Parameter 'ajseptransceiver' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:126
(Diff revision 12)
> +  mConduit = nullptr;
> +  RUN_ON_THREAD(mStsThread, WrapRelease(mRtpFlow.forget()), NS_DISPATCH_NORMAL);
> +  RUN_ON_THREAD(mStsThread, WrapRelease(mRtcpFlow.forget()), NS_DISPATCH_NORMAL);
> +}
> +
> +TransceiverImpl::~TransceiverImpl()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]
Comment on attachment 8900495 [details]
Bug 1290948 - Part 6: Remove some unused code. r+drno

https://reviewboard.mozilla.org/r/171870/#review196930


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: media/mtransport/transportlayerice.cpp:98
(Diff revision 12)
>  
>  TransportLayerIce::~TransportLayerIce() {
>    // No need to do anything here, since we use smart pointers
>  }
>  
> -void TransportLayerIce::SetParameters(RefPtr<NrIceCtx> ctx,
> +void TransportLayerIce::SetParameters(RefPtr<NrIceMediaStream> stream,

Warning: The parameter 'stream' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]
(In reply to Byron Campen [:bwc] from comment #169)
> ^ That was a rebase only, please ignore.

:clangbot, I am disappoint
Comment on attachment 8900492 [details]
Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver.

https://reviewboard.mozilla.org/r/171864/#review196126

> Yeah, it would be kinda weird to create a sendrecv transceiver in this case.

Filed https://github.com/w3c/webrtc-pc/issues/1640
Comment on attachment 8900494 [details]
Bug 1290948 - Part 5: TransceiverImpl and some major refactoring. r+drno

https://reviewboard.mozilla.org/r/171868/#review197254


C/C++ static analysis found 18 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:349
(Diff revision 13)
>      EXPECT_EQ(mozilla::kMediaConduitNoError, err);
>  
>      audio_stream_track_ = new FakeAudioStreamTrack();
>    }
>  
> -  virtual void CreatePipelines_s(bool aIsRtcpMux) {
> +  virtual void CreatePipeline(bool aIsRtcpMux) {

Warning: Prefer using 'override' or (rarely) 'final' instead of 'virtual' [clang-tidy: modernize-use-override]

  virtual void CreatePipeline(bool aIsRtcpMux) {
               ^
                                               override

::: media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:397
(Diff revision 13)
>          static_cast<mozilla::AudioSessionConduit *>(audio_conduit_.get())->
>          ConfigureRecvMediaCodecs(codecs);
>      EXPECT_EQ(mozilla::kMediaConduitNoError, err);
>    }
>  
> -  virtual void CreatePipelines_s(bool aIsRtcpMux) {
> +  virtual void CreatePipeline(bool aIsRtcpMux) {

Warning: Prefer using 'override' or (rarely) 'final' instead of 'virtual' [clang-tidy: modernize-use-override]

  virtual void CreatePipeline(bool aIsRtcpMux) {
               ^
                                               override

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:672
(Diff revision 13)
>                      filter),
>                  NS_DISPATCH_NORMAL);
>  }
>  
>  void
> -MediaPipeline::UpdateTransport_s(int level,
> +MediaPipeline::UpdateTransport_s(RefPtr<TransportFlow> rtp_transport,

Warning: The parameter 'rtp_transport' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1417
(Diff revision 13)
>      const std::string& pc,
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> +    bool is_video,
>      dom::MediaStreamTrack* domtrack,
> -    const std::string& track_id,
> +    RefPtr<MediaSessionConduit> conduit) :

Warning: The parameter 'conduit' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1418
(Diff revision 13)
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> +    bool is_video,
>      dom::MediaStreamTrack* domtrack,
> -    const std::string& track_id,
> -    int level,
> +    RefPtr<MediaSessionConduit> conduit) :
> +  MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit),

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit),
                              ^
                              std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1418
(Diff revision 13)
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> +    bool is_video,
>      dom::MediaStreamTrack* domtrack,
> -    const std::string& track_id,
> -    int level,
> +    RefPtr<MediaSessionConduit> conduit) :
> +  MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit),

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit),
                                           ^
                                           std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2010
(Diff revision 13)
>  MediaPipelineReceive::MediaPipelineReceive(
>      const std::string& pc,
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> -    SourceMediaStream *stream,
> -    const std::string& track_id,
> +    RefPtr<MediaSessionConduit> conduit) :
> +  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
                             ^
                             std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2010
(Diff revision 13)
>  MediaPipelineReceive::MediaPipelineReceive(
>      const std::string& pc,
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> -    SourceMediaStream *stream,
> -    const std::string& track_id,
> +    RefPtr<MediaSessionConduit> conduit) :
> +  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
                                          ^
                                          std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2010
(Diff revision 13)
>  MediaPipelineReceive::MediaPipelineReceive(
>      const std::string& pc,
>      nsCOMPtr<nsIEventTarget> main_thread,
>      nsCOMPtr<nsIEventTarget> sts_thread,
> -    SourceMediaStream *stream,
> -    const std::string& track_id,
> +    RefPtr<MediaSessionConduit> conduit) :
> +  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),

Warning: Parameter 'conduit' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
                                                      ^
                                                      std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2143
(Diff revision 13)
> -    const std::string& media_stream_track_id,
> -    TrackID numeric_track_id,
> -    int level,
>      RefPtr<AudioSessionConduit> conduit,
> -    RefPtr<TransportFlow> rtp_transport,
> -    RefPtr<TransportFlow> rtcp_transport,
> +    SourceMediaStream* aStream) :
> +  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
                           ^
                           std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2143
(Diff revision 13)
> -    const std::string& media_stream_track_id,
> -    TrackID numeric_track_id,
> -    int level,
>      RefPtr<AudioSessionConduit> conduit,
> -    RefPtr<TransportFlow> rtp_transport,
> -    RefPtr<TransportFlow> rtcp_transport,
> +    SourceMediaStream* aStream) :
> +  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
                                        ^
                                        std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2318
(Diff revision 13)
> -    const std::string& media_stream_track_id,
> -    TrackID numeric_track_id,
> -    int level,
>      RefPtr<VideoSessionConduit> conduit,
> -    RefPtr<TransportFlow> rtp_transport,
> -    RefPtr<TransportFlow> rtcp_transport,
> +    SourceMediaStream* aStream) :
> +  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
                           ^
                           std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2318
(Diff revision 13)
> -    const std::string& media_stream_track_id,
> -    TrackID numeric_track_id,
> -    int level,
>      RefPtr<VideoSessionConduit> conduit,
> -    RefPtr<TransportFlow> rtp_transport,
> -    RefPtr<TransportFlow> rtcp_transport,
> +    SourceMediaStream* aStream) :
> +  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
                                        ^
                                        std::move()


::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:534
(Diff revision 13)
> +
> +// Accessing the PCMedia should be safe here because we shouldn't
> +// have enqueued this function unless it was still active and
> +// the ICE data is destroyed on the STS.
> +static void
> +FinalizeTransportFlow_s(RefPtr<PeerConnectionMedia> aPCMedia,

Warning: The parameter 'apcmedia' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

FinalizeTransportFlow_s(RefPtr<PeerConnectionMedia> aPCMedia,
                                                    ^
                        const                      &

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:535
(Diff revision 13)
> +// Accessing the PCMedia should be safe here because we shouldn't
> +// have enqueued this function unless it was still active and
> +// the ICE data is destroyed on the STS.
> +static void
> +FinalizeTransportFlow_s(RefPtr<PeerConnectionMedia> aPCMedia,
> +                        RefPtr<TransportFlow> aFlow, size_t aLevel,

Warning: The parameter 'aflow' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                        RefPtr<TransportFlow> aFlow, size_t aLevel,
                                              ^
                        const                &

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:537
(Diff revision 13)
> +// the ICE data is destroyed on the STS.
> +static void
> +FinalizeTransportFlow_s(RefPtr<PeerConnectionMedia> aPCMedia,
> +                        RefPtr<TransportFlow> aFlow, size_t aLevel,
> +                        bool aIsRtcp,
> +                        nsAutoPtr<PtrVector<TransportLayer> > aLayerList)

Warning: The parameter 'alayerlist' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                        nsAutoPtr<PtrVector<TransportLayer> > aLayerList)
                                                              ^
                        const                                &

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:553
(Diff revision 13)
> +  aLayerList->values.clear();
> +  (void)aFlow->PushLayers(layerQueue); // TODO(bug 854518): Process errors.
> +}
> +
> +static void
> +AddNewIceStreamForRestart_s(RefPtr<PeerConnectionMedia> aPCMedia,

Warning: The parameter 'apcmedia' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

AddNewIceStreamForRestart_s(RefPtr<PeerConnectionMedia> aPCMedia,
                                                        ^
                            const                      &

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:554
(Diff revision 13)
> +  (void)aFlow->PushLayers(layerQueue); // TODO(bug 854518): Process errors.
> +}
> +
> +static void
> +AddNewIceStreamForRestart_s(RefPtr<PeerConnectionMedia> aPCMedia,
> +                            RefPtr<TransportFlow> aFlow,

Warning: The parameter 'aflow' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                            RefPtr<TransportFlow> aFlow,
                                                  ^
                            const                &
Comment on attachment 8900495 [details]
Bug 1290948 - Part 6: Remove some unused code. r+drno

https://reviewboard.mozilla.org/r/171870/#review197256


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: media/mtransport/transportlayerice.cpp:98
(Diff revision 13)
>  
>  TransportLayerIce::~TransportLayerIce() {
>    // No need to do anything here, since we use smart pointers
>  }
>  
> -void TransportLayerIce::SetParameters(RefPtr<NrIceCtx> ctx,
> +void TransportLayerIce::SetParameters(RefPtr<NrIceMediaStream> stream,

Warning: The parameter 'stream' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]
Comment on attachment 8900492 [details]
Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver.

https://reviewboard.mozilla.org/r/171864/#review197700

Lgtm! Could we add a test for the "no stream" case?

::: dom/media/PeerConnection.js:813
(Diff revisions 7 - 9)
> +      this.logWarning("offerToReceiveVideo: false is ignored now. If you " +
> +                      "want to stop receiving a track, you can use " +
> +                      "RTCRtpTransceiver.setDirection");

On wording: People probably weren't using {offerToReceiveVideo: false} to stop tracks, rather it was the way to force sendonly on the track they just added (to override the do-unto-others default) I think.

RTCRtpTransceiver.setDirection sounds like a static method. Also, people need to consider backwars compat. How about:

    '{offerToReceiveVideo: false} is ignored now. Consider using pc.getTransceivers()[i].setDirection("sendrecv")'

?

Also note I've just filed https://github.com/w3c/webrtc-pc/issues/1643 !
Attachment #8900492 - Flags: review?(jib) → review+
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #185)
> Also, people need to consider backwars compat. How about:

I meant "backwards". Freudian slip.
Flags: needinfo?(jib)
Blocks: 1414807
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #187)
> Note https://github.com/w3c/webrtc-pc/pull/1647.

Updated!
Flags: needinfo?(docfaraday)
Comment on attachment 8913686 [details]
Bug 1290948 - Part 8: Don't cause ICE to fail if there's no streams to establish. r+drno

https://reviewboard.mozilla.org/r/185086/#review202454


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8913686 [details]
Bug 1290948 - Part 8: Don't cause ICE to fail if there's no streams to establish. r+drno

https://reviewboard.mozilla.org/r/185086/#review202476


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bbe53fb92e21
Part 1: Mochitest for transceivers. r+jib r=jib
https://hg.mozilla.org/integration/autoland/rev/49878c508ce6
Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan r=ehsan+251051,jib
https://hg.mozilla.org/integration/autoland/rev/56c169018ceb
Part 3: Implement JS part of RTCRtpTransceiver. r=jib
https://hg.mozilla.org/integration/autoland/rev/ffb6e6da955f
Part 4: Transceivers JSEP/SDP work. r+drno r=drno
https://hg.mozilla.org/integration/autoland/rev/1a5f090502b0
Part 5: TransceiverImpl and some major refactoring. r+drno r=drno
https://hg.mozilla.org/integration/autoland/rev/314675023cd5
Part 6: Remove some unused code. r+drno r=drno
https://hg.mozilla.org/integration/autoland/rev/8ff38e646037
Part 7: Update existing mochitests to track the spec fixes in previous parts. r+drno r=drno
https://hg.mozilla.org/integration/autoland/rev/97a271bf671e
Part 8: Don't cause ICE to fail if there's no streams to establish. r+drno r=drno
On it. Need a DOM reviewer so I can touch test_interfaces.js.
Flags: needinfo?(docfaraday)
Attachment #8928253 - Flags: review?(bugs)
Comment on attachment 8928253 [details]
Bug 1290948 - Part 9: Add RTCRtpTransceiver to test_interfaces.js

https://reviewboard.mozilla.org/r/199478/#review204572
Attachment #8928253 - Flags: review?(bugs) → review+
This also fails media tests on Android and creates a mix of unexpected passes, fails and timeouts in web platform tests.
So much for try syntax... guess I'm back to running try_all.
Attachment #8928253 - Attachment is obsolete: true
Comment on attachment 8928326 [details]
Bug 1290948 - Part 10: Stop expecting failure on lots of webrtc web-platform-tests.

https://reviewboard.mozilla.org/r/199532/#review204748

LGTM
Attachment #8928326 - Flags: review+
Comment on attachment 8928326 [details]
Bug 1290948 - Part 10: Stop expecting failure on lots of webrtc web-platform-tests.

https://reviewboard.mozilla.org/r/199532/#review204752

LGTM
Attachment #8928325 - Flags: review?(bugs)
Attachment #8929644 - Flags: review?(drno)
Attachment #8929645 - Flags: review?(jib)
Comment on attachment 8929644 [details]
Bug 1290948 - Part 11: Fix a nullptr crash that web-platform-tests is hitting.

https://reviewboard.mozilla.org/r/200920/#review206174

LGTM
Attachment #8929644 - Flags: review?(drno) → review+
Comment on attachment 8929645 [details]
Bug 1290948 - Part 12: Disable most of the webrtc web-platform-tests until bug 1418367 is fixed. r+jib

https://reviewboard.mozilla.org/r/200922/#review206178

To record the rationale for why we're doing this here (from email with bwc):

"There is a fairly small limit on the number of AudioSessionConduits that can exist at any one time. This limit seems to be 16 on windows (so much for conferencing!), 24 on linux32, and 64 on linux64. Pre-transceivers, we held off on creating the conduits until negotiation was complete. In the transceivers work, we create them much earlier, when the transceiver is created (this is far simpler to implement and understand, and it makes sense to acquire this resource at this time anyway). Sadly, this means that the web-platform-tests now exceed this limit (and fail) because they don't close their PCs and we have to wait for garbage collection"
Attachment #8929645 - Flags: review?(jib) → review+
FWIW I don't think it'd be unreasonable to require WPT tests to call pc.close() after they're done.
Comment on attachment 8928325 [details]
Bug 1290948 - Part 9: Add RTCRtpTransceiver to test_interfaces.js r+smaug

https://reviewboard.mozilla.org/r/199530/#review206208
Attachment #8928325 - Flags: review?(bugs) → review+
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30498580def1
Part 1: Mochitest for transceivers. r+jib r=jib
https://hg.mozilla.org/integration/autoland/rev/18788603a88a
Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan r=ehsan+251051,jib
https://hg.mozilla.org/integration/autoland/rev/614535c1f7e1
Part 3: Implement JS part of RTCRtpTransceiver. r=jib
https://hg.mozilla.org/integration/autoland/rev/66fc816219cb
Part 4: Transceivers JSEP/SDP work. r+drno r=drno
https://hg.mozilla.org/integration/autoland/rev/ab0fc6ccf118
Part 5: TransceiverImpl and some major refactoring. r+drno r=drno
https://hg.mozilla.org/integration/autoland/rev/921b8fc790a5
Part 6: Remove some unused code. r+drno r=drno
https://hg.mozilla.org/integration/autoland/rev/d900259bc6e6
Part 7: Update existing mochitests to track the spec fixes in previous parts. r+drno r=drno
https://hg.mozilla.org/integration/autoland/rev/b0672b835923
Part 8: Don't cause ICE to fail if there's no streams to establish. r+drno r=drno
https://hg.mozilla.org/integration/autoland/rev/fa09cc1ad116
Part 9: Add RTCRtpTransceiver to test_interfaces.js r+smaug r=smaug
https://hg.mozilla.org/integration/autoland/rev/1c8c98131db8
Part 10: Stop expecting failure on lots of webrtc web-platform-tests. r=drno
https://hg.mozilla.org/integration/autoland/rev/59786b49cb87
Part 11: Fix a nullptr crash that web-platform-tests is hitting. r=drno
https://hg.mozilla.org/integration/autoland/rev/29bcc63fc4ce
Part 12: Disable most of the webrtc web-platform-tests until bug 1418367 is fixed. r+jib r=jib
Depends on: 1421965
Depends on: 1425618
Depends on: 1425621
Depends on: 1425873
Depends on: 1425956
Blocks: 1375540
This was a fun one -- it kept getting bigger as I realized things that needed to be covered to fully cover this topic. I finally had to cut myself off at one point. :)

I am setting a needinfo flag on this asking for a review by the assignee. Byron, if you can look these over and either fix anything you see wrong or let me know what I got wrong, I would sure appreciate it!

I will assume this documentation is correct and reasonably complete unless I hear otherwise.

New pages:

https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/addTransceiver()
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpTransceiver
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpTransceiver/setCodecPreferences
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpTransceiver/stop
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpTransceiver/currentDirection
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpTransceiver/direction
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpTransceiver/mid
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpTransceiver/receiver
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpTransceiver/stopped
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpTransceiver/sender
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpTransceiverInit
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpTransceiverDirection
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpCodecParameters
https://developer.mozilla.org/en-US/docs/Web/API/WebRTC_API/Intro_to_RTP

Updated pages:

https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/addTrack
https://developer.mozilla.org/en-US/docs/Glossary/Codec

And this was added to Firefox 59 for developers.
Flags: needinfo?(docfaraday)
Keywords: dev-doc-needed → dev-doc-complete
FWIW: I'm aware for what it's worth that the individual members of RTCRtpCodecParameters don't yet have their own pages; that was where I finally had to draw the line on following the rabbit down the hole. :)
Depends on: 1437832
Blocks: 1437741
Depends on: 1449042
Depends on: 1466175
Flags: needinfo?(docfaraday)
Depends on: 1502899
See Also: → 1621770
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: