<p>Joshua Colp <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/6032">View Change</a></p><div style="white-space:pre-wrap">Approvals:
George Joseph: Looks good to me, approved
Joshua Colp: Approved for Submit
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">bridge_softmix / res_rtp_asterisk: Fix packet loss and renegotiation issues.<br><br>This change does a few things to improve packet loss and renegotiation:<br><br>1. On outgoing RTP streams we will now properly reflect out of order<br>packets and packet loss in the sequence number. This allows the<br>remote jitterbuffer to better reorder things.<br><br>2. Video updates can now be discarded for a period of time<br>after one has been sent to prevent flooding of clients.<br><br>3. For declined and removed streams we will now release any<br>media session resources associated with them. This was not<br>previously done and caused an issue where old state was being<br>used for a new stream.<br><br>4. RTP bundling was not actually removing bundled RTP instances<br>from the parent. This has been resolved by removing based on<br>the RTP instance itself and not the SSRC.<br><br>5. The code did not properly handle explicitly unbundling an<br>RTP instance from its parent. This now works as expected.<br><br>ASTERISK-27143<br><br>Change-Id: Ibd91362f0e4990b6129638e712bc8adf0899fd45<br>---<br>M apps/app_confbridge.c<br>M apps/confbridge/conf_config_parser.c<br>M apps/confbridge/include/confbridge.h<br>M bridges/bridge_softmix.c<br>M bridges/bridge_softmix/include/bridge_softmix_internal.h<br>M configs/samples/confbridge.conf.sample<br>M include/asterisk/bridge.h<br>M include/asterisk/frame.h<br>M main/bridge.c<br>M main/rtp_engine.c<br>M res/res_pjsip_session.c<br>M res/res_rtp_asterisk.c<br>12 files changed, 108 insertions(+), 9 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/apps/app_confbridge.c b/apps/app_confbridge.c<br>index c6372fa..b2d612d 100644<br>--- a/apps/app_confbridge.c<br>+++ b/apps/app_confbridge.c<br>@@ -1485,6 +1485,7 @@<br> ast_bridge_set_talker_src_video_mode(conference->bridge);<br> } else if (ast_test_flag(&conference->b_profile, BRIDGE_OPT_VIDEO_SRC_SFU)) {<br> ast_bridge_set_sfu_video_mode(conference->bridge);<br>+ ast_bridge_set_video_update_discard(conference->bridge, conference->b_profile.video_update_discard);<br> }<br> <br> /* Link it into the conference bridges container */<br>diff --git a/apps/confbridge/conf_config_parser.c b/apps/confbridge/conf_config_parser.c<br>index cc8fcfe..bfd9f4f 100644<br>--- a/apps/confbridge/conf_config_parser.c<br>+++ b/apps/confbridge/conf_config_parser.c<br>@@ -450,6 +450,16 @@<br> </enumlist><br> </description><br> </configOption><br>+ <configOption name="video_update_discard" default="2000"><br>+ <synopsis>Sets the amount of time in milliseconds after sending a video update to discard subsequent video updates</synopsis><br>+ <description><para><br>+ Sets the amount of time in milliseconds after sending a video update request<br>+ that subsequent video updates should be discarded. This means that if we<br>+ send a video update we will discard any other video update requests until<br>+ after the configured amount of time has elapsed. This prevents flooding of<br>+ video update requests from clients.<br>+ </para></description><br>+ </configOption><br> <configOption name="template"><br> <synopsis>When using the CONFBRIDGE dialplan function, use a bridge profile as a template for creating a new temporary profile</synopsis><br> </configOption><br>@@ -1652,6 +1662,8 @@<br> break;<br> }<br> <br>+ ast_cli(a->fd,"Video Update Discard: %u\n", b_profile.video_update_discard);<br>+<br> ast_cli(a->fd,"sound_only_person: %s\n", conf_get_sound(CONF_SOUND_ONLY_PERSON, b_profile.sounds));<br> ast_cli(a->fd,"sound_only_one: %s\n", conf_get_sound(CONF_SOUND_ONLY_ONE, b_profile.sounds));<br> ast_cli(a->fd,"sound_has_joined: %s\n", conf_get_sound(CONF_SOUND_HAS_JOINED, b_profile.sounds));<br>@@ -2220,6 +2232,7 @@<br> aco_option_register(&cfg_info, "regcontext", ACO_EXACT, bridge_types, NULL, OPT_CHAR_ARRAY_T, 0, CHARFLDSET(struct bridge_profile, regcontext));<br> aco_option_register(&cfg_info, "language", ACO_EXACT, bridge_types, "en", OPT_CHAR_ARRAY_T, 0, CHARFLDSET(struct bridge_profile, language));<br> aco_option_register_custom(&cfg_info, "^sound_", ACO_REGEX, bridge_types, NULL, sound_option_handler, 0);<br>+ aco_option_register(&cfg_info, "video_update_discard", ACO_EXACT, bridge_types, "2000", OPT_UINT_T, 0, FLDSET(struct bridge_profile, video_update_discard));<br> /* This option should only be used with the CONFBRIDGE dialplan function */<br> aco_option_register_custom(&cfg_info, "template", ACO_EXACT, bridge_types, NULL, bridge_template_handler, 0);<br> <br>diff --git a/apps/confbridge/include/confbridge.h b/apps/confbridge/include/confbridge.h<br>index cf30d5c..adf9b86 100644<br>--- a/apps/confbridge/include/confbridge.h<br>+++ b/apps/confbridge/include/confbridge.h<br>@@ -218,6 +218,7 @@<br> unsigned int mix_interval; /*!< The internal mixing interval used by the bridge. When set to 0 the bridgewill use a default interval. */<br> struct bridge_profile_sounds *sounds;<br> char regcontext[AST_MAX_CONTEXT];<br>+ unsigned int video_update_discard; /*!< Amount of time after sending a video update request that subsequent requests should be discarded */<br> };<br> <br> /*! \brief The structure that represents a conference bridge */<br>diff --git a/bridges/bridge_softmix.c b/bridges/bridge_softmix.c<br>index 3dd2660..132ff08 100644<br>--- a/bridges/bridge_softmix.c<br>+++ b/bridges/bridge_softmix.c<br>@@ -968,6 +968,8 @@<br> */<br> static int softmix_bridge_write_control(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel, struct ast_frame *frame)<br> {<br>+ struct softmix_bridge_data *softmix_data = bridge->tech_pvt;<br>+<br> /*<br> * XXX Softmix needs to use channel roles to determine what to<br> * do with control frames.<br>@@ -975,7 +977,11 @@<br> <br> switch (frame->subclass.integer) {<br> case AST_CONTROL_VIDUPDATE:<br>- ast_bridge_queue_everyone_else(bridge, NULL, frame);<br>+ if (!bridge->softmix.video_mode.video_update_discard ||<br>+ ast_tvdiff_ms(ast_tvnow(), softmix_data->last_video_update) > bridge->softmix.video_mode.video_update_discard) {<br>+ ast_bridge_queue_everyone_else(bridge, NULL, frame);<br>+ softmix_data->last_video_update = ast_tvnow();<br>+ }<br> break;<br> default:<br> break;<br>diff --git a/bridges/bridge_softmix/include/bridge_softmix_internal.h b/bridges/bridge_softmix/include/bridge_softmix_internal.h<br>index 9daae4c..f93e663 100644<br>--- a/bridges/bridge_softmix/include/bridge_softmix_internal.h<br>+++ b/bridges/bridge_softmix/include/bridge_softmix_internal.h<br>@@ -198,6 +198,8 @@<br> * (does not guarantee success)<br> */<br> unsigned int binaural_init;<br>+ /*! The last time a video update was sent into the bridge */<br>+ struct timeval last_video_update;<br> };<br> <br> struct softmix_mixing_array {<br>diff --git a/configs/samples/confbridge.conf.sample b/configs/samples/confbridge.conf.sample<br>index 0e07f6b..265b953 100644<br>--- a/configs/samples/confbridge.conf.sample<br>+++ b/configs/samples/confbridge.conf.sample<br>@@ -218,6 +218,12 @@<br> ; Default is en (English).<br> <br> ;regcontext=conferences ; The name of the context into which to register conference names as extensions.<br>+;video_update_discard=2000 ; Amount of time (in milliseconds) to discard video update requests after sending a video<br>+ ; update request. Default is 2000. A video update request is a request for a full video<br>+ ; intra-frame. Clients can request this if they require a full frame in order to decode<br>+ ; the video stream. Since a full frame can be large limiting how often they occur can<br>+ ; reduce bandwidth usage at the cost of increasing how long it may take a newly joined<br>+ ; channel to receive the video stream.<br> <br> ; All sounds in the conference are customizable using the bridge profile options below.<br> ; Simply state the option followed by the filename or full path of the filename after<br>diff --git a/include/asterisk/bridge.h b/include/asterisk/bridge.h<br>index bc0e9c8..8d5c502 100644<br>--- a/include/asterisk/bridge.h<br>+++ b/include/asterisk/bridge.h<br>@@ -134,6 +134,7 @@<br> struct ast_bridge_video_single_src_data single_src_data;<br> struct ast_bridge_video_talker_src_data talker_src_data;<br> } mode_data;<br>+ unsigned int video_update_discard;<br> };<br> <br> /*!<br>@@ -903,6 +904,14 @@<br> void ast_bridge_set_sfu_video_mode(struct ast_bridge *bridge);<br> <br> /*!<br>+ * \brief Set the amount of time to discard subsequent video updates after a video update has been sent<br>+ *<br>+ * \param bridge Bridge to set the minimum video update wait time on<br>+ * \param video_update_discard Amount of time after sending a video update that others should be discarded<br>+ */<br>+void ast_bridge_set_video_update_discard(struct ast_bridge *bridge, unsigned int video_update_discard);<br>+<br>+/*!<br> * \brief Update information about talker energy for talker src video mode.<br> */<br> void ast_bridge_update_talker_src_video_mode(struct ast_bridge *bridge, struct ast_channel *chan, int talker_energy, int is_keyfame);<br>diff --git a/include/asterisk/frame.h b/include/asterisk/frame.h<br>index 2f6c365..8f0dacc 100644<br>--- a/include/asterisk/frame.h<br>+++ b/include/asterisk/frame.h<br>@@ -137,6 +137,8 @@<br> AST_FRFLAG_HAS_TIMING_INFO = (1 << 0),<br> /*! This frame has been requeued */<br> AST_FRFLAG_REQUEUED = (1 << 1),<br>+ /*! This frame contains a valid sequence number */<br>+ AST_FRFLAG_HAS_SEQUENCE_NUMBER = (1 << 2),<br> };<br> <br> struct ast_frame_subclass {<br>diff --git a/main/bridge.c b/main/bridge.c<br>index 3a358d9..a1a1a6f 100644<br>--- a/main/bridge.c<br>+++ b/main/bridge.c<br>@@ -3816,6 +3816,13 @@<br> ast_bridge_unlock(bridge);<br> }<br> <br>+void ast_bridge_set_video_update_discard(struct ast_bridge *bridge, unsigned int video_update_discard)<br>+{<br>+ ast_bridge_lock(bridge);<br>+ bridge->softmix.video_mode.video_update_discard = video_update_discard;<br>+ ast_bridge_unlock(bridge);<br>+}<br>+<br> void ast_bridge_update_talker_src_video_mode(struct ast_bridge *bridge, struct ast_channel *chan, int talker_energy, int is_keyframe)<br> {<br> struct ast_bridge_video_talker_src_data *data;<br>diff --git a/main/rtp_engine.c b/main/rtp_engine.c<br>index abd4b1f..64b2b31 100644<br>--- a/main/rtp_engine.c<br>+++ b/main/rtp_engine.c<br>@@ -3384,7 +3384,7 @@<br> {<br> int res = -1;<br> <br>- if (child->engine != parent->engine) {<br>+ if (parent && (child->engine != parent->engine)) {<br> return -1;<br> }<br> <br>diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c<br>index fe3680f..0ad2c8f 100644<br>--- a/res/res_pjsip_session.c<br>+++ b/res/res_pjsip_session.c<br>@@ -785,6 +785,12 @@<br> * we remove it as a result of the stream limit being reached.<br> */<br> if (ast_stream_get_state(stream) == AST_STREAM_STATE_REMOVED) {<br>+ /* This stream is no longer being used so release any resources the handler<br>+ * may have on it.<br>+ */<br>+ if (session_media->handler) {<br>+ session_media_set_handler(session_media, NULL);<br>+ }<br> continue;<br> }<br> <br>diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c<br>index a2e63ec..70561d0 100644<br>--- a/res/res_rtp_asterisk.c<br>+++ b/res/res_rtp_asterisk.c<br>@@ -266,7 +266,8 @@<br> unsigned int lastitexttimestamp;<br> unsigned int lastotexttimestamp;<br> unsigned int lasteventseqn;<br>- int lastrxseqno; /*!< Last received sequence number */<br>+ int lastrxseqno; /*!< Last received sequence number, from the network */<br>+ int expectedseqno; /*!< Next expected sequence number, from the core */<br> unsigned short seedrxseqno; /*!< What sequence number did they start with?*/<br> unsigned int seedrxts; /*!< What RTP timestamp did they start with? */<br> unsigned int rxcount; /*!< How many packets have we received? */<br>@@ -3245,6 +3246,7 @@<br> rtp->ssrc = ast_random();<br> ast_uuid_generate_str(rtp->cname, sizeof(rtp->cname));<br> rtp->seqno = ast_random() & 0x7fff;<br>+ rtp->expectedseqno = -1;<br> rtp->sched = sched;<br> ast_sockaddr_copy(&rtp->bind_address, addr);<br> <br>@@ -3274,7 +3276,7 @@<br> * \return 0 if element does not match.<br> * \return Non-zero if element matches.<br> */<br>-#define SSRC_MAPPING_ELEM_CMP(elem, value) ((elem).ssrc == (value))<br>+#define SSRC_MAPPING_ELEM_CMP(elem, value) (elem.instance == value)<br> <br> /*! \pre instance is locked */<br> static int ast_rtp_destroy(struct ast_rtp_instance *instance)<br>@@ -3289,7 +3291,7 @@<br> <br> ao2_lock(rtp->bundled);<br> bundled_rtp = ast_rtp_instance_get_data(rtp->bundled);<br>- AST_VECTOR_REMOVE_CMP_UNORDERED(&bundled_rtp->ssrc_mapping, rtp->themssrc, SSRC_MAPPING_ELEM_CMP, AST_VECTOR_ELEM_CLEANUP_NOOP);<br>+ AST_VECTOR_REMOVE_CMP_UNORDERED(&bundled_rtp->ssrc_mapping, instance, SSRC_MAPPING_ELEM_CMP, AST_VECTOR_ELEM_CLEANUP_NOOP);<br> ao2_unlock(rtp->bundled);<br> <br> ao2_lock(instance);<br>@@ -3897,6 +3899,7 @@<br> unsigned int ms = calc_txstamp(rtp, &frame->delivery);<br> struct ast_sockaddr remote_address = { {0,} };<br> int rate = rtp_get_rate(frame->subclass.format) / 1000;<br>+ unsigned int seqno;<br> <br> if (ast_format_cmp(frame->subclass.format, ast_format_g722) == AST_FORMAT_CMP_EQUAL) {<br> frame->samples /= 2;<br>@@ -3963,6 +3966,40 @@<br> rtp->lastdigitts = rtp->lastts;<br> }<br> <br>+ /* Assume that the sequence number we expect to use is what will be used until proven otherwise */<br>+ seqno = rtp->seqno;<br>+<br>+ /* If the frame contains sequence number information use it to influence our sequence number */<br>+ if (ast_test_flag(frame, AST_FRFLAG_HAS_SEQUENCE_NUMBER)) {<br>+ if (rtp->expectedseqno != -1) {<br>+ /* Determine where the frame from the core is in relation to where we expected */<br>+ int difference = frame->seqno - rtp->expectedseqno;<br>+<br>+ /* If there is a substantial difference then we've either got packets really out<br>+ * of order, or the source is RTP and it has cycled. If this happens we resync<br>+ * the sequence number adjustments to this frame. If we also have packet loss<br>+ * things won't be reflected correctly but it will sort itself out after a bit.<br>+ */<br>+ if (abs(difference) > 100) {<br>+ difference = 0;<br>+ }<br>+<br>+ /* Adjust the sequence number being used for this packet accordingly */<br>+ seqno += difference;<br>+<br>+ if (difference >= 0) {<br>+ /* This frame is on time or in the future */<br>+ rtp->expectedseqno = frame->seqno + 1;<br>+ rtp->seqno += difference;<br>+ }<br>+ } else {<br>+ /* This is the first frame with sequence number we've seen, so start keeping track */<br>+ rtp->expectedseqno = frame->seqno + 1;<br>+ }<br>+ } else {<br>+ rtp->expectedseqno = -1;<br>+ }<br>+<br> if (ast_test_flag(frame, AST_FRFLAG_HAS_TIMING_INFO)) {<br> rtp->lastts = frame->ts * rate;<br> }<br>@@ -3974,7 +4011,7 @@<br> int hdrlen = 12, res, ice;<br> unsigned char *rtpheader = (unsigned char *)(frame->data.ptr - hdrlen);<br> <br>- put_unaligned_uint32(rtpheader, htonl((2 << 30) | (codec << 16) | (rtp->seqno) | (mark << 23)));<br>+ put_unaligned_uint32(rtpheader, htonl((2 << 30) | (codec << 16) | (seqno) | (mark << 23)));<br> put_unaligned_uint32(rtpheader + 4, htonl(rtp->lastts));<br> put_unaligned_uint32(rtpheader + 8, htonl(rtp->ssrc));<br> <br>@@ -4011,7 +4048,13 @@<br> }<br> }<br> <br>- rtp->seqno++;<br>+ /* If the sequence number that has been used doesn't match what we expected then this is an out of<br>+ * order late packet, so we don't need to increment as we haven't yet gotten the expected frame from<br>+ * the core.<br>+ */<br>+ if (seqno == rtp->seqno) {<br>+ rtp->seqno++;<br>+ }<br> <br> return 0;<br> }<br>@@ -5474,6 +5517,7 @@<br> rtp->f.datalen = res - hdrlen;<br> rtp->f.data.ptr = read_area + hdrlen;<br> rtp->f.offset = hdrlen + AST_FRIENDLY_OFFSET;<br>+ ast_set_flag(&rtp->f, AST_FRFLAG_HAS_SEQUENCE_NUMBER);<br> rtp->f.seqno = seqno;<br> rtp->f.stream_num = rtp->stream_num;<br> <br>@@ -6082,7 +6126,7 @@<br> static int ast_rtp_bundle(struct ast_rtp_instance *child, struct ast_rtp_instance *parent)<br> {<br> struct ast_rtp *child_rtp = ast_rtp_instance_get_data(child);<br>- struct ast_rtp *parent_rtp = ast_rtp_instance_get_data(parent);<br>+ struct ast_rtp *parent_rtp;<br> struct rtp_ssrc_mapping mapping;<br> struct ast_sockaddr them = { { 0, } };<br> <br>@@ -6099,7 +6143,7 @@<br> /* The child lock can't be held while accessing the parent */<br> ao2_lock(child_rtp->bundled);<br> bundled_rtp = ast_rtp_instance_get_data(child_rtp->bundled);<br>- AST_VECTOR_REMOVE_CMP_UNORDERED(&bundled_rtp->ssrc_mapping, child_rtp->themssrc, SSRC_MAPPING_ELEM_CMP, AST_VECTOR_ELEM_CLEANUP_NOOP);<br>+ AST_VECTOR_REMOVE_CMP_UNORDERED(&bundled_rtp->ssrc_mapping, child, SSRC_MAPPING_ELEM_CMP, AST_VECTOR_ELEM_CLEANUP_NOOP);<br> ao2_unlock(child_rtp->bundled);<br> <br> ao2_lock(child);<br>@@ -6113,6 +6157,8 @@<br> return 0;<br> }<br> <br>+ parent_rtp = ast_rtp_instance_get_data(parent);<br>+<br> /* We no longer need any transport related resources as we will use our parent RTP instance instead */<br> rtp_deallocate_transport(child, child_rtp);<br> <br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/6032">change 6032</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/6032"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: merged </div>
<div style="display:none"> Gerrit-Change-Id: Ibd91362f0e4990b6129638e712bc8adf0899fd45 </div>
<div style="display:none"> Gerrit-Change-Number: 6032 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>