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

Web SessionDescriptionHandler - Refresh #815

Merged
merged 45 commits into from
Jul 6, 2020

Conversation

john-e-riordan
Copy link
Collaborator

A rewrite of the default web SessionDescriptionHandler.

Addresses #789
Addresses #728
Addresses #711
Addresses #709
Addresses #631
Addresses #564

Work on this is still on going, but wanted to start looking for feedback at this point.

  • The existing Web SDH got moved to an "old" directory and should be removed.
  • The React SDH needs to be redone once work on this settles.
  • The "events" npm dependency should be removed once this is accepted.

@john-e-riordan
Copy link
Collaborator Author

SDH Options & Modifiers Cleanup per TODO

What follows covers SessionDescriptionHandlerOptions, but it's the same for SessionDescriptionHandlerModifiers.

Issues With Current Approach:

  1. Not clear when constructor options are used and when options passed to member methods are used
  2. Using constructor options for re-INVITE answer seems broken
  3. No way to specify options for incoming re-INVITE handling if initial INVITE was incoming (Invitation)
  4. No way to specify different options for re-INVITE ACK (an edge case, but...)

Documentation of Current Approach is as Follows :

Inviter.constructor() - SDH options may be passed to constructor

Inviter.invite() - sending initial INVITE

  • uses SDH options passed to constructor for offer in INVITE
  • uses SDH options passed to constructor for answer in 1xx
  • uses SDH options passed to constructor for answer in 200
  • uses SDH options passed to constructor for offer in 200 and answer in ACK

Invitation.progress() - sending 100 to initial INVITE

  • uses SDH options passed to progress() method for offer or answer in 1xx
  • uses SDH options passed to progress() method for offer or answer in PRACK

Invitation.accept() - sending 200 to initial INVITE

  • uses SDH options passed to accept() method for offer or answer in 1xx
  • uses SDH options passed to accept() method for offer or answer in 200
  • uses SDH options passed to accept() method for offer or answer in PRACK

Session.invite() - sending re-INVITE

  • uses SDH options passed to invite() method for offer in INVITE
  • uses SDH options passed to constructor for answer in 200
  • uses SDH options passed to invite() method for offer in 200 and answer in ACK

Session.onInviteRequest() - receiving and handling a re-INVITE

  • uses SDH options passed to constructor for offer or answer in 200

Session.onAckRequest() - receiving and handling an in dialog ACK for initial INVITE and re-INVITE

  • uses SDH options passed to constructor for answer in ACK

Proposed New Approach:

Goal is to address/fix current issues…

Add two new Session properties:

  1. Session.sessionDescriptionHandlerOptions
  • used in all cases when handling the initial INVITE transaction as either UAC or UAS
  • may be set directly at anytime
  • may optionally be set via constructor option
  • may optionally be set via options passed to Inviter.invite() or Invitation.accept()
  1. Session.sessionDescriptionHandlerOptionsReInvite
  • used in all cases when handling a re-INVITE transaction as either UAC or UAS
  • may be set directly at anytime
  • may optionally be set via constructor option
  • may optionally be set via options passed to Session.invite()

This is pretty much backwards compatible with what we currently have (except it changes some cases which seem currently very broken) and I believe is much easier to understand and reason about.

@john-e-riordan
Copy link
Collaborator Author

Rebased onto master and force pushed

@john-e-riordan
Copy link
Collaborator Author

I've addressed all the feedback I've gotten so far on this and I believe it may be good to go. In the meantime I'll be continuing work with it and perhaps add some more documentation and tests along the way.

@james-criscuolo james-criscuolo merged commit 9b5e618 into onsip:master Jul 6, 2020
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.

2 participants