Skip to content

Report disconnection event immediately when hanging up a call #2582

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.

Sign up for GitHub

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

Closed
sauwming wants to merge 2 commits into from
Closed

Report disconnection event immediately when hanging up a call #2582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff0fc7c
Add immediate hangup feature
sauwming Oct 23, 2020
cf73676
immediate call hangup
sauwming Nov 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pjsip-apps/src/pjsua/pjsua_app_legacy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -911,7 +911,7 @@ static void ui_hangup_call(char menuin[])
pjsua_call_hangup_all();
} else {
/* Hangup current calls */
pjsua_call_hangup(current_call, 0, NULL, NULL);
pjsua_call_hangup2(current_call, PJ_TRUE, 0, NULL, NULL);
}
}

Expand Down
34 changes: 34 additions & 0 deletions pjsip/include/pjsua-lib/pjsua.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5573,6 +5573,40 @@ PJ_DECL(pj_status_t) pjsua_call_hangup(pjsua_call_id call_id,
const pj_str_t *reason,
const pjsua_msg_data *msg_data);

/**
* Hangup call by using method that is appropriate according to the
* call state. This function is different than answering the call with
* 3xx-6xx response (with #pjsua_call_answer()), in that this function
* will hangup the call regardless of the state and role of the call,
* while #pjsua_call_answer() only works with incoming calls on EARLY
* state.
*
* If parameter immediate is set to PJ_TRUE, call DISCONNECTED state
* will be immediately reported to app and the call slot can be reused.
* The only exception is if media transport creation is in progress
* during call setup. In this case, the hangup will be pending upon
* the completion of the media transport.
Copy link
Copy Markdown
Member

@nanangizz nanangizz Nov 17, 2020

Choose a reason for hiding this comment

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

For the exception condition, will the app still be notified immediately?

Copy link
Copy Markdown
Member Author

@sauwming sauwming Nov 18, 2020

Choose a reason for hiding this comment

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

No, the app will be notified upon completion of media transport.

Copy link
Copy Markdown
Member

@nanangizz nanangizz Nov 18, 2020

Choose a reason for hiding this comment

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

Why is that?

Not sure if this is relevant. With trickle ICE, media transport completion may be reported immediately, while internally ICE is actually still gathering candidates. So for this case, which completion will the pjsua-lib wait for?

Copy link
Copy Markdown
Member Author

@sauwming sauwming Nov 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It is the current behavior of call hangup, i.e. to delay the hangup until media transport creation completes:
https://github.com/pjsip/pjproject/blob/master/pjsip/src/pjsua-lib/pjsua_call.c#L2760

The reason this behavior is not changed is because of the following complexity:
In order to support immediate hangup, we move the current pjsua_var.call instance into another variable pjsua_var.hangup_call and handle the hangup process from this new variable. For media related callbacks however, since currently we are using &call.media[i] as the user data, this will point to the old call media instance. It is possible to overcome this issue, by:

  • Creating the user data in the heap (using dialog's pool?), perhaps a struct of { dialog and media index }.
  • Modifying pjsua_media.c callbacks to use this new user data.

But there is still the complexity of race condition, i.e. since the media transport creation is async, there is a possibility that the media callback will happen at the same time with call hangup. This may require us to hold dialog lock in all media related callbacks. So I'm not sure if the potentially major modifications required is worth the advantage, i.e. to be able to immediately hangup the call when async med tp creation is in progress in the beginning of the call (again, note that sync media reinit in the middle of the call should be unaffected by this).

As for trickle ICE, I believe it will be upon the report of completion, because the hangup will proceed in pjsua_call.c's on_make/incoming_call_med_tp_complete(). I'm not sure if this can cause any complications though, since trickle ICE is a new feature.

Copy link
Copy Markdown
Member

@nanangizz nanangizz Nov 18, 2020

Choose a reason for hiding this comment

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

As the ticket is about immediate hangup notification and handle the rest in background, I am thinking that media transport creation completion should be part of things to be done in background.

Re: &call.media[i] user data in media callbacks, have not checked the patch details, but perhaps the problem raises because the call instance may now be reused, right? What if it cannot be reused until the hangup process completed, e.g: simply add a flag hangingup in pjsua_call and alloc_call_id() filters out call slot with that flag set. This may be a completely different approach compared to this current patch (which seems to move the hanging up call instance to pjsua_var.hangup_call, cmiiw).

Re: trickle ICE, from PJSUA point of view, create_ice_media_transport() will set call_med->tp_ready to PJ_SUCCESS (instead of PJ_EPENDING as in current behavior). So perhaps PJSUA will try to destroy the call_med->tp immediately on call hangup, may be safe though.

Copy link
Copy Markdown
Member

@nanangizz nanangizz Nov 30, 2020

Choose a reason for hiding this comment

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

A few notes when trying to implement the new approach:

  • In general, there's a problem with the requirement in PJSUA-LIB should report disconnection event immediately after pjsua_call_hangup() is called #1049: cease any PJSUA call related callback once call is declared to be disconnected. More about this below.

  • on_stream_destroyed() will be called after the DISCONNECTED on_call_state(). But then, so is the current behaviour. Except that since we now report the DISCONNECTED state much earlier, the delay can be longer between the two callbacks. Trying to call on_stream_destroyed() earlier poses two problems:

    • The media will already be deactivated upon call disconnection, resulting in call dump not showing any useful end-of-call statistics. But this is minor and the workaround is probably to do the call dump before the media deinit.

Ic.

  • The bigger problem is the async media transport creation, so no matter what, we still need to wait for it to complete.

If on_stream_created() is not called yet (currently this should be the case when med tp is not completed yet), we can skip `on_stream_destroyed()' perhaps?

Perhaps we can invoke the callback using user event?

Copy link
Copy Markdown
Member Author

@sauwming sauwming Dec 1, 2020

Choose a reason for hiding this comment

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

  • The media will already be deactivated upon call disconnection, resulting in call dump not showing any useful end-of-call statistics. But this is minor and the workaround is probably to do the call dump before the media deinit.

Calling pjsua_call_dump() in the application's on_stream_destroyed() seems to solve this issue.

If on_stream_created() is not called yet (currently this should be the case when med tp is not completed yet), we can skip `on_stream_destroyed()' perhaps?

Yes.

  • How about on_call_tsx_state()? In Callback on_tsx_state_changed() not invoked on call disconnection #1627 (Callback on_tsx_state_changed() not invoked on call disconnection), the workaround is to call it before on_call_state(DISCONNECTED), but since we now report the DISCONNECTED state earlier in pjsua_call_hangup(), we don't have the pjsip_event.
    Perhaps we can invoke the callback using user event?

on_call_tsx_state() also has pjsip_transaction * parameter. Passing NULL may cause application to crash.

Copy link
Copy Markdown
Member

@nanangizz nanangizz Dec 1, 2020

Choose a reason for hiding this comment

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

on_call_tsx_state() also has pjsip_transaction * parameter. Passing NULL may cause application to crash.

Ah okay.
For immediate hangup, perhaps we should just skip on_call_tsx_state() altogether as any tsx state change, if any, happens in background. And the docs should explicitly specify this.

Copy link
Copy Markdown

@calanor calanor Dec 13, 2020

Choose a reason for hiding this comment

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

About calling pjsua_call_dump() in on_stream_destroyed().
With this change, if we receive the hang up, the call_dump appears as "DISCONNCTD", but if we are the ones who hang up the call, it appears as "CONFIRMED".
Before, in on_call_state, in all cases the call dump appears as "DISCONNCTD".

Copy link
Copy Markdown
Member Author

@sauwming sauwming Dec 14, 2020

Choose a reason for hiding this comment

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

Will continue this discussion in the relevant PR #2600 instead.

* The library will then process the call hangup, such as notifying
* the remote and cleaning up used resources, in the background.
*
* @param call_id Call identification.
* @param immediate Set to PJ_TRUE to hangup the call immediately.
* @param code Optional status code to be sent when we're rejecting
* incoming call. If the value is zero, "603/Decline"
* will be sent.
* @param reason Optional reason phrase to be sent when we're rejecting
* incoming call. If NULL, default text will be used.
* @param msg_data Optional list of headers etc to be added to outgoing
* request/response message.
*
* @return PJ_SUCCESS on success, or the appropriate error code.
*/
PJ_DECL(pj_status_t) pjsua_call_hangup2(pjsua_call_id call_id,
pj_bool_t immediate,
unsigned code,
const pj_str_t *reason,
const pjsua_msg_data *msg_data);

/**
* Accept or reject redirection response. Application MUST call this function
* after it signaled PJSIP_REDIRECT_PENDING in the \a on_call_redirected()
Expand Down
13 changes: 11 additions & 2 deletions pjsip/include/pjsua-lib/pjsua_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ struct pjsua_call
struct {
call_answer answers;/**< A list of call answers. */
pj_bool_t hangup;/**< Call is hangup? */
pj_bool_t immediate;/**< Immediate call hangup? */
pjsip_dialog *replaced_dlg; /**< Replaced dialog. */
} inc_call;
} call_var;
Expand All @@ -205,6 +206,12 @@ struct pjsua_call
created yet. This temporary
variable is used to handle such
case, see ticket #1916. */

pj_timer_entry hangup_timer; /**< Hangup retry timer. */
unsigned hangup_retry; /**< Number of hangup retries. */
unsigned hangup_code; /**< Hangup code. */
pj_str_t hangup_reason; /**< Hangup reason. */
pjsua_msg_data *hangup_msg_data;/**< Hangup message data. */
};


Expand Down Expand Up @@ -491,6 +498,8 @@ struct pjsua_data
unsigned call_cnt; /**< Call counter. */
pjsua_call calls[PJSUA_MAX_CALLS];/**< Calls array. */
pjsua_call_id next_call_id; /**< Next call id to use*/
pjsua_call hangup_calls[PJSUA_MAX_CALLS];/**< Calls in
hangup process. */

/* Buddy; */
unsigned buddy_cnt; /**< Buddy count. */
Expand Down Expand Up @@ -694,7 +703,7 @@ pj_status_t pjsua_media_channel_create_sdp(pjsua_call_id call_id,
pj_status_t pjsua_media_channel_update(pjsua_call_id call_id,
const pjmedia_sdp_session *local_sdp,
const pjmedia_sdp_session *remote_sdp);
pj_status_t pjsua_media_channel_deinit(pjsua_call_id call_id);
pj_status_t pjsua_media_channel_deinit(pjsua_call *call);

/*
* Error message when media operation is requested while another is in progress
Expand All @@ -712,7 +721,7 @@ pj_status_t pjsua_call_media_init(pjsua_call_media *call_med,
void pjsua_call_cleanup_flag(pjsua_call_setting *opt);
void pjsua_set_media_tp_state(pjsua_call_media *call_med, pjsua_med_tp_st tp_st);

void pjsua_media_prov_clean_up(pjsua_call_id call_id);
void pjsua_media_prov_clean_up(pjsua_call *call);

/* Callback to receive media events */
pj_status_t on_media_event(pjmedia_event *event, void *user_data);
Expand Down
Loading