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

Reconsider replaceTrack's blocking on the operations queue. #3017

Open
jan-ivar opened this issue Nov 4, 2024 · 5 comments
Open

Reconsider replaceTrack's blocking on the operations queue. #3017

jan-ivar opened this issue Nov 4, 2024 · 5 comments

Comments

@jan-ivar
Copy link
Member

jan-ivar commented Nov 4, 2024

replaceTrack says: "6. Return the result of chaining the following steps to connection's operations chain:"

We don't have a test for this so I wrote one. Firefox and Safari pass, but Chrome fails it:

FAIL replaceTrack should block on the operations queue: - Expected haveSLD, got haveReplaced failed

This suggests Chrome is doing the remaining steps right away rather than chaining them on the operations chain. This timing difference might cause subtle interop problems.

The Chrome behavior has some advantages, like negotiation not being able to interfere with (delay) replaceTrack.

This begs the question what this was supposed to solve in the first place.

In #1769 back in 2018, we seemed preoccupied with updating sender.track in time for createOffer or createAnswer. Presumably this mattered for track ids in the a=msid at the time. Since then, it's understood that using track ids to "correlate local and remote tracks" no longer works. So this reason no longer seems to hold.

I propose we remove this requirement and align with Chrome's behavior.

@alvestrand
Copy link
Contributor

alvestrand commented Nov 4, 2024

Fails on Chrome (if I read it correctly).
The purpose of blocking on the operations queue was to make it deterministic whether the SLD wouild operate on the tracks current when we called SetLocalDescription or the tracks current after ReplaceTrack. Seems that not doing so is a bug. Should we ask for the test to be submitted to WPT and a bug to be filed on Chromium?
Tagging @henbos for the obvious reasons :-)

@jan-ivar
Copy link
Member Author

jan-ivar commented Nov 4, 2024

The purpose of blocking on the operations queue was to make it deterministic whether the SLD wouild operate on the tracks current when we called SetLocalDescription or the tracks current after ReplaceTrack. Seems that not doing so is a bug.

I can't seem to find a case where this matters in any browser. https://jsfiddle.net/jib1/Ltsed06f/10 creates a transceiver with a null track, but waits for replaceTrack(track) to finish before first negotiation, yet the msid line does not contain that late-added track's id (it contains a random id instead).

Firefox always creates a random track id in the msid, unless you set media.peerconnection.sdp.encode_track_id = true in about:config. So if any website is relying on msid track ids to match, they'd be broken in Firefox, and I think we would have heard about it.

@alvestrand
Copy link
Contributor

actually stream ids is the part of msid that is not random these days.
The fiddle run on Chrome doesn't seem to log the msid lines despite the msid lines being present in the SDP (as checked by chrome://webrtc-internals), so there may be bugs in the fiddle too.

@jan-ivar
Copy link
Member Author

jan-ivar commented Nov 5, 2024

actually stream ids is the part of msid that is not random these days.

Right, and replaceTrack does not alter stream ids.

The fiddle run on Chrome doesn't seem to log the msid lines despite the msid lines being present in the SDP (as checked by chrome://webrtc-internals), so there may be bugs in the fiddle too.

That's strange, it WFM. Here's a version without the msid filter and without adapter: https://jsfiddle.net/jib1/Ltsed06f/13/

  await pc1.setLocalDescription();
  pc1.localDescription.sdp
    .split('\r\n')
    .slice(1)
    .forEach(l => console.log(l));

If you see a bug in it please LMK.

@dontcallmedom-bot
Copy link

This issue had an associated resolution in WebRTC November 19 2024 meeting – 19 November 2024 (Issue #3017: Reconsider replaceTrack's blocking on the operations queue):

RESOLUTION: move with forward with PR to align spec with Chrome behavior

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

No branches or pull requests

4 participants