1136871 - RTCRtpSender.replaceTrack doesn't work for a second call

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Mark Banner (:standard8)]

In bug 1131574 we're attemping to use replaceTrack to replace a tab (browser) stream with another one.

However, it ssems to be triggering an ICE restart:

"Error while setting RemoteDescription InvalidSessionDescriptionError: ICE restart is unsupported at this time (new remote description changes either the ice-ufrag or ice-pwd)ice-ufrag (old): 7965321fice-ufrag (new): 4fc4d0a1ice-pwd (old): 074cdf2e09eff2a9b5002b2fc4ac7e3bice-pwd (new): 0baa151296c2295c0eb06cde61f849db" sdk.js:1268:12

19:47:21.443 "OT.exception :: title: Unable to Publish (1500) msg: Publisher PeerConnection Error: Error while setting RemoteDescription InvalidSessionDescriptionError: ICE restart is unsupported at this time (new remote description changes either the ice-ufrag or ice-pwd)ice-ufrag (old): 7965321fice-ufrag (new): 4fc4d0a1ice-pwd (old): 074cdf2e09eff2a9b5002b2fc4ac7e3bice-pwd (new): 0baa151296c2295c0eb06cde61f849db"

Summary of code:

navigator.getUserMedia(<new constraints including window Id>, function(newStream) {

  peerConnection.getSenders().forEach(function(sender) {
    if (sender.track.kind === 'video' && newStream.getVideoTracks().length) {
      sender.replaceTrack(newStream.getVideoTracks()[0];
    }
  }
})

Bryon,

Comment 1

[Mark Banner (:standard8)]

Byron, do you have any ideas what's going on here?

Comment 2

[Byron Campen [:bwc]]

Let me look.

Comment 3

[Byron Campen [:bwc]]

So, you're only going to get that error if you're trying to renegotiate, which replaceTrack doesn't cause. However, if you're going through TokBox's API, they may implement replaceTrack using RemoveTrack/AddTrack, which will trigger a renegotiation and perhaps this problem. What browsers were the endpoints in this call?

(FWIW, I think that implementing ICE restart shouldn't be too hard, I'll just have to make the time to do it.)

Comment 4

[Mark Banner (:standard8)]

I'm currently testing with Firefox Nightly on the receiving end, and a latest-fx-team build on the sending end.

I've just re-verified on my end, and with the sdk the new stream is a LocalMediaStream object, and the sender in "sender.replaceTrack" is an RTCRtpSender - so this looks to be calling the platform replaceTrack function.

Comment 5

[Byron Campen [:bwc]]

Hmm. Then why is renegotiation happening? Is TokBox just doing it for fun? And if they are triggering renegotiation, I would not expect any endpoint based on Firefox to change the ice-ufrag or ice-pwd; if that is changing, and the other end is Firefox, the only possibility is a whole new PeerConnection has been created, and is now attempting to connect to an old PeerConnection on the other side, which is all kinds of wrong.

Can I get logging from both sides? (NSPR_LOG_MODULES=signaling:6,jsep:6,PeerConnectionImpl:6)

Comment 6

[Mark Banner (:standard8)]

Aha, you're right, there appears to be something going on that if I comment out, then it works. So I think they are doing something incorrectly and accidentally triggering the renegotiation.

Now that it works fine for a single call to replace track. I'm having an issue with subsequent calls, I'm getting:

TypeError: pc._onReplaceTrackError is not a function PeerConnection.js:1288:4

Which does indeed seem to be true:

http://mxr.mozilla.org/mozilla-central/search?string=_onReplaceTrackError

If I add in extra debug, the code is "3" and the message is "Track to replace is not in stream".

If you still want logging for this, I can get it tomorrow morning utc.

Comment 7

[Mark Banner (:standard8)]

I should probably say, that coupled with these failures, I'm getting assertion failures on shutdown similar to that below, but I'm kinda assuming this is a consequence of the failed replaceTrack:

Assertion failure: outputTrack->GetEnd() == GraphTimeToStreamTime(interval.mStart) (Samples missing), at /Users/mark/loop/gecko-dev/dom/media/TrackUnionStream.cpp:279
#01: mozilla::MediaStreamGraphImpl::ProduceDataForStreamsBlockByBlock(unsigned int, int, long long, long long)[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1f36390]
#02: mozilla::MediaStreamGraphImpl::Process(long long, long long)[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1f368e1]
#03: mozilla::MediaStreamGraphImpl::OneIteration(long long, long long, long long, long long)[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1f36ff4]
#04: mozilla::AudioCallbackDriver::DataCallback(float*, long)[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1eee6ca]
#05: audiounit_output_callback[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL +0x325a603]
#06: AUConverterBase::RenderBus(unsigned int&, AudioTimeStamp const&, unsigned int, unsigned int)[/System/Library/Components/CoreAudio.component/Contents/MacOS/CoreAudio +0x8230]
#07: AUBase::DoRenderBus(unsigned int&, AudioTimeStamp const&, unsigned int, AUOutputElement*, unsigned int, AudioBufferList&)[/System/Library/Components/CoreAudio.component/Contents/MacOS/CoreAudio +0x5c61]
#08: AUBase::DoRender(unsigned int&, AudioTimeStamp const&, unsigned int, unsigned int, AudioBufferList&)[/System/Library/Components/CoreAudio.component/Contents/MacOS/CoreAudio +0x4515]
#09: AUHAL::AUIOProc(unsigned int, AudioTimeStamp const*, AudioBufferList const*, AudioTimeStamp const*, AudioBufferList*, AudioTimeStamp const*, void*)[/System/Library/Components/CoreAudio.component/Contents/MacOS/CoreAudio +0xb878]
#10: HALC_ProxyIOContext::IOWorkLoop()[/System/Library/Frameworks/CoreAudio.framework/Versions/A/CoreAudio +0x279db]
#11: HALC_ProxyIOContext::IOThreadEntry(void*)[/System/Library/Frameworks/CoreAudio.framework/Versions/A/CoreAudio +0x26add]
#12: HALB_IOThread::Entry(void*)[/System/Library/Frameworks/CoreAudio.framework/Versions/A/CoreAudio +0x2699d]
#13: _pthread_body[/usr/lib/system/libsystem_pthread.dylib +0x1899]
#14: _pthread_struct_init[/usr/lib/system/libsystem_pthread.dylib +0x172a]

Comment 8

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

Attachment: Part 1: PeerConnection typo in replaceTrackFailure

Fix typo discovered in comment 6.

Comment 9

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

Should let us see the real error.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e05aa7a82b

Comment 11

[Mark Banner (:standard8)]

Attachment: Part 3: Testcase

I did some debugging into what's happening here.

As far as I can tell, the initial track is being replaced correctly, the RTCRtpSender object gets updated with the new stream.

However, for the next replaceTrack, when we go back to the peer connection, the sender still has the old stream information.

I think this testcase partially reflects the issue - the list of video tracks hasn't been updated in the pc's copy of the RTCRtpSender. This then causes the second replaceTrack to fail.

Comment 12

[Mark Banner (:standard8)]

I also forgot to confirm, the error is "Track to replace is not in stream".

Comment 13

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

It's replaceTrack, not replaceStream. That is to say, replaceTrack does not alter the local stream, it only alters which track RTPSender is sending.

The test checks pc.getLocalStreams().getVideoTracks()[0].id which remains the old id, because the old track is still in the stream.

Not saying that's ideal, but it' not clear how it would work any other way. Streams are a gUM concept, and PeerConnection is potentially just one of many users of them.

Comment 14

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

Once we have mediaStream.addTrack (Bug 1103188) I think the idea was that you'd do:

  stream.addTrack(newtrack);
  sender.replaceTrack(newtrack);
  stream.removeTrack(oldtrack);

Comment 15

[Mark Banner (:standard8)]

Ok, so the test case might not be 100% valid. However, there's still an issue here somewhere. In the sdk code we're doing this:

peerConnection.getSenders().forEach(function(sender) {
  if (sender.track.kind === 'video' && newStream.getVideoTracks().length) {
    sender.replaceTrack(newStream.getVideoTracks()[0]);
  }
});

sender is the RTCRtpSender in PeerConnection.js:

http://hg.mozilla.org/mozilla-central/annotate/dcc86f78d75e/dom/media/PeerConnection.js#l1334

and then to:

http://hg.mozilla.org/mozilla-central/annotate/dcc86f78d75e/dom/media/PeerConnection.js#l865

The stream is the RTCRtpSender._stream.

When replaceTrack fires on the peer connection the first time, the stream is correct with the current track.

The onReplaceTrackSuccess is then firing, and seems to update the "sender" with the new track:

http://hg.mozilla.org/mozilla-central/annotate/dcc86f78d75e/dom/media/PeerConnection.js#l1278

If I catch the resolved promise then I can see the sender has been updated.

However, the next time the sdk code gets called, the peerConnection.getSenders() returns the original object with the old track value.

That seems kinda strange when it looks like onReplaceTrackSuccess was trying to update the sender object.

Comment 16

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

Attachment: pc_test_swap.html - works for me to flip video multiple times

Comment 17

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

Comment on attachment 8570017 [details]
pc_test_swap.html - works for me to flip video multiple times

Pilot error on my part. I can reproduce it. Here's a simpler test-case:

http://jsfiddle.net/8nem95k7

Comment 18

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

Seems to be a regression as it works in 37.

Comment 19

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #18)
> Seems to be a regression as it works in 37.

So one of Byron's landings probably then

Comment 20

[Byron Campen [:bwc]]

So what precisely is the problem here? That we're passing the wrong stream here when we flip-flop?

https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#2090

Comment 21

[Byron Campen [:bwc]]

Attachment: MozReview Request: bz://1136871/bwc

/r/4409 - Bug 1136871 - Part 2: Clear up some inconsistencies with ReplaceTrack.

Pull down this commit:

hg pull review -r 1d091e50c8f52c604c9091ee540f006e96ce74e7

Comment 22

[Byron Campen [:bwc]]

Comment on attachment 8570226 [details]
MozReview Request: bz://1136871/bwc

DOM review for small webidl change, rest is for jib, or mt if that makes more sense.

Comment 23

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

Attachment: Part 3: replaceTrack-friendly pruning of RtpSenders by not relying on streams

Fixes "TypeError: pc1.getSenders(...)[0] is undefined" on second replaceTrack call with Byron's patch, since we're apparently aggressively cleaning up empty streams now (including the stream of the now-gone replaced track).

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b7e05aa7a82b

Comment 25

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

Comment on attachment 8569781 [details] [diff] [review]
Part 3: Testcase

Mark's test now works!

Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=b39f0c277718

Comment 26

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

From https://github.com/w3c/webrtc-pc/issues/189

Comment 27

[Mark Banner (:standard8)]

The patches make replaceTrack work for me - though there's a crash on exiting the window, which I've filed as bug 1137632.

Comment 28

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

Attachment: Part 5: more tests + collapse some testcode

Refactored replacetrack test a bit so it can be used twice, and added and invalid arg test.

Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=b27de86d8b5d

Comment 29

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

Attachment: Part 4: more tests + collapse some testcode (2)

Right patch.

Comment 30

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

Comment on attachment 8570226 [details]
MozReview Request: bz://1136871/bwc

https://reviewboard.mozilla.org/r/4407/#review3599

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +                   << " was never added.");

"was not found"? (not that this can happen).

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
(Diff revision 1)
> +LocalSourceStreamInfo::TakePipelineFrom(RefPtr<LocalSourceStreamInfo>& info,
> +                                        const std::string& oldTrackId,
> +                                        const std::string& newTrackId)

Maybe LocalSourceStreamInfo& for info (since RefPtr is unused)?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
(Diff revision 1)
> +LocalSourceStreamInfo::ForgetPipelineByTrackId_m(const std::string& trackId)
> +{
> +  ASSERT_ON_THREAD(mParent->GetMainThread());
> +
> +  // Refuse to hand out references if we're tearing down.
> +  // (Since teardown involves a dispatch to and from STS before MediaPipelines
> +  // are released, it is safe to start other dispatches to and from STS with a
> +  // RefPtr<MediaPipeline>, since that reference won't be the last one
> +  // standing)

What "other dispatches"? - I'm a bit puzzled by this comment (and the _m) since you're basically never leaving the main thread here.

Code looks sound though. Looks like mPipelines is only ever walked on main thread.

Comment 31

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

https://reviewboard.mozilla.org/r/4407/#review3621

> Maybe LocalSourceStreamInfo& for info (since RefPtr is unused)?

Nevermind, that's not a const reference.

Comment 32

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/4407/#review3623

> "was not found"? (not that this can happen).

I wanted to keep this consistent with the similar error logging for RemoveTrack.

> What "other dispatches"? - I'm a bit puzzled by this comment (and the _m) since you're basically never leaving the main thread here.
> 
> Code looks sound though. Looks like mPipelines is only ever walked on main thread.

I think most of this comment can go.

Comment 33

[Martin Thomson [:mt:]]

Comment on attachment 8570276 [details] [diff] [review]
Part 3: replaceTrack-friendly pruning of RtpSenders by not relying on streams

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

I think that this could be better.  Also, Reviewboard please, I needed to see more of the code than just the diff.

::: dom/media/PeerConnection.js
@@ +893,5 @@
>      let senders = [];
> +    // prune senders in case any tracks have disappeared down below
> +    this._senders.forEach(sender =>
> +      streams.some(stream =>
> +        (stream.getTracks().indexOf(sender.track) != -1) && senders.push(sender)));

If you need to do this, then use .filter() here and below.

return this._senders.filter(sender => streams.some(stream => stream.getTracks().indexOf(sender.track)));

@@ +894,5 @@
> +    // prune senders in case any tracks have disappeared down below
> +    this._senders.forEach(sender =>
> +      streams.some(stream =>
> +        (stream.getTracks().indexOf(sender.track) != -1) && senders.push(sender)));
> +    return this._senders = senders;

I'm a little uncomfortable with this getter mutating this._senders.  I'd rather have the necessary add/remove functions trigger mutations.  I notice that removeTrack (above) doesn't even touch the senders list.

Comment 34

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

(In reply to Martin Thomson [:mt] from comment #33)
> > +    // prune senders in case any tracks have disappeared down below
> > +    this._senders.forEach(sender =>
> > +      streams.some(stream =>
> > +        (stream.getTracks().indexOf(sender.track) != -1) && senders.push(sender)));
> > +    return this._senders = senders;
> 
> I'm a little uncomfortable with this getter mutating this._senders.

What are you worried will happen? It seemed more robust to me to wrap getLocalStreams (which will go away eventually) which already holds the truth, rather than have the JS guard all the mutation points to cache the same. We'll never have bugs where c++ and JS get out of sync.

> I'd rather have the necessary add/remove functions trigger mutations.

Do streams only ever go away in response to entry-points that pass through JS? What about receivers? I wasn't sure.

> I notice that removeTrack (above) doesn't even touch the senders list.

Right, and that would have been a bug had we done this differently.

Comment 35

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

I should also say this approach predates this patch.

Comment 36

[Martin Thomson [:mt:]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #34)
> > I'm a little uncomfortable with this getter mutating this._senders.
> 
> What are you worried will happen? It seemed more robust to me to wrap
> getLocalStreams (which will go away eventually) which already holds the
> truth, rather than have the JS guard all the mutation points to cache the
> same. We'll never have bugs where c++ and JS get out of sync.

I'm concerned that we're doing fixups after the fact because we don't understand our own code.  And those fixups result in getters with side-effects.  We can do better than that.

I don't much care that this landed in this state, but I'd like to stop the rot.

> > I'd rather have the necessary add/remove functions trigger mutations.
> 
> Do streams only ever go away in response to entry-points that pass through
> JS? What about receivers? I wasn't sure.

That's what concerns me.  Let's make sure that we understand the mutation story first.  I think that the only local track changes occur in addTrack, removeTrack (which are called from addStream/removeStream, I hope), and replaceTrack.  Other than replaceTrack, add and remove for local tracks shouldn't affect the set of RTCRtpSenders until we move to the stable state (this cleanup can coincide with our check to see if we need to renegotiate).

Remote tracks are created in reponse to calling setRemoteDescription and removed on the transition back to stable (that might be the same action).

> > I notice that removeTrack (above) doesn't even touch the senders list.
> 
> Right, and that would have been a bug had we done this differently.

Indeed.  That was an oversimplified view.

Have I ever mentioned how I think that this whole negotiation design is a giant clusterf

Comment 37

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

Attachment: MozReview Request: bz://1136871/jib

/r/4523 - Bug 1136871 - replaceTrack-friendly pruning of RtpSenders by not relying on streams

Pull down this commit:

hg pull review -r 2421fdd9e3d47b500fe2884169e3634062c55a24

Comment 38

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

OK ignore review request, I was just excited to get Review Board up. I see comment 36 and will address it.

Comment 39

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

(In reply to Martin Thomson [:mt] from comment #36)
> Other than replaceTrack, add and remove for local tracks
> shouldn't affect the set of RTCRtpSenders until we move to the stable state

That does not match my reading of the spec (see URL), which is:

  pc.addTrack returns a sender in all states but closed,
  pc.removeTrack removes the sender before returning in all states but closed,
  sender.replaceTrack does not affect the sender.

> Remote tracks are created in reponse to calling setRemoteDescription

Spec says: "When a user agent has reached the point where a MediaStreamTrack can be created to represent an incoming component", but does not mention setRemoteDescription explicitly.

> and removed on the transition back to stable (that might be the same action).

I don't find any mention of this, rather it is removed when 'ended' (stopped remotely).

Comment 40

[Martin Thomson [:mt:]]

You are right about local tracks.  And now you know exactly what to do.  If there is anything going on "down there", it's probably wrong.

I was largely guessing when it came to remote tracks (and clearly for local tracks too).  Either way, we should work it out properly and then tell them how to fix the spec.

The *intent* has always been that setRemote would expose what media the peer wanted to send, you would get a chance to reject any you didn't want and then have setLocal act on it.  I said stable transition because of PRANSWER, but feel free to ignore that little wart on the spec.

Comment 41

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #39)
>   sender.replaceTrack does not affect the sender.

I meant: sender.replaceTrack does not affect the set of senders.

Comment 42

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

Comment on attachment 8571739 [details]
MozReview Request: bz://1136871/jib

/r/4523 - imported patch 1136871_testcase
/r/4525 - Bug 1136871 - test invalid replaceTrack arg
/r/4527 - Bug 1136871 - cleanup RtpSenders accounting to not rely on streams

Pull down these commits:

hg pull review -r 54415a28b5b7d1af78babaf65a6bb6366a9112e8

Comment 43

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

Review board left me no choice but to push part 3 and 4.
No need to review twice obviously.

Comment 44

[Mark Banner (:standard8)]

Comment on attachment 8570531 [details] [diff] [review]
Part 4: more tests + collapse some testcode (2)

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

Look fine to me, though I haven't run it against the patches to confirm it passes.

Comment 45

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

I have a green try on that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82b5bb8d7f83

Comment 46

[:Gavin Sharp [email: [e-mail]]]

Mark tells me this is broken in 38 and that we need to get it uplifted there for Hello's tab sharing.

Comment 47

[Martin Thomson [:mt:]]

Comment on attachment 8571739 [details]
MozReview Request: bz://1136871/jib

https://reviewboard.mozilla.org/r/4521/#review3819

A couple of minor issues here, but this looks a lot better, thanks.

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html
(Diff revision 2)
> -        var track = stream.getVideoTracks()[0];
> +      var track = stream.getVideoTracks()[0];

Maybe rename to oldtrack, since you are replacing it.

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html
(Diff revision 2)
> +          var track = stream.getVideoTracks()[0];

As noted above, but more serious here, since this shadows the earlier variable declaration.

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html
(Diff revision 2)
> +        return sender.replaceTrack(track) // same track
> +          .then(() => ok(false, "replacing with itself should fail"),
> +                e => is(e.name, "InvalidParameterError",
> +                        "replacing with itself should fail"));

I would have let this short circuit successfully.  What does the spec say?

Is this it? https://github.com/w3c/webrtc-pc/pull/195/files

Because that doesn't say anything about this.

Comment 48

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

https://reviewboard.mozilla.org/r/4521/#review3877

> I would have let this short circuit successfully.  What does the spec say?
> 
> Is this it? https://github.com/w3c/webrtc-pc/pull/195/files
> 
> Because that doesn't say anything about this.

Holding me to my own PR... Now short-circuits, which I like, and added code for rejecting track.kind != withKind.kind as well.

Comment 49

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

Comment on attachment 8571739 [details]
MozReview Request: bz://1136871/jib

/r/4523 - imported patch rb4407.patch
/r/4525 - imported patch 1136871_testcase
/r/4527 - Bug 1136871 - test invalid replaceTrack arg
/r/4777 - Bug 1136871 - cleanup RtpSenders accounting to not rely on streams

Pull down these commits:

hg pull review -r 539dbb2a8fcafe4534007142f4839200fd93a5bb

Comment 50

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

Using mq and review board together is a bit messed up, so for clarification the only review needed here is: https://reviewboard.mozilla.org/r/4777/diff/0

Comment 51

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

https://reviewboard.mozilla.org/r/4777/diff

Comment 52

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/4523/#review3879

We can't land this as it stands.  I just had a big discussion with Byron about how this will interact with the renegotiation code and it's not pretty.

Changing the identifiers that JsepSession sees for a given track have serious ramifications for (re)negotiation.  Basically, the receiving peer will be forced to treat a change in a=msid as a new track.  That is definitely going to cause problems here; even if applications are prepared to deal with the change (we need to verify that), the whole media pipeline is going to enter a strange state at the point when the new media is signaled.

It is possible that we could lie to the receiver about the identifiers.  That seems like one reasonable way to deal with this.  I'm almost inclined to do that for every track anyway (see below).  However, we still need to have a consistent internal view about what belongs to what on the sender side.  This is currently consistent, but if we start lying, then we are contemplating 

We need several things before this is ready:
 1. We need to know exactly what the solution we want looks like.
 2. We need to go to the W3C and the IETF and raise the issue and fix it.  replaceTrack is still a proposal, so maybe we can unilaterally change it, but I wouldn't recommend that we do anything major to it without consultation.  At the very least, it would be a courtesy to those who might be contemplating its implementation.
 3. We need test cases that tests out the various renegotiation scenarios.  I don't see that in this patch.

If only we didn't have a design where the identifiers for tracks and streams were mirrored.  It would not have been a big problem if there was a new `remoteId` parameter on tracks and streams.  That could even change over time.  Of course, a=msid would be inadequate to support that concept.

If you want, we can arrange a call with Byron (and maybe ekr) and we can talk through the various options here (I have limited availability next week though).

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
(Diff revision 3)
> +  // Refuse to hand out references if we're tearing down.
> +  // (Since teardown involves a dispatch to and from STS before MediaPipelines
> +  // are released, it is safe to start other dispatches to and from STS with a
> +  // RefPtr<MediaPipeline>, since that reference won't be the last one
> +  // standing)

Not sure that I follow this comment.  Isn't it just the case that this code needs to make the two checks: i.e., whether the corresponding pipeline has even been setup yet.

I'm lazy, can you explain what mMediaStream being null signifies?  (As a comment)

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 3)
> +  it->mTrack->SetStreamId(newStreamId);
> +  it->mTrack->SetTrackId(newTrackId);

So the consequence of this is that when we renegotiate, the SDP will include the new track identifiers.  See meta-comment for details.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
(Diff revision 3)
> +  if (mMediaStream) {
> +    if (mPipelines.count(trackId)) {
> +      RefPtr<MediaPipeline> pipeline(mPipelines[trackId]);
> +      mPipelines.erase(trackId);
> +      return pipeline.forget();
> +    }
> +  }
> +
> +  return nullptr;

I'd have done a return early here, which is easier to scan.

    if (!mMediaStream) {
      return nullptr;
    }
    if (!mPipelines.count(trackId)) {
      return nullptr;
    }

Comment 53

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/4777/#review3881

::: dom/media/PeerConnection.js
(Diff revision 1)
>        this._onReplaceTrackSender = sender;
>        this._onReplaceTrackWithTrack = withTrack;
>        this._onReplaceTrackSuccess = resolve;
>        this._onReplaceTrackFailure = reject;

This archaic form of state management disturbs me greatly.  Why not just have the Impl generate a promise and then deal with setting the sender in the .then().

Even if the impl didn't generate promises here, you could at least do this:

```
return this._win.Promise((resolve, reject) => {
  this._onReplaceTrackSuccess = resolve;
  this._onReplaceTrackFailure = reject;
  this._impl.replaceTrack(sender.track, withTrack);
}).then(() => { sender.track = withTrack; });
```

Then all the code is here, not scattered around.

Comment 54

[Martin Thomson [:mt:]]

Comment on attachment 8571739 [details]
MozReview Request: bz://1136871/jib

https://reviewboard.mozilla.org/r/4521/#review3887

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 3)
> +  it->mTrack->SetStreamId(newStreamId);
> +  it->mTrack->SetTrackId(newTrackId);

My previous comments apply here.

Comment 55

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

https://reviewboard.mozilla.org/r/4523/#review3893

> Not sure that I follow this comment.  Isn't it just the case that this code needs to make the two checks: i.e., whether the corresponding pipeline has even been setup yet.
> 
> I'm lazy, can you explain what mMediaStream being null signifies?  (As a comment)

This is Byron's patch, so he'll have to answer. Basically, using review board forced me to push the other patches in this bug, patches that have already been r+'ed. It's always good with extra eyes though. :-)

Comment 56

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

https://reviewboard.mozilla.org/r/4523/#review3897

> So the consequence of this is that when we renegotiate, the SDP will include the new track identifiers.  See meta-comment for details.

(ugh, review board ate my comment, retyping)

ReplaceTrack is predicated on lying to the receiver. Its utility is when we're NOT renegotiating. If another event causes renegotiation, and keeping the lie going past that is hard, then couldn't it come clean at that point? (Disclaimer: I don't comprehend all the details).

Brainstorming: What would happen if replaceTrack switched the ids of track and withTrack? "Sean, you're now Castor, and Castor you're now Sean, and switch places you two. Sean, you look different somehow!" - It would be as if replaceTrack did a deep copy between the two, replacing all innards except the id (when behind the curtain we know what really happened). Would that pass the id tracking code?

Comment 57

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

https://reviewboard.mozilla.org/r/4777/#review3899

> This archaic form of state management disturbs me greatly.  Why not just have the Impl generate a promise and then deal with setting the sender in the .then().
> 
> Even if the impl didn't generate promises here, you could at least do this:
> 
> ```
> return this._win.Promise((resolve, reject) => {
>   this._onReplaceTrackSuccess = resolve;
>   this._onReplaceTrackFailure = reject;
>   this._impl.replaceTrack(sender.track, withTrack);
> }).then(() => { sender.track = withTrack; });
> ```
> 
> Then all the code is here, not scattered around.

Please lets limit review to the patch. I agree the state management can be improved. I've looked at wiring promises into PeerConnectionImpl before and ain't pretty, for several reasons:

1) Using promises would make the PeerConnection.js code marginally cleaner, at a cost of making the corresponding c++ uglier, largely because the Promise<> template operates in JS types. I think pretty c++ should win, since there's more of it. This isn't a general-purpose interface anyway, so I think we shold lean on the JS here.

2) MOZ_INTERNAL_API - need I say more?

3) There are other consumers of the c++ code (a library of sorts), and historically there's friction wheneveer we couple more tightly to DOM and JS.

Take it to Bug 1111742.

Comment 58

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

Jesup, thoughts on comment 52? Lets also plan to meet tomorrow.

Comment 59

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/4523/#review3895

> This is Byron's patch, so he'll have to answer. Basically, using review board forced me to push the other patches in this bug, patches that have already been r+'ed. It's always good with extra eyes though. :-)

It is nulled out when |DetachMedia_m| is called, which is the first step in the two-part teardown sequence (the second part happens on STS in |ShutdownMediaTransport_s|).

> (ugh, review board ate my comment, retyping)
> 
> ReplaceTrack is predicated on lying to the receiver. Its utility is when we're NOT renegotiating. If another event causes renegotiation, and keeping the lie going past that is hard, then couldn't it come clean at that point? (Disclaimer: I don't comprehend all the details).
> 
> Brainstorming: What would happen if replaceTrack switched the ids of track and withTrack? "Sean, you're now Castor, and Castor you're now Sean, and switch places you two. Sean, you look different somehow!" - It would be as if replaceTrack did a deep copy between the two, replacing all innards except the id (when behind the curtain we know what really happened). Would that pass the id tracking code?

I would be ok with the "coming clean" approach, but it will violate the primary requirement for replaceTrack in the first place when it happens (playback will halt while new streams/tracks are set up). In my opinion, the spec is simply broken here, and the thing that is broken is an optimization, so I won't be too sad if the optimization doesn't work in some cases. We do need to figure out how to fix the spec here, though.

Comment 60

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

https://reviewboard.mozilla.org/r/4777/#review3917

> Please lets limit review to the patch. I agree the state management can be improved. I've looked at wiring promises into PeerConnectionImpl before and ain't pretty, for several reasons:
> 
> 1) Using promises would make the PeerConnection.js code marginally cleaner, at a cost of making the corresponding c++ uglier, largely because the Promise<> template operates in JS types. I think pretty c++ should win, since there's more of it. This isn't a general-purpose interface anyway, so I think we shold lean on the JS here.
> 
> 2) MOZ_INTERNAL_API - need I say more?
> 
> 3) There are other consumers of the c++ code (a library of sorts), and historically there's friction wheneveer we couple more tightly to DOM and JS.
> 
> Take it to Bug 1111742.

I forgot to comment on this part:

    }).then(() => { sender.track = withTrack; });

would introduce an extra dispatch for no reason, so not sure of the cost-benefit there, since the gain is marginal.

Comment 61

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

(Annoying that review board doesn't wordwap quoted text)

(In reply to Byron Campen [:bwc] from comment #59)
> In my opinion, the spec is simply broken here, and the thing that is broken
> is an optimization, so I won't be too sad if the optimization doesn't work
> in some cases.

Which part do you mean is broken? replaceTrack or something else?

Comment 62

[Martin Thomson [:mt:]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #60)
> I forgot to comment on this part:
> 
>     }).then(() => { sender.track = withTrack; });
> 
> would introduce an extra dispatch for no reason, so not sure of the
> cost-benefit there, since the gain is marginal.

Seriously?

Comment 63

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

Why don't we store the remote id in the sender?

Comment 64

[Martin Thomson [:mt:]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #63)
> Why don't we store the remote id in the sender?

I think that it needs to go a bit deeper than that.  Unless you plan to frob SDP on the way out of create{Offer|Answer} too.

Comment 65

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

(In reply to Martin Thomson [:mt] from comment #52)
> It is possible that we could lie to the receiver about the identifiers. 
> That seems like one reasonable way to deal with this.  I'm almost inclined
> to do that for every track anyway (see below).  However, we still need to
> have a consistent internal view about what belongs to what on the sender
> side.  This is currently consistent, but if we start lying, then we are
> contemplating 

? Looks like something was missing here

So: as you suggest, I think separating the internal and external Id's is useful.  I.e. this would mean that RTPSender would have a unique Id, and that anything could feed into it (and ReplaceTrack becomes clean).  If no ReplaceTrack is done, then the local id to remote id mapping remains stable.  So you could call this fallout from continuing the conversion from streams to RTPSender/Receiver - that the pair should have a unique ID (and may also be the Id of the output track on the receiver side), but that it wouldn't necessarily match the id of the (current) source track.  Perhaps to minimize disruption, we could say that the RTPSender/Receiver ID-space is separate from the track ID-space, and thus the RTPSender/Receiver ID could match the original track ID if not in use already.

(Question: what happens if we attach the same track multiple times now?  And we currently have the case that a track could be sent back to us, and have the same id as another track - and what if that were then attached to the connection again?  Ok, this is likely stupid to do....)  Having unique IDs for RTPSender/Receiver pairs would resolve this issue.

If we had MediaStreamProcessing, we could more easily munge a track to take data from one of multiple source tracks, and feed the resultant track to the PeerConnection.  But we don't have this (not without a lot of hoop-jumping and overhead I don't want to contemplate right now, and bits that aren't done yet).

tl;dr: here's the proposal:
Add an id attribute (with a ID-space separate from the track IDs) to RTPSender, and to the receiver as well.  To minimize disruption we could state (or just implement) that the default RTPSender/Receiver ID is same as the initial track's ID, and the same for receiver track IDs.  

This might also allow us to remove the wart in the spec of "you can get duplicate id's due to tracks forwarded back to you, or if (somehow) the remote had a track with the same ID". (which per above likely isn't fully dealt with yet)

> If only we didn't have a design where the identifiers for tracks and streams
> were mirrored.  It would not have been a big problem if there was a new
> `remoteId` parameter on tracks and streams.  That could even change over
> time.  Of course, a=msid would be inadequate to support that concept.

See my proposal above, this might resolve it without having to touch msid (I hope).  Byron, Martin, would this (as I proposed) cause any problematic changes to JSEP/msid/etc?

Comment 66

[Byron Campen [:bwc]]

So for this proposal to help, the msid attributes in the SDP would need to contain the id from the RTPSender, correct? And while we're at it, I should point out that if we allow replaceTrack to substitute a track from a different stream, we will need another identifier on RTPSender that acts as a stream group identifier, and put that in msid instead of the stream id (since we don't want that changing). This is a pretty big change to the specs (including both w3c and IETF), and a lot of implementation work. We might need to make RTPSender/RTPReceiver c++ classes. Architecturally, this idea has some appeal to me (making RTPSender/RTPReceiver more of a first-class citizen).

In the meantime, I think it makes sense to move forward with the "coming clean" approach to renegotiation; yes it will cause hiccups in some cases, but they need not be the end of the world. We will need to make sure that renegotiation after replaceTrack doesn't completely hose things, though.

Comment 67

[Byron Campen [:bwc]]

Actually, since everybody is putting ssrcs in SDP now, let's just use those to disambiguate cases where the msid is a change in name only, and when it is actually a new track. That way, "coming clean" ends up being cheap; if the msid changes but the ssrc does not, we just tweak some ids on stuff but leave everything running uninterrupted. We do have the question of what kind of callbacks content will get, though.

Comment 68

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

I like that! Where does that leave patches in this bug?

Comment 69

[Martin Thomson [:mt:]]

I like it too.  I think that it's a much less disruptive path as well.

The patches in the bug are part of this.  What they would need is (at a minimum):

1. tests for renegotiation
2. track renaming on the receiver side

Note the one hole we need to consider is if replacement causes different stream-track relationships to emerge (as they no doubt will).

Byron, do you want to take this to the W3C, or should Jan-Ivar update his pull request first?

Comment 70

[Byron Campen [:bwc]]

Let's update the PR, and I can figure out what to do from there.

Comment 71

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

https://reviewboard.mozilla.org/r/4521/#review4085

> My previous comments apply here.

Not resolved so much as proceeding as agreed in bugzilla/irc conversation.

(I don't know where else to add comments in this review board workflow, so here goes)

Only the last patch /r/4777 has any changes in it again, as before. I've addressed nits and added a replaceTrack-followed-by-renegotiation test. Byron please file a follow-up bug on remote-side track-renaming, and optimization.

Comment 72

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

Comment on attachment 8571739 [details]
MozReview Request: bz://1136871/jib

/r/4523 - imported patch rb4407.patch
/r/4525 - imported patch 1136871_testcase
/r/4527 - Bug 1136871 - test invalid replaceTrack arg
/r/4777 - Bug 1136871 - cleanup RtpSenders accounting to not rely on streams

Pull down these commits:

hg pull review -r 6e73e71d0804989a79cbefd99597d73d791672bd

Comment 73

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

Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=81fbdf0d10ae

Comment 74

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

https://reviewboard.mozilla.org/r/4521/#review4087

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
(Diff revisions 3 - 4)
> -    : SourceStreamInfo(aMediaStream, aParent, aId)
> +    : SourceStreamInfo(aMediaStream, aParent, aId),
> +      mPipelinesCreated(false)

FYI: This appears to be an artifact of review board. There is no such change in this patch.

Comment 75

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

I've updated the two versions of the PR:

https://github.com/w3c/webrtc-pc/pull/195
https://github.com/w3c/webrtc-pc/pull/196

Comment 76

[Martin Thomson [:mt:]]

Comment on attachment 8571739 [details]
MozReview Request: bz://1136871/jib

https://reviewboard.mozilla.org/r/4521/#review4089

::: dom/media/tests/mochitest/test_peerConnection_replaceVideoThenRenegotiate.html
(Diff revision 4)
> +    title: "Renegotiation: replaceTrack followed by adding a second video stream"

Check this title.

::: dom/media/tests/mochitest/test_peerConnection_replaceVideoThenRenegotiate.html
(Diff revision 4)
> +    test.setMediaConstraints([{video:true}], [{video:true}]);

I'd move this up above the call to addRenegotiation().  It's a little confusing with the current ordering.

Comment 77

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

https://reviewboard.mozilla.org/r/4521/#review4159

> Check this title.

What's wrong with it?

> I'd move this up above the call to addRenegotiation().  It's a little confusing with the current ordering.

Will do though the other renegotiation tests follow the same pattern.

Comment 78

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

https://reviewboard.mozilla.org/r/4521/#review4165

> What's wrong with it?

Ah, I see I need to:
-        function PC_LOCAL_REPLACE_VIDEO_TRACK_THEN_REMOVE_AUDIO_TRACK(test) {
+        function PC_LOCAL_REPLACE_VIDEO_TRACK_THEN_ADD_SECOND_STREAM(test) {

Comment 79

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

Comment on attachment 8571739 [details]
MozReview Request: bz://1136871/jib

/r/4523 - imported patch rb4407.patch
/r/4525 - imported patch 1136871_testcase
/r/4527 - Bug 1136871 - test invalid replaceTrack arg
/r/4777 - Bug 1136871 - cleanup RtpSenders accounting to not rely on streams

Pull down these commits:

hg pull review -r 15a54be6613f3968473e9134ac3c023c6f798509

Comment 80

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

Comment on attachment 8571739 [details]
MozReview Request: bz://1136871/jib

Carrying forward r=mt.

Comment 81

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ebb5fddda8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/d318975b8a35
https://hg.mozilla.org/integration/mozilla-inbound/rev/189fef0922c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/eed5b92642ef

Comment 82

[Maire Reavy [:mreavy]]

Hi Jib - Just doing a needinfo as a reminder to write the Aurora uplift approval for this.  Thanks!

Comment 83

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

Comment on attachment 8571739 [details]
MozReview Request: bz://1136871/jib

Approval Request Comment
[Feature/regressing bug #]: Bug 1017888
[User impact if declined]:
Tab sharing in Hello broken in 38. Any in-the-wild uses of RTCRtpSender.replaceTrack is likewise be affected, though I don't know of any such uses.
[Describe test coverage new/current, TreeHerder]: Landed on m-i with new tests.
[Risks and why]: 
Fix is well contained to the RtpSender.replaceTrack method being used for this, which without these fixes is pretty much broken. The method isn't used for much else. Worst case some SDP gets messed up only when this method is used, so I'm going to say low risk.
[String/UUID change made/needed]: none

Comment 84

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

Aurora request in comment 83 is for all patches in this bug.

Comment 85

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ebb5fddda8f
https://hg.mozilla.org/mozilla-central/rev/d318975b8a35
https://hg.mozilla.org/mozilla-central/rev/189fef0922c6
https://hg.mozilla.org/mozilla-central/rev/eed5b92642ef

Comment 86

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8571739 [details]
MozReview Request: bz://1136871/jib

Approving for uplift to 38 since we are aiming to ship this feature in 38 and it sounds like the effects of these changes will be isolated to WebRTC.

Comment 87

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/838d2dcbcb63
https://hg.mozilla.org/releases/mozilla-aurora/rev/fe2a63ef54e7
https://hg.mozilla.org/releases/mozilla-aurora/rev/d4a23c33ce27
https://hg.mozilla.org/releases/mozilla-aurora/rev/8bbd2e9d75f9

Comment 88

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e7f06142f3b5

Comment 90

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

Attachment: MozReview Request: Bug 1136871 - cleanup RtpSenders accounting to not rely on streams

Comment 91

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

Attachment: MozReview Request: imported patch rb4407.patch

Comment 92

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

Attachment: MozReview Request: imported patch 1136871_testcase

Comment 93

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

Attachment: MozReview Request: Bug 1136871 - test invalid replaceTrack arg

Comment 95

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1136871 - Part 2: Clear up some inconsistencies with ReplaceTrack.