Merge "bridge_softmix / res_rtp_asterisk: Fix packet loss and renegotiation issues."

apps/app_confbridge.c

@@ -1485,6 +1485,7 @@ static struct confbridge_conference *join_conference_bridge(const char *conferen
 			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 */

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 @@ static char *handle_cli_confbridge_show_bridge_profile(struct ast_cli_entry *e,
 		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 @@ int conf_load_config(void)
 	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);
 

apps/confbridge/include/confbridge.h

@@ -218,6 +218,7 @@ struct bridge_profile {
 	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 */

bridges/bridge_softmix.c

@@ -985,6 +985,8 @@ static void softmix_bridge_write_voice(struct ast_bridge *bridge, struct ast_bri
  */
 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.
@@ -992,7 +994,11 @@ static int softmix_bridge_write_control(struct ast_bridge *bridge, struct ast_br
 
 	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;

bridges/bridge_softmix/include/bridge_softmix_internal.h

@@ -198,6 +198,8 @@ struct softmix_bridge_data {
 	 * (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 {

configs/samples/confbridge.conf.sample

@@ -218,6 +218,12 @@ type=bridge
                            ; 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. A video update request is a request for a full video
+                           ; intra-frame. Clients can request this if they require a full frame in order to decode
+                           ; the video stream. Since a full frame can be large limiting how often they occur can
+                           ; reduce bandwidth usage at the cost of increasing how long it may take a newly joined
+                           ; channel to receive the video stream.
 
 ; 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

include/asterisk/bridge.h

@@ -134,6 +134,7 @@ struct ast_bridge_video_mode {
 		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;
 };
 
 /*!
@@ -902,6 +903,14 @@ void ast_bridge_set_talker_src_video_mode(struct ast_bridge *bridge);
  */
 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.
  */

include/asterisk/frame.h

@@ -137,6 +137,8 @@ enum {
 	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 {

main/bridge.c

@@ -3816,6 +3816,13 @@ void ast_bridge_set_sfu_video_mode(struct ast_bridge *bridge)
 	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;

main/rtp_engine.c

@@ -3386,7 +3386,7 @@ int ast_rtp_instance_bundle(struct ast_rtp_instance *child, struct ast_rtp_insta
 {
 	int res = -1;
 
-	if (child->engine != parent->engine) {
+	if (parent && (child->engine != parent->engine)) {
 		return -1;
 	}
 

res/res_pjsip_session.c

@@ -785,6 +785,12 @@ static int handle_negotiated_sdp(struct ast_sip_session *session, const pjmedia_
 		 * 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;
 		}
 

res/res_rtp_asterisk.c

@@ -266,7 +266,8 @@ struct ast_rtp {
 	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 @@ static int ast_rtp_new(struct ast_rtp_instance *instance,
 	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 @@ static int ast_rtp_new(struct ast_rtp_instance *instance,
  * \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 @@ static int ast_rtp_destroy(struct ast_rtp_instance *instance)
 
 		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 @@ static int rtp_raw_write(struct ast_rtp_instance *instance, struct ast_frame *fr
 	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,40 @@ static int rtp_raw_write(struct ast_rtp_instance *instance, struct ast_frame *fr
 		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;
+
+			/* If there is a substantial difference then we've either got packets really out
+			 * of order, or the source is RTP and it has cycled. If this happens we resync
+			 * the sequence number adjustments to this frame. If we also have packet loss
+			 * things won't be reflected correctly but it will sort itself out after a bit.
+			 */
+			if (abs(difference) > 100) {
+				difference = 0;
+			}
+
+			/* Adjust the sequence number being used for this packet accordingly */
+			seqno += difference;
+
+			if (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 +4011,7 @@ static int rtp_raw_write(struct ast_rtp_instance *instance, struct ast_frame *fr
 		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 +4048,13 @@ static int rtp_raw_write(struct ast_rtp_instance *instance, struct ast_frame *fr
 		}
 	}
 
-	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 +5517,7 @@ static struct ast_frame *ast_rtp_read(struct ast_rtp_instance *instance, int rtc
 	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 +6126,7 @@ static void ast_rtp_set_stream_num(struct ast_rtp_instance *instance, int stream
 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 +6143,7 @@ static int ast_rtp_bundle(struct ast_rtp_instance *child, struct ast_rtp_instanc
 		/* 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 +6157,8 @@ static int ast_rtp_bundle(struct ast_rtp_instance *child, struct ast_rtp_instanc
 		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);