Skip to content
This repository has been archived by the owner on Aug 23, 2022. It is now read-only.

On negotiate PoC #7

Closed
wants to merge 4 commits into from
Closed

Conversation

k0nserv
Copy link
Member

@k0nserv k0nserv commented Dec 7, 2021

This is a modified version of the reflect example. Instead of adding the reflected output tracks immediately it waits until the remote track is added and adds the corresponding output track at this point instead.

NOTE: Even if the negotiate callback fired correctly it wouldn't work because the negotiation over the data channel is not implemented.

Lowering the debug log verbosity from Trace to Debug is advisable

Steps

  1. Use the following JSFiddle
  2. Build the example
  3. Run it with the SDP from the fiddle(make sure to turn on debug mode)

Expected behaviour

Adding a new output track in response to an incoming track in on_track should cause the on_negotiation_needed callback to fire. If the data channel was wired up to do the negotiation in response to this it would have have the effect of the code working as intended.

Actual behaviour

The on_negotiation_needed callback never fires, "Negotiation needed fired" never appears in the log.

Environment

M1 Macbook Pro running macOS 12.0.1, Google chrome

@rainliu
Copy link
Member

rainliu commented Dec 12, 2021

@k0nserv, webrtc git repo commit: webrtc-rs/webrtc@668445c should fix this bug.

Use your example and above webrtc git repo commit, the following log shows:

examples/reflect/reflect.rs:174 [INFO] 22:29:04.570749 - Negotiation needed fired. Track added false
examples/reflect/reflect.rs:181 [INFO] 22:29:04.570781 - Negotiation needed op ran. Track added false

Could you try the latest webrtc git repo?

Let me know if it fixed your issue, so I can prepare a new release. Thanks.

@k0nserv
Copy link
Member Author

k0nserv commented Dec 13, 2021

Thanks for looking into this.

Maybe I am misunderstanding things, but I'd expect TRACK_ADDED to be true by the time that on_negotiation_needed fires. Now, if I comment out all the code to create the new track in on_track on_negotiation_needed still fires and I'm not sure what causes it?

Further, if I try to create an offer in response to on_negotiation_needed, as I have in fec2496, it fails with ErrExcessiveRetries.

This is what I'd expect(keep in mind that I haven't read the spec and this could be wrong):

  1. Initial SDP negotiation takes place
  2. on_track is called
  3. We create the reflection track
  4. We set TRACK_ADDED to true
  5. Because we call RTCPeerConnection::add_track a negotiation is required
  6. on_negotiation_needed fires and TRACK_ADDED is true.

@k0nserv
Copy link
Member Author

k0nserv commented Dec 13, 2021

So doing some more investigation:

We end up in do_negotiation_needed_inner with negotiation_needed_state being NegotiationNeededState::Queue and params.ops.length being 0. This seems to be a violation of the invariant for NegotiationNeededState::Queue, its docs say:

 /// NegotiationNeededStateEmpty running and queue
    Queue,

I feel like the state should be NegotiationNeededState::Empty because we have finished the initial negotiation at this point and there should be no remaining operations.

@k0nserv
Copy link
Member Author

k0nserv commented Dec 13, 2021

Thinking about this more(and reading the Pion source code + the spec) the fix in webrtc-rs/webrtc@668445c does seem correct. But as outlined above it causes problem when negotiation is attempted in response to on_negotiation_needed firing

This showcase a problem where using `add_track` to create a new track in
response to an incoming track does not trigger negotiation.
@k0nserv
Copy link
Member Author

k0nserv commented Aug 23, 2022

I think I've fixed this somewhere

@k0nserv k0nserv closed this Aug 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants