Skip to content

Commit

Permalink
Fix assertion on call hangup from DTMF callback (#3970)
Browse files Browse the repository at this point in the history
  • Loading branch information
@nanangizz
nanangizz authored May 27, 2024
1 parent a250942 commit 2e8f361
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 8 deletions.
29 changes: 27 additions & 2 deletions pjmedia/src/pjmedia/stream.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ struct dtmf
struct pjmedia_stream
{
pjmedia_endpt *endpt; /**< Media endpoint. */
pj_grp_lock_t *grp_lock; /**< Group lock. */
pjmedia_codec_mgr *codec_mgr; /**< Codec manager instance. */
pjmedia_stream_info si; /**< Creation parameter. */
pjmedia_port port; /**< Port interface. */
Expand Down Expand Up @@ -299,6 +300,7 @@ static pj_status_t send_rtcp(pjmedia_stream *stream,
pj_bool_t with_xr,
pj_bool_t with_fb);

static void stream_on_destroy(void *arg);

#if TRACE_JB

Expand Down Expand Up @@ -2453,7 +2455,7 @@ PJ_DEF(pj_status_t) pjmedia_stream_create( pjmedia_endpt *endpt,

PJ_ASSERT_RETURN(endpt && info && p_stream, PJ_EINVAL);

if (pool == NULL) {
if (1 || pool == NULL) {

This comment has been minimized.

Sign in to view

Copy link
@mfroeschl

mfroeschl Sep 9, 2024

Contributor

I am aware this PR has been long merged, but still I was wondering about this line change. The if clause conditon will now always evaluate to "true". If this is intentional (is it?), then why not just remove the if clause alltogether?

This comment has been minimized.

Sign in to view

Copy link
@sauwming

sauwming Sep 9, 2024

Member

There was a previous discussion here:
https://github.com/pjsip/pjproject/pull/3970/files#r1615411595

This comment has been minimized.

Sign in to view

Copy link
@mfroeschl

mfroeschl Sep 9, 2024 •
edited
Loading

Contributor

Understood that it is intentional, thank you! Still, if it always evaluates to true do you even need the || or the if clause? Of course this is just cosmetics so I'll leave it to your descretion. Cheers!
own_pool = pjmedia_endpt_create_pool( endpt, "strm%p",
PJMEDIA_STREAM_SIZE,
PJMEDIA_STREAM_INC);
Expand Down Expand Up @@ -2897,6 +2899,14 @@ PJ_DEF(pj_status_t) pjmedia_stream_create( pjmedia_endpt *endpt,
att_param.rtp_cb2 = &on_rx_rtp;
att_param.rtcp_cb = &on_rx_rtcp;

/* Attach handler to group lock from transport */
if (tp->grp_lock) {
stream->grp_lock = tp->grp_lock;
pj_grp_lock_add_ref(stream->grp_lock);
pj_grp_lock_add_handler(stream->grp_lock, pool, stream,
&stream_on_destroy);
}

/* Only attach transport when stream is ready. */
stream->transport = tp;
status = pjmedia_transport_attach2(tp, &att_param);
Expand Down Expand Up @@ -3060,6 +3070,15 @@ PJ_DEF(pj_status_t) pjmedia_stream_create( pjmedia_endpt *endpt,
}


static void stream_on_destroy(void *arg)
{
pjmedia_stream* stream = (pjmedia_stream*)arg;

PJ_LOG(4,(stream->port.info.name.ptr, "Stream destroyed"));
pj_pool_safe_release(&stream->own_pool);
}


/*
* Destroy stream.
*/
Expand All @@ -3069,6 +3088,8 @@ PJ_DEF(pj_status_t) pjmedia_stream_destroy( pjmedia_stream *stream )

PJ_ASSERT_RETURN(stream != NULL, PJ_EINVAL);

PJ_LOG(4,(stream->port.info.name.ptr, "Stream destroying"));

/* Send RTCP BYE (also SDES & XR) */
if (stream->transport && !stream->rtcp_sdes_bye_disabled) {
#if defined(PJMEDIA_HAS_RTCP_XR) && (PJMEDIA_HAS_RTCP_XR != 0)
Expand Down Expand Up @@ -3167,7 +3188,11 @@ PJ_DEF(pj_status_t) pjmedia_stream_destroy( pjmedia_stream *stream )
}
#endif

pj_pool_safe_release(&stream->own_pool);
if (stream->grp_lock) {
pj_grp_lock_dec_ref(stream->grp_lock);
} else {
stream_on_destroy(stream);
}

return PJ_SUCCESS;
}
Expand Down
30 changes: 24 additions & 6 deletions pjmedia/src/pjmedia/transport_udp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ static void on_rtp_data_sent(pj_ioqueue_key_t *key,
static void on_rx_rtcp(pj_ioqueue_key_t *key,
pj_ioqueue_op_key_t *op_key,
pj_ssize_t bytes_read);
static void transport_on_destroy(void *arg);

/*
* These are media transport operations.
Expand Down Expand Up @@ -355,6 +356,7 @@ PJ_DEF(pj_status_t) pjmedia_transport_udp_attach( pjmedia_endpt *endpt,
goto on_error;

pj_grp_lock_add_ref(grp_lock);
pj_grp_lock_add_handler(grp_lock, pool, tp, &transport_on_destroy);
tp->base.grp_lock = grp_lock;

/* Setup RTP socket with the ioqueue */
Expand Down Expand Up @@ -436,6 +438,15 @@ PJ_DEF(pj_status_t) pjmedia_transport_udp_attach( pjmedia_endpt *endpt,
}


static void transport_on_destroy(void *arg)
{
struct transport_udp *udp = (struct transport_udp*) arg;

PJ_LOG(4, (udp->base.name, "UDP media transport destroyed"));
pj_pool_safe_release(&udp->pool);
}


/**
* Close UDP transport.
*/
Expand All @@ -449,6 +460,8 @@ static pj_status_t transport_destroy(pjmedia_transport *tp)
/* Must not close while application is using this */
//PJ_ASSERT_RETURN(!udp->attached, PJ_EINVALIDOP);

PJ_LOG(4,(udp->base.name, "UDP media transport destroying"));

/* The following calls to pj_ioqueue_unregister() will block the execution
* if callback is still being called because allow_concurrent is false.
* So it is safe to release the pool immediately after.
Expand All @@ -474,9 +487,6 @@ static pj_status_t transport_destroy(pjmedia_transport *tp)

pj_grp_lock_dec_ref(tp->grp_lock);

PJ_LOG(4,(udp->base.name, "UDP media transport destroyed"));
pj_pool_release(udp->pool);

return PJ_SUCCESS;
}

Expand Down Expand Up @@ -574,6 +584,10 @@ static void on_rx_rtp(pj_ioqueue_key_t *key,
call_rtp_cb(udp, bytes_read, &rem_switch);
}

/* Transport may be destroyed from the callback! */
if (!udp->rtp_key || !udp->started)
break;

#if defined(PJMEDIA_TRANSPORT_SWITCH_REMOTE_ADDR) && \
(PJMEDIA_TRANSPORT_SWITCH_REMOTE_ADDR == 1)
if (rem_switch &&
Expand Down Expand Up @@ -617,9 +631,9 @@ static void on_rx_rtp(pj_ioqueue_key_t *key,
bytes_read = sizeof(udp->rtp_pkt);
udp->rtp_addrlen = sizeof(udp->rtp_src_addr);
status = pj_ioqueue_recvfrom(udp->rtp_key, &udp->rtp_read_op,
udp->rtp_pkt, &bytes_read, 0,
&udp->rtp_src_addr,
&udp->rtp_addrlen);
udp->rtp_pkt, &bytes_read, 0,
&udp->rtp_src_addr,
&udp->rtp_addrlen);

if (status != PJ_EPENDING && status != PJ_SUCCESS) {
if (transport_restarted && last_err == status) {
Expand Down Expand Up @@ -709,6 +723,10 @@ static void on_rx_rtcp(pj_ioqueue_key_t *key,
do {
call_rtcp_cb(udp, bytes_read);

/* Transport may be destroyed from the callback! */
if (!udp->rtcp_key || !udp->started)
break;

#if defined(PJMEDIA_TRANSPORT_SWITCH_REMOTE_ADDR) && \
(PJMEDIA_TRANSPORT_SWITCH_REMOTE_ADDR == 1)
/* Check if RTCP source address is the same as the configured
Expand Down

0 comments on commit 2e8f361

Please sign in to comment.