-
Notifications
You must be signed in to change notification settings - Fork 702
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
Session description refactor #426
Conversation
- Takes an array of functions each returning a promise and an initial value - Calls the array of functions in order passing the result of the previous to the next - The initial value is passed as a resolved promise - Returns a promise based on the outcome of the assembled chain
- A lot of functionality was merged into the SessionDescriptionHandler - Allow description modifiers to be used instead of forcing a full handler to be written - Remove dependencies on enviornment and WebRTC - Remove any sort of notion of "supported" - Remove actual handling of media, this should be done application level - Session description check just checks that the content type matches, leave the message parsing to the session layer - Move away from using enviornment, instead just allow the user to provide their own SessionDescriptionHandler - Remove browser prefixes, browsers no longer require prefixed WebRTC commands - Remove MediaStream Manager - Remove WebRTC.js to make SIP.js more agnostic to the underlying media TODO: - Implement SessionDescriptionHandler.close() - Events - Comments - Tests (150 failing at time of commit) - Determine if promisifiy is still needed for a lot of description handling code - SDP modifying options on the UA need to be read by the descriptionHandler and coded up. - Make sure there are no sdp modifications happening in Session.js - UA - Session - Documentation
TODO: - Refactor names to not be mediaHandler and mediaHandlerFactory - Pass constraints in a consistent manner
Remove all references to MediaHandler, MediaHandlerFactory, and MediaStreamManager TODO: - Fix Tests
Promisifiy getDescription
Fix tests for session that were dependent on the media handler Fix bugs uncovered by tests
var self = this; | ||
|
||
// We will override the saved constraints with new constraints if provided, otherwise use saved constraints | ||
this.constraints = constraints || this.constraints || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could mess up the underlying WebRTC library if the constraints
argument passed to getDescription
is an empty object.
Cleanup default WebRTC SessionDescriptionHandler to prepare for refer
…that it is more consistent Split options for session description handler and its factory Remove validation of session description options from the UA Fix tests related to changes
I have made the On the default WebRTC I have also separated the options passed by the Lastly, the |
@@ -211,8 +211,7 @@ RegisterContext.prototype = { | |||
options = options || {}; | |||
|
|||
if(!this.registered && !options.all) { | |||
this.logger.warn('already unregistered'); | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm... it is not immediately clear to me how removing this return
breaks all of our tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is in the test cleanup and how we are cleaning up the UA that is causing the tests to break. We are blindly calling unregister in places, and expecting it to return out of the function if it is not registered.
The issue is, that it is valid to send an unregister even if you are not registered. There are cases where you would want to do this. Either we need to re-write the tests, and confirm that we are not breaking anything. Or we need to keep the current behavior and add some extra behavior to allow this to continue in certain situations.
- close() still maintains the previous behavior - Adjust tests accordingly - Whitespace cleanup on RegisterContext Tests
TODO: - Move construction of Session Description Handler in ISC to after accept - Move SessionDescriptionHandler.hasDescription() into static context
- Move sessionDescriptionHandler creation out of the constructor for ISC - Fix Invite w/o SDP and no 100rel case TODO: - Fix Content-Disposition - Events There may be some straggling edge cases where the sessionDescriptionHandler is not actually constructed before it is used. This should be cleaned up.
…now contained in a Promise
…lock to setDescription. See TODO in file
|
||
methodName = self.hasOffer('remote') ? 'createAnswer' : 'createOffer'; | ||
|
||
return SIP.Utils.promisify(pc, methodName, true)() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to stop using promisify as it breaks Safari
// GUM and set myself up | ||
self.logger.log('acquiring local media'); | ||
// TODO: Constraints should be named MediaStreamConstraints | ||
return this.acquire(self.constraints) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about providing already acquired mediastream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will need to write your own sessionDescriptionHandler to handle that. It is not something we want to deal with on the default implementation.
i came here with problem sipwise/rtpengine#393 Google Chrome | 59.0.3071.86 (64 bit) In Firefox 52.0.2 (32-bit) After update Firefox to 52.0.3 If i use master branch - i'm don't got this errors |
Your browsers are a bit out of date. Chrome should be 60+, and FF is 55+. This branch definitely will not work on your version of Firefox. That being said, it appears that Chrome cannot access your media devices. Could you provide a gist of more detailed logging after updating your browsers? |
Chorme 61.0.3163.79, (64 bit) |
In your UA configuration can you try adding the following
|
SimpleSo I have added a What is Simple?
APIConstructor
optionsAt least one remote media element is required.
MethodscallStart a new call to the uri provided.
answerAnswer an incoming call that is ringing.
hangupHangup a call.
muteMute the local media of a call.
unmuteUnmute the local mdia of a call.
holdPlace a call on hold and mute the local media.
unholdTake a call off hold and unmute the local media.
sendDTMFSend a DTMF event via SIP Message
EventsEvents are emitted the same in both incoming and outgoing calls. registeredEmitted when the UA registers. unregisteredEmitted when the UA unregisters. newEmitted when there is a new call. Before a progress message is sent or received. connectingEmitted once a progress message is sent or received. connectedEmitted when the call is connected. endedEmitted when the call is completed, either from a hangup or an error. holdEmitted when a call is placed on hold or you place a call on hold. unholdEmitted when a call is taken off hold. muteEmitted when the mute function is called. unmuteEmitted when the unmute function is called. dtmfEmitted when a SIP DTMF message is received StatesSimple has 5 states that it can be in at any given time. The states will always flow in a single direction from STATUS_NULLA blank state for when Simple is instantiated. STATUS_NEWA new call has been initialized. STATUS_CONNECTINGThe call is setting up, but is not yet finalized. STATUS_CONNECTEDThe call is now connected and media is flowing. STATUS_COMPLETEDThe call is completed. |
make call.. try Hold Browser console - https://gist.github.com/falconsapsan/b230856a34c500da6d3111c25a39add7 |
@falconsapsan I cannot reproduce. Can you check that your media server is behaving as expected? |
Add modifier to strip g722 from sdp (Safari bug)
Some notes on Safari (as of Tech Preview Release 39)The media devices are chosen as the system defaults. The user has no option when allowing media to choose a different set of devices, like in Chrome or Firefox. Safari does not allow the auto-playing of a media element from an async javascript function. The play function must be called from a synchronous user action. This is a bit annoying, but to get around it you can set the autoplay attribute on the media element that you are trying to do. This is pretty well documented as a Safari 'quirk' and documentation around the internet on this is pretty good. On my Late 2016 MacBook Pro, when Safari requests the microphone it puts the internal microphone in 4-channel 24-bit audio mode. Why does it need this many channels? I could not say. However Firefox cannot handle the internal Macbook Pro microphone being in this mode. The error message that it spits out is extremely vague at best.
The way to fix this is to go into |
Assuming that there are no glaring issues, I am starting on documentation with the expectation to merge this and do a version bump first thing tomorrow. |
Session Description Refactor
This is a pretty big code refactor in an attempt to simplify SIP.js and the handling of SDP and media. The idea is that SIP.js should be more strictly a signaling library, and not get involved in the handling of the actual media. Handling the media will be the responsibility of the end application. That being said, some of the simplicity of getting up and running with SIP.js has been removed. I am hoping to write a cover library that really makes getting up and running very simple, and then having users who want more control come and use SIP.js directly. With all that being said, let me outline the changes.
hacks.js
hacks.js
and all the functions within have been removed. These all seemed to be related to old browser problems and SDP manipulation. The idea was that we could use it as a reference to see which browsers were working as closely to intended behavior as possible. The reality of it was, that once something was added, it was almost never removed. Some hacks never made it into this file. And it was not signaling related, but SDP in almost all cases. There is a new way to do SDP manipulation which will be outlined later.environment.js
The deprecation of
environment.js
. The idea with the environment was to essentially act a shim. It had a list of things that any environment running SIP.js had to implement, and then SIP.js would work as expected. The reality is that it caused more trouble than it solved. It required some things that only the browsers implemented and were not actually required for the library to function, like functions required by the media handler. I have started the process of removing everything from here and the hope moving forward is to offer up a different library that acts a better shim in the future.Session Description Handler
The change of
MediaHandler
toSessionDescriptionHandler
. This is the big one, and it starts with the name. The name MediaHandler does not accurately describe what is actually being handled. SIP.js does not want to handle media directly, that is what WebRTC is for. We want to handle the signaling. The newSessionDescriptionHandler
will attempt to do just that, it will set up the PeerConnection and signal that is has done so. The application will then be responsible for doing what it wants with the media, similar to how the application is responsible for implementing it's own user interface.The mute and unmute options are no longer available on a session.
Example of media handling in application
SDP Modifiers
The next thing that has changed with the Session Description Handler is the mangling of SDP. We have come up with a scalable way to modify SDP before passing it to the PeerConnection and after receiving it from the PeerConnection. These are SDP modifiers. An SDP modifier function is simply a function that takes SDP and returns a promise which resolves with the modified SDP. The session description handler then runs through this array in order passing the result of the previous manipulation to the next function. Currently getting these modifiers into the session description handler is a bit fickle, but I hope to make this process easier over time.
Hold
Hold is now done by just defining a
holdModifier
on the session description handler. When the hold function on the session is called, the session simply callsgetDescription
from the handler and passes it theholdModifier
to mangle the sdp appropriately.Renegotiation
Renegotiation now works correctly. This is tested in Chrome (59+) and Firefox (54+). There is no longer any fake hold being done with the media streams.
Creating your own handler
Just implement the functions in the outline below, wrap it in a factory. That is it. No need to deal with rendering, or muting, etc. That is for the application to worry about.
Session Description Handler Options
These options should now all be contained within a single key on the UA configuration. Validation is still done on UA side, but it should change in the future to be done on the SessionDescriptionHandler itself.
Removing WebRTC Dependencies
All WebRTC dependencies should be included in the default session description handler provided by SIP.js. The goal here is to allow SIP.js to run in environments that do not have WebRTC by simply overriding the default
sessionDescriptionHandlerFactory
. This can be done even with compiled versions of SIP.js by just passing the new factory as a UA parameter. There are no longer any mysteries ofisReady()
or needing to call WebRTC functions to get SIP.js to run.TODO's
There are a few remaining items left to do.
Lastly, I would like for this pull request to become SIP.js 0.8.0 release. I expect there are definitely some things that I have missed and some bugs in my code. I encourage the community to try it out and give me your feedback. I am sure a few more commits will make it on to this branch before we merge and release.