-
Notifications
You must be signed in to change notification settings - Fork 32
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
removeTrack(track) then addTrack(track) yields bogus SDP #842
Comments
I think the way out of this is to randomly generate the track id portion of a=msid attributes. They don't really mean much anyhow. |
Example. Works in Chrome and Firefox 57, but not Firefox Nightly which implements transceivers. |
Is this a spec bug, or an implementation bug? cc @pthatcherg |
The spec requires putting the dupe track id in the SDP. The spec might not require reacting badly to bad SDP, but the sad fact is we need to deal with backwards compat. Maybe the way forward is to accept the fact that track ids are not signaled reliably enough to make them useful to the other end, and say that track ids in SDP take an implementation-defined value (what we need to mitigate this backward compat ugliness), with a SHOULD-level requirement to include no value at all (what we should have done in the first place). Thoughts? |
https://tools.ietf.org/html/draft-ietf-mmusic-msid-16#section-3 doesn't allow you to leave the msid in place in a subsequent offer. That was added later, to allow replaceTrack() to be negotiation-free and not noticeable on the receiver end. sender1 = pc.addTrack(A) sender1 = pc.addTrack(A) Told you replaceTrack was complicated. |
Ok, so we have two specs that need fixing here. What's the fix? |
By the way, this msid stuff is busted even if you remove replaceTrack and the requirement that a=msid be invariant. The msid spec says that removing the a=msid means the track has ended, but removeTrack isn't really ending the track anymore, it is pausing it. We should never have put track ids in SDP, I think. Rip them out, and most of these problems simply disappear. |
Yeah, e.g. if I add |
MSID response: yes, this is busted. Three options: I agree that a) seems most aesthetically pleasing, but unsure what breakage this will cause. |
the whole thread feels like we're punishing ourselves for previous suboptimal choices. The msid spec said that you remove the msid attribute from an m-section when the track is removed from it. And by design, it doesn't forbid you from putting the same msid in multiple m-sections - it even says out loud that this means you're sending the track multiple times (unless that wording has been removed in a later edit - it used to be there) (but we have forbidden that in webrtc-pc for AddTrack). it's replaceTrack that causes the issues here. But back to basics: Why are collisions a problem, again? |
Let's say having duplicates is ok because it means you're sending the same thing more than once. None of the scenarios above are actually sending the same track, though, so we're right back to having bogus SDP. I should also reiterate that you can get into one of these situations without using replaceTrack at all, by calling removeTrack(t) then addTrack(t). |
If we have two identical MSIDs for different transceivers, should this result in distinct MSTs at the receiver side with the same ID? It seems confusing, to say the least. |
you can tell them apart in ontrack at least by looking at event.transceiver |
Of course, but this breaks any app that is trying to just use track.id to correlate tracks. Having thought about this a bit more, I think we should just mark the msid and id-passing functionality as deprecated, and advise people not to use it, but continue to support it to avoid breaking existing apps. The recommended solution should be what you propose, i.e., matching transceivers (using event.transceiver), not tracks. |
FWIW the msid system still correlates stream ids, which is arguably useful, and necessary to make sure remote tracks end up in the same stream when they should. |
Having an "a"=streamid" field is useful. Having "a=msid" (which contains both the stream and track id) is not. |
@jan-ivar is #842 (comment) an argument for going back to just use the MediaStream id in msid (if memory serves msid originally just carried that info, track id was added later)? |
I am in favor of just using a=msid:<stream-id>, personally, although this could cause some (more) interop headaches. |
We've talked about this before (specifically, at the September 2017 interim), and back then people were supportive of the idea to always generate the We're going to have to deal with interop headaches either way. If you follow that last link, I explain how in most cases, the track ID from "a=msid" will end up ignored. It's only picked up if you call |
Completely agreed with @taylor-b. The spec should never allow a remote decide the |
OK, sounds like we're agreed on just going with a=msid:<stream-id>. This will require a JSEP document change. |
@juberti Can you clarify what you mean by "just going with a=msid:"? |
oops, I flubbed the markdown. PTAL at the comment again, I meant to say a=msid:<stream-id>. |
@alvestrand or @juberti: Do you plan to update draft-ietf-mmusic-msid? Does this need more discussion on the mailing list first (and if so, should I get the ball rolling)? |
Bump! I'm surfacing transceivers in Chrome and this issue is affecting me in my work-in-progress CL. We have a test that adds a track, removes it, and then adds it again, causing createOffer() to fail because of this (https://crbug.com/webrtc/8734). I believe a lot of applications mute and unmute by removing and re-adding tracks, so not being able to do that would lead to pretty widespread regressions in switching from Plan B to Unified Plan. What do we need to do to get the ball rolling? |
I don't think we have to remove track ID from a=msid to fix that; we can just start randomly generating track IDs all the time (which would be a sort of "soft" removal of track ID signaling). But I'll try making an updated version of draft-ietf-mmusic-msid; I think we've already discussed this enough and someone just needs to make a proposal. |
MSID carried track ID from day one. We're reopening a design choice made in 2013. |
The original error here is that the MSID spec, which specifies that you remove the msid line when removeTrack() has been called, has not been followed. Why don't we fix that first, and see if that solves the problem? |
The remote track survives removeTrack. Its id will match MSID only ~25% of the time. Why signal MSID? |
See https://blog.mozilla.org/webrtc/rtcrtptransceiver-explored/ for sender<->recever.track 1-1. |
Fixes rtcweb-wg#842. And also w3c/webrtc-pc#1718. Since introducing transceivers, replaceTrack, early media, etc., the definition of a MediaStreamTrack has changed and track ID signaling has become somewhat pointless. In many cases, "sender.track.id" on one side will not match "receiver.track.id" on the other side; endpoints must use MIDs or m= section indices or some other means to identify which track is which. Thus this PR just removes track ID signaling altogether and the lingering problems it causes.
Status? |
Once a track has been negotiated, calling removeTrack on it does not remove the a=msid attribute from the SDP. A subsequent addTrack(track) will result in a new transceiver being created, resulting in an additional m-section with the same a=msid line. It is even possible for both m-sections to be negotiated to be sendrecv or sendonly if you give the old transceiver a track with replaceTrack.
The text was updated successfully, but these errors were encountered: