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

Changing RTCRtpTransceiver direction from recvonly to sendrecv on the answerer side doesn't fire negotiationneeded event #2919

Closed
mickel8 opened this issue Jan 1, 2024 · 9 comments

Comments

@mickel8
Copy link

mickel8 commented Jan 1, 2024

5.3.3 in checking if negotiation is needed says:

If description is of type "answer", and the direction of the associated m= section in the description does not match transceiver.[[Direction]] intersected with the offered direction (as described in [RFC8829] (section 5.3.1.)), return true.

This means that if offerer offered sendonly, answerer answered with recvonly and then the answerer changed its transceiver's direction to sendrecv, negotiationneeded event won't be fired.

The intersection of transceiver direction with the offered direction is recvonly (sendrecv x sendonly = recvonly) and the direction in the answer is also recvonly so there is no change.

Currently, browsers do the opposite thing i.e. they fire negotiationneeded event when the described situation occurs.

What's the expected behaviour?

Code:

pc1 = new RTCPeerConnection();
pc2 = new RTCPeerConnection();

pc1.onnegotiationneeded = _ => console.log("onnegotiationneeded1");
pc2.onnegotiationneeded = _ => console.log("onnegotiationneeded2");
tr = pc1.addTransceiver("audio", {direction: "sendonly"});
offer = await pc1.createOffer();
await pc1.setLocalDescription(offer);
await pc2.setRemoteDescription(offer);
answer = await pc2.createAnswer();
await pc2.setLocalDescription(answer);
await pc1.setRemoteDescription(answer);

console.log("changing pc2 tr direction");
pc2.getTransceivers()[0].direction = "sendrecv";
@alvestrand
Copy link
Contributor

alvestrand commented Jan 1, 2024

After negotiation, pc2.transceivers[0] in "description" is "recvonly" (since pc1 has it as "sendonly" and pc2 didn't set it).

Then you change it to "sendrecv". pc2.transceivers[0].[[Direction]] is now "sendrecv".

The intersection of "sendrecv" and "recvonly" is "recvonly". This does not match transceiver.[[Direction]], which is "sendrecv".

Therefore "true" should be returned. Current behavior is correct.

(Suggestions for language to make this more obvious are welcome, of course)

@mickel8
Copy link
Author

mickel8 commented Jan 2, 2024

Assuming the following labels:

tr_direction - transceiver's direction
local_mline_direction - transceiver's direction in current local description
remote_mline_direction - transceiver's direction in current remote description

What is the algorithm when current local description is of type "answer"?

I understand this

If description is of type "answer", and the direction of the associated m= section in the description does not match transceiver.[[Direction]] intersected with the offered direction (as described in [RFC8829] (section 5.3.1.)), return true.

where "description is described as:

Let description be connection.[[CurrentLocalDescription]].

as:

  1. Calculate intersection of tr_direction and remote_mline_direction and save the result as "x"
  2. Return x == local_mline_direction

while the reasoning you described sounds like

  1. Calculate intersection of tr_direction and local_mline_direction and save the results as "x"
  2. Return x == tr_direction

@fippo
Copy link
Contributor

fippo commented Jan 2, 2024

fiddles FTW https://jsfiddle.net/fippo/0dxnwuyc/

Firing ONN is what needs to be done since you need to create an offer to get out of the recvonly state.

@mickel8
Copy link
Author

mickel8 commented Jan 2, 2024

fiddles FTW https://jsfiddle.net/fippo/0dxnwuyc/

I'm sorry, will remember next time!

Firing ONN is what needs to be done since you need to create an offer to get out of the recvonly state.

I totally agree. My concern is that, this doesn't arise from the W3C specification or I read it incorrectly. Splitting the W3C algorithm into pieces and looking from the pc2 perspective:

Let description be connection.[[CurrentLocalDescription]].
...
If description is of type "answer", and the direction of the associated m= section in the description does not match transceiver.[[Direction]] intersected with the offered direction (as described in [RFC8829] (section 5.3.1.)), return true.

--

If description is of type "answer"

That's true, "description" from the pc2 perspecitve, is of type "answer"

the direction of the associated m= section in the description

I mark this as local_mline_direction and it is equal to recvonly

transceiver.[[Direction]] intersected with the offered direction

I mark this as intersection_direction and it is equal to sendrecv intersected with sendonly (as the other side offered sendonly in the recent exchange). sendrecv x sendonly = recvonly (from pc2 perspective, so pc2 can only receive as the other side said it can only send)

At the end of the day, local_mline_direction == intersection_direction so we should not fire negotiationneeded (while I agree that in fact we should but this does not arise from the W3C algorithm)

@mickel8
Copy link
Author

mickel8 commented Jan 2, 2024

In other words, can't we just write

If description is of type "answer", and the direction of the associated m= section in the description does not match transceiver.[[Direction]]

@alvestrand
Copy link
Contributor

The present language seems to be intended to not fire negotiationneeded when you change from sendrecv to recvonly or sendonly. I'm not sure that's wise, but that's how I interpret the language.

@fippo
Copy link
Contributor

fippo commented Jan 3, 2024

The present language seems to be intended to not fire negotiationneeded when you change from sendrecv to recvonly or sendonly. I'm not sure that's wise, but that's how I interpret the language.

Hrm... could that lead to removeTrack on a sendrecv transceiver without a track not firing which is unexpected:
https://jsfiddle.net/fippo/vd3mgp5k/1/
With addTransceiver('audio') it does not fire, with addTransceiver(audioTrack) it fires.
Now that seems ok since no track is removed in the first case but it means that removeTrack behaves quite differently depending on the history of the transceiver.

Footgun alert...

@alvestrand
Copy link
Contributor

The removeTrack algorithm bails in step 7 if the track is null, so it will never hit the "update the negotiation-needed flag" in step 12.

@mickel8
Copy link
Author

mickel8 commented Jan 3, 2024

The same will happen if we call replaceTrack(null) before calling removeTrack

The present language seems to be intended to not fire negotiationneeded when you change from sendrecv to recvonly or sendonly. I'm not sure that's wise, but that's how I interpret the language.

I am not sure. To have sendrecv in local answer the other side had to offer sendrecv so after changing to sendonly or recvonly on the answerer side, the intersection will always differ.

Take a look here, pc2 changes direction from sendrecv to recvonly and negotiationneeded is fired https://jsfiddle.net/pxrzh086/1/

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