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

Create renegotiation method #68

Closed
cusspvz opened this issue Mar 7, 2016 · 15 comments
Closed

Create renegotiation method #68

cusspvz opened this issue Mar 7, 2016 · 15 comments

Comments

@cusspvz
Copy link

cusspvz commented Mar 7, 2016

This should allow us to:

@feross has any work been initiated regarding renegotiation?

@feross
Copy link
Owner

feross commented Mar 7, 2016

How do you propose this would work? Can you provide a complete code example and explain a bit more?

@cusspvz
Copy link
Author

cusspvz commented Mar 16, 2016

@feross sorry for my late response, had to switch priorities. I've written a renegotiation capable software two years ago, but I cannot recall how it worked, have to dig in.
I've already read your codebase, and I have two ideas for an implementation but I have to test the first one, because I don't know if it would work:

  • The first one is trying to re-use the same PeerConnection, by handling streams on it and creating offers and answers as soon as we spot a renegotiation is needed, which I think is impossible but is worth to try, and if we have success, it would be the easiest implementation.
  • The second one is to abstract PeerConnection interface from your current API, this way we could refactor some code and fix some bugs you had (related with stream destroying) without causing Breaking Changes on your current API design. This way there would be two main classes:
    • SimplePeer
      • Streamable Passthrough from the currentPeerConnection;
      • PeerConnections Manager (handling renegotiations against streaming changes and long-reconnecting delays);
      • Signalling transporter gate from all PeerConnections (so we can initiate new ones before having data flowing through)
    • PeerConnection
      • Streamable implementation of PeerConnection with friendly bindings;

@cusspvz
Copy link
Author

cusspvz commented Mar 16, 2016

On the second approach, I think it could be a good Idea to have separated PeerConnections for different streams, it would make renegotiation easier.

@feross
Copy link
Owner

feross commented Mar 16, 2016

@cusspvz Thanks for explaining your thoughts.

My concern is that this all sounds really complicated. If there's no way to do this simply using the same RTCPeerConnection, then I don't think this belongs in simple-peer. We can try to expose the right events so that this can be built as a layer on top of simple-peer.

@cusspvz
Copy link
Author

cusspvz commented Mar 17, 2016

Yeah, I admit the second one seems a little tricky to implement, besides, its complexity doesn't go well along with this repo's purpose...

I think I can write out some tests and start playing along with the first approach until the end of this week.

@feross
Copy link
Owner

feross commented Mar 18, 2016

@cusspvz Sounds good. Happy to check out what you come up with!

@cusspvz
Copy link
Author

cusspvz commented Mar 19, 2016

Already PR'ed some changes! :)
Seems the first approach worked, the only issue I'm having is related with firefox and I think it is because of testing with fake MediaStreams, but I have to dig a little bit more to have sure.

Unfortunately I'm running out of time to work on it today, I can only finish this up on Monday.

@matthieusieben
Copy link

FireFox does not support more than one stream in their P2P connection (inbound or outbound). The problem with this is that it is simple to limit the number of streams on an instance in ff (if(isFF && streams.length > 0) throw new Error()), but it is much more complicated to handle the case when a chrome browser sends two or more streams. In that case, the renegotiation will fail (due to the differences in the way ff and chrome express multi stream connections). This will typically require to 1) detect that the remote peer is FF 2) potentially create a new peerconnection

@ben-pr-p
Copy link

So what's the recommended way to mute / add video after the fact? Should I be creating a new Peer object on both clients?

@gnazarkin
Copy link

@ben-pr-p I'm closing and recreating the connections. It works but having problem with one use case where the initiator isn't broadcasting a stream, but only aims to receive it. Since the stream isn't set to anything it seems that even if the other party sends over the stream, the "on" stream event never gets called. I also enable/disable the local tracks using something like

localStream.getAudioTracks()[0].enabled = false;
localStream.getVideoTracks()[0].enabled = false;

@ben-pr-p
Copy link

ben-pr-p commented May 2, 2017

I've been using audioOrVideoTrack.enabled = false as well – the only downside is that it means I'm asking for audio and video initially even if they're only going to use audio.

@gnazarkin
Copy link

@ben-pr-p What I'm doing as a workaround is closing the connections and re-creating them. So I don't have to ask for all of them upfront. Only problem with this solution is that it seems the initiator has to have a stream, if not then the "on" stream event doesn't fire so you don't end up receiving any of the streams from the other peers

@t-mullen
Copy link
Collaborator

t-mullen commented Jun 8, 2017

@gnazarkin See #95 for a workaround to that issue.

@cusspvz
Copy link
Author

cusspvz commented Mar 3, 2018

Being solved at #250

@t-mullen
Copy link
Collaborator

Implemented in #250.

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

6 participants