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

[WIP] Implementation of .renegotiate and .negotiate API #72

Closed
wants to merge 2 commits into from

Conversation

cusspvz
Copy link

@cusspvz cusspvz commented Mar 19, 2016

  • Add .renegotiate
  • Add .negotiate
  • Add negotiate event
  • Add .addStream
  • Add .removeStream
  • Add .addDataStream - have a question if we should allow multiple datastreams
  • Add .removeDataStream
  • Add tests where a fake MediaStream instance is added into peers post their first established connection
    - [x] Added .editorconfig
    - [x] Added node_modules/ to .gitignore and .npmignore

Current stage:

  • firefox is giving an error because of differences on sdp both m lines, unfortunately I don't have the time to finish this implementation [EDIT]

Related issues:

@cusspvz
Copy link
Author

cusspvz commented Mar 19, 2016

NOTE: I've seen that there are linting issues on the CD, I will fix them just before landing the PR.

trim_trailing_whitespace = false

[COMMIT_EDITMSG]
max_line_length = 0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't commit this file

@feross
Copy link
Owner

feross commented May 1, 2016

I think it's a mistake to stop using the onnegotiationneeded event. That was required for things to work for me before -- has that changed?

@feross feross closed this May 1, 2016
@cusspvz
Copy link
Author

cusspvz commented May 1, 2016

I have to finish this up, the only issue I had was with the Firefox implementation, tests were flawless on chrome.
Regarding your question related with onnegotiationneeded, it is called whenever a MediaStream is added, perhaps you had async issues and you were calling offer before having the stream?

Seen that you've closed the PR, are you still interested in having renegotiation here?

@feross feross reopened this May 2, 2016
@feross
Copy link
Owner

feross commented May 2, 2016

Sorry for closing the PR -- that was an accident. Before I can merge this, you need to fix the merge conflicts, fix the code style, and fix it on Firefox.

Looks like you also might need to add:

if (renegotiating) return

to the top of the _onIceCandidate function for Chrome.

See: https://www.webrtc-experiment.com/docs/how-to-switch-streams.html

@feross
Copy link
Owner

feross commented May 2, 2016

Also, the test style is kind of weird. Can you clean it up a bit?

var test = require('tape')

var getUserMedia = typeof navigator !== 'undefined' && (
navigator.webkitGetUserMedia && function ( options, fulfill, reject ) {
Copy link
Owner

@feross feross May 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use if statements. That would be much simpler.

@MatthieuLemoine
Copy link

@cusspvz Are you still working on it ?

@princemaple
Copy link

princemaple commented Mar 30, 2017

Hi @cusspvz , are you not interested in this anymore?
(Oops, didn't see the previous comment from one day ago, sorry about the double ping)

@cusspvz
Copy link
Author

cusspvz commented Mar 30, 2017

@MatthieuLemoine @princemaple I am but not for now, but can arrange some time to inject some changes on the PR.
As far as I can remember, there was an issue on Firefox upon a renegotiation.

Also, seems firefox only accepts one data stream per RTConnection.

@feross if I implement addDataStream should it error whenever it isn't possible?

@jeregrine
Copy link
Contributor

addStream is going to be deprecated in future versions. Firefox supports addTrack. They have a migration plan with chrome back-support here https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/addStream

@t-mullen
Copy link
Collaborator

I think we should leave addStream in the simple-peer API. It's much simpler than adding individual tracks (although a secondary track API couldn't hurt). We already use addTrack internally.

@jeregrine
Copy link
Contributor

addStream is being deprecated. If simple-peer wants to support an addStream that simply calls addTracks on each track then that would be fine. But long term the API is going away.

@cusspvz
Copy link
Author

cusspvz commented Mar 3, 2018

Closing due to #250
Thanks for putting your efforts on it @rationalcoding.

@cusspvz cusspvz closed this Mar 3, 2018
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

Successfully merging this pull request may close these issues.

6 participants