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