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

Differences between pc.removeTrack(sender) and sender.replaceTrack(null) #2024

Open
ibc opened this Issue Nov 5, 2018 · 11 comments

Comments

Projects
None yet
7 participants
@ibc
Copy link

commented Nov 5, 2018

The spec does not make clear whether a transceiver can be reused for sending a new track (or the previous one) after having called pc.removeTrack(transceiver.sender).

Assuming a simple scenario with a single transceiver for just sending audio (don't receive anything).

  • Q1: Is sender.replaceTrack(track) allowed after pc.removeTrack(sender)?

    • Chrome and Firefox allow it. Safari fails, but it seems they are fixing it [1][2].
  • Q2: Should sender.replaceTrack(null) automatically set a=inactive?

    • Browsers don't do this and direction remains "sendonly".
  • Q3: Is there any constraints regarding when transceiver.direction can be changed by the app? According to the spec there is not, and both Chrome and Firefox never complain, but Safari 12.1 produces an exception when setting the direction in certain circumstances (hard to explain them since it depends on different factors and probably it's just a bug).

  • Q4: Should the browser keep sending RTCP SDES and RTCP SenderReports for the corresponding ssrc(s) after calling sender.replaceTrack(null)?

    • Chrome still sends both SDES and SenderReports.
    • If transceiver.direction is manually set to "inactive", then Chrome just sends SDES.
  • Q5: Should the browser keep sending RTCP SDES for the corresponding ssrc(s) after calling pc.replaceTrack(sender)?

    • Chrome does. Firefox does not.
  • Q6: If the answer to Q1 is yes, what's the purpose of pc.removeTrack(sender) having already sender.replaceTrack(null)? Just those items come to my mind:

    • pc.removeTrack(sender) cleans RtpSender settings (such as configured sending parameters, used SSRCs, stats, etc).
    • sender.replaceTrack(null) does not, so attaching a new track to the sender will send it with previous sender configuration/encodings.

[1] https://bugs.webkit.org/show_bug.cgi?id=191202
[2] https://bugs.webkit.org/attachment.cgi?id=353842&action=diff

@ibc ibc referenced this issue Nov 5, 2018

Closed

Moving to Unified-Plan (list of issues) #50

11 of 16 tasks complete
@steveanton

This comment has been minimized.

Copy link

commented Nov 5, 2018

As far as I can tell removeTrack == replaceTrack(null) plus removing the 'send' component from the RtpTransceiver's direction (and flipping the negotiation-needed flag).

@ibc

This comment has been minimized.

Copy link
Author

commented Nov 5, 2018

As far as I can tell removeTrack == replaceTrack(null) plus removing the 'send' component from the RtpTransceiver's direction (and flipping the negotiation-needed flag).

Yes, but what does "removing the 'send' component from the transceiver" mean? I mean: can I reuse the same transceiver.sender later to send a new (or the previous) track using transceiver.sender.replaceTrack(track) or not?

  • As told above, this works fine in Chrome and Firefox.
@docfaraday

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2018

* Q1: Is `sender.replaceTrack(track)` allowed after `pc.removeTrack(sender)`?

Spec seems to say yes; removeTrack just sets the sender's track to null, and unsets the send bit. Then, replaceTrack sets the track, but does not alter the direction.

* Q2: Should `sender.replaceTrack(null)` automatically set `a=inactive`?

I don't think replaceTrack is intended to automatically change the direction.

* Q3: Is there any constraints regarding when `transceiver.direction` can be changed by the app? According to the spec there is not, and both Chrome and Firefox never complain, but Safari 12.1 produces an exception when setting the `direction` in certain circumstances (hard to explain them since it depends on different factors and probably it's just a bug).

App should be free to alter direction at will, although maybe I could see throwing an error if the transceiver is stopped.

As for questions about what replaceTrack and removeTrack mean for things like RTCP, ssrcs, and so forth, I think you're right that the spec could use some clarification.

@ibc

This comment has been minimized.

Copy link
Author

commented Nov 5, 2018

Thanks @docfaraday.

@aboba aboba added the question label Nov 8, 2018

@henbos henbos self-assigned this Nov 8, 2018

@alvestrand

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

replaceTrack is not supposed to cause negotiationNeeded. Changing the direction would require negotiationNeeded.
removeTrack is allowed to cause negotiationNeeded, so it can modify the direction.

I think the steps in removeTrack (http://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-removetrack) are supposed to be exactly equivalent to replaceTrack(null) + set direction and update the negotiation-needeed flag.

The "enuqueued steps" (step 6) in replaceTrack (http://w3c.github.io/webrtc-pc/#dom-rtcrtpsender-replacetrack) reduce to "set sender's track attribute to null" when withTrack is null - but do have the "stop sending" be explicit. Perhaps this should be a common algorithm called by both removeTrack and replaceTrack?

(Another difference is that removeTrack is synchronous, while replaceTrack has two levels of "queue a task". Do we care?)

@aboba

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

Q4: sender.replaceTrack(null) causes the sender to stop sending, but continuing to send RTCP SDES and sender reports makes it clear that the sender has not disappeared and could resume. So Chrome behavior seems reasonable.

Q5: sender.replaceTrack(withTrack) just replaces the track but keeps the settings, so it shouldn't affect SSRCs and RTCP should continue operating as before.

Do you think we should add clarifications relating to these?

@henbos

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

Slides added in the form of a Q&A. If we want to define removeTrack() by invoking replaceTrack() + setting the direction or adding a clarification I think that is an editorial issue.

@henbos

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

(Another difference is that removeTrack is synchronous, while replaceTrack has two levels of "queue a task". Do we care?)

I guess this optimization is possible because removeTrack() does not need to acquire new encoding resources. replaceTrack(null) could probably special-case to be synchronous, but it already returns a promise, so I think it would just be confusing. "Do we care?" Meh. But considering this is all shipped, we shouldn't change the async/sync behaviors.

@henbos

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

I think adding a "NOTE" that says "this is basically the same as replaceTrack() + setting the direction (with caveats)" is enough to resolve the confusion.

@henbos henbos added the Editorial label Mar 12, 2019

@jan-ivar

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

FWIW I think the spec is super-clear how to implement pc.removeTrack(sender). It's just counterintuitive, a misnomer. Unsure what note will solve that except "No, really". I mean it takes a sender...

It's there mostly for legacy reasons and best ignored IMHO. Might this be better covered under examples? I think it's fair to say that (with transceiver.direction == "sendrecv" as a starting point):

pc.removeTrack(transceiver.sender);

is a synchronous version of:

transceiver.direction = "recvonly"; await transceiver.sender.replaceTrack(null); 

Or imagine we had a synchronous setter that threw on anything but null:

transceiver.direction = "recvonly"; transceiver.sender.track = null; // null or TypeError 

This is possible because sender.replaceTrack(null) cannot fail, and could have been done synchronously, whereas sender.replaceTrack(withTrack) cannot.

replaceTrack(null) could probably special-case to be synchronous, but it already returns a promise, so I think it would just be confusing.

It was like that before we fixed it in #1769.

@henbos

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.