Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove paragraph about removeTrack causing track to be ended remotely. #1168

Merged
merged 1 commit into from
Jun 15, 2017

Conversation

taylor-b
Copy link
Contributor

@taylor-b taylor-b commented May 5, 2017

This no longer happens, because you can still call replaceTrack on the
RTCRtpSender, and thus the track tied to the RTCRtpReceiver on the
remote side is not fully dead yet. The remote track is never actually
ended unless the transceiver is stopped/m= section rejected, since
that's the only irreversible change.

This paragraph dates back to before we had transceivers/RtpSenders, so
I believe it's just obsolete and hasn't been noticed until now.

This no longer happens, because you can still call replaceTrack on the
RTCRtpSender, and thus the track tied to the RTCRtpReceiver on the
remote side is not fully dead yet. The remote track is never actually
ended unless the transceiver is stopped/m= section rejected, since
that's the only irreversible change.

This paragraph dates back to before we had transceivers/RtpSenders, so
I believe it's just obsolete and hasn't been noticed until now.
@jan-ivar
Copy link
Member

jan-ivar commented May 5, 2017

(I deleted earlier comments where I was confused) Doing a sanity recap:

In my book we've got 3 fundamentally different versions of this API:

  1. streams (addStream/removeStream) (Chrome)
  2. tracks (addTrack/removeTrack) (Firefox)
  3. transceivers (addTransceiver) (Nobody)

A difference between pc.removeTrack(sender); pc.addTrack(withTrack) and await sender.replaceTrack(withTrack) is that the former renegotiates (assuming pc.onnegotiationneeded is set up properly) whereas the latter does not.

So, with pc.removeTrack(sender); await wait(10000); pc.addTrack(withTrack) I think the big difference is that for 10 seconds I'd expect there to be no track, not a black track, remotely.

Specifically, I'd expect a renegotiation to fire ended on my old track, which would be removed from the peerconnection-managed stream, then 10 seconds later, another renegotiation would trigger atrack event on my peer connection with my "new" track, now added to the peerconnection-managed stream.

This was certainly the expectation before transceivers.

Maybe receiver.track should be nullable after all?

@taylor-b
Copy link
Contributor Author

taylor-b commented May 5, 2017

I think we can do what you describe, but without ending or replacing the RTCRtpReceiver's track. You would just get a trackremoved event followed by a track event 10 seconds later with the same track.

Making receiver.track nullable would be a pretty big change, though. It could also break some early media use cases; maybe the application wants to keep the remote track attached, even after it's removed on the sender side, so it can be re-added seamlessly.

@jan-ivar
Copy link
Member

jan-ivar commented May 5, 2017

maybe the application wants to keep the remote track attached, even after it's removed on the sender side, so it can be re-added seamlessly

@taylor-b Isn't that what replaceTrack is for?

@jan-ivar
Copy link
Member

jan-ivar commented May 5, 2017

Let me ask differently: What if the application wants to end the remote track? How does it do it?

@taylor-b
Copy link
Contributor Author

taylor-b commented May 5, 2017

Isn't that what replaceTrack is for?

Meaning, why not just do replaceTrack(null) and later replaceTrack(newTrack)? Good question; I think the only real difference is the directional attribute in SDP, which could affect some things.

What if the application wants to end the remote track? How does it do it?

The only way right now is with transceiver.stop, as far as I know.

If we want to change the way senders' and receivers' tracks relate (which I tried to summarize here: #1169), it may still be be possible since Chrome/Firefox haven't implemented it yet. But it would be a somewhat significant change to the spec.

@jan-ivar
Copy link
Member

jan-ivar commented May 5, 2017

To be clear, this is a breaking change. The following works in both Chrome and Firefox. That is: ended fires on renegotiation.

It's unfortunate that we didn't catch this earlier when we added transceivers, but we've caught it now. I think we need to discuss how to fix transceivers to not break this.

@fippo
Copy link
Contributor

fippo commented May 9, 2017

I still think this is plain wrong. So we have moved away from events firing on the track to explicitly surfacing an event on the peerconnection in case of an audio-only call upgrading to an audio-video one. What used to be this

pc.setRemoteDescription(type=offer, audio-sdp)
addStream with a stream that has only only
...
pc.setRemoteDescription(type=offer, audiovideo-sdp)
stream.onaddtrack fires

is now surfacing addtrack

pc.setRemoteDescription(type=offer, audio-sdp)
pc.ontrack(audiotrack)
...
pc.setRemoteDescription(type=offer, audiovideo-sdp)
pc.ontrack(videotrack)
stream.onaddtrack fires

This is great because it increases transparancy of what happens on the peerconnection.
Note that we have not removed the stream.onaddtrack. More on that later.

Now if we were to remove the requirement that ended fires when the track is removed remotely there is a different area where we use the same pattern: datachannels. you only get a signal for adding one (ondatachannel) and then need to listen for its onclose method. If you remove this design pattern from MediaStreamTrack it should also be removed from the datachannel and I think we agree that nobody wants that (hopefully)

@taylor-b
Copy link
Contributor Author

taylor-b commented May 9, 2017

But data channels are symmetrical between the two PeerConnection instances, while tracks (since transceivers were introduced) are not. So I don't think it's fair to compare them.

@aboba
Copy link
Contributor

aboba commented Jun 8, 2017

I believe that there was agreement at the virtual interim that this paragraph was a leftover.

@stefhak
Copy link
Contributor

stefhak commented Jun 15, 2017

Looks fine to me. Though, as discussed at the June interim, we should get counters in browsers to learn how much breakage this causes.

@aboba aboba merged commit cc8635f into w3c:master Jun 15, 2017
@jan-ivar
Copy link
Member

I think this is still tied up with #1181 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants