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

Unassociated, stopped transceivers after createOffer: Present or absent? #2576

Closed
alvestrand opened this issue Sep 21, 2020 · 9 comments
Closed

Comments

@alvestrand
Copy link
Contributor

Consider this code snippet:

const pc = new RTCPeerConnection();
const transceiver = pc.addTrack('audio');
pc.addTrack(video);
transceiver.stop();
await pc.setLocalDescription(await pc.createOffer()); // Offer is video only
// MYSTERY PLACE
await pc.setRemoteDescription(remoteAnswer); // Answer is also video only. Creating answer left out for compactness.
assert_equals(pc.getTransceivers().length, 1);    // Only video remains
assert_equals(transceiver.currentDirection, null);  // The stopped one has never been used.

Now: In the MYSTERY PLACE, what length shoiuld pc.getTransceivers() return?

According to the spec-as-written, it should return 2 - the stopped one and the video one - because stopped transceivers are ONLY removed from the set of transceivers when applying answers. Not when creating offers.

This means that pc.getTransceivers() will contain an element that is irrelevant to the current offer/answer exchange.
Is that a correct reading of the spec?
Is that a reasonable behavior?

Two possibilities (if correct):

  • It is correct and reasonable. No change needed.
  • It is correct, but not reasonable. Modify pc.setLocalDescription to discard stopped transceivers that have never been associated with a media section.

Discuss.

@henbos
Copy link
Contributor

henbos commented Sep 22, 2020

I believe your interpretation of the spec is correct.

And I believe that yes, the transceiver is utterly pointless to have between SLD and SRD, but I still think it is reasonable to only remove transceivers when applying the answer (no spec change needed).

Why?

Because "stopped" is something that you normally have to negotiate, so all other transceivers are only to be replaced at SRD. Yes, in this edge case we could remove it earlier, like an optimization, but I think having removal happen at different times depending on edge case conditions makes the API less predictable to use. After all, the transceiver doesn't do any harm by existing until the next answer.

And because even a stopped transceiver has an effect on number of m= sections that exists, I think it should stick around until the negotiation is complete.

An alternative that I think would perhaps make sense, is if the deletion happens even earlier: As soon as you invoke stop(). In this case you wouldn't have to negotiate an inactive/stopped m= section, and it would truly be as if the transceiver never existed in the first place. In fact, you can think of scenarios where you avoid onnegotiationneeded and an event firing!

@henbos
Copy link
Contributor

henbos commented Sep 22, 2020

So, what about removing the unassociated transceiver as soon as stop is called? "If it doesn't have a mid, kill it quickly."

Arguments against this:

  • More edge cases makes the API unpredictable.
  • Not clear if there is a valid use case. The simple case of creating the transceiver only to stop it prior to the initial offer would be "stupid use of API" rather than a real use case.

Arguments for this and possible use case:

  • I create a transceiver for sending, but before I have a chance to negotiate this I get a remote offer that creates a sendrecv transceiver. Realizing I no longer need my transceiver, I stop it, and I complete the negotiation process. The plan is to use the remote sendrecv transceiver for sending instead. If my transceiver was instantly removed, there wouldn't be a pointless follow-up offer. If my transceiver wasn't removed, I'd have to do a follow-up O/A exchange that says "here's an inactive and stopped m= section for you, there you go"

@henbos
Copy link
Contributor

henbos commented Sep 22, 2020

Discuss...

@alvestrand
Copy link
Contributor Author

An unassociated stopped transceiver won't have any effect on outgoing offers, according to JSEP:

https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-25#section-5.2.1 (Initial offers)

"The next step is to generate m= sections, as specified in [RFC4566],
Section 5.14. An m= section is generated for each RtpTransceiver
that has been added to the PeerConnection, excluding any stopped
RtpTransceivers"

5.2.2 Subsequent offers

o If an RtpTransceiver is stopped and is not associated with an m=
section, an m= section MUST NOT be generated for it. This
prevents adding back RtpTransceivers whose m= sections were
recycled and used for a new RtpTransceiver in a previous offer/
answer exchange, as described above.

If we add the words "if the transceiver is not associated with a media section, remove it from the set of transceivers" to the procedure for transceiver.stop(), I think we end up in a less confusing place.

@henbos
Copy link
Contributor

henbos commented Sep 22, 2020

OK. That might make sense. What's @jan-ivar's take?

@jan-ivar
Copy link
Member

Having tranceivers sometimes (but not always) disappear on return from transceiver.stop() might be more confusing, not less.

Unless someone can point to some assumption that falls apart, it seems less surprising for the stopped transceiver to linger and disappear at answer time, just like it would were it stop()ed later.

To address the two arguments for a change:

This means that pc.getTransceivers() will contain an element that is irrelevant to the current offer/answer exchange.

Sure, but that's not unusual, since transceivers can be added by JS at any time, whether there's an ongoing O/A or not.

If my transceiver was instantly removed, there wouldn't be a pointless follow-up offer. If my transceiver wasn't removed, I'd have to do a follow-up O/A exchange that says "here's an inactive and stopped m= section for you, there you go"

As @alvestrand points out above this does not cause a follow-up O/A, so not an issue.

So I don't think I see enough arguments here for a change (which would be a normative one).

JSEP clearly tolerates the presence of stopped transceivers, which I interpret to mean that having them linger was by design.

@alvestrand
Copy link
Contributor Author

From a browser-code-practical standpoint, I'm happy to have them linger, since that's what I've implemented now (and changed the WPT tests to tolerate).

I think JSEP was written without thought for letting transceivers disappear; we added disappearing at S*D(answer) later in order to permit stopped transceivers to be garbage collected; otherwise the collection would grow forever. I could argue that this was a new principle of "transceivers disappear when they're no longer relevant". So I think the current state is messy.

But I can easily live with them lingering until O/A completes.

@henbos
Copy link
Contributor

henbos commented Sep 23, 2020

If the unassociated stopped transceiver doesn't cause ONN to fire (do we have a WPT?), i.e. a pointless follow-up O/A is not an issue, then I agree there is little point to adding this special case logic.

In the use case I described, where you create a sendonly transceiver but get a remote offer that creates a sendrecv transceiver so you use that instead and stop the first transceiver, this will get removed as soon as you apply the answer it sounds like, which means the transceiver isn't really lingering on until another O/A exchange in this case. Because you're already performing an O/A exchange.

Now there is an edge case where you create and stop the transceiver before any offer is being made, but I cannot think of any real use case where you would do this.

I'm happy to close this issue then.

@alvestrand
Copy link
Contributor Author

OK, closing issue as "no change needed".

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

No branches or pull requests

3 participants