replaceTrack and removeTrack: Synchronous?

[henbos]

Paraphrasing the spec ignoring some details... replaceTrack is asynchronous:

• In parallel, check that track has not ended and queue a task to set the track if the pc is not closed and resolve the promise.

The track could be muted before the queued task is executed, this could be a problem, but ignoring that for now.
removeTrack is synchronous:

• Set the sender's track to null, etc.

What is the expected behavior of this?

sender = pc.addTrack(t1);
sender.replaceTrack(t2);
pc.removeTrack(sender);

I imagine this will result in the following steps:

sender = sender with track = t1;  // pc.addTrack
sender.track = null;  // pc.removeTrack
sender.track = t2;  // replaceTrack

The removeTrack still changes the direction of the transceiver, so I suspect the renegotiation will cause the remote track to be removed regardless of the order of things, but there might be races with what sender.track is when renegotiating?

[henbos]

I argue that either all operations (addTrack, replaceTrack, removeTrack) should be synchronous or all operations should be asynchronous.

I don't see any benefit to having these be asynchronous. Can we make them all synchronous?

[alvestrand]

@fluffy @jan-ivar thoughts?

I don't remember why we decided that replaceTrack() should be asynchronous. AddTrack (which is synchronous) seems to be strictly more complicated than the others.

[henbos]

In the meantime I'm planning on implementing Chrome to do it synchronously and immediately resolve the promise.

[stefhak]

I have a faint memory that we thought step 10.1,
Determine if negotiation is needed to transmit withTrack in place of the sender's existing track.
could in some situations take some time being the reason for replaceTrack being asynchronous.

[aboba]

I agree that the actual steps outlined in the document do not appear to require asynchronous operation. As I recall, the argument for asynchronous operation mostly related to hardware encoders within media capture devices, under the theory that there might be an interaction between the negotiated codecs and the capabilities of the device hardware encoder. However, my own experience in general is that hardware encoders are backstopped by software encoders, so that if the negotiated codec isn't supported in hardware, software is used instead. This can easily be determined (e.g. you know what codecs the hardware encoder supports), so doesn't require asynchronous operation by itself.

[henbos]

I might be ignorant here, but why would changing a track require renegotiation? Isn't it just a different stream of images, and resolution, bitrate, etc, all handled by RTP packets and whatnot, not negotiated through SDP?

setParameters() on the other hand allows you to make changes to the encoder settings. I imagine getting capabilities of HW or SW to require thread/process jumps, and encoders could fail at any time.

Still isn't what the encoder is capable of a problem for createOffer() etc, when checking if negotiation is needed wouldn't you just look at the state of the current local description?

[stefhak]

I'd really like @jan-ivar and @fluffy to comment.

[henbos]

I was told changing the track might require changing encoder due to limitations to what can be encoded, and what is negotiated would be the remote description, not local, but my above question remains the same: Is getting capabilities a relevant step to determining if negotiation is needed for replaceTrack/setParameters?

[jan-ivar]

I argue that either all operations (addTrack, replaceTrack, removeTrack) should be synchronous

I don't think we should entertain any symmetry here. addTrack adds a transceiver, removeTrack takes a sender as argument, but doesn't undo addTrack, and they're both synchronous methods on the pc that change local state only and fire the negotiationneeded event (on the pc).

In contrast, replaceTrack is on the sender, asynchronously replaces what's being transmitted - which is observable remotely - before it resolves, and doesn't fire negotiationneeded.

there might be races with what sender.track is when renegotiating?

createOffer and createAnswer are impervious to races, so I don't see a problem here.

As to whether we should rethink the replaceTrack API, I'm not hearing any new arguments that weren't discussed 3 years ago when Firefox implemented it.

In the meantime I'm planning on implementing Chrome to do it synchronously

That would observably be in violation of the spec, since the spec says sender.track must be unchanged when the method returns, so I'm hoping we can avoid that.

[henbos]

My main concern was if replaceTrack() would "undo" part of what removeTrack() was doing, like setting the track after it was null, which would arguably lead to unexpected behavior, whether or not racy. If this is a problem though, replaceTrack() could abort if it notices it has been removed when dispatching. I may be prematurely alarmed.

In the meantime I'm planning on implementing Chrome to do it synchronously

That would observably be in violation of the spec, since the spec says sender.track must be unchanged when the method returns, so I'm hoping we can avoid that.

Ack. I ran into problems because until Chrome supports transceivers, senders are removed (not just inactivated) on removeTrack(), but I should be able to get around those problems if async is the right approach.

From 3 years ago:

However, in our implementation, it's likely that confirming that a
track is a compatible replacement could require asynchronous
dispatches to a separate thread. Blocking the main processing thread
for a synchronous dispatch would be very bad. In general, I'd prefer
to have things that need to look at media be asynchronous to avoid any
risk that a synchronous dispatch is needed.

In an earlier discussion Harald brought up the example of cameras that
have built in encoders. If you switch track, isn't there a risk that the
new track is sourced by a camera that produces a format that can be
handled locally, but not by the remote end - and that won't be found out
until after an SDP O/A?

Sounds good, I suspect this has already been thought of and replaceTrack() should remain asynchronous.
But I still don't understand one crucial thing: Why does confirming that a track produces a compatible format require any interaction with an encoder (or any other thread), if which formats are compatible have already been negotiated and decided on setRemoteDescription()? Would this not be an "if format is in list of formats" type of operation?

[jan-ivar]

setting the track after it was null, which would arguably lead to unexpected behavior, whether or not racy

Most async methods are spec'ed to modify JS state on success, so this doesn't seem unexpected to me, much like both of these produce the same result:

let x;
Promise.resolve().then(() => x = 1);
x = 2;

let x = 2;
Promise.resolve().then(() => x = 1);

Similarly, both of these produce the same result:

sender = pc.addTrack(t1);
sender.replaceTrack(t2);
pc.removeTrack(sender);

sender = pc.addTrack(t1);
pc.removeTrack(sender);
sender.replaceTrack(t2);

What may be unexpected is that removeTrack is just a glorified setter for track and direction.

In practice, I expect racing like this is usually a bug, and best avoided:

sender = pc.addTrack(t1);
await sender.replaceTrack(t2);
pc.removeTrack(sender);

[jan-ivar]

Would this not be an "if format is in list of formats" type of operation?

I think the concern was whether any (off-main-thread) examination of the source would be needed to determine compatibility against each listed codec.

FWIW I believe all browsers today internally deal with raw frames only, where this isn't an issue.

[aboba]

@henbos Edge implemented setTrack (the original name for the method) synchronously 3 years ago, and we have had no issues.

[stefhak]

FWIW, it seems to me that replaceTrack does quite a bit of things (I mean that the content of the RTP packets going over the wire change), while addTrack does almost nothing (it mostly returns a sender, but that sender's transports are null). To get anything out of an addTrack you need to do the createOffer, setLocal..... things.

As @jan-ivar points out in #1677 (comment) there can be cases when an off-main-thread examination of the source would be needed for replaceTrack.

Are there strong reasons why we would change replaceTrack?

[henbos]

Are there strong reasons why we would change replaceTrack?

No, feel free to close this.
I was concerned about mixing synchronous and asynchronous operations yielding surprising results, but this is well-defined and there is already precedence for it (e.g. most async operations and pc.close()).

This does expose another example of how Chromium does async operations incorrectly (https://crbug.com/webrtc/8692) but that needs to be addressed regardless.

[stefhak]

@aboba unless you disagree I'll close this Issue per #1677 (comment)

[aboba]

@stefhak I think you can close it.